Wrapper hygiene for typed arrays and friends

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

Trunk
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
Created attachment 611128 [details] [diff] [review]
Implement isProxy testing command for JS shell
Attachment #611128 - Flags: review?(luke)
(Assignee)

Comment 2

5 years ago
Created attachment 611129 [details] [diff] [review]
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
Attachment #611129 - Flags: review?(luke)
(Assignee)

Comment 3

5 years ago
Created attachment 611130 [details] [diff] [review]
Use UnwrapObjectChecked instead of UnwrapObject wherever applicable, and ensure that array buffer views and their array buffers are in the same compartment
Attachment #611130 - Flags: review?(luke)
(Assignee)

Comment 4

5 years ago
I'm not sure if  it should be isProxy or isWrapper.
Depends on: 741040

Comment 5

5 years ago
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) ?
Attachment #611128 - Flags: review?(luke) → review+

Comment 6

5 years ago
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).

Updated

5 years ago
Attachment #611129 - Flags: review?(luke)

Updated

5 years ago
Attachment #611130 - Flags: review?(luke)
(Assignee)

Comment 7

5 years ago
(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.
Assignee: sphink → nobody
Component: JavaScript Engine → Keyboard: Navigation
QA Contact: general → keyboard.navigation
Target Milestone: --- → mozilla14
Version: unspecified → Trunk
(Assignee)

Comment 8

5 years ago
Created attachment 618474 [details] [diff] [review]
patch 1 - Use UnwrapObjectChecked instead of UnwrapObject wherever applicable
Assignee: nobody → sphink
Attachment #611129 - Attachment is obsolete: true
Attachment #611130 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 9

5 years ago
Created attachment 618475 [details] [diff] [review]
patch 2 - Ensure array buffer views and their buffers are in the same compartment
(Assignee)

Updated

5 years ago
Assignee: sphink → general
Component: Keyboard: Navigation → JavaScript Engine
QA Contact: keyboard.navigation → general
(Assignee)

Comment 10

5 years ago
Created attachment 623306 [details] [diff] [review]
Use UnwrapObjectChecked instead of UnwrapObject wherever applicable
Attachment #623306 - Flags: review?(luke)
(Assignee)

Comment 11

5 years ago
Created attachment 623309 [details] [diff] [review]
Ensure ArrayBufferViews and their ArrayBuffers are in the same compartment
Attachment #623309 - Flags: review?(luke)

Comment 12

5 years ago
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...
Attachment #623306 - Flags: review?(luke) → review+

Comment 13

5 years ago
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

5 years ago
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.
Attachment #623309 - Flags: review?(luke) → review+
(Assignee)

Updated

5 years ago
Attachment #618474 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #618475 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Duplicate of this bug: 734215
Blocks: 755122
(Assignee)

Comment 16

5 years ago
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
Target Milestone: mozilla14 → mozilla15
https://hg.mozilla.org/mozilla-central/rev/df84190b1c0a
https://hg.mozilla.org/mozilla-central/rev/73b380d3edd8
https://hg.mozilla.org/mozilla-central/rev/a9f05eabf20b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 757682
Marking assignee according to patch author.
Assignee: general → sphink
(Assignee)

Updated

5 years ago
No longer depends on: 757682
Depends on: 758398

Updated

5 years ago
Depends on: 760904
Depends on: 786903
You need to log in before you can comment on or make changes to this bug.