355474 - Hang with WAY_TOO_MUCH_GC, iterating over e4x

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

f = function () { for each (var z in <y/>) { print(z); } }; f(); f();

hangs, but only with WAY_TOO_MUCH_GC.

Security-sensitive because GC scares me.

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 241284 [details] [diff] [review]
fix

>@@ -387,16 +388,17 @@ js_ValueToIterator(JSContext *cx, uintN 
> 
>     JS_ASSERT(obj);
>     JS_PUSH_TEMP_ROOT_OBJECT(cx, obj, &tvr);
> 
>     atom = cx->runtime->atomState.iteratorAtom;
>     if (!JS_GetMethodById(cx, obj, ATOM_TO_JSID(atom), &obj, vp))
>         goto bad;
> 
>+    tvr.u.object = obj;
>     if (JSVAL_IS_VOID(*vp)) {

The JS_GetMethodById API is hazardous: passing obj, id, &obj means that even in the not-found success case (true return value with JSVAL_IS_VOID(*vp)), obj may mutate.  It turns out only in the case of E4X can this happen, as in this bug's testcase.  It seems wrong to enumerate the string object created to wrap the simple string content of the XML element denoted by obj, having failed to find __iterator__ in the string object.  Why wouldn't we try to enumerate the XML element?

This suggests a better patch.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: alterna-fix

Comment 4

[Igor Bukanov]

Comment on attachment 241284 [details] [diff] [review]
fix

This is no the right fix as one do not want to iterate over String in

for (var i in <a>text</a>) print(i)

The problem is that xml_getMethod sets obj to the last target it considered for the source of methods. A simple fix would be to ask for obj2 and if the method is not found, consider the original.

A better fix would be not to call JS_GetMethodByID but rather check for XML and if so call GetFunction from jsxml.c there. Otherwise call OBJ_GET_PROPERTY. I.e. when searching for __iterator__ in XML, the code should not even consider the first child of xml list or String. That would be very similar to what happended before Object.prototype.__iterator__.

Comment 5

[Igor Bukanov]

Comment on attachment 241289 [details] [diff] [review]
alterna-fix

>-        if (!js_InternalInvoke(cx, obj, *vp, JSINVOKE_ITERATOR, 1, &arg, vp))
>+        if (!js_InternalInvoke(cx, obj2, *vp, JSINVOKE_ITERATOR, 1, &arg, vp))
>             goto bad;

Do we really want changes in String.prototype.__iterator__ to affect all XML iterators?

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> (From update of attachment 241289 [details] [diff] [review] [edit])
> >-        if (!js_InternalInvoke(cx, obj, *vp, JSINVOKE_ITERATOR, 1, &arg, vp))
> >+        if (!js_InternalInvoke(cx, obj2, *vp, JSINVOKE_ITERATOR, 1, &arg, vp))
> >             goto bad;
> 
> Do we really want changes in String.prototype.__iterator__ to affect all XML
> iterators?

Only XML elements with simple content?

I have the sinking feeling JS_GetMethodById is just a bad API.  OTOH, it would be good to avoid special-casing E4X all over the place.

/be

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> 
> Only XML elements with simple content?

No, for each (var i in <><a>text</a></>) print(i.toXMLString()); would also iterate over the string!

> 
> I have the sinking feeling JS_GetMethodById is just a bad API.  OTOH, it would
> be good to avoid special-casing E4X all over the place.

jsiter.c already contains a few XML checks. One more would not harm IMO.

Comment 8

[Brendan Eich [:brendan]]

Igor, could you take this bug?  I'm overcommitted elsewhere and you're doing great cleaning up jsiter.c.

/be

Comment 9

[Igor Bukanov]

I think this bug in general exposed a bad IMO E4X decision. It would be better if that auto-string conversion would not happen. It sounds cool in theory but it only works because String methods are read-only. __iterator__ is nor read-only as you can use it to modify the target.

Comment 10

[Igor Bukanov]

Attachment: Fix v3

Look only XML and its prototype chain for iterators.

Comment 11

[Igor Bukanov]

With the patch the following test case correctly prints <a>text</a>:

for each (var i in <><a>text</a></>) print(i.toXMLString());

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 241293 [details] [diff] [review]
Fix v3

Nice.  Igor, if you agree this bug exposes at most an iloop, please mark it not security-sensitive.

/be

Comment 13

[Igor Bukanov]

(In reply to comment #12)
> (From update of attachment 241293 [details] [diff] [review] [edit])
> Nice.  Igor, if you agree this bug exposes at most an iloop, please mark it not
> security-sensitive.

The bug exposes theoretically exploitable GC-hazard that was there even before removal of Object.prototype.__iterator__. The String object instance corresponding to simple XML text becomes unrooted when js_NewObject creates another GCX_OBJECT. Then when the slots are allocated, it can be GC-ed and is GC-ed with WAY_TOO_MUCH_GC.

This can be exploited if one tries to repeat enumeration to trigger GC at the right moment.

Comment 14

[Igor Bukanov]

i committed the patch from comment 13 to the trunk:

Checking in jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.50; previous revision: 3.49
done
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.129; previous revision: 3.128
done
Checking in jsxml.h;
/cvsroot/mozilla/js/src/jsxml.h,v  <--  jsxml.h
new revision: 3.14; previous revision: 3.13
done

Comment 15

[Igor Bukanov]

Comment on attachment 241293 [details] [diff] [review]
Fix v3

Asking for 1.8.1 for 2 reasons:

1. This fixes a regression that was introduced few days before.

2. It fixes a GC hazard that existed for quite some time already.

Comment 16

[Brendan Eich [:brendan]]

Slight preference to keep the assertion my bogo-fix "fix" patch added:

+        JS_ASSERT(obj != iterobj);
         iterobj->slots[JSSLOT_PROTO] = OBJECT_TO_JSVAL(obj);

/be

Comment 17

[Igor Bukanov]

I will commit that assert.

Comment 18

[Igor Bukanov]

Attachment: Fix v3 + assert for references

This is the commited patch + extra committed assert.

Comment 19

[Igor Bukanov]

I ask for 1.8.1 blocking instead of approval on the patch as the branch requires separated patch if this has to be included into 1.8.1 without the cleanup patch from bug 354982.

Comment 20

[Bob Clary [:bc] (inactive)]

Attachment: e4x/Regress/regress-355474.js

Comment 21

[Bob Clary [:bc] (inactive)]

Attachment: e4x/Regress/regress-355474-02.js

I also renamed e4x/Regress/regress-355474.js to e4x/Regress/regress-355474-01.js locally.

Comment 22

[Igor Bukanov]

(In reply to comment #21)
> Created an attachment (id=241318) [edit]
> e4x/Regress/regress-355474-02.js

Note that this test exposes the problem without WAY_TOO_MUCH_GC.

Comment 23

[Bob Clary [:bc] (inactive)]

Attachment: e4x/Regress/regress-355474-01.js

Comment 24

[Bob Clary [:bc] (inactive)]

Attachment: e4x/Regress/regress-355474-02.js

fix erroneous writeLineToLog. I am going to remove all occurences of writeLineToLog and replace them with shell|browser compatible calls to print.

Comment 25

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20061005 windows/linux based on -02 but not with WAY_TOO_MUCH_GC build.

Comment 26

[Brendan Eich [:brendan]]

Igor, could you make a 1.8-branc-only patch that does not depend on the cleanup patch from  bug 354982 ?

/be

Comment 27

[Igor Bukanov]

(In reply to comment #26)
> Igor, could you make a 1.8-branc-only patch that does not depend on the cleanup
> patch from  bug 354982 ?

Sure.

Comment 28

[Igor Bukanov]

Attachment: 1.8.1-only patch

This is the patch that does not assume that cleanup is there but then js_GetXMLFunction has to be called twice.

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking for Fx2 RC3

Comment 30

[Mike Schroepfer]

Comment on attachment 241293 [details] [diff] [review]
Fix v3

Approved for RC3.

Comment 31

[Igor Bukanov]

I committed the patch from comment 28 to MOZILLA_1_8_BRANCH:

Checking in jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.17.2.25; previous revision: 3.17.2.24
done
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.50.2.49; previous revision: 3.50.2.48
done
Checking in jsxml.h;
/cvsroot/mozilla/js/src/jsxml.h,v  <--  jsxml.h
new revision: 3.10.4.3; previous revision: 3.10.4.2

Comment 32

[Igor Bukanov]

(In reply to comment #30)
> (From update of attachment 241293 [details] [diff] [review] [edit])

I presume that was for the attachment 241483 [details] [diff] [review] from comment 28.

Comment 33

[Bob Clary [:bc] (inactive)]

passes 1.8 1.9 20061007 windows/linux, note since test passed 1.8 before the check in to 1.8 the tests don't really exercise the bug.

Comment 34

[Bob Clary [:bc] (inactive)]

verified fixed 20061009 1.8 windows/linux/mac* 1.9 windows/linux

Comment 35

[Jay Patel [:jay]]

Igor/Brendan:  Looks like this bug already landed on the 1.8.1 branch before we released 2.0, but just wanted to make sure, since we are double checking that we have taken all dependency fixes that go along with bug 355658.  Can someone please confirm that there is nothing left to do with this bug and remove/clear the approval flag with a note.  Thanks!

Comment 36

[Igor Bukanov]

Comment on attachment 241483 [details] [diff] [review]
1.8.1-only patch

This was indeed fixed and verified for 1.8.1

Comment 37

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/e4x/Regress/regress-355474-01.js,v  <--  regress-355474-01.js

/cvsroot/mozilla/js/tests/e4x/Regress/regress-355474-02.js,v  <--  regress-355474-02.js