571014 - Assertion when accessing ArrayBuffer properties on non-ArrayBuffer objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Joachim Kuebart]

Attachment: Test that causes assertion.

ArrayBuffer::fromJSObject() asserts that it's called on objects of class ArrayBuffer, but that's incorrect when an ArrayBuffer acts as a prototype to an object of a different class (see attached test).

Possible fixes are to throw an exception (I think some Array members do that), return NULL (to treat it as unset, like on the uninitialised prototype), or to search up the prototype chain to find the ArrayBuffer that triggered the invocation in the first place...

Comment 1

[Joachim Kuebart]

Attachment: Search the prototype chain for ArrayBuffer object

the third option in comment #1.

Comment 2

[Andreas Gal :gal]

This is likely exploitable. At the very least its a dos, but probably worse.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Do we need the same code in the other fromJSObject?

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 450159 [details] [diff] [review]
Search the prototype chain for ArrayBuffer object

We don't assert non-null -- if obj goes null in the loop body you'll get a clean null+4 death in obj->getClass() on the next evaluation of the loop condition.

/be

Comment 5

[Brendan Eich [:brendan]]

Joachim, thanks for finding this (I certainly should have! :-().

It looks like there's a similar bug in fun_slice, which can be called on any class of object with arbitrary prototype chain members.

/be

Comment 6

[Joachim Kuebart]

Attachment: Search prototype chain, v2

Remove NULL-assert, relying on NULL dereference, as per comment #4.

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #3)
> Do we need the same code in the other fromJSObject?

If it's used only by object ops (obj_* functions) and fun_slice, then you want something else for the object ops: assertion that we have the exact class; and for fun_slice you need a JS_InstanceOf test passing vp + 2, propagating failure.

/be

Comment 8

[Joachim Kuebart]

(In reply to comment #3)
> Do we need the same code in the other fromJSObject?

Yes, because TypedArray::fromJSObject() is used from, for example, TypedArray::prop_getBuffer.

This creates a JSCLASS_HAS_PRIVATE assertion when the object that has the TypedArray on the prototype chain has no private, or returns a bogus foreight private pointer otherwise.

What the right solution is in each case w.r.t comment #7 I'm not sure yet.

Comment 9

[Joachim Kuebart]

Attachment: JSCLASS_HAS_PRIVATE assertion when accessing Int32Array.buffer through Object.

Test case for comment #8.

Comment 10

[Joachim Kuebart]

Attachment: Assertion in TypedArray.slice().

Just for the sake of unit testing, the assertion mentioned in comment #7 triggered in Int32Array.slice().

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: like so?

This look right?

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ignore the "return true" -> "return false" change; it's bogus, I've put it back to "return true".

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 450240 [details] [diff] [review]
like so?

>@@ -823,9 +826,12 @@ class TypedArrayTemplate
>         argv = JS_ARGV(cx, vp);
>         obj = JS_THIS_OBJECT(cx, vp);
> 
>+        if (!JS_InstanceOf(cx, obj, ThisTypeArray::fastClass(), vp+2))
>+            return false;
>+
>         ThisTypeArray *tarray = ThisTypeArray::fromJSObject(obj);
>         if (!tarray)
>             return true;

The JS_InstanceOf check applies only to obj, it does not search up the proto chain for an object of the given class. So the fromJSObject is pointless, you could just reinterpret_cast obj->getPrivate(). If someone makes an object delegating to a typed array, do they want slice to operate on the proto? If so, then you really want fromJSObject to be null-defensive as it walks up, since it could walk right off. If it finds no obj of the given class then fun_slice could call JS_InstanceOf or (better) just report the same error it reports, the same way.

/be

Comment 14

[Joachim Kuebart]

(In reply to comment #13)
> (From update of attachment 450240 [details] [diff] [review])
> >@@ -823,9 +826,12 @@ class TypedArrayTemplate
> >         argv = JS_ARGV(cx, vp);
> >         obj = JS_THIS_OBJECT(cx, vp);
> > 
> >+        if (!JS_InstanceOf(cx, obj, ThisTypeArray::fastClass(), vp+2))
> >+            return false;
> >+
> >         ThisTypeArray *tarray = ThisTypeArray::fromJSObject(obj);
> >         if (!tarray)
> >             return true;
> 
> The JS_InstanceOf check applies only to obj, it does not search up the proto
> chain for an object of the given class. So the fromJSObject is pointless, you

I think the patch is ok as it is. The quoted code in fun_slice calls TypeArrayTemplate<>::fromJSObject, which still JS_ASSERTs that getClass() == fastClass() without walking the proto chain. It is TypedArray::fromJSObject which has the prototype walking code added because it is used from the prop_* methods.

> could just reinterpret_cast obj->getPrivate(). If someone makes an object
> delegating to a typed array, do they want slice to operate on the proto? If so,

I think it is counter-intuitive to modify the prototype with a delegated method, and since fun_slice in general probably can't do anything useful with the delegating object itself, it's best to throw an exception using JS_InstanceOf as in the above patch.

This patch works for me.

Joachim

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 450240 [details] [diff] [review] [details])
> > >@@ -823,9 +826,12 @@ class TypedArrayTemplate
> > >         argv = JS_ARGV(cx, vp);
> > >         obj = JS_THIS_OBJECT(cx, vp);
> > > 
> > >+        if (!JS_InstanceOf(cx, obj, ThisTypeArray::fastClass(), vp+2))
> > >+            return false;
> > >+
> > >         ThisTypeArray *tarray = ThisTypeArray::fromJSObject(obj);
> > >         if (!tarray)
> > >             return true;
> > 
> > The JS_InstanceOf check applies only to obj, it does not search up the proto
> > chain for an object of the given class. So the fromJSObject is pointless, you
> 
> I think the patch is ok as it is. The quoted code in fun_slice calls
> TypeArrayTemplate<>::fromJSObject, which still JS_ASSERTs that getClass() ==
> fastClass() without walking the proto chain. It is TypedArray::fromJSObject
> which has the prototype walking code added because it is used from the prop_*
> methods.

Oh right -- thanks.

> > could just reinterpret_cast obj->getPrivate(). If someone makes an object
> > delegating to a typed array, do they want slice to operate on the proto? If so,
> 
> I think it is counter-intuitive to modify the prototype with a delegated
> method, and since fun_slice in general probably can't do anything useful with
> the delegating object itself, it's best to throw an exception using
> JS_InstanceOf as in the above patch.

fun_slice does not modify its |this| parameter, it extracts a slice. But your point above means we don't care, so the patch is good (with the return true; fix for the case of the class prototype).

/be

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 450240 [details] [diff] [review]
like so?

With return true fix of course.

/be

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/tracemonkey/rev/b1cd46b52868

Comment 18

[Jesse Ruderman]

To keep myself from filing dups, here are the assertion failures in question.

Testcase in comment 0:

Assertion failure: obj->getClass() == &ArrayBuffer::jsclass, at ../jstypedarray.cpp:80

Testcase in comment 10:

Assertion failure: obj->getClass() == fastClass(), at ../jstypedarray.cpp:886

Comment 19

[Jesse Ruderman]

Object.create allows a more straightforward testcase:

var o = Object.create(new ArrayBuffer(1));
o.byteLength;

Comment 20

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/b1cd46b52868

Comment 21

[Steve Fink [:sfink] [:s:]]

Note that these properties have now been changed to accessors and, following WebIDL, will now throw a TypeError.