Last Comment Bug 737245 - Typed Arrays should handle cross-compartment wrappers
: Typed Arrays should handle cross-compartment wrappers
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Steve Fink [:sfink] [:s:]
:
Mentors:
: 738966 (view as bug list)
Depends on: 743000
Blocks: cpg
  Show dependency treegraph
 
Reported: 2012-03-19 16:17 PDT by Bobby Holley (busy)
Modified: 2012-04-05 17:34 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 - Kill JS_GetTypedArrayBuffer. v1 (1.19 KB, patch)
2012-03-20 14:12 PDT, Bobby Holley (busy)
luke: review+
Details | Diff | Review
part 2 - Add some sanity-checking asserts in the typed array api. v1 (6.15 KB, patch)
2012-03-20 14:12 PDT, Bobby Holley (busy)
luke: review+
Details | Diff | Review
part 3 - Unwrap wrappers in the typed array API. v1 (2.56 KB, patch)
2012-03-20 14:12 PDT, Bobby Holley (busy)
luke: review+
Details | Diff | Review
part 4 - Repurpose TypedArray::getTypedArray to do object unwrapping. v1 (909 bytes, patch)
2012-03-20 14:13 PDT, Bobby Holley (busy)
luke: review-
Details | Diff | Review
part 5 - Implement js_IsTypedArrayOfType and use it instead of explicit class checks. v1 (4.30 KB, patch)
2012-03-20 14:14 PDT, Bobby Holley (busy)
no flags Details | Diff | Review
[slight bitrot] 607715: part 2 - Add some sanity-checking asserts in the typed array api. v1 (4.56 KB, patch)
2012-04-04 02:44 PDT, Mark Hammond [:markh]
no flags Details | Diff | Review

Description Bobby Holley (busy) 2012-03-19 16:17:07 PDT
Currently they're broken in all sorts of ways. Working up some patches now.
Comment 1 Bobby Holley (busy) 2012-03-20 14:12:21 PDT
Created attachment 607714 [details] [diff] [review]
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 (busy) 2012-03-20 14:12:38 PDT
Created attachment 607715 [details] [diff] [review]
part 2 - Add some sanity-checking asserts in the typed array api. v1
Comment 3 Bobby Holley (busy) 2012-03-20 14:12:54 PDT
Created attachment 607716 [details] [diff] [review]
part 3 - Unwrap wrappers in the typed array API. v1
Comment 4 Bobby Holley (busy) 2012-03-20 14:13:36 PDT
Created attachment 607717 [details] [diff] [review]
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 (busy) 2012-03-20 14:14:02 PDT
Created attachment 607718 [details] [diff] [review]
part 5 - Implement js_IsTypedArrayOfType and use it instead of explicit class checks. v1
Comment 6 Luke Wagner [:luke] 2012-03-20 15:09:21 PDT
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] 2012-03-20 15:13:20 PDT
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 (busy) 2012-03-23 10:41:01 PDT
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 9 Masatoshi Kimura [:emk] 2012-03-24 12:38:42 PDT
*** Bug 738966 has been marked as a duplicate of this bug. ***
Comment 10 Steve Fink [:sfink] [:s:] 2012-04-02 10:47:36 PDT
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:] 2012-04-02 10:49:39 PDT
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:] 2012-04-02 10:50:56 PDT
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] 2012-04-04 02:44:44 PDT
Created attachment 612137 [details] [diff] [review]
[slight bitrot] 607715: part 2 - Add some sanity-checking asserts in the typed array api. v1
Comment 14 Mark Hammond [:markh] 2012-04-04 02:53:05 PDT
*** Bug 734215 has been marked as a duplicate of this bug. ***
Comment 15 Mozilla RelEng Bot 2012-04-04 10:31:49 PDT
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/mhammond@skippinet.com.au-815c44bf33eb
Comment 16 Mark Hammond [:markh] 2012-04-04 16:06:58 PDT
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] 2012-04-04 16:42:55 PDT
Pushed to inbound:

changeset:   91015:1cbedc2d11c6
changeset:   91016:e67ea61f46b6
changeset:   91017:17e95355ad77
Comment 19 :Ms2ger 2012-04-05 12:59:28 PDT
(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] 2012-04-05 17:11:36 PDT
(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] 2012-04-05 17:34:03 PDT
Pushed a change to backout the pymake chunk to inbound as rev 91135:1caaf6428d93

Note You need to log in before you can comment on or make changes to this bug.