1297667 - Move array buffer compartment checking assertions up to the API level

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

Attachment: check-array-buffer-api

There are some compartment checking asserts in the ArrayBufferObject::stealContents and detach methods.

Steve, does it make more sense to move these up to the API level?

JS_StealArrayBufferContents calls ArrayBufferObject::stealContents after unwrapping is argument.  I think this this wrong.

However, stealContents is also called during structured cloning so we would loose the assert from that path.

I'm not sure what the best thing to do is.  Maybe we should keep both.

The patch also adds some standard API asserts for the other public functions.

Comment 1

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

Comment on attachment 8784332 [details] [diff] [review]
check-array-buffer-api

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

You also have a good point about the unwrapping in JS_StealArrayBufferContents. Well, the unwrapping seems ok, but it seems like the unwrapped compartment should be entered before stealContents is called. (Either that, or have stealContents enter the compartment. But if we eliminated stealContents as an external entry point, then there wouldn't be much difference between the two.)

::: js/src/vm/ArrayBufferObject.cpp
@@ -263,5 @@
>  /* static */ void
>  ArrayBufferObject::detach(JSContext* cx, Handle<ArrayBufferObject*> buffer,
>                            BufferContents newContents)
>  {
> -    assertSameCompartment(cx, buffer);

I would prefer to keep this one, since this method is the one that depends on the compartment being correct, and so if we ever add another path to it that doesn't go through a compartment assertion, it'll get caught before it does damage. (Previously, I hadn't thought that likely, but given that it happened with structured cloning, I'm paranoid.)

@@ -729,5 @@
>  ArrayBufferObject::stealContents(JSContext* cx, Handle<ArrayBufferObject*> buffer,
>                                   bool hasStealableContents)
>  {
>      MOZ_ASSERT_IF(hasStealableContents, buffer->hasStealableContents());
> -    assertSameCompartment(cx, buffer);

and this one I'd like to keep because it's the entry point for structured cloning. Which means this is an external API. Which is probably wrong, and structured clone should be calling JS_StealArrayBufferContents or something similar. IIRC, I did not do that because structured cloning needs to send in a different value of hasStealableContents than what JS_StealArrayBufferContents uses. So a better fix would be to add some kind of parameter that makes sense to the public API and switch to using that.

But until that is done, I'd prefer to keep the assertion here.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7850a18f39d
Add array buffer compartment checking assertions at the API level r=sfink

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b7850a18f39d