Closed Bug 665355 Opened 11 years ago Closed 11 years ago

cyclic __proto__ is possible for ArrayBuffer


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox7 - ---


(Reporter: jruderman, Assigned: nsm)



(Keywords: crash, regression, testcase, Whiteboard: fixed-in-tracemonkey)


(2 files, 2 obsolete files)

var b = new ArrayBuffer([]);
b.__proto__ = b;

  Crash due to too much recursion through js::ArrayBuffer::obj_lookupProperty

  "TypeError: cyclic __proto__ value" at line 2

Regression from:

user:        Nikhil Marathe
date:        Tue Jun 14 15:33:11 2011 -0400
summary:     Bug 656519 - Avoid a malloc (and a finalizer) by storing the malloc'd array in our slots instead of in a separate malloc'd structure in our private field. r=mrbkap
Attached patch Fix recursionSplinter Review
Attachment #540340 - Flags: review?(mrbkap)
Attachment #540340 - Flags: review?(jruderman)
Comment on attachment 540340 [details] [diff] [review]
Fix recursion

The patch fixes the problem for me.
Attachment #540340 - Flags: review?(jruderman)
Blocks: 665356
Should be applied over attachment 540340 [details] [diff] [review].

Fixes issue where accessing __proto__ after setting it to null caused a property lookup on the delegate object instead which returned its own __proto__.
Attachment #541194 - Flags: review?(mrbkap)
Attachment #540340 - Flags: review?(mrbkap) → review+
Comment on attachment 541194 [details] [diff] [review]
ArrayBuffer and internal delegate share prototype chain

r=me with a followup bug filed on fixing the fact that this doesn't actually delegate to Object.prototype.__proto__'s setter (and the behavioral differences that are visible because of that.
Attachment #541194 - Flags: review?(mrbkap) → review+
No longer blocks: 667047
Depends on: 667047
updated with testcase to show fix for bug 665961
Attachment #541194 - Attachment is obsolete: true
Attachment #542202 - Flags: review?(mrbkap)
Comment on attachment 542202 [details] [diff] [review]
ArrayBuffer and internal delegate share prototype chain

Review of attachment 542202 [details] [diff] [review]:

::: js/src/jstypedarray.cpp
@@ +293,5 @@
> +            return false;
> +
> +        if (!SetProto(cx, obj, pobj, true)) {
> +            // restore proto on delegate
> +            SetProto(cx, delegate, oldDelegateProto, true);

It'd probably be good to use JS_ALWAYS_TRUE here.
Attachment #542202 - Flags: review?(mrbkap) → review+
But a SetProto call can fail with an exception in the case of
x.__proto__ = x
which should raise an exception but not abort the interpreter.

So it seems to be a better idea to restore the delegate prototype chain.
Sorry, the quoting there wasn't the best. I was talking about the second call to SetProto. In that case, we are already resetting the delegate's proto to the original proto, and we know that that was a valid proto to begin with.

The first call (that's already in the if statement's condition) definitely needs to be a runtime check.
Attachment #543009 - Flags: review?(mrbkap) → review+
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
This bug was nominated for tracking Firefox 7. Is the fix suitable for landing on Aurora? If so, please nominate the attachment with an approval request and an explanation of the value of the patch and the associated risk. Thanks.
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-665355.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.