665355 - cyclic __proto__ is possible for ArrayBuffer

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

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

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

Expected:
  "TypeError: cyclic __proto__ value" at line 2

Regression from:

changeset:   http://hg.mozilla.org/tracemonkey/rev/b89374bc34ee
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

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Fix recursion

Comment 2

[Jesse Ruderman]

Comment on attachment 540340 [details] [diff] [review]
Fix recursion

The patch fixes the problem for me.

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: ArrayBuffer and internal delegate share prototype chain

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__.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: ArrayBuffer and internal delegate share prototype chain

updated with testcase to show fix for bug 665961

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 10

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: ArrayBuffer and internal delegate share prototype chain

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/b4f74f6e8396
http://hg.mozilla.org/tracemonkey/rev/c3ceee49ac37

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/b4f74f6e8396
http://hg.mozilla.org/mozilla-central/rev/c3ceee49ac37

Comment 13

[Asa Dotzler [:asa]]

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.

Comment 14

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-665355.js.