357063 - GC hazard in XMLEquality

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

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]

Attachment: Fix v1

The patch switches XMLEquality to use JSXMLArrayCursor to navigate arrays to compare.

Comment 2

[Igor Bukanov]

Attachment: Fix v2

The previous version had some artifacts left after my initial implementation.

Comment 3

[Jeff Walden [:Waldo]]

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]

Attachment: Fix v2 for real

This is what I meant by v2 originally.

Comment 5

[Brendan Eich [:brendan]]

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]]

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]

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]

Attachment: 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] (inactive)]

Attachment: e4x/GC/regress-357063-01.js

Comment 10

[Bob Clary [:bc] (inactive)]

Attachment: e4x/GC/regress-357063-02.js

Comment 11

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20061028 windows/linux

Comment 12

[Daniel Veditz [:dveditz]]

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]

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]

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] (inactive)]

verified fixed 1.8.0.9, 1.8.1.1 20061108 windows/linux/mac*

Comment 16

[Bob Clary [:bc] (inactive)]

/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