939472 - "Assertion failure: view" with gcslice and neuter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

var a = new Uint8Array();
a.subarray();
gcslice(0);
gcslice(1781);
neuter(a.buffer);

Assertion failure: view, at js/src/vm/TypedArrayObject.cpp:824

Using the trick in bug 929786 comment 4, the first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/0efef923cc17
user:        Steve Fink
date:        Tue Oct 15 23:48:01 2013 -0700
summary:     Bug 861925 - Allow grabbing data from ArrayBuffers and neutering them independently (in addition to Steal, which does both at the same time). r=Waldo

Comment 1

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

Attachment: Remove buffer from multiview list when neutered

We maintain a list of ArrayBuffers with more than view. Sadly, we do not remove them from the list when neutering.

"We", here, is pronounced "sfink".

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8334141 [details] [diff] [review]
Remove buffer from multiview list when neutered

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

::: js/src/vm/TypedArrayObject.cpp
@@ +350,5 @@
>          if (!ArrayBufferObject::neuterAsmJSArrayBuffer(cx, *this))
>              return false;
>      }
>  
> +    // remove buffer from the list of buffers with > 1 view

Please capitalize and add a period at the end.

Comment 3

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/335ec0f81ead

Comment 4

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

This needed a rating before it went in and since it affects Aurora and Trunk both, it needed security approval. 

See https://wiki.mozilla.org/Security/Bug_Approval_Process.

Can you suggest a security rating and answer the sec-approval? template questions?

Comment 5

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

Comment on attachment 8334141 [details] [diff] [review]
Remove buffer from multiview list when neutered

Requesting sec-approval, but on further reflection, I don't think this is sensitive at all.

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

It should be fairly easy to cause a near-NULL crash. I have no idea how to do anything more than that. I don't see how it's possible.

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

It's pretty clear from the patch what the actual code flaw and how to construct a scenario that hits it. It's not very clear why it's a security problem.

Which older supported branches are affected by this flaw?

Everything since Firefox 18.

If not all supported branches, which bug introduced the flaw?

Bug 789295

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

The code would be slightly different, but just because different helper functions were introduced at some point. The behavior would be the same.

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

The situation where this arises is uncommon, so if it does introduce regressions, they won't hit very often in normal usage.

To be specific, to hit this bug you would need to:

1. Create an ArrayBuffer with multiple typed array views
2. Do a postMessage that transfers the ArrayBuffer
3. Timing it so that step #2 happens between incremental GC slices

The result is an ArrayBuffer with zero views on the list of "multiview" ArrayBuffers. Since the list is threaded through each buffer's first view, that means that when traversing the list, it'll dereference the address 0x8 or 0x10 or something like that. The exact offset is fixed; the user cannot interfere with it. (The list is only traversed during sweeping. So it would happen at the end of the current incremental GC.)

Comment 6

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

Comment on attachment 8334141 [details] [diff] [review]
Remove buffer from multiview list when neutered

I agree with Andrew. This sounds like a sec-low so you're all good to go. Low and Medium rated issues don't need approval.

Comment 7

[Carsten Book [:Tomcat]]

landed on https://hg.mozilla.org/mozilla-central/rev/335ec0f81ead

Comment 8

[David Chan [:dchan]]

Per comment 5, this bug has been around since fx18

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Steve, can you please nominate this for uplift?

Comment 10

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

Comment on attachment 8334141 [details] [diff] [review]
Remove buffer from multiview list when neutered

Sorry for the premature landing.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 789295
User impact if declined: crashes
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): similar crashes, if we missed something in the logic
String or IDL/UUID changes made by this patch: none

Comment 11

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

For a sec-low, it may be a bit late in the cycle to take churn. I can see approving this for Aurora but probably not Beta.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ab93ba38200e

Do we want to take this on the b2g branches?

Comment 13

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

Comment on attachment 8334141 [details] [diff] [review]
Remove buffer from multiview list when neutered

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 789295
User impact if declined: crashes
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): similar crashes, if we missed something in the logic
String or IDL/UUID changes made by this patch: none

It might make sense to land this for b2g, since I think they use workers a bit more and may be more likely to encounter this issue? I don't really know though.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Preeti, do we want this on b2g18 as well?

Steve, can you please make at least a b2g26 patch?

Comment 15

[Preeti Raghunath(:Preeti)]

Ryan

This is not needed for 1.1HD, can track it for 1.1.1 though.

Comment 16

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

Attachment: Remove buffer from multiview list when neutered.

Patch for b2g26.

Comment 17

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

http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/9db448e74615 - backed out a87f1f662a6c because I missed an important clause in the original patch
http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/a87f1f662a6c - relanded

Comment 18

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

https://hg.mozilla.org/releases/mozilla-b2g18/rev/d703f1b991f2

Comment 19

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

Followup fix https://hg.mozilla.org/releases/mozilla-b2g18/rev/7ef4780f267f (I seem to be incapable of doing trivial rebases)

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Turns out v1.1hd is still taking security fixes after all.

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/d703f1b991f2
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/7ef4780f267f