Closed Bug 355474 Opened 16 years ago Closed 16 years ago

Hang with WAY_TOO_MUCH_GC, iterating over e4x

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: igor)

References

Details

(Keywords: hang, testcase, verified1.8.1, Whiteboard: [sg:critical] js1.7 feature)

Attachments

(5 files, 4 obsolete files)

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.
Attached patch fix (obsolete) — Splinter Review
Assignee: general → brendan
Status: NEW → ASSIGNED
Attachment #241284 - Flags: review?(igor.bukanov)
OS: Mac OS X 10.4 → All
Hardware: Macintosh → All
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
Attached patch alterna-fix (obsolete) — Splinter Review
Attachment #241289 - Flags: review?(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__.
Attachment #241284 - Flags: review?(igor.bukanov) → review-
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?
(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
(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.
Igor, could you take this bug?  I'm overcommitted elsewhere and you're doing great cleaning up jsiter.c.

/be
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.
Assignee: brendan → igor.bukanov
Status: ASSIGNED → NEW
Attached patch Fix v3Splinter Review
Look only XML and its prototype chain for iterators.
Attachment #241284 - Attachment is obsolete: true
Attachment #241289 - Attachment is obsolete: true
Attachment #241293 - Flags: review?(brendan)
Attachment #241289 - Flags: review?(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 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
Attachment #241293 - Flags: review?(brendan)
Attachment #241293 - Flags: review+
Attachment #241293 - Flags: approval1.8.1.1?
Blocks: js1.7src
(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.
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
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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.
Attachment #241293 - Flags: approval1.8.1?
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
I will commit that assert.
Whiteboard: [sg:critical]
This is the commited patch + extra committed assert.
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.
Flags: blocking1.8.1?
Attachment #241293 - Flags: approval1.8.1?
Attached file e4x/Regress/regress-355474-02.js (obsolete) —
I also renamed e4x/Regress/regress-355474.js to e4x/Regress/regress-355474-01.js locally.
Flags: in-testsuite+
(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.
Attachment #241309 - Attachment is obsolete: true
fix erroneous writeLineToLog. I am going to remove all occurences of writeLineToLog and replace them with shell|browser compatible calls to print.
Attachment #241318 - Attachment is obsolete: true
verified fixed 1.9 20061005 windows/linux based on -02 but not with WAY_TOO_MUCH_GC build.
Status: RESOLVED → VERIFIED
Igor, could you make a 1.8-branc-only patch that does not depend on the cleanup patch from  bug 354982 ?

/be
(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.
Attached patch 1.8.1-only patchSplinter Review
This is the patch that does not assume that cleanup is there but then js_GetXMLFunction has to be called twice.
Attachment #241483 - Flags: review?(brendan)
Attachment #241483 - Flags: review?(brendan)
Attachment #241483 - Flags: review+
Attachment #241483 - Flags: approval1.8.1?
Attachment #241293 - Flags: approval1.8.1.1? → approval1.8.1?
Blocking for Fx2 RC3
Flags: blocking1.8.1? → blocking1.8.1+
Comment on attachment 241293 [details] [diff] [review]
Fix v3

Approved for RC3.
Attachment #241293 - Flags: approval1.8.1? → approval1.8.1+
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
Keywords: fixed1.8.1
(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.
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.
Whiteboard: [sg:critical] → [sg:critical] js1.7 feature
verified fixed 20061009 1.8 windows/linux/mac* 1.9 windows/linux
Depends on: 354982
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 on attachment 241483 [details] [diff] [review]
1.8.1-only patch

This was indeed fixed and verified for 1.8.1
Attachment #241483 - Flags: approval1.8.1?
Group: security
/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
You need to log in before you can comment on or make changes to this bug.