741041 - Wrapper hygiene for typed arrays and friends

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Use UnwrapObjectChecked where possible, and ensure that array buffers and their views are in the same compartment: when creating a view, it should be created in the same compartment as the array buffer and then wrapped for the calling compartment

Comment 1

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

Attachment: Implement isProxy testing command for JS shell

Comment 2

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

Attachment: Implement isSameCompartment JS shell command for tests

Shell test function for checking if the fully unwrapped form of two objects are in the same compartment

Comment 3

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

Attachment: Use UnwrapObjectChecked instead of UnwrapObject wherever applicable, and ensure that array buffer views and their array buffers are in the same compartment

Comment 4

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

I'm not sure if  it should be isProxy or isWrapper.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 611128 [details] [diff] [review]
Implement isProxy testing command for JS shell

Review of attachment 611128 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/TestingFunctions.cpp
@@ +134,5 @@
> +        JS_ReportError(cx, "the function takes exactly one argument");
> +        return false;
> +    }
> +    if (!args[0].isObject()) {
> +        args.rval().setBoolean(true);

Perhaps setBoolean(false) ?

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 611129 [details] [diff] [review]
Implement isSameCompartment JS shell command for tests

The semantics of this function are a bit twitchy.  What about instead phrasing the test in terms of globals (which will hopefully soon have the same meaning as "compartment") by adding a "getGlobalForObject(o)" which returns JS_GetGlobalForObject?  Then you could assert things in terms of globals.

This just made me realize something about these typed array changes, though: they change visible semantics by changing the global of the typed array.  Conceivably, this breaks 'instanceof' and I think implicit 'this'?  I suppose this could work by wrapping the correct prototype (from the correct global).

Comment 7

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

(In reply to Luke Wagner [:luke] from comment #6)
> Comment on attachment 611129 [details] [diff] [review]
> Implement isSameCompartment JS shell command for tests
> 
> The semantics of this function are a bit twitchy.  What about instead
> phrasing the test in terms of globals (which will hopefully soon have the
> same meaning as "compartment") by adding a "getGlobalForObject(o)" which
> returns JS_GetGlobalForObject?  Then you could assert things in terms of
> globals.

That's a good idea. On further thought, though, I don't think this test is necessary. I have asserts in the code that would trigger if it were possible to create a cross-compartment typed array. So I think I'll just scrap this.

> This just made me realize something about these typed array changes, though:
> they change visible semantics by changing the global of the typed array. 
> Conceivably, this breaks 'instanceof' and I think implicit 'this'?  I
> suppose this could work by wrapping the correct prototype (from the correct
> global).

Uploading a new pair of patches, one to do everything but the cross-compartment case, and one to set the proto to a wrapper of the proto from the calling compartment. I won't request review until I land some other patches, though.

Comment 8

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

Attachment: patch 1 - Use UnwrapObjectChecked instead of UnwrapObject wherever applicable

Comment 9

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

Attachment: patch 2 - Ensure array buffer views and their buffers are in the same compartment

Comment 10

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

Attachment: Use UnwrapObjectChecked instead of UnwrapObject wherever applicable

Comment 11

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

Attachment: Ensure ArrayBufferViews and their ArrayBuffers are in the same compartment

Comment 12

[Luke Wagner [:luke]]

Comment on attachment 623306 [details] [diff] [review]
Use UnwrapObjectChecked instead of UnwrapObject wherever applicable

>+js_NewTypedArray(JSContext *cx, uint32_t atype, int32_t nelements);

I don't see any use/definition of this...

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 623309 [details] [diff] [review]
Ensure ArrayBufferViews and their ArrayBuffers are in the same compartment

Review of attachment 623309 [details] [diff] [review]:
-----------------------------------------------------------------

This is really pretty clean, nice job.

::: js/src/jstypedarray.cpp
@@ +1537,5 @@
> +    // compartment. constructWrapped() is invoked in the ArrayBuffer's
> +    // compartment, and is passed in a (wrapped) prototype object from the
> +    // origin compartment to use as the newly-created view's proto.
> +    static JSBool
> +    constructWrapped(JSContext *cx, unsigned argc, Value *vp)

How about just naming this fromBuffer (overload resolution should pick the right one for Proxy::nativeCall) and dropping all mention of wrappers since, technically, this is just a native that calls fromBuffer ; nothing wrapper-y about it.  Instead, the comment can go next to...

@@ +1726,5 @@
> +        if (bufobj->isWrapper()) {
> +            JSObject *wrapped = UnwrapObjectChecked(cx, bufobj);
> +            if (!wrapped)
> +                return NULL;
> +            if (wrapped->isArrayBuffer()) {

... this piece of code.  Now, instead of explicit wrapper testing, could you add a new ESClass so that this could be written:

  /*
   * Normally, NonGenericMethodGuard handles the case of transparent wrappers.
   * However, we have a peculiar situation: we want to construct the new typed
   * array in the compartment of the buffer. This is necessary since typed arrays
   * need to point directly at their buffer's data. Instead, we use the underlying
   * machinery directly to proxy the native call.
   *
  if (!ObjectClassIs(cx, *bufobj, ESClass_ArrayBuffer, cx)) {
      JS_ReportErrorNumber(cx, ...);
      return NULL;
  }

  JS_ASSERT(bufobj->isArrayBuffer() || bufobj->isProxy());
  if (bufobj->isProxy()) {
     ... Proxy::nativeCall ...
  }

  ... normal path

@@ +1741,5 @@
> +
> +                CallArgs args = CallArgsFromVp(argc, argv);
> +                if (!Proxy::nativeCall(cx, bufobj, &ArrayBufferClass, constructWrapped, args))
> +                    return NULL;
> +                return args.rval().toObjectOrNull();

Can this be &args.rval().toObject()?

@@ +2301,5 @@
>      return true;
>  }
>  
>  JSBool
> +DataViewObject::constructWrapped(JSContext *cx, unsigned argc, Value *vp)

Same comments as above, mutatis mutandis.

Comment 14

[Luke Wagner [:luke]]

Comment on attachment 623309 [details] [diff] [review]
Ensure ArrayBufferViews and their ArrayBuffers are in the same compartment

r+ with the UnwrapObject's replaced with ObjectClassIs as mentioned above.

Comment 16

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

Requested changes made. I landed the two non-test patches merged because it was easier to do the update for RootedVarObject&friends that way.

https://hg.mozilla.org/integration/mozilla-inbound/rev/df84190b1c0a
https://hg.mozilla.org/integration/mozilla-inbound/rev/73b380d3edd8

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/df84190b1c0a
https://hg.mozilla.org/mozilla-central/rev/73b380d3edd8
https://hg.mozilla.org/mozilla-central/rev/a9f05eabf20b

Comment 18

[David Mandelin [:dmandelin]]

Marking assignee according to patch author.