Closed Bug 736609 Opened 12 years ago Closed 12 years ago

Malloc error with ArrayBuffer, Uint32Array and Uint8Array

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla14
Tracking Status
firefox12 + fixed
firefox13 + fixed
firefox14 + fixed
firefox-esr10 12+ fixed

People

(Reporter: gkw, Assigned: dmandelin)

References

Details

(Keywords: crash, testcase, Whiteboard: [sg:critical][qa?])

Attachments

(2 files)

Attached file 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.
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
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
Blocks: 664249
Whiteboard: js-triage-needed → [sg:critical] js-triage-needed
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).
Assignee: general → dmandelin
Whiteboard: [sg:critical] js-triage-needed → [sg:critical]
Attached patch PatchSplinter Review
Great test case: it's a bad bug but very easy to spot with this test case + the memory corruption thing Visual Studio has.
Attachment #609492 - Flags: review?(sphink)
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 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.
Attachment #609492 - Flags: review?(sphink) → review+
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?
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.
[Triage Comment]

Please nominate the patch for ESR landing as per https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
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
Attachment #609492 - Flags: approval-mozilla-esr10?
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.
Attachment #609492 - Flags: approval-mozilla-esr10?
Attachment #609492 - Flags: approval-mozilla-esr10+
Attachment #609492 - Flags: approval-mozilla-beta+
Attachment #609492 - Flags: approval-mozilla-aurora+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
JSBugMon: This bug has been automatically verified fixed.
Status: RESOLVED → VERIFIED
Group: core-security
Could someone please point me to directions for how to create a 32-bit opt debug shell for verification?
Whiteboard: [sg:critical] → [sg:critical][qa?]
(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)?
Mac 10.7, I guess.
(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 )
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.