Status

()

Core
JavaScript Engine
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({verified1.8.0.9, verified1.8.1.1})

Trunk
verified1.8.0.9, verified1.8.1.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.1 +
blocking1.8.0.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

11 years ago
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)
(Assignee)

Updated

11 years ago
No longer depends on: 354145
(Assignee)

Updated

11 years ago
Depends on: 354145
(Assignee)

Updated

11 years ago
Blocks: 355044
(Assignee)

Comment 1

11 years ago
Created attachment 242600 [details] [diff] [review]
Fix v1

The patch switches XMLEquality to use JSXMLArrayCursor to navigate arrays to compare.
Attachment #242600 - Flags: review?(brendan)
(Assignee)

Comment 2

11 years ago
Created attachment 242602 [details] [diff] [review]
Fix v2

The previous version had some artifacts left after my initial implementation.
Attachment #242600 - Attachment is obsolete: true
Attachment #242602 - Flags: review?(brendan)
Attachment #242600 - Flags: review?(brendan)
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
Whiteboard: [sg:critical?]
(Assignee)

Updated

11 years ago
Attachment #242602 - Flags: review?(jwalden+bmo)
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
Attachment #242602 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 4

11 years ago
Created attachment 243287 [details] [diff] [review]
Fix v2 for real

This is what I meant by v2 originally.
Attachment #242602 - Attachment is obsolete: true
Attachment #243287 - Flags: review?(brendan)
Attachment #242602 - Flags: review?(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
Attachment #243287 - Flags: review?(brendan)
Attachment #243287 - Flags: review+
Attachment #243287 - Flags: approval1.8.1.1?
Oops, bug already marked.  I must have been thinking of the other one for which I just reviewed a patch.

/be
(Assignee)

Comment 7

11 years ago
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 8

11 years ago
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.
Attachment #243629 - Flags: approval1.8.0.9?

Comment 9

11 years ago
Created attachment 243844 [details]
e4x/GC/regress-357063-01.js

Comment 10

11 years ago
Created attachment 243845 [details]
e4x/GC/regress-357063-02.js

Updated

11 years ago
Flags: in-testsuite+

Comment 11

11 years ago
verified fixed 1.9 20061028 windows/linux
Status: RESOLVED → VERIFIED
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+
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
Attachment #243629 - Flags: approval1.8.0.9? → approval1.8.0.9+
Attachment #243287 - Flags: approval1.8.1.1? → approval1.8.1.1+
(Assignee)

Comment 13

11 years ago
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
Keywords: fixed1.8.1.1
(Assignee)

Comment 14

11 years ago
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
Keywords: fixed1.8.0.9

Comment 15

11 years ago
verified fixed 1.8.0.9, 1.8.1.1 20061108 windows/linux/mac*
Keywords: fixed1.8.0.9, fixed1.8.1.1 → verified1.8.0.9, verified1.8.1.1
Group: security

Comment 16

11 years ago
/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
You need to log in before you can comment on or make changes to this bug.