Closed Bug 1297667 Opened 3 years ago Closed 3 years ago

Move array buffer compartment checking assertions up to the API level

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jonco, Assigned: jonco)

Details

Attachments

(1 file)

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.
Attachment #8784332 - Flags: review?(sphink)
Attachment #8784332 - Attachment is patch: true
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.
Attachment #8784332 - Flags: review?(sphink) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7850a18f39d
Add array buffer compartment checking assertions at the API level r=sfink
https://hg.mozilla.org/mozilla-central/rev/b7850a18f39d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.