Last Comment Bug 736609 - Malloc error with ArrayBuffer, Uint32Array and Uint8Array
: Malloc error with ArrayBuffer, Uint32Array and Uint8Array
Status: VERIFIED FIXED
[sg:critical][qa?]
: crash, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- critical (vote)
: mozilla14
Assigned To: David Mandelin [:dmandelin]
:
Mentors:
Depends on:
Blocks: jsfunfuzz 664249
  Show dependency treegraph
 
Reported: 2012-03-16 14:38 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2013-01-19 14:16 PST (History)
11 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
+
fixed
12+
fixed


Attachments
stack (10.32 KB, text/plain)
2012-03-16 14:38 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
Patch (1004 bytes, patch)
2012-03-26 14:30 PDT, David Mandelin [:dmandelin]
sphink: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Gary Kwong [:gkw] [:nth10sd] 2012-03-16 14:38:40 PDT
Created attachment 606742 [details]
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] 2012-03-18 01:41:18 PDT
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 2012-03-18 04:23:35 PDT
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] 2012-03-22 13:20:26 PDT
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] 2012-03-26 14:30:50 PDT
Created attachment 609492 [details] [diff] [review]
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] 2012-03-26 14:33:31 PDT
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:] 2012-03-26 14:47:02 PDT
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] 2012-03-26 17:08:23 PDT
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] 2012-03-26 17:39:21 PDT
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 2012-04-06 15:02:48 PDT
[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] 2012-04-09 10:33:58 PDT
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] 2012-04-11 16:28:46 PDT
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 13 Christian Holler (:decoder) 2012-04-16 13:03:40 PDT
JSBugMon: This bug has been automatically verified fixed.
Comment 14 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-21 15:18:52 PDT
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] 2012-06-21 15:19:40 PDT
(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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-21 15:24:16 PDT
Mac 10.7, I guess.
Comment 17 Gary Kwong [:gkw] [:nth10sd] 2012-06-21 16:17:04 PDT
(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) 2013-01-19 14:16:05 PST
Automatically extracted testcase for this bug was committed:

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

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