736609 - Malloc error with ArrayBuffer, Uint32Array and Uint8Array

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

try {
    a = ArrayBuffer(76);
    b = Uint32Array(a);
    uneval()
    c = Uint8Array(a);
    c.set(b)
} catch (e) {}

crashes js debug and opt shell on m-c changeset c7ef262e3024 with a malloc error:

Tested on 32-bit.

malloc: *** error for object 0xb74ae4: incorrect checksum for freed object - object was probably modified after being freed.
*** set a breakpoint in malloc_error_break to debug

s-s because malloc errors seem scary. A Valgrind log is attached.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

a = Int32Array(6)
Uint8Array(a.buffer).set(a)

is a testcase which causes a similar error - the testcase manifests as the following on Linux:

*** glibc detected *** ./js-opt-32-im-linux: free(): invalid next size (fast): 0x095e3a18 ***
======= Backtrace: =========
/lib32/libc.so.6(+0x6ff02)[0xf751cf02]
/lib32/libc.so.6(+0x70ba2)[0xf751dba2]
/lib32/libc.so.6(cfree+0x6d)[0xf7520c5d]
./js-opt-32-im-linux[0x81844d0]

/snip

Comment 2

[Jesse Ruderman]

autoBisect.py -p -a64 --valgrind t.js

Due to skipped revisions, the first bad revision could be any of:
changeset:   96f52c040d20
user:        Nikhil Marathe
date:        Mon Aug 01 15:26:54 2011 -0700
summary:     Bug 667047 - Ensure proper __proto__ behaviour as a normal property after setting it to null once. r=mrbkap

changeset:   794fe3408d3a
user:        Nikhil Marathe
date:        Mon Aug 01 15:49:51 2011 -0700
summary:     Bug 664249 - Inline TypedArray properties into slots. r=mrbkap

Comment 3

[Daniel Veditz [:dveditz]]

Assigning to dmandelin to find a real assignee. This flaw has been in the code a while but was recently found when we changed our fuzzers to look for more things like the flaw that was used in the Pwn2Own contest. Others may be out there looking for things like this too (at the very least, Willem and Vincenzo might be).

Comment 4

[David Mandelin [:dmandelin]]

Attachment: Patch

Great test case: it's a bad bug but very easy to spot with this test case + the memory corruption thing Visual Studio has.

Comment 5

[David Mandelin [:dmandelin]]

Alex, can I get your advice on landing this? If you look at the patch you'll see that it's *very* easy to spot what's going on here. We have some typed array feature work scheduled to land in the next few weeks that might work as cover bugs if that would be useful. I don't think we could land those bugs on the ESR though.

Comment 6

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

Comment on attachment 609492 [details] [diff] [review]
Patch

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

::: js/src/jstypedarray.cpp
@@ +1955,5 @@
>          }
>  
>          // We have to make a copy of the source array here, since
>          // there's overlap, and we have to convert types.
> +        void *srcbuf = cx->malloc_(getByteLength(tarray));

Um, yeah. Your version is better.

Comment 7

[David Mandelin [:dmandelin]]

Thanks for the quick review. I guess now we need a cover bug to land it in. Steve, do you think we can land it in one of your typed array bugs? That would cover it pretty well, but we probably don't want to land those bugs on ESR. But I'm not sure how we do land things on ESR. Do we use cover bugs, or do we just land the security fixes directly because it's the last place they go?

Comment 8

[Daniel Veditz [:dveditz]]

Cover bugs are useful sometimes, if we have something plausible at hand. For ESR and non-trunk branches (e.g. Firefox 12) you don't want to land array feature changes and I can't imagine any bug you could write that would obscure what's going on here if what's actually checked in is a one-line patch.

Landing this safe patch right at the release freeze date for Firefox 12 and ESR will have to be good enough. It's a pretty straightforward heap buffer overrun and likely to be easy to incorporate into an exploit.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

[Triage Comment]

Please nominate the patch for ESR landing as per https://wiki.mozilla.org/Release_Management/ESR_Landing_Process

Comment 10

[David Mandelin [:dmandelin]]

Comment on attachment 609492 [details] [diff] [review]
Patch

[Approval Request Comment]
Regression caused by (bug #): probably 667047 or 664249 
User impact if declined: leaves in a buffer overflow vulnerability
Testing completed (on m-c, etc.): standard js regression test suite
Risk to taking this patch (and alternatives if risky): none, AFAICT
String changes made by this patch: none

Comment 11

[Alex Keybl [:akeybl]]

Comment on attachment 609492 [details] [diff] [review]
Patch

(In reply to David Mandelin from comment #5)
> Alex, can I get your advice on landing this? If you look at the patch you'll
> see that it's *very* easy to spot what's going on here. We have some typed
> array feature work scheduled to land in the next few weeks that might work
> as cover bugs if that would be useful. I don't think we could land those
> bugs on the ESR though.

Let's move forward with landing this fix on Aurora/Beta/ESR before our code freeze date (4/13). Pre-approving for Aurora 13 and Beta 12, assuming this patch applies.

Comment 12

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/8f783fc30dd3
http://hg.mozilla.org/releases/mozilla-esr10/rev/14c0e5f4ad2a
http://hg.mozilla.org/releases/mozilla-beta/rev/07381182a1e8
http://hg.mozilla.org/releases/mozilla-aurora/rev/863691ed9f15

Comment 13

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 14

[u279076]

Could someone please point me to directions for how to create a 32-bit opt debug shell for verification?

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #14)
> Could someone please point me to directions for how to create a 32-bit opt
> debug shell for verification?

Which platform (Win/Lin/Mac 10.6/10.7)?

Comment 16

[u279076]

Mac 10.7, I guess.

Comment 17

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #16)
> Mac 10.7, I guess.

I run something like:

LD=ld CROSS_COMPILE=1 CC="clang -Qunused-arguments -fcolor-diagnostics -arch i386" RANLIB=ranlib CXX="clang++ -Qunused-arguments -fcolor-diagnostics -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments -fcolor-diagnostics" HOST_CXX="clang++ -Qunused-arguments -fcolor-diagnostics" sh ./configure --target=i386-apple-darwin8.0.0 --enable-optimize --disable-debug

in my js/src directory. (I use Clang)

(I also removed parameters that you aren't likely to need, e.g. --enable-profiling --enable-methodjit --enable-type-inference --enable-more-deterministic --disable-tests --enable-valgrind )

Comment 18

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929