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
:
:
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 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 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 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 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 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 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 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 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 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 Bob Clary [:bc:] 2006-10-27 14:44:24 PDT
Created attachment 243844 [details]
e4x/GC/regress-357063-01.js
Comment 10 Bob Clary [:bc:] 2006-10-27 14:44:55 PDT
Created attachment 243845 [details]
e4x/GC/regress-357063-02.js
Comment 11 Bob Clary [:bc:] 2006-10-28 16:16:26 PDT
verified fixed 1.9 20061028 windows/linux
Comment 12 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 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 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 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 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.