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

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

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

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

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

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

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

Marking assignee according to patch author.