Last Comment Bug 357063 - GC hazard in XMLEquality
: GC hazard in XMLEquality
Status: VERIFIED FIXED
[sg:critical?]
: verified1.8.0.9, verified1.8.1.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
-- normal (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 354145
Blocks: js1.7src
  Show dependency treegraph
 
Reported: 2006-10-17 20:33 PDT by Igor Bukanov
Modified: 2007-02-08 16:33 PST (History)
3 users (show)
dveditz: blocking1.8.1.1+
dveditz: blocking1.8.0.9+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix v1 (3.02 KB, patch)
2006-10-17 20:54 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
Fix v2 (3.02 KB, patch)
2006-10-17 20:58 PDT, Igor Bukanov
jwalden+bmo: review+
Details | Diff | Splinter Review
Fix v2 for real (2.94 KB, patch)
2006-10-23 21:18 PDT, Igor Bukanov
brendan: review+
dveditz: approval1.8.1.1+
Details | Diff | Splinter Review
Fix v2 for 1.8.0 branch (2.95 KB, patch)
2006-10-26 06:53 PDT, Igor Bukanov
dveditz: approval1.8.0.9+
Details | Diff | Splinter Review
e4x/GC/regress-357063-01.js (2.38 KB, text/plain)
2006-10-27 14:44 PDT, Bob Clary [:bc:]
no flags Details
e4x/GC/regress-357063-02.js (2.26 KB, text/plain)
2006-10-27 14:44 PDT, Bob Clary [:bc:]
no flags Details

Description User image Igor Bukanov 2006-10-17 20:33:20 PDT
The bug 354145 did not fix all the problems with script-induced XML tree mutations while the code walk through the tree. In particular, it did not address the case of XMLEquality code. As was pointed out in bug 354145 comment 82, the function contains the loop:

        xvec = (JSXML **) xml->xml_kids.vector;
        vvec = (JSXML **) vxml->xml_kids.vector;
        for (i = 0; i < n; i++) {
            xobj = js_GetXMLObject(cx, xvec[i]);
            vobj = js_GetXMLObject(cx, vvec[i]);
            if (!xobj || !vobj)
                return JS_FALSE;
            if (!js_XMLObjectOps.equality(cx, xobj, OBJECT_TO_JSVAL(vobj), bp))
                return JS_FALSE;
            if (!*bp)
                break;
        }

This is a GC hazard since xml_equality called by js_XMLObjectOps.equality calls in certain cases js_ValueToString. Through manipulating XML.prototype.function::toString that can be triggered to call script-supplied code as the following 2 examples demonstrates:

@callisto~/m/trunk/mozilla/js/src> cat ~/m/files/y1.js
var xml = new XML("<xml><a>text</a><a>text</a></xml>");
var xml2 = new XML("<xml><a>text</a><a>text</a></xml>");
var list1 = xml.a;
var list2 = xml2.a;

XML.prototype.function::toString = function() {
  if (xml2) {
    delete list2[1];
    delete list2[0];
    xml2 = null;
    gc();
  }
  return "text";
}

var value = list1 == list2;

@callisto~/m/trunk/mozilla/js/src> ./Linux_All_DBG.OBJ/js ~/m/files/y1.js 
before 9232, after 9232, break 08186000
Segmentation fault (core dumped)



@callisto~/m/trunk/mozilla/js/src> cat ~/m/files/y2.js
var xml = new XML("<xml>text<a>B</a></xml>");
var xml2 = new XML("<xml>text<a>C</a></xml>");

XML.prototype.function::toString = function() {
  if (xml2) {
    delete xml2.*;
    xml2 = null;
    gc();
  }
  return "text";
}

if (xml == xml2)
  throw "unexpected result";
@callisto~/m/trunk/mozilla/js/src> ./Linux_All_DBG.OBJ/js ~/m/files/y1.js 
before 9232, after 9232, break 08186000
Segmentation fault (core dumped)
Comment 1 User image Igor Bukanov 2006-10-17 20:54:22 PDT
Created attachment 242600 [details] [diff] [review]
Fix v1

The patch switches XMLEquality to use JSXMLArrayCursor to navigate arrays to compare.
Comment 2 User image Igor Bukanov 2006-10-17 20:58:06 PDT
Created attachment 242602 [details] [diff] [review]
Fix v2

The previous version had some artifacts left after my initial implementation.
Comment 3 User image Jeff Walden [:Waldo] (remove +bmo to email) 2006-10-23 20:25:01 PDT
Comment on attachment 242602 [details] [diff] [review]
Fix v2

>+            ok = xobj && vobj &&
>+                 xml_equality(cx, xobj, OBJECT_TO_JSVAL(vobj), bp);
>+            ok = js_XMLObjectOps.equality(cx, xobj, OBJECT_TO_JSVAL(vobj), bp);

You obviously only need the first of these.  (I speculate that you already have this fixed in the actual v2 of this patch, not the v2 [v1 actually] that you uploaded.  ;-) )

r=me with that change
Comment 4 User image Igor Bukanov 2006-10-23 21:18:20 PDT
Created attachment 243287 [details] [diff] [review]
Fix v2 for real

This is what I meant by v2 originally.
Comment 5 User image Brendan Eich [:brendan] 2006-10-24 05:27:03 PDT
Comment on attachment 243287 [details] [diff] [review]
Fix v2 for real

Nice -- can you mark up the bug for js1.7src and 1.8.1.1?  Thanks,

/be
Comment 6 User image Brendan Eich [:brendan] 2006-10-24 05:27:45 PDT
Oops, bug already marked.  I must have been thinking of the other one for which I just reviewed a patch.

/be
Comment 7 User image Igor Bukanov 2006-10-26 06:37:02 PDT
I committed the patch from comment 4 to the trunk:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.133; previous revision: 3.132
done
Comment 8 User image Igor Bukanov 2006-10-26 06:53:05 PDT
Created attachment 243629 [details] [diff] [review]
Fix v2 for 1.8.0 branch

The patch for trunk/1.8.1 does not apply to 1.8.0 branch since that branch does not have js_EqualStrings which XMLEquality from later branches uses.
Comment 9 User image Bob Clary [:bc:] 2006-10-27 14:44:24 PDT
Created attachment 243844 [details]
e4x/GC/regress-357063-01.js
Comment 10 User image Bob Clary [:bc:] 2006-10-27 14:44:55 PDT
Created attachment 243845 [details]
e4x/GC/regress-357063-02.js
Comment 11 User image Bob Clary [:bc:] 2006-10-28 16:16:26 PDT
verified fixed 1.9 20061028 windows/linux
Comment 12 User image Daniel Veditz [:dveditz] 2006-11-07 11:02:11 PST
Comment on attachment 243629 [details] [diff] [review]
Fix v2 for 1.8.0 branch

Approved for 1.8.0/1.8 branches, a=dveditz for drivers
Comment 13 User image Igor Bukanov 2006-11-07 12:11:23 PST
I committed the patch from the comment 4 to MOZILLA_1_8_BRANCH:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.50.2.54; previous revision: 3.50.2.53
done
Comment 14 User image Igor Bukanov 2006-11-07 12:19:05 PST
I committed the patch from comment 8 to MOZILLA_1_8_0_BRANCH:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.50.2.15.2.25; previous revision: 3.50.2.15.2.24
done
Comment 15 User image Bob Clary [:bc:] 2006-11-09 12:37:32 PST
verified fixed 1.8.0.9, 1.8.1.1 20061108 windows/linux/mac*
Comment 16 User image Bob Clary [:bc:] 2007-02-08 16:33:36 PST
/cvsroot/mozilla/js/tests/e4x/GC/regress-357063-01.js,v  <--  regress-357063-01.js

/cvsroot/mozilla/js/tests/e4x/GC/regress-357063-02.js,v  <--  regress-357063-02.js

Note You need to log in before you can comment on or make changes to this bug.