737245 - Typed Arrays should handle cross-compartment wrappers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

Currently they're broken in all sorts of ways. Working up some patches now.

Comment 1

[Bobby Holley (:bholley)]

Attachment: part 1 - Kill JS_GetTypedArrayBuffer. v1

This will do nasty things in the cross-compartment case. Thankfully, nobody uses it. Kill it.

Comment 2

[Bobby Holley (:bholley)]

Attachment: part 2 - Add some sanity-checking asserts in the typed array api. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: part 3 - Unwrap wrappers in the typed array API. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: part 4 - Repurpose TypedArray::getTypedArray to do object unwrapping. v1

TypedArray::getTypedArray currently doesn't do anything useful. All of the callers check js_IsTypedArray before calling it, so I don't see how it could ever walk up the proto chain like it purports to.

I was tempted to remove it, but I was also running into issues where things like TypedArray::getLength were borking, and this seemed like a convenient fix. I'm not sure it's the right way to go though.

Comment 5

[Bobby Holley (:bholley)]

Attachment: part 5 - Implement js_IsTypedArrayOfType and use it instead of explicit class checks. v1

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 607717 [details] [diff] [review]
part 4 - Repurpose TypedArray::getTypedArray to do object unwrapping. v1

From irc: this is not the right strategy: random unwrapping mixes objects from different compartments which is a big cross-compartment hazard.

To see some examples of what I think the "right" fix is, just grep ObjectClassIs.  The strategy is: ask "are you X type" with ObjectClassIs and then use operations that work on proxies (like ToNumber, GetProperty, etc).  A particularly relevant example is regexps where the proxy-safe operation is RegExpToShared.  I think we need to do the analogous thing here; I'm thinking something like:

  // all of these assert ObjectClassIs(obj, ESClass_TypedArray)
  uint32_t *GetTypedArrayLength(JSObject *obj);
  void *GetTypedDataOffset(JSObject *obj);
  JSObject *GetTypedArrayBuffer(JSObject *obj);

(Note that GetTypedArrayBuffer would return a wrapped ArrayBuffer if obj was a ccw.)

The last creepy possibility is when you say "new Int8Array(buffer)" where buffer in a different compartment.  Typed arrays point into the buffer of their ArrayBuffer so I think we absolutely cannot have the typed array in a different compartment from its buffer.  Thus, the only thing that would work is to create the typed array in the buffer's compartment.  Fortunately, there is already machinery to automatically call a native (in this case, TypedArrayTemplate::create) in another compartment: ProxyHandler::nativeCall.

I'm sorry this is looking so invasive, but I don't think there is any other safe way.  I would be happy to write the patch in a couple weeks once I've finished my current project.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 607718 [details] [diff] [review]
part 5 - Implement js_IsTypedArrayOfType and use it instead of explicit class checks. v1

Can you take fastClasses/slowClasses out of the TypedArray class?  mxr shows two other uses of fastClasses and I'm sure more will pop up if we don't make it unavailable.

Comment 8

[Bobby Holley (:bholley)]

sfink is going to take over this work and merge it into what he's working on, and hopefully get it landed by mid next week. Thanks Steve!

Comment 10

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

I'm effectively moving the changes from bugs 4 and 5 into bug 741041. Which means that the remaining patches in this bug are now landable.

Comment 11

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

Comment on attachment 607717 [details] [diff] [review]
part 4 - Repurpose TypedArray::getTypedArray to do object unwrapping. v1

Bug 711843 will change the JSAPI and add the checking at all relevant entry points, eliminating the use of getTypedArray outside of the internal typed array code.

Comment 12

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

Comment on attachment 607718 [details] [diff] [review]
part 5 - Implement js_IsTypedArrayOfType and use it instead of explicit class checks. v1

Bug 711843 adds a different but equivalent API for this.

Comment 13

[Mark Hammond [:markh] [:mhammond]]

Attachment: [slight bitrot] 607715: part 2 - Add some sanity-checking asserts in the typed array api. v1

Comment 15

[Mozilla RelEng Bot]

Try run for 815c44bf33eb is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=815c44bf33eb
Results (out of 231 total builds):
    success: 201
    warnings: 28
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-815c44bf33eb

Comment 16

[Mark Hammond [:markh] [:mhammond]]

These try failures all seem unrelated to the patch (the android builds seem to be failing to connect to a host 10.250.51.93 and the linux debug builds seem to have issues with hg).

Is it appropriate for me to push to inbound?

Comment 17

[Mark Hammond [:markh] [:mhammond]]

Pushed to inbound:

changeset:   91015:1cbedc2d11c6
changeset:   91016:e67ea61f46b6
changeset:   91017:17e95355ad77

Comment 18

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/1cbedc2d11c6
http://hg.mozilla.org/mozilla-central/rev/e67ea61f46b6
http://hg.mozilla.org/mozilla-central/rev/17e95355ad77

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Ehsan Akhgari [:ehsan] from comment #18)
> http://hg.mozilla.org/mozilla-central/rev/17e95355ad77

This contained a change to pymake. Please explain.

Comment 20

[Mark Hammond [:markh] [:mhammond]]

(In reply to Ms2ger from comment #19)
> (In reply to Ehsan Akhgari [:ehsan] from comment #18)
> > http://hg.mozilla.org/mozilla-central/rev/17e95355ad77
> 
> This contained a change to pymake. Please explain.

Ack - damn, sorry about that.  I was attempting to debug a failure in pymake but thought I'd kept that as a separate patch item but I screwed up.  While it should have no negative impact it should be reverted.  Should I do that (and if so, should I just to it directly to mozilla-central, or should I open a new bug, or something else)

Comment 21

[Mark Hammond [:markh] [:mhammond]]

Pushed a change to backout the pymake chunk to inbound as rev 91135:1caaf6428d93