The following testcase crashes on mozilla-central revision 8883360b1edb (run with --fuzzing-safe): var SECTION = ""; gcPreserveCode() function test() { var f32 = new Float32Array(10); f32[0] = 5; var i = 0; do { f32[i + 1] = f32[i] - 1; SECTION += 1; } while (f32[i]); } test();

Crash traces: Program received signal SIGSEGV, Segmentation fault. js::Nursery::forwardBufferPointer (this=0x1831be0, pSlotsElems=0x7fffffffc3f8) at js/src/gc/Nursery.cpp:446 446 JS_ASSERT(IsWriteableAddress(*pSlotsElems)); #0 js::Nursery::forwardBufferPointer (this=0x1831be0, pSlotsElems=0x7fffffffc3f8) at js/src/gc/Nursery.cpp:446 #1 0x00000000006405f9 in UpdateIonJSFrameForMinorGC (frame=..., trc=<optimized out>) at js/src/jit/IonFrames.cpp:923 #2 js::jit::UpdateJitActivationsForMinorGC (rt=<optimized out>, trc=0x7fffffffc0c0) at js/src/jit/IonFrames.cpp:1201 #3 0x0000000000533e6b in js::Nursery::collect (this=<optimized out>, rt=0x1830e90, reason=JS::gcreason::ALLOC_TRIGGER, pretenureTypes=0x0) at js/src/gc/Nursery.cpp:777 #4 0x0000000000859774 in MinorGC (reason=JS::gcreason::ALLOC_TRIGGER, rt=0x1830e90) at js/src/jsgc.cpp:5078 #5 Collect (reason=JS::gcreason::ALLOC_TRIGGER, gckind=js::GC_NORMAL, budget=0, incremental=true, rt=0x1830e90) at js/src/jsgc.cpp:4951 #6 Collect (rt=0x1830e90, incremental=true, budget=0, gckind=js::GC_NORMAL, reason=JS::gcreason::ALLOC_TRIGGER) at js/src/jsgc.cpp:4896 #7 0x00000000007c5c48 in js::InvokeInterruptCallback (cx=0x1845a00) at js/src/jscntxt.cpp:1020 rax 0x40a00000 4647714816530579456 rdx 0x1830e90 25366160 => 0x501368 <js::Nursery::forwardBufferPointer(js::HeapSlot**)+88>: mov (%rax),%rdx Program received signal SIGSEGV, Segmentation fault. 0x00007ffff7e4d2b7 in ?? () #0 0x00007ffff7e4d2b7 in ?? () #1 0x0000000000000000 in ?? () rcx 0x0 0 r10 0x40a00000 4647714816530579456 => 0x7ffff7e4d2b7: movss (%r10,%rcx,4),%xmm0 0x7ffff7e4d2bd: ucomiss %xmm0,%xmm0 Dangerous crash with weird address, assuming sec-critical for now.

Reproduced. #0 0x00000000005e6066 in IsWriteableAddress (ptr=0x4080000040a00000) at /home/jon/work/dev/js/src/gc/Nursery.cpp:419 #1 0x00000000005e5ff5 in js::Nursery::forwardBufferPointer (this=0x1cfc198, pSlotsElems=0x7fffffffaf00) at /home/jon/work/dev/js/src/gc/Nursery.cpp:442 #2 0x0000000000830331 in js::jit::UpdateIonJSFrameForMinorGC (trc=0x7fffffffa9a0, frame=...) at /home/jon/work/dev/js/src/jit/IonFrames.cpp:924 #3 0x0000000000830167 in js::jit::UpdateJitActivationsForMinorGC (rt=0x1cfb400, trc=0x7fffffffa9a0) at /home/jon/work/dev/js/src/jit/IonFrames.cpp:1202 #4 0x00000000005e6fc8 in js::Nursery::collect (this=0x1cfc198, rt=0x1cfb400, reason=JS::gcreason::ALLOC_TRIGGER, pretenureTypes=0x0) at /home/jon/work/dev/js/src/gc/Nursery.cpp:775 #5 0x0000000000a5df20 in js::MinorGC (rt=0x1cfb400, reason=JS::gcreason::ALLOC_TRIGGER) at /home/jon/work/dev/js/src/jsgc.cpp:5091 ... 0x4080000040a00000 is the contents of the Float32Array. So there is a pointer to the array data on the JIT stack that needs updating, but there's no forwarding pointer set.

The JIT may keep pointers to the base of the array data on the stack, so we need to be able to forward these. The data can be inline in the fixed slots, so we need to add a forwarding pointer in that case. Also, make sure there are enough slots to fit the forwarding pointer in.

Comment on attachment 8404048 [details] [diff] [review] bug993768-typedArrays Review of attachment 8404048 [details] [diff] [review]: ----------------------------------------------------------------- Great! r=me

sorry had to back this out for test failure like https://tbpl.mozilla.org/php/getParsedLog.php?id=37570917&tree=Mozilla-Inbound Assertion failure: !isInside(src->getPrivate()), at /builds/slave/m-in-osx64-d-00000000000000000/build/js/src/gc/Nursery.cpp:591

The following assertion that I added in Nursery::forwardTypedArrayPointers() was failing: JS_ASSERT_IF(typedArray.buffer(), !isInside(src->getPrivate())); Which would in fact be better written as: JS_ASSERT_IF(typedArray.buffer(), !isInside(typedArray.dataPointer())) Do you know how this can happen as I didn't think we allocated array buffer contents in the nursery any more? It concerns me that whatever causes this isn't exercised by any of our tests. I'm going to do a try run with the assertion taken out, but if it is legal maybe we need to forward the buffer data too.

The problem that was causing this crash seems to be that when array buffer objects are neutered, their views are updated twice - once by neuter() and then again by changeContents(). This second update happens before the buffer's data pointer is set to the new value and results in the view's data pointers being set to a garbage value. I don't know this code too well but it seems we don't need to update the views twice in this case. Steve, does this look right to you?

Clearing need-info.

Comment on attachment 8405442 [details] [diff] [review] arraybuffer-neuter Review of attachment 8405442 [details] [diff] [review]: ----------------------------------------------------------------- Ouch. This looks like breakage from bug 982974. Bug 982974 wallpapered over a class of bugs by giving neutered objects a dummy chunk of data to point at. A second patch updated views' data pointer to point at the dummy data, which introduced this bug. Can you make the cosmetic updates in my review and then request re-review from :Waldo? I really want a second set of eyes on this. Waldo: seems like I screwed up the bug 982974 part 2 review. This is what it looks like is happening: ABO::neuter(dummyBuffer): foreach view view.data = dummyBuffer ABO::changeContents(dummyBuffer) foreach view view.data += dummyBuffer - buffer.data buffer.data = dummyBuffer So every view's data pointer gets set to dummyBuffer + dummyBuffer - the original buffer.data. Before the bug 982974 part 2 workaround, view.data would have been NULLed out and the second update would have no effect. This mangled pointer won't be accessible during normal conditions, since the view.length is zero, but with the bug 982974 hack you'll be able to read/write through it. (And GGC will stumble across it, apparently.) This seems harder to exploit, at least. Not relevant, but for the record, here is the mainline sequence of versions: 1. c9f1ddebc404 - original code 2. 355266055005 - Bug 982974 part 1 landed, making neutered ABOs point to dummy data 3. ce6a8fa5db7d - bhackett's change to store ABO metadata in the object instead of in an elements header - this reshuffled all the code, but didn't really change anything relevant to this bug 993768 - in particular, the first loop over the views in the above pseudocode used to be called neuterViews() 4. e82736761361 - additional bug 982974 fix to point views at the dummy data, introducing this bug 993768 5. 1f27124a6e08+f7ff31a0bbd3 - backout+reland of the above ::: js/src/vm/ArrayBufferObject.cpp @@ +367,5 @@ > } > } > > void > +ArrayBufferObject::changeBufferContents(JSContext *cx, void *newData) Can you change this to take a FreeOp*? Looks too scary otherwise (like it could GC or something.) I also think it should have a different name. "Changing buffer contents" implies more to me than just updating the data pointer. Though this function also juggles ownership. How about acceptOwnedData()? Kind of clunky. setNewOwnedData()? Either works for me. And assert newData != old data, at least if ownsData(), since an unconditional free makes me nervous. Alternatively, since the caller already does exactly this check, you could move the check into the top of this function and early-return if they're the same. @@ +379,5 @@ > + setDataPointer(static_cast<uint8_t *>(newData), OwnsData); > +} > + > +void > +ArrayBufferObject::changeContentsAndUpdateViews(JSContext *cx, void *newData) I think this can then just be changeContents() or changeBufferContents(). @@ +395,5 @@ > uint8_t *viewDataPointer = view->dataPointer(); > if (viewDataPointer) { > JS_ASSERT(newData); > + size_t offset = viewDataPointer - oldDataPointer; > + viewDataPointer = static_cast<uint8_t *>(newData) + offset; I don't think I like using size_t for this, since it's a signed value being stored in an unsigned type. ptrdiff_t seems more appropriate.

JSBugMon: Bisection requested, result: === Tinderbox Build Bisection Results by autoBisect === The "good" changeset has the timestamp "20140407112905" and the hash "14b5fbfa2163". The "bad" changeset has the timestamp "20140407114801" and the hash "e35851f07b67". Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=14b5fbfa2163&tochange=e35851f07b67

The first patch seems to fix some (pdf.js) JIT top crashes (under IonCannon), like this one: https://crash-stats.mozilla.com/report/index/1405738f-a727-43a9-9600-352bb2140414 (Debug build crashes in forwardBufferPointer.)

Sigh, the landing of (probably) bug 945152 bitrotted this. Still going to look somewhat, and hopefully understand things.

Comment on attachment 8406721 [details] [diff] [review] bug993768-arraybuffer-neuter v2 Review of attachment 8406721 [details] [diff] [review]: ----------------------------------------------------------------- I grok this much better than I used to. It looks good as a spot-fix. (I think we need to return to the drawing board, once all this neutering mess is fixed security-wise, and rewrite the whole algorithm to be much much more obviously simple and correct by construction. I don't think that's something to even attempt backporting, except at absolute extremes.) However, the bitrotting for bug 945152 realistically requires another pass here, I think. Post a patch that addresses that, and I'll compare it against whatever I come up with today, and hopefully we'll both be on the same page and can move on with this. Also, joy of joys, you probably should start working on branch backports. Happily, bug 945152's changes here won't get in the way of those backports. (Consider this patch r+'d as concerns the state of code a week ago, for purposes of backporting. If the backports require too many extra changes, flag sfink/me as usual, of course.) Nope, only fun will be the constant stream of changes we've made to this code over the last several cycles, ESRs, b2gs. :-\ ::: js/src/vm/ArrayBufferObject.cpp @@ +397,5 @@ > uint8_t *viewDataPointer = view->dataPointer(); > if (viewDataPointer) { > JS_ASSERT(newData); > + ptrdiff_t offset = viewDataPointer - oldDataPointer; > + viewDataPointer = static_cast<uint8_t *>(newData) + offset; Hoo-rah for getting rid of += here. Lots more understandable to be re-extracting the old offset, then applying it to the new data pointer here.

Steve, the current code in the bitrotted part looks like this: if (buffer->isMappedArrayBuffer()) buffer->changeContents(cx, nullptr); else if (newData != buffer->dataPointer()) buffer->changeContents(cx, newData); Why isn't the mapped-array-buffer arm doing the same thing the other arm is doing, in terms of allocating an equal-sized backing buffer to avoid the out-of-bounds accesses that were an issue before? Because memory? If so, we should back out (yet again, sigh) this XHR fix until we have the neutering issue *actually* fixed, with no one using stale pointers or lengths to do any indexing. In theory that's relatively soon -- patches to fix all the typed array/arraybuffer methods are in bug 991981, with only JSAPI users remaining to be fixed. So it wouldn't be so bad, I think.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #18) > Steve, the current code in the bitrotted part looks like this: > > if (buffer->isMappedArrayBuffer()) > buffer->changeContents(cx, nullptr); > else if (newData != buffer->dataPointer()) > buffer->changeContents(cx, newData); > > Why isn't the mapped-array-buffer arm doing the same thing the other arm is > doing, in terms of allocating an equal-sized backing buffer to avoid the > out-of-bounds accesses that were an issue before? Because memory? If so, > we should back out (yet again, sigh) this XHR fix until we have the > neutering issue *actually* fixed, with no one using stale pointers or > lengths to do any indexing. In theory that's relatively soon -- patches to > fix all the typed array/arraybuffer methods are in bug 991981, with only > JSAPI users remaining to be fixed. So it wouldn't be so bad, I think. You're right. Bug 999140.

Updated patch. Racing with sfink's patch in bug 999140.

Comment on attachment 8410390 [details] [diff] [review] bug993768-arraybuffer-neuter v3 Review of attachment 8410390 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, given that other patch just removes the if-mapped special case, that seems easy to rebase through.

https://hg.mozilla.org/integration/mozilla-inbound/rev/956c2f9e62ff https://hg.mozilla.org/integration/mozilla-inbound/rev/2fc0375a7dd2

https://hg.mozilla.org/mozilla-central/rev/956c2f9e62ff https://hg.mozilla.org/mozilla-central/rev/2fc0375a7dd2

JSBugMon: The testcase found in this bug no longer reproduces (tried revision c318868b33e9).

Erm, doesn't this fix need backporting/application to other branches? Nothing about the fix as I remember it, conditions it to only trunk. (In which case, it should have gotten sec-approval before landing, too.)

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #25) Ah, you're right, the typed array issue was caused by bug 982974 and is also present in 30.

Comment on attachment 8410390 [details] [diff] [review] bug993768-arraybuffer-neuter v3 [Security approval request comment] Requesting approval post landing - sorry, I should have done this before landing the patch. How easily could an exploit be constructed based on the patch? Not trivially. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? I now wish I had chosen a more subtle commit message. But either way I don't think it would be that easy to create an exploit. Which older supported branches are affected by this flaw? Version 30. If not all supported branches, which bug introduced the flaw? Bug 982974. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The same fix should apply fine. How likely is this patch to cause regressions; how much testing does it need? Unlikely.

I'm not proposing to land the GGC fix on older versions as GGC was only enabled by default in 31.

Comment on attachment 8410390 [details] [diff] [review] bug993768-arraybuffer-neuter v3 Sec-approval+ for landing.

(In reply to Al Billings [:abillings] from comment #29) > Sec-approval+ for landing. Reminder to land this patch :)

(In reply to Jan de Mooij [:jandem] from comment #30) So it turns out that this doesn't apply cleanly to the 30 branch after all :( I'm working on making a patch that does at the moment.

Here's a patch to apply the same fix to version 30. I'm requesting re-review because it's security related and the base code is fairly different. I had forgotten this had changed so much recently.

Let's get this into Beta and we're done.

JSBugMon: This bug has been automatically verified fixed.

Tagging this for QA verification once it's fixed on 30.

Please nominate for beta uplift

Comment on attachment 8418069 [details] [diff] [review] bug993768-arraybuffer-neuter-beta [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 982974 User impact if declined: Potential security issue Testing completed (on m-c, etc.): Fix on m-c for several weeks, although the fix for beta required backporting due to substantial changes in this source file Risk to taking this patch (and alternatives if risky): Low String or IDL/UUID changes made by this patch: None

Waiting for the review of the patch before uplifting it...

Waldo can we get a review here so it can be uplifted if all's good?

Comment on attachment 8418069 [details] [diff] [review] bug993768-arraybuffer-neuter-beta Review of attachment 8418069 [details] [diff] [review]: ----------------------------------------------------------------- I'm...not entirely sure that bug 999755 didn't interfere with this patch at the margins. I've stared at this several hours now and feel like *something* beta-specific is maybe off about it, or off in combination with current beta tip. But it's also 02:00 here, and I've been awake for 20ish hours now, so like it or not I'm not going to be able to push the rest of the way through this now, not confidently. Back in the morning for another go. Maybe by then a newer patch will be up, and/or you can make a convincing argument that this is sane in some edge case that's tickling my mind that I can't quite figure out right now?

Thanks for spending so much time looking at this. Bug 999755 adds a way for test code to request whether to change the buffer contents or not when neutering, and adds test code that uses it. This allows the new possibility of neutering certain types of array buffer without replacing their contents. This patch splits out the part of ArrayBufferObject::changeContents() that replaces the elements into setNewData(), leaving changeContents() to call this after updating the views. It's a straight copy so existing callers of changeContents() operate as before. setNewData() is only called in one place outside of this, which is ArrayBufferObject::neuter(), and this in turn is only called from js::NeuterArrayBuffer() and ArrayBufferObject::stealContents(), both of which previously call neuterViews() to update the views. So in these cases the views will only get updated once, which is what we want to happen. Possible interactions would have to come from not replacing the array data in places where this previously would have happened and this somehow making a difference to what happens. ArrayBufferObject::neuter() does conditionally call setNewData() (previously changeContents()) based on whether we are changing the data, but js::NeuterArrayBuffer() always updates the views regardless of whether it's replacing the data. So it looks ok to me, although I am by no means an expert on this code. Did you figure out what it was that was bothering you?

Uploaded rebased patch.

I'll look at this in a couple hours and, if it checks out, push it to beta. (I have other beta stuff to land anyway, easy to watch.) Just to keep people apprised of status here...

Sigh, forgot about this yesterday in other-backporting rush. I think I've figured out what looks screwball here. Consider ArrayBufferObject::stealContents. Its algorithm in beta, *right now*, before any patchwork here, is so: 1. Will the buffer's contents be stolen? a. If so: i. Allocate a new, zeroed header as newHeader. ii. Let transferableHeader be the old header. b. Else: i. Allocate a new header, transferableHeader. ii. Copy current data into transferableHeader. 2. Set outparams to transferableHeader info. 3. Call ArrayBufferObject::neuterViews, which will: * throw if an asm.js buffer * update pointers/lengths in all views of the buffer, invalidate JIT code * remove buffer from GC lists a bit 4. If the buffer's contents will be stolen: a. Call ArrayBufferObject::changeContents, which will * AGAIN update pointers/lengths in all views of the buffer, invalidate JIT code * clear this object's header's view list * mutate this object's elements pointer * reset the object's view list to its original value 5. Call ArrayBufferObject::neuter, which will * Check if we're stealing stealable contents * If so, call changeContents *again* and update pointers/lengths a *third* time * Else, just mutate the object's elements pointer * Zero the buffer's length * Mark the object as a neutered buffer That's three times we update pointers/lengths of the buffer's views, if the buffer's contents will be stolen. ("Three times! Ah ah ah ah!") Your patch as you say doesn't change the semantics of changeContents at all. The only semantics that change are of ArrayBufferObject::neuter. So with your patch, we will update pointers/lengths/invalidate for all views in neuterViews. If the buffer's contents will be stolen, we'll update a second time for the changeContents call. And then ArrayBufferObject::neuter will *not* update. So in the non-contents-stealing case we'll have one update, good. But in the contents-stolen case we'll still have two updates. This was a bit of a pre-existing problem, I think, that I only noticed from having read all this code way too many times across way too many branches recently. :-\ Given decreasing time and all, and your being across the pond and I think even technically on PTO right now, I'll see if I can't whip up a patch that addresses this issue as well as the one originally noted.

Or, no, not quite. In step 5, we cannot be stealing stealable contents, because the changeContents in step 4 reverted to inline element storage. And inline element storage is not stealable. So we have *either* the update in step 4 xor the update in step 5. So max two all-views pointer/length updates. Your patch eliminates the redundant update in step 5. It does not eliminate the possible second update in step 4. So there's still a problem, just not dire to quite the same degree as I diagnosed. But bad enough.

Up for review from the both of you in case one of you gets to it sooner, given the timing issues here. Without this patch, a simple test like var ab = new ArrayBuffer(256); var ta1 = new Uint8Array(ab); var ta2 = new Uint16Array(ab); serialize(ab, [ab]); will, if I look in a debugger, update view privates twice. With this patch, updating occurs only once. This passes all jstests and jit-tests (the latter with --tbpl), *including* all the tests I've curated for bug 991981, bug 999651, bug 995679, bug 1009952, and bug 1011007. But, because neuter doesn't invoke stealContents (blargh), a separate try run is probably valuable here: https://tbpl.mozilla.org/?tree=Try&rev=05d44e0f18d8 It'll be all sorts of orange for many things, to be sure, but hopefully it'll give good enough data to have reasonable landing confidence in it.

And here's the full patch, if that happens to be more readable at all.

RyanVM asked about whether this change is needed anywhere else, I think somewhat as a double-check. So let's double-check, again flagging the both of you to race on this. I see the same code pattern and issue in b2g28. But we don't need this there, because GCC isn't on in b2g28 (or even in 28 [?], for that matter). Is that right? b2g26 has a different pattern, that doesn't do any pointer-rewriting. So it doesn't need a fix along these lines. (And from recent esr24 experience, that branch has the same different pattern, so no fix is needed there, either.) (And, uh, I see bug 982974 landed in b2g26 and esr24, but we seem to be storing NULL as the data pointer for views when neutering. Which is not strictly a *problem* any more, now that bug 991981 is fixed [or just waiting approvals, in esr24's case]. But it does appear to me that the fake memory swapped into place in bug 982974 isn't actually *effective* for views of neutered buffers, in b2g26 or esr24. Blah. Such backporting. Very mistakes. Much mess. Ow.)

Comment on attachment 8430722 [details] [diff] [review] bug993768-arraybuffer-neuter-beta v2 So this is sort of okay as far as it goes, but it does need the extra hunk added to it for it to be a full fix. So, I guess f+? The moral equivalent of NS_SUCCESS_I_DID_SOMETHING here. Or something.

Comment on attachment 8432051 [details] [diff] [review] Delta atop posted patch, that gets rid of second round of view-neutering (and neutering to garbage pointers) in the case noted Review of attachment 8432051 [details] [diff] [review]: ----------------------------------------------------------------- Ah yes, I didn't spot that stealContents() was updating the views again as well. This patch takes out that update so the views should be left with the original update to newHeader by the call to neuterViews() above. So I think this is good now! GGC is not enabled in any B2G yet so we're fine on that front. Comment 12 talks about the mangled pointer being visible via the hack in bug 982974, but if that is fixed then I think there are no more issues.

Comment on attachment 8432051 [details] [diff] [review] Delta atop posted patch, that gets rid of second round of view-neutering (and neutering to garbage pointers) in the case noted [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 982974 User impact if declined: before bug 991981, crashes, conceivably sec-critical; after, potentially nothing *if* that patch is complete, but if it's not, it's probably still sec-critical Testing completed (on m-c, etc.): previous patches landed on trunk/aurora, but this patch is somewhat different enough that that maybe doesn't say a huge amount; try-push with bug 991981, bug 999651, and the rest of that lot came back clean, tho -- and between JS tests and browser tests involving stealing/transfers, that should give pretty good code coverage Risk to taking this patch (and alternatives if risky): we somehow are still screwing up transfers, with unclear consequences String or IDL/UUID changes made by this patch: N/A

Comment on attachment 8432051 [details] [diff] [review] Delta atop posted patch, that gets rid of second round of view-neutering (and neutering to garbage pointers) in the case noted OK we're taking this to RC landing because it's a sec-critical issue that has a testcase to verify with.

Removing [qa+] flag and setting verified status due to comment 34.

Setting status-firefox31 flag to match Status field.