1264053 - MessagePort should support transferable objects in multi-e10s

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Markus Stange [:mstange]]

Attachment: zipped addon source (requires jpm to run)

STR:
 1. Run the attached jetpack add-on using $ jpm run --binary /Applications/FirefoxNightly.app
 2. Wait for about 10 seconds (until the next GC happens)
 3. Watch the content process crash.

Here's what the addon is doing:

function transferTypedArrayToTabThroughMessageChannel(tab) {
  const browser = getBrowserForTab(viewFor(tab));
  const mm = browser.messageManager;
  mm.loadFrameScript(frameScript, true);

  const MessageChannel = viewFor(tab.window).MessageChannel;

  const channel = new MessageChannel();
  const thisSidePort = channel.port1;
  const otherSidePort = channel.port2;

  mm.sendAsyncMessage("HeresYourPort", {}, {}, undefined, [otherSidePort]);

  const typedArray = new Uint32Array([1, 3, 6]);
  thisSidePort.postMessage({}, [typedArray.buffer]);
}

The content process crashes in the JS Helper thread with this stack trace:

Thread 7 Crashed:: JS Helper
0   __pthread_kill + 10
1   pthread_kill + 90
2   abort + 129
3   free + 425
4   unsigned long js::gc::Arena::finalize<JSObject>(js::FreeOp*, js::gc::AllocKind, unsigned long) + 414
5   FinalizeArenas(js::FreeOp*, js::gc::Arena**, js::gc::SortedArenaList&, js::gc::AllocKind, js::SliceBudget&, js::gc::ArenaLists::KeepArenasEnum) + 255
6   js::gc::ArenaLists::backgroundFinalize(js::FreeOp*, js::gc::Arena*, js::gc::Arena**) + 289
7   js::gc::GCRuntime::sweepBackgroundThings(js::gc::ZoneList&, js::LifoAlloc&, js::ThreadType) + 325
8   js::GCHelperState::doSweep(js::AutoLockGC&) + 189
9   js::GCHelperState::work() + 143
10  js::HelperThread::threadLoop() + 890
11  _pt_root + 218
12  _pthread_body + 131
13  _pthread_start + 168
14  thread_start + 13

Comment 1

[Andrea Marchesini [:baku]]

#0  __GI___libc_free (mem=0x4347b40) at malloc.c:2933
#1  0x00007ff8a616cd87 in js_free (p=0x4347b40) at /home/baku/Sources/m/bugs2/build/dist/include/js/Utility.h:275
#2  0x00007ff8a61a3a6c in js::FreeOp::free_ (this=0x7ff8837fa880, p=0x4347b40) at /home/baku/Sources/m/bugs2/src/js/src/vm/Runtime.h:1643
#3  0x00007ff8a6787ed4 in js::ArrayBufferObject::releaseData (this=0x7ff828118700, fop=0x7ff8837fa880) at /home/baku/Sources/m/bugs2/src/js/src/vm/ArrayBufferObject.cpp:561
#4  0x00007ff8a6788d37 in js::ArrayBufferObject::finalize (fop=0x7ff8837fa880, obj=0x7ff828118700) at /home/baku/Sources/m/bugs2/src/js/src/vm/ArrayBufferObject.cpp:784
#5  0x00007ff8a66a3a5b in JSObject::finalize (this=0x7ff828118700, fop=0x7ff8837fa880) at /home/baku/Sources/m/bugs2/src/js/src/jsobjinlines.h:82
#6  0x00007ff8a66b53b0 in js::gc::Arena::finalize<JSObject> (this=0x7ff828118000, fop=0x7ff8837fa880, thingKind=js::gc::AllocKind::OBJECT4_BACKGROUND, thingSize=64)
    at /home/baku/Sources/m/bugs2/src/js/src/jsgc.cpp:519
#7  0x00007ff8a6698514 in FinalizeTypedArenas<JSObject> (fop=0x7ff8837fa880, src=0x7ff8837f9780, dest=..., thingKind=js::gc::AllocKind::OBJECT4_BACKGROUND, budget=..., keepArenas=js::gc::ArenaLists::KEEP_ARENAS)
    at /home/baku/Sources/m/bugs2/src/js/src/jsgc.cpp:577
#8  0x00007ff8a6678deb in FinalizeArenas (fop=0x7ff8837fa880, src=0x7ff8837f9780, dest=..., thingKind=js::gc::AllocKind::OBJECT4_BACKGROUND, budget=..., keepArenas=js::gc::ArenaLists::KEEP_ARENAS)
    at /home/baku/Sources/m/bugs2/src/js/src/jsgc.cpp:622
#9  0x00007ff8a66808b7 in js::gc::ArenaLists::backgroundFinalize (fop=0x7ff8837fa880, listHead=0x7ff82816b000, empty=0x7ff8837fa858) at /home/baku/Sources/m/bugs2/src/js/src/jsgc.cpp:3022
#10 0x00007ff8a6682249 in js::gc::GCRuntime::sweepBackgroundThings (this=0x12d4ed8, zones=..., freeBlocks=..., threadType=js::BackgroundThread) at /home/baku/Sources/m/bugs2/src/js/src/jsgc.cpp:3447
#11 0x00007ff8a66832f4 in js::GCHelperState::doSweep (this=0x12d7460, lock=...) at /home/baku/Sources/m/bugs2/src/js/src/jsgc.cpp:3694
#12 0x00007ff8a6682aae in js::GCHelperState::work (this=0x12d7460) at /home/baku/Sources/m/bugs2/src/js/src/jsgc.cpp:3577
#13 0x00007ff8a6881b81 in js::HelperThread::handleGCHelperWorkload (this=0x11d3070) at /home/baku/Sources/m/bugs2/src/js/src/vm/HelperThreads.cpp:1674
#14 0x00007ff8a6881ea9 in js::HelperThread::threadLoop (this=0x11d3070) at /home/baku/Sources/m/bugs2/src/js/src/vm/HelperThreads.cpp:1738
#15 0x00007ff8a68804f9 in js::HelperThread::ThreadMain (arg=0x11d3070) at /home/baku/Sources/m/bugs2/src/js/src/vm/HelperThreads.cpp:1348
#16 0x00007ff8aa3d9036 in _pt_root (arg=0xf75080) at /home/baku/Sources/m/bugs2/src/nsprpub/pr/src/pthreads/ptthread.c:216
#17 0x00007ff8a9fe26aa in start_thread (arg=0x7ff8837fb700) at pthread_create.c:333
#18 0x00007ff89eaf3e9d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109

Comment 2

[Andrew McCreight [:mccr8]]

Sounds like some kind of use-after-free or other badness.

Comment 3

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

It's sending a structured clone buffer between processes, and it contains a Transferable ArrayBuffer. It's crashing in the destination process when it tries to free a pointer from the source address space.

The JS stack in the source process is:

0 postMessage() ["resource://gre/modules/PromiseWorker.jsm":292]
1 next(val = [object Object],[object Object],[object Object]) ["self-hosted":1026]
    this = [object Generator]
2 TaskImpl_run(aSendResolved = true, aSendValue = [object Object],[object Object],[object Object]) ["resource://gre/modules/Task.jsm":319]
    this = [object Object]
3 bound TaskImpl_run([object Object],[object Object],[object Object]) ["self-hosted":871]
4 Handler.prototype.process() ["resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js":937]
    this = [object Object]
5 this.PromiseWalker.walkerLoop() ["resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js":816]
6 bound () ["self-hosted":829]
    this = [object Object]
7 bound bound () ["self-hosted":829]
    this = null
8 this.PromiseWalker.scheduleWalkerLoop/<(undefined) ["resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js":750]

The ArrayBuffer is 41076 bytes long in my test run, and it's plain malloc'ed data (as opposed to wasm data or mmapped data).

Hopefully that's something to go on. I have this captured in an rr recording, so let me know what else I can provide.

Comment 4

[Andrea Marchesini [:baku]]

> It's sending a structured clone buffer between processes, and it contains a
> Transferable ArrayBuffer. It's crashing in the destination process when it
> tries to free a pointer from the source address space.


Oh right. This is a problem... because all the transferable objects supported by MessagePorts are fully transferable between processes, but this is not the case for JS ArrayBuffers. I see 2 options:

1. disable ArrayBuffer transferring in MessagePort.
2. find a way to support them, somehow. SharedMemory?

SharedMemory btw would be a good optimization for several other reasons because we don't have to serialize and deserialize as much as we do currently.

Comment 5

[Andrea Marchesini [:baku]]

I and Steve discussed this issue on IRC and probably the best approach would be to copy the all ArrayBuffer in the buffer for MessagePort. I think Steve will work on this from the JS side and, once that is ready, I'll change the MessagePort code to use this new feature.

Steve, can you confirm?

Comment 6

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

Yes, that matches my understanding, thanks.

Comment 7

[Andrew Overholt [:overholt]]

Should this be assigned to you, then, Steve? (sec-crit unassigned and all ...)

Comment 8

[Andrew Overholt [:overholt]]

Moving to SM based on comment 5.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 48 and 49, sec-critical

Comment 10

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

Attachment: Prevent structured clone buffers from being used from wrong thread or wrong address space

baku, this is sort of what I'm thinking of right now. But I'm not really sure when you'd like to specify the supported scope -- it seems like it'd be nice to have JSAutoStructuredCloneBuffer always know what scope it is valid for, so I stuck it in as a required constructor parameter, but that means that StructredCloneHolderBase subclasses would need to know, and I'm not sure if you want to have to tell it so soon. Can you take a look and tell me what would work best?

This patch is incomplete. The JS shell and tests will compile with it, but the Gecko portions are only partially adapted because of the above question. Also, I'm writing the valid scope into the clone data header, which will require an IndexedDB format bump, and I haven't done that yet either.

I'd kind of rather first make JSAutoStructuredCloneBuffer look a little more like StructuredCloneHolder, and then add this stuff in, but given the nature of this bug it seems better to go for a minimal fix first.

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 8754973 [details] [diff] [review]
Prevent structured clone buffers from being used from wrong thread or wrong address space

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

I like it.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 50 too.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Steve, are you still aiming to check this patch in? Please ask for sec-approval before you do.

Comment 14

[Andrea Marchesini [:baku]]

This patch is required for e10s multi and addons using MessagePort. It's not ready to land yet.
I have to work on it. Between this week and the next I'll submit something to review.

Comment 15

[Andrea Marchesini [:baku]]

Attachment: messagePort.patch

Steve, I don't think you can review your own JS code, but I don't know who else can review the JS part.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8768469 [details] [diff] [review]
messagePort.patch

>   // If cloning is supported, this object will clone objects such as Blobs,
>   // FileList, ImageData, etc.
>   // If transferring is supported, we will transfer MessagePorts and in the
>   // future other transferrable objects.
>-  // The ContextSupport is useful to know where the cloned/transferred data can
>-  // be read and written. Additional checks about the nature of the objects
>-  // will be done based on this context value because not all the objects can
>-  // be sent between threads or processes.
>+  // The StructuredCloneScope is useful to know where the cloned/transferred
>+  // data can be read and written. Additional checks about the nature of the
>+  // objects will be done based on this context value because not all the
>+  // objects can be sent between threads or processes.
The comment keeps talking about 'context' even though you replace it with the scope thingie.
 
> // Major schema version. Bump for almost everything.
>-const uint32_t kMajorSchemaVersion = 23;
>+const uint32_t kMajorSchemaVersion = 24;
Someone else needs to review these IDB versions changes. I don't know what all it means.
Like, does it break Firefox profiles or something?


And someone else needs to review js/*

Comment 17

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

Comment on attachment 8768469 [details] [diff] [review]
messagePort.patch

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

Yeah, it's my code so I don't think I can review it. I would actually be ok with you reviewing almost all of it, but in this case the header stuff is questionable enough that I'd like jorendorff's take on it.

jorendorff, this request is really only for StructuredClone.{h,cpp}, but of course you're free to look at anything and everything.

::: dom/base/StructuredCloneHolder.h
@@ +30,5 @@
>  
>  class StructuredCloneHolderBase
>  {
>  public:
> +  typedef JS::StructuredCloneScope StructuredCloneScope;

Wouldn't |using JS::StructuredCloneScope| work? Seems more clear to me.

::: js/src/builtin/TestingFunctions.cpp
@@ +2231,5 @@
>      RootedValue deserialized(cx);
>      if (!JS_ReadStructuredClone(cx, obj->data(), obj->nbytes(),
> +                                JS_STRUCTURED_CLONE_VERSION,
> +                                JS::StructuredCloneScope::SameProcessSameThread,
> +                                &deserialized, nullptr, nullptr)) {

{ on next line in spidermonkeyland.

::: js/src/vm/StructuredClone.cpp
@@ +331,5 @@
>      SCOutput out;
>  
> +    // The (address space, thread) scope within which this clone is valid.
> +    JS::StructuredCloneScope scope;
> +    

whitespace

Comment 18

[Andrea Marchesini [:baku]]

Comment on attachment 8768469 [details] [diff] [review]
messagePort.patch

Janv, can you take a look at the IDB versioning part?

Comment 19

[Jan Varga [:janv]]

Comment on attachment 8768469 [details] [diff] [review]
messagePort.patch

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

::: dom/indexedDB/ActorsParent.cpp
@@ +4101,5 @@
>  }
>  
>  nsresult
> +UpgradeSchemaFrom23_0To24_0(mozIStorageConnection* aConnection,
> +                            const nsACString& aOrigin)

aOrigin is not used here at all, please remove it.

Comment 20

[Andrea Marchesini [:baku]]

ping?

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Too late for 48. We release tomorrow!

Comment 22

[Andrew McCreight [:mccr8]]

Does this only affect e10s with multiple content processes? Or is it a problem otherwise?

Comment 23

[Andrew McCreight [:mccr8]]

I emailed Jason about the review.

Comment 24

[Jason Orendorff [:jorendorff]]

looking.

Comment 25

[Jason Orendorff [:jorendorff]]

Comment on attachment 8768469 [details] [diff] [review]
messagePort.patch

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

r=me.

::: dom/base/StructuredCloneHolder.cpp
@@ +284,3 @@
>      for (uint32_t i = 0, len = mBlobImplArray.Length(); i < len; ++i) {
>        if (!mBlobImplArray[i]->MayBeClonedToOtherThreads()) {
>          aRv.Throw(NS_ERROR_DOM_DATA_CLONE_ERR);

Now that the writer has a scope field, this kind of error could be detected in WriteBlob(), which seems cleaner to me. It would require adding a JSAPI function to query the writer's scope field; feel free to do so.

Comment 26

[Andrea Marchesini [:baku]]

Comment on attachment 8768469 [details] [diff] [review]
messagePort.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Just addons can do this at the moment and only if e10s is enabled.
But yes, probably it's possible to exploit because we are going to read random data from a random pointer.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

no. The patch is about making MessagePort/Channel to work for e10s.

Which older supported branches are affected by this flaw?

all.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Easy to write.

How likely is this patch to cause regressions; how much testing does it need?

We change the way we write transferred arrayBuffer for e10s. It's not risky but it's a big change. It should work fine, and it has been reviewed by 4 people.

Comment 27

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8768469 [details] [diff] [review]
messagePort.patch

sec-approval+. We'll want this on Aurora and Beta once it is on trunk as well.

Comment 28

[Andrea Marchesini [:baku]]

Attachment: part 2

Comment 29

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

Comment on attachment 8781663 [details] [diff] [review]
part 2

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

::: dom/base/StructuredCloneHolder.cpp
@@ +696,5 @@
>    MOZ_ASSERT(aBlob);
>    MOZ_ASSERT(aHolder);
>  
> +  if (JS_GetStructuredCloneScope(aWriter) != JS::StructuredCloneScope::SameProcessSameThread &&
> +      !aBlob->Impl()->MayBeClonedToOtherThreads()) {

{ on its own line with there's a multi-line conditional.

::: js/src/vm/StructuredClone.cpp
@@ +458,5 @@
>      SCInput::getPair(point++, &tag, &data);
> +
> +    if (tag == SCTAG_HEADER) {
> +        SCInput::getPair(point++, &tag, &data);
> +    }

No curly brackets (sorry, SM style).

Any chance of getting a test that reliably fails without this?

Comment 30

[Al Billings [:abillings - ex-MoCo]]

Does Part 2 need to go in with the approved Part 1?

Comment 31

[Andrea Marchesini [:baku]]

Yes, it does. That patch contains the comments plus a 1 line fix for a freeing MessagePort correctly by the StructuredClone algorithm.

Comment 32

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

https://hg.mozilla.org/mozilla-central/rev/cf702debce85
https://hg.mozilla.org/mozilla-central/rev/e3096cf1de90
https://hg.mozilla.org/mozilla-central/rev/2355c90ee8c8

Comment 33

[Sylvestre Ledru [:Sylvestre]]

Andrea, could you fill the uplift requests to aurora & beta? Thanks

Comment 34

[Andrea Marchesini [:baku]]

Comment on attachment 8768469 [details] [diff] [review]
messagePort.patch

Approval Request Comment
[Feature/regressing bug #]: MessagePort + e10s
[User impact if declined]: transferring an arrayBuffer makes FF to crash.
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: the patch implements a new way to serialize arrayBuffer. It has been reviewed by many people and it's fully green in m-i/m-c. I think there are not big risks.
[String/UUID change made/needed]: none

Comment 35

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8768469 [details] [diff] [review]
messagePort.patch


Needed on aurora too. Taking it on aurora now to improve the coverage.
Liz will make the call about take it in beta 8 (or not)

Comment 36

[Liz Henry (:lizzard) (relman/hg->git project)]

Baku: Do we think this would affect people using 49, since users still won't have e10s enabled if they are using addons ? Does it affect users who aren't using multiple content processes? 
Jimm, what do you think? 
We can take this for the beta 8 build tomorrow, but it does sound risky so I want to make sure we are certain that we should.

Comment 37

[Andrea Marchesini [:baku]]

At the moment, also with e10s, single process, MessagePort without this patches still works fine.
The only problem is if an addon (running in the parent process) uses MessagePort+transferring array buffer to speak with the content process.

So, for beta, I would say, we can skip this patch.

Comment 38

[Liz Henry (:lizzard) (relman/hg->git project)]

Sounds good to me. Thanks baku!
Jimm or Dan, if you disagree and think this needs to be in beta please let me know. Just double checking since this is rated sec-critical.

Comment 39

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8768469 [details] [diff] [review]
messagePort.patch

Holding off on this patch as we are late in beta, and I think we won't need it on release until 50.

Comment 40

[Daniel Veditz [:dveditz]]

This doesn't need to be "critical" if it can't be triggered from the web directly. Skipping 49 is fine.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b6788af0c176
https://hg.mozilla.org/releases/mozilla-aurora/rev/cf237b104704
https://hg.mozilla.org/releases/mozilla-aurora/rev/5aec8a7d79ca

Comment 42

[Alexandru Simonca [:asimonca], Release Desktop QA]

Hello,
I have been trying to verify this bug but I don't really know if I'm doing it right. I'll detail the steps I went through and, hopefully, someone can correct me if I'm wrong.
First of all I am using WINDOWS 10 x64 with Command Prompt to launch the build with add-on from the attachments.

1. I added "JPM_FIREFOX_BINARY" to the "Environment Variables" in Windows with the path to the version of Firefox that I was testing and ran the command "jpm run" in a command prompt window opened from the add-on folder that I downloaded from the attachments. 

2. Beta 50.0b11 does not let the add-on to be added to the build due to the fact that it is not signed. Turning the pref "xpinstall.signatures.required" to "false" does not solve this issue. 

I used another 50 build instead: Firefox Aurora 50.0a2 (id: 20160826004001) and if the above pref is set to "false" a tab is opened by the add-on and that tab crashes. See the message here: http://imgur.com/a/Uwi9W

3. Aurora 51.0a2 (latest) lets the add-on to be added to the build if the pref "xpinstall.signatures.required" is set to "false" but as in the previous point, the tab crashes after some time (~10 seconds). 

So, this is either not fixed or I'm not doing it right. Markus do you have any tips?

Comment 43

[Markus Stange [:mstange]]

I just tested the addon in Nightly and can confirm that the tab still crashes.

Comment 44

[Andrea Marchesini [:baku]]

Attachment: crash_mp.patch

Luke, can you take a look? This is a test where this crash is reproducible 100% (e10s enabled, of course).
The crash happens in here:

#0  arena_dalloc (ptr=ptr@entry=0x7f543c32a520, offset=offset@entry=173344) at /home/baku/Sources/m/bugs2/src/memory/mozjemalloc/jemalloc.c:4735
#1  0x000000000041b476 in je_free (ptr=0x7f543c32a520) at /home/baku/Sources/m/bugs2/src/memory/mozjemalloc/jemalloc.c:6485
#2  0x00007f581d33854c in js_free (p=<optimised out>) at /home/baku/Sources/m/bugs2/build/dist/include/js/Utility.h:257
#3  js::FreeOp::free_ (this=<optimised out>, p=<optimised out>) at /home/baku/Sources/m/bugs2/src/js/src/vm/Runtime.h:1356
#4  js::ArrayBufferObject::releaseData (this=0x7f57f00e4580, fop=<optimised out>) at /home/baku/Sources/m/bugs2/src/js/src/vm/ArrayBufferObject.cpp:840
#5  0x00007f581d2b1183 in js::Class::doFinalize (this=<optimised out>, obj=0x7f57f00e4580, fop=0x7f5805af67d0) at /home/baku/Sources/m/bugs2/build/dist/include/js/Class.h:814
#6  JSObject::finalize (this=this@entry=0x7f57f00e4580, fop=fop@entry=0x7f5805af67d0) at /home/baku/Sources/m/bugs2/src/js/src/jsobjinlines.h:87

Comment 45

[Luke Wagner [:luke]]

Reading the code in JSStructuredCloneWriter::transferOwnership(), I'm not sure how this is supposed to possibly work across a process boundary: on the sender's side, if scope == DifferentProcess, we go ahead and malloc() a clone of the buffer and then write a *pointer* to that buffer into the structured-clone-buffer that we send to the other process.

Comment 46

[Luke Wagner [:luke]]

Comment on attachment 8809042 [details] [diff] [review]
crash_mp.patch

(Clearing since baku says this isn't critical to fix before sfink can get back from PTO)

Comment 47

[Marcia Knous [:marcia]]

Tracking 53+.

Comment 48

[Gerry Chang [:gchang]]

Track 51+/52+ as sec-high.

Comment 49

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

Hah! Yes, this was a total brainfart on my part. If you are DifferentProcess, this just says "ooh, you can't share this pointer, so let's make a copy and give you a different, equally-invalid pointer. Now instead of just crashing, you can leak too!"

Oops.

This should copy the data, almost as if you weren't transferring at all, but still detach the source buffer.

Comment 50

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

Attachment: Transfer DifferentProcess ArrayBuffers by copying

Ok, this seems more reasonable. And it has tests.

The basic idea is that when Transferring ArrayBuffers between processes, mostly behave like you're not Transferring them, except detach the origin AB after you've copied out its data. I store a slot in the transferable map at the beginning of the clone buffer, both because it makes it easy to know what to detach and because it keeps the indexing the same between that map and the transferable list passed in.

This patch also comments and clarifies the various meanings of the scope. When writing, it determines how to serialize certain objects. That scope is stored within the buffer so the reader knows how to interpret the buffer. When reading, you must specify what set of scopes are *allowed* (so if you're reading a clone buffer in a child process, you pass in DifferentProcess to reject any other buffers that might somehow reach you. It should perhaps be an assertion rather than a dynamic check.) But also, during reading some things need to be interpreted differently based on the scope of the buffer itself (which is NOT necessarily the same as the allowed scope.)

Comment 51

[Jason Orendorff [:jorendorff]]

Comment on attachment 8816379 [details] [diff] [review]
Transfer DifferentProcess ArrayBuffers by copying

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

Looks good, modulo a few comments.

::: js/src/builtin/TestingFunctions.cpp
@@ +2407,5 @@
> +            auto scope = ParseCloneScope(cx, str);
> +            if (!scope) {
> +                JS_ReportErrorASCII(cx, "Invalid structured clone scope");
> +                return false;
> +            } else {

else after return

::: js/src/tests/js1_8_5/extensions/clone-transferables.js
@@ +4,5 @@
>  
> +function* buffer_options() {
> +  for (var scope of ["SameProcessSameThread", "SameProcessDifferentThread", "DifferentProcess"]) {
> +    for (var size of [0, 8, 16, 200, 1000, 4096, 8192, 65536]) {
> +      yield {scope: scope, size: size};

`yield {scope, size};` would work.

@@ +14,4 @@
>  function test() {
> +    for (var {scope, size} of buffer_options()) {
> +        var old = new ArrayBuffer(size, { scope: scope });
> +        var copy = deserialize(serialize([old, old], [old]));

Is this testing the right thing? It seems like the `{scope}` options object should be passed to serialize and deserialize, not ArrayBuffer.

@@ +40,5 @@
>              assertEq(buf, old_arr.buffer);
>              if (!dataview)
>                  assertEq(old_arr.length, size / old_arr.BYTES_PER_ELEMENT);
>  
> +            var copy_arr = deserialize(serialize(old_arr, [ buf ], { scope: scope }));

Again, it seems like the scope option should be passed to both functions.

::: js/src/vm/StructuredClone.cpp
@@ +318,5 @@
>      SCInput& in;
>  
> +    // What scopes the caller can accept (eg if you can accept same thread
> +    // clone buffers, you can definitely accept different process clone
> +    // buffers.)

It'd be nice for this comment to be better, but thanks for adding a comment in the first place.

@@ +1512,2 @@
>              // The current setup of the array buffer inheritance hierarchy doesn't
>              // lend itself well to generic manipulation via proxies.

While I'm wishing, I also wish I understood this comment.

@@ +1523,5 @@
> +            if (scope == JS::StructuredCloneScope::DifferentProcess) {
> +                ownership = JS::SCTAG_TMO_UNOWNED;
> +                content = nullptr;
> +                extraData = 0;
> +                JS_DetachArrayBuffer(cx, arrayBuffer);

Just from casual inspection of JS_DetachArrayBuffer(), it looks like it will set an exception pending in a weird case involving Wasm. If that is really possible, it seems wrong to ignore the error here. We have to at least clear it.

Comment 52

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

(In reply to Jason Orendorff [:jorendorff] from comment #51)
> Comment on attachment 8816379 [details] [diff] [review]
> Transfer DifferentProcess ArrayBuffers by copying
> 
> ::: js/src/tests/js1_8_5/extensions/clone-transferables.js
> @@ +4,5 @@
> >  
> > +function* buffer_options() {
> > +  for (var scope of ["SameProcessSameThread", "SameProcessDifferentThread", "DifferentProcess"]) {
> > +    for (var size of [0, 8, 16, 200, 1000, 4096, 8192, 65536]) {
> > +      yield {scope: scope, size: size};
> 
> `yield {scope, size};` would work.

Ooh, shiny. Applied to a few more places too.

> @@ +14,4 @@
> >  function test() {
> > +    for (var {scope, size} of buffer_options()) {
> > +        var old = new ArrayBuffer(size, { scope: scope });
> > +        var copy = deserialize(serialize([old, old], [old]));
> 
> Is this testing the right thing? It seems like the `{scope}` options object
> should be passed to serialize and deserialize, not ArrayBuffer.

Ugh. I started out with this completely wrong, ran some tests to verify I was passing the different scope through, fixed one such place and saw that I was getting a DifferentProcess scope at least once in the whole test, and called it good -- with the end results of having it just be mostly wrong.

Fixed in all the places in this file.

> ::: js/src/vm/StructuredClone.cpp
> @@ +318,5 @@
> >      SCInput& in;
> >  
> > +    // What scopes the caller can accept (eg if you can accept same thread
> > +    // clone buffers, you can definitely accept different process clone
> > +    // buffers.)
> 
> It'd be nice for this comment to be better, but thanks for adding a comment
> in the first place.

I rewrote the comment. Whether it's now better, I'm not entirely sure.

> @@ +1512,2 @@
> >              // The current setup of the array buffer inheritance hierarchy doesn't
> >              // lend itself well to generic manipulation via proxies.
> 
> While I'm wishing, I also wish I understood this comment.

Added by bholley in bug 1050340. I guess he wished he didn't have to unwrap to do anything with ABOs? I'm not sure I've ever really understood this comment.

> @@ +1523,5 @@
> > +            if (scope == JS::StructuredCloneScope::DifferentProcess) {
> > +                ownership = JS::SCTAG_TMO_UNOWNED;
> > +                content = nullptr;
> > +                extraData = 0;
> > +                JS_DetachArrayBuffer(cx, arrayBuffer);
> 
> Just from casual inspection of JS_DetachArrayBuffer(), it looks like it will
> set an exception pending in a weird case involving Wasm. If that is really
> possible, it seems wrong to ignore the error here. We have to at least clear
> it.

Bleh. That unchecked function call is quite bold, isn't it?

I don't believe it is possible for it to fail as currently written. The immediately preceding code checks IsArrayBufferObject and !isWasm. But I'll propagate errors anyway. I wonder if a previously iteration had a void return? More likely I'm just an idiot.

Comment 53

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

Ouch. Fixing the parameters to the test showed up a nasty problem. DifferentProcess clones now match the latest html5 spec, but other clones (and the previous behavior) do not. Specifically, consider:

  let ab = new ArrayBuffer(8);
  let view = new Int32Array(ab);
  view[0] = 1;
  view[1] = 2;
  let pre_mutator = { get foo() { view[0] = 10; } };
  let post_mutator = { get foo() { view[1] = 20; } };
  let clone = structured_clone([pre_mutator, ab, post_mutator], [ab]);
  let new_view = new Int32Array(clone[1]);

As I read the current spec, new_view[0] and new_view[1] should reflect both mutations (so would be 10 and 20), since:

1. The initial Transferable parsing just records the identity of objects
2. The cloning pass does pre_mutator
3. The cloning pass sees it already knows about the AB
4. The cloning pass does post_mutator
5. Only then does the final Transferable pass grab out the internal data

The existing implementation follows this spec, as does the new behavior with scope=SameProcessSameThread. Unfortunately, with DifferentProcess, my posted implementation would result in new_view=[10,2], because in step 3 above it would *not* know about the AB, and would clone it in place with the contents after pre_mutator but before post_mutator.

Comment 54

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

(In reply to Steve Fink [:sfink] [:s:] from comment #53)
> Ouch. Fixing the parameters to the test showed up a nasty problem.
> DifferentProcess clones now match the latest html5 spec, but other clones
> (and the previous behavior) do not. Specifically, consider:

Oops, sorry. That summary is backwards. Forgot to go back to change it. DifferentProcess after my patch does *not* match the spec, but other clones and the previous behavior do.

Comment 55

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

Attachment: Transfer DifferentProcess ArrayBuffers by copying

Sorry for the re-review request, but the previous approach just won't work with the spec semantics.

I am aware that this patch is not beautiful. :-( The basic problem is that we need to write out the Transferred ArrayBuffer at the very end, so that it will reflect any nasty mutations that happened during cloning. I now have it just appending the ArrayBuffer at the end of the clone buffer -- but it must be read in *first*, so it requires a forward pointer from the transfer map entry to where the actual data is stored. Sadly, structured clone has been rebased on top of segmented BufferLists, and those have problems with invalidating iterators if the underlying BufferList is written to. Hence this patch.

The changes are restricted to StructuredClone.cpp, mfbt/BufferList.h, and the tests. I can get someone else to review the BufferList.h part, if you're ok with this approach in the first place. (There are several other options; I could avoid the iterator invalidation by collecting the things to be written at the end and only writing them when everything else is done, but I'd still need to seek back to fill in the forward pointers, and the BufferList iterator stuff doesn't support that either.)

Comment 56

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

Attachment: Transfer DifferentProcess ArrayBuffers by copying

I just glanced at this patch and noticed I had left in a comment describing an alternate approach that I ended up not using. Commented with the approach implemented, instead. What a concept.

Comment 57

[Jason Orendorff [:jorendorff]]

Comment on attachment 8828527 [details] [diff] [review]
Transfer DifferentProcess ArrayBuffers by copying

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

::: js/src/vm/StructuredClone.cpp
@@ +288,5 @@
>      bool getPair(uint32_t* tagp, uint32_t* datap);
>  
> +    const BufferIterator& tell() const { return point; }
> +    void seekTo(const BufferIterator& pos) { point = pos; }
> +    void seek(size_t pos) { point += pos; }

`seekTo` and `seekBy`?

@@ +1548,5 @@
> +            if (scope == JS::StructuredCloneScope::DifferentProcess) {
> +                // Write Transferred ArrayBuffers in DifferentProcess scope at
> +                // the end of the clone buffer, and store the offset within the
> +                // buffer to where the ArrayBuffer was written. Note that this
> +                // will invalidate the current position iterator.

It looks like both the reader and the writer have to do some slightly tricky "seeking" for this. Why not write the data "here", in the middle of this table? It's not like the rest of this format is really all fixed-width and tabular.

Just a thought -- don't change it if it seems unnecessary.

@@ +1557,5 @@
> +                content = nullptr;
> +                extraData = out.tell() - pointOffset; // Offset from tag to current end of buffer
> +                if (!writeArrayBuffer(arrayBuffer))
> +                    return false;
> +                // Must refresh the point iterator after its collection has

Nit: blank line before comment

::: mfbt/BufferList.h
@@ +218,5 @@
>        }
>        return true;
>      }
>  
> +    size_t BytesUntil(const BufferList& aBuffers, const IterImpl& aTarget) const {

This seems more like a method for BufferList than IterImpl. `bufList.RangeLength(start, stop)`.

Either way, it needs a comment. There's a contract beyond what the types enforce.

@@ +229,5 @@
> +        data = aBuffers.mSegments[++segment].mData;
> +      }
> +
> +      MOZ_RELEASE_ASSERT(segment == aTarget.mSegment);
> +      MOZ_RELEASE_ASSERT(aTarget.mData >= data);

These assertions are robust against the most likely mistakes but not against errors mismatching `aBuffers` and `this` or `aBuffers` and `aTarget`. We could check:

    private:
      bool IsIn(const BufferList& aBuffers) const {
        return mSegment < aBuffers.mSegments.length() &&
               mData >= aBuffers.mSegments[mSegment].mData &&
               mData < aBuffers.mSegments[mSegment].End();
      }

and then assert `this->IsIn(aBuffers)` and `aTarget.isIn(aBuffers)`.

Comment 58

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

(In reply to Jason Orendorff [:jorendorff] from comment #57)
> Comment on attachment 8828527 [details] [diff] [review]
> Transfer DifferentProcess ArrayBuffers by copying
> 
> Review of attachment 8828527 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/StructuredClone.cpp
> @@ +288,5 @@
> >      bool getPair(uint32_t* tagp, uint32_t* datap);
> >  
> > +    const BufferIterator& tell() const { return point; }
> > +    void seekTo(const BufferIterator& pos) { point = pos; }
> > +    void seek(size_t pos) { point += pos; }
> 
> `seekTo` and `seekBy`?

Yes, that's better, thanks.

> @@ +1548,5 @@
> > +            if (scope == JS::StructuredCloneScope::DifferentProcess) {
> > +                // Write Transferred ArrayBuffers in DifferentProcess scope at
> > +                // the end of the clone buffer, and store the offset within the
> > +                // buffer to where the ArrayBuffer was written. Note that this
> > +                // will invalidate the current position iterator.
> 
> It looks like both the reader and the writer have to do some slightly tricky
> "seeking" for this. Why not write the data "here", in the middle of this
> table? It's not like the rest of this format is really all fixed-width and
> tabular.

The writer doesn't really seek with this, but it has to work around the limitation that even appending to a buffer invalidates iterators. I should perhaps fix that instead, or encapsulate the workaround in the iterator.

Though saying it doesn't seek is a little deceptive; the previous code fills in a pointer in the transfer map, the new code fills in an offset in the DifferentProcess case instead (which is kind of an internal pointer.) But it's more "random write" than "seek".

> ::: mfbt/BufferList.h
> @@ +218,5 @@
> >        }
> >        return true;
> >      }
> >  
> > +    size_t BytesUntil(const BufferList& aBuffers, const IterImpl& aTarget) const {
> 
> This seems more like a method for BufferList than IterImpl.
> `bufList.RangeLength(start, stop)`.

Hm, I hadn't considered that.

> Either way, it needs a comment. There's a contract beyond what the types
> enforce.

Ok.

> @@ +229,5 @@
> > +        data = aBuffers.mSegments[++segment].mData;
> > +      }
> > +
> > +      MOZ_RELEASE_ASSERT(segment == aTarget.mSegment);
> > +      MOZ_RELEASE_ASSERT(aTarget.mData >= data);
> 
> These assertions are robust against the most likely mistakes but not against
> errors mismatching `aBuffers` and `this` or `aBuffers` and `aTarget`. We
> could check:
> 
>     private:
>       bool IsIn(const BufferList& aBuffers) const {
>         return mSegment < aBuffers.mSegments.length() &&
>                mData >= aBuffers.mSegments[mSegment].mData &&
>                mData < aBuffers.mSegments[mSegment].End();
>       }
> 
> and then assert `this->IsIn(aBuffers)` and `aTarget.isIn(aBuffers)`.

Ok.

Comment 59

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

(In reply to Steve Fink [:sfink] [:s:] from comment #58)
> (In reply to Jason Orendorff [:jorendorff] from comment #57)
> > @@ +1548,5 @@
> > > +            if (scope == JS::StructuredCloneScope::DifferentProcess) {
> > > +                // Write Transferred ArrayBuffers in DifferentProcess scope at
> > > +                // the end of the clone buffer, and store the offset within the
> > > +                // buffer to where the ArrayBuffer was written. Note that this
> > > +                // will invalidate the current position iterator.
> > 
> > It looks like both the reader and the writer have to do some slightly tricky
> > "seeking" for this. Why not write the data "here", in the middle of this
> > table? It's not like the rest of this format is really all fixed-width and
> > tabular.
> 
> The writer doesn't really seek with this, but it has to work around the
> limitation that even appending to a buffer invalidates iterators. I should
> perhaps fix that instead, or encapsulate the workaround in the iterator.
> 
> Though saying it doesn't seek is a little deceptive; the previous code fills
> in a pointer in the transfer map, the new code fills in an offset in the
> DifferentProcess case instead (which is kind of an internal pointer.) But
> it's more "random write" than "seek".

Oh! Sorry, I didn't actually respond to the initial question.

I started writing it like that, but the API supports custom transfer callbacks, and those are only allowed to write into the fixed-size fields. (Or limit custom transfers to be able to do less than the built-in ones. Which still isn't a totally solid argument, because I didn't extend the API enough for a custom transfer to implement what is in this bug, either.)

More importantly, the transfer map is written at the beginning, and it is easier to do so because regularly cloned data wants to be able to use backpointers to transferred objects. But the DifferentProcess ABO data cannot be written at the beginning, because the spec requires the contents to reflect the data in the ArrayBuffer *after* the regular data has been cloned. (Transferring happens at the end, per spec.) So I'd have to reserve the right amount of space, then seek back and write it in later. Which is more seeking than the not-really-seeking that the write is doing with this patch.

Comment 60

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

Comment on attachment 8828527 [details] [diff] [review]
Transfer DifferentProcess ArrayBuffers by copying

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It's not straightforward, but it's much easier with multi-e10s than it was with a single content process. I would say it's not likely to cause too much trouble before multi-e10s, since you'd probably need an addon to trigger it.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Not really. It's certainly possible to figure out from the patch, but the code and the comments sound like they're dealing with a correctness problem in a particular configuration, rather than a security problem.

Which older supported branches are affected by this flaw?

It goes way back, though it hasn't been highly relevant for long.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This patch should apply pretty well back to 51, which is when the BufferList was introduced in bug 1264642. It would require a rewritten (but much simpler!) patch before then.

How likely is this patch to cause regressions; how much testing does it need?

It's not a simple change, but most problems would break things badly and would be easy to detect.

Comment 61

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

Attachment: Transfer DifferentProcess ArrayBuffers by copying

Patch updated for review comments.

Comment 62

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

Comment on attachment 8831345 [details] [diff] [review]
Transfer DifferentProcess ArrayBuffers by copying

Sorry, I realized I should upload the final patch for eventual backporting. See comment 60 for sec-approval request.

Carrying forward jorendorff's r+.

Comment 63

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8831345 [details] [diff] [review]
Transfer DifferentProcess ArrayBuffers by copying

sec-approval+ for trunk.

Comment 64

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

Comment on attachment 8831345 [details] [diff] [review]
Transfer DifferentProcess ArrayBuffers by copying

Approval Request Comment
[Feature/Bug causing the regression]: e10s-multi or addons using cross-process MessagePorts
[User impact if declined]: crashes
[Is this code covered by automated tests?]: yes, there are JS shell tests that go along with this fix
[Has the fix been verified in Nightly?]: not yet, just landed today
[Needs manual test from QE? If yes, steps to reproduce]: see comment 0
[List of other uplifts needed for the feature/fix]: none. This patch should apply back to 51. I don't think we would want to uplift the prerequisite before that; it'd be better to rewrite the patch, but I don't think backporting that far is necessary.
[Is the change risky?]: somewhat
[Why is the change risky/not risky?]: it's a bunch of new logic in structured cloning, though it will only apply in the problematic case (cross-process cloning). It shouldn't change anything in regular cases, which reduces the risk quite a bit. (But the behavior it's replacing is about as wrong as you can get, so there shouldn't be much risk of this making anything worse.)
[String changes made/needed]: none

Comment 65

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

https://hg.mozilla.org/mozilla-central/rev/468f75ba2034

Comment 66

[Julien Cristau [:jcristau]]

Comment on attachment 8831345 [details] [diff] [review]
Transfer DifferentProcess ArrayBuffers by copying

let's get this in aurora and beta before 52.0b3

Comment 67

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4aa072b1110c

Comment 68

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/294e61b943c4

Comment 69

[Gerry Chang [:gchang]]

Mark 51 as won't fix as 51 was released.

Comment 70

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial crash issue using old Nightly from 2017-01-29, verified that builds after the fix does not crash when loading the testcase provided in comment 0. Tested using Firefox 52.0 RC add-on devel, latest Nightly 54.0a1 and latest Developer Edition 53.0a2 on macOS 10.12.3 and Ubuntu 14.04 x64.