Closed
Bug 756600
Opened 12 years ago
Closed 12 years ago
OOM Testing: Glibc Abort due to invalid free in js::TokenStream::~TokenStream
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: decoder, Assigned: jandem)
References
Details
(Keywords: crash, sec-critical, testcase, Whiteboard: [qa-][advisory-tracking+])
Attachments
(1 file)
1.41 KB,
patch
|
n.nethercote
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta-
lsblakk
:
approval-mozilla-esr10+
|
Details | Diff | Splinter Review |
The following command crashes on ionmonkey revision 8c54899dae82 (dbg build): js -e 'const libdir = "js/src/jit-test/lib/";' -A 7046 -f js/src/jit-test/tests/debug/breakpoint-resume-01.js
Reporter | ||
Comment 1•12 years ago
|
||
The reason for the missing signature is a glibc abort here: (gdb) bt #0 0x00007ffff6eb8d05 in raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64 #1 0x00007ffff6ebcab6 in abort () at abort.c:92 #2 0x00007ffff6ef1d7b in __libc_message (do_abort=2, fmt=0x7ffff6fda400 "*** glibc detected *** %s: %s: 0x%s ***\n") at ../sysdeps/unix/sysv/linux/libc_fatal.c:189 #3 0x00007ffff6efda8f in malloc_printerr (av=<value optimized out>, p=0x7fffffffc0e0) at malloc.c:6283 #4 _int_free (av=<value optimized out>, p=0x7fffffffc0e0) at malloc.c:4795 #5 0x00007ffff6f018e3 in __libc_free (mem=<value optimized out>) at malloc.c:3738 #6 0x0000000000403b6f in js_free (p=0x7fffffffc0f0) at ../dist/include/js/Utility.h:201 #7 0x0000000000403b89 in js::Foreground::free_ (p=0x7fffffffc0f0) at ../dist/include/js/Utility.h:620 #8 0x0000000000419688 in JSRuntime::free_ (this=0x7ffff7f9a010, p=0x7fffffffc0f0) at /tmp/abc-8c54899dae82-tNQ40M/compilePath/js/src/jscntxt.h:895 #9 0x00000000004196dc in JSContext::free_ (this=0xda05e0, p=0x7fffffffc0f0) at /tmp/abc-8c54899dae82-tNQ40M/compilePath/js/src/jscntxt.h:1320 #10 0x00000000006c4bd6 in js::TokenStream::~TokenStream (this=0x7fffffffbf90, __in_chrg=<value optimized out>) at /tmp/abc-8c54899dae82-tNQ40M/compilePath/js/src/frontend/TokenStream.cpp:252 #11 0x00000000006a5350 in js::Parser::~Parser (this=0x7fffffffbf40, __in_chrg=<value optimized out>) at /tmp/abc-8c54899dae82-tNQ40M/compilePath/js/src/frontend/Parser.cpp:152 #12 0x0000000000673779 in js::frontend::CompileScript (cx=0xda05e0, scopeChain=0x7ffff6516060, callerFrame=0x0, principals=0x0, originPrincipals=0x0, compileAndGo=true, noScriptRval=false, needScriptGlobal=false, chars=0xdaf660, length=137, filename=0xda9791 "/home/decoder/Mozilla/JSBugMon/repos/ionmonkey/js/src/jit-test/tests/debug/breakpoint-resume-01.js", lineno=17, version=JSVERSION_ECMA_5, source=0x7ffff6629820, staticLevel=0) at /tmp/abc-8c54899dae82-tNQ40M/compilePath/js/src/frontend/BytecodeCompiler.cpp:122 I'm not getting this abort on mozilla-central. I assume this is not security-related since the test used is jsdbg2-only.
Summary: IonMonkey: OOM Testing: Crash/Signal 6 (no signature available) → IonMonkey: OOM Testing: Glibc Abort due to invalid free in js::TokenStream::~TokenStream
Reporter | ||
Comment 3•12 years ago
|
||
Still valid with ballast patch, same parameters.
Assignee | ||
Updated•12 years ago
|
Group: core-security
Assignee | ||
Comment 4•12 years ago
|
||
Not an IonMonkey bug but the fix is straight-forward and this seems security sensitive. Nicholas, flagging you for review since you've done a lot of frontend work lately. The problem is that in Parser::init: bool Parser::init(...) { JSContext *cx = context; if (!cx->ensureParseMapPool()) return false; tempPoolMark = cx->tempLifoAlloc().mark(); if (!tokenStream.init(base, length, filename, lineno, version)) { cx->tempLifoAlloc().release(tempPoolMark); return false; } ... If cx->ensureParseMapPool() fails, we don't call tokenStream.init. This means TokenStream::sourceMap is now uninitialized and the TokenStream destructor crashes when it tries to free sourceMap. The patch initializes sourceMap to NULL in the constructor instead of init, so that it's no longer uninitialized if we don't call init().
Assignee | ||
Updated•12 years ago
|
Attachment #625994 -
Flags: review? → review?(n.nethercote)
Comment 5•12 years ago
|
||
Comment on attachment 625994 [details] [diff] [review] Patch Review of attachment 625994 [details] [diff] [review]: ----------------------------------------------------------------- This is fine as a spot fix. I'm going to do a follow-up that will make this kind of thing impossible in the future.
Attachment #625994 -
Flags: review?(n.nethercote) → review+
Comment 6•12 years ago
|
||
> I'm going to do a follow-up that will make this kind of thing impossible in > the future. Bug 757690. It removes the unnecessary split between TokenStream::TokenStream and TokenStream::init, ensuring that all TokenStream fields are always initialized.
Assignee | ||
Comment 7•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb27363cfba1
Target Milestone: --- → mozilla15
Assignee | ||
Updated•12 years ago
|
Whiteboard: [sg:critical]
Assignee | ||
Updated•12 years ago
|
Keywords: sec-critical
Whiteboard: [sg:critical]
Assignee | ||
Comment 8•12 years ago
|
||
Comment on attachment 625994 [details] [diff] [review] Patch [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 674283 User impact if declined: Freeing arbitrary memory if an attacker can trigger an OOM in Parser::init. Testing completed (on m-c, etc.): None, patch just landed. Risk to taking this patch (and alternatives if risky): Very small, patch changes two lines: initialize a class member in the constructor instead of init function. String or UUID changes made by this patch: None.
Attachment #625994 -
Flags: approval-mozilla-esr10?
Attachment #625994 -
Flags: approval-mozilla-beta?
Attachment #625994 -
Flags: approval-mozilla-aurora?
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cb27363cfba1
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
status-firefox15:
--- → fixed
Resolution: --- → FIXED
Comment 10•12 years ago
|
||
(In reply to Jan de Mooij (:jandem) from comment #8) > User impact if declined: Freeing arbitrary memory if an attacker can trigger > an OOM in Parser::init. Our final beta is being built Monday. We weighed the risk of exploit vs the risk of regression and decided to let this ride till FF14.
status-firefox13:
--- → wontfix
Updated•12 years ago
|
Attachment #625994 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Updated•12 years ago
|
status-firefox-esr10:
--- → affected
status-firefox14:
--- → affected
tracking-firefox-esr10:
--- → 14+
tracking-firefox14:
--- → +
tracking-firefox15:
--- → +
Comment 11•12 years ago
|
||
Comment on attachment 625994 [details] [diff] [review] Patch [Triage Comment] Approving for Aurora 14, however. We'll approve for ESR once we've bumped the version (after 6/5).
Attachment #625994 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 12•12 years ago
|
||
Pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/f38511c875c8
Comment 13•12 years ago
|
||
Christian, can you verify that this is fixed on trunk?
Comment 14•12 years ago
|
||
Comment on attachment 625994 [details] [diff] [review] Patch [Triage Comment] Time to get this landed for release alongside FF14.
Attachment #625994 -
Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Assignee | ||
Comment 15•12 years ago
|
||
(In reply to Lukas Blakk [:lsblakk] from comment #14) > > [Triage Comment] > Time to get this landed for release alongside FF14. https://hg.mozilla.org/releases/mozilla-esr10/rev/643bd5945098
Comment 16•12 years ago
|
||
What's the difference between an IonMonkey debug JS Shell and a Firefox debug JS Shell? and how can I set one up to verify for Firefox 10.0.6esr if there is a difference?
Whoops, this was not an IonMonkey bug, so you shouldn't have to worry about dealing with that branch.
Summary: IonMonkey: OOM Testing: Glibc Abort due to invalid free in js::TokenStream::~TokenStream → OOM Testing: Glibc Abort due to invalid free in js::TokenStream::~TokenStream
Comment 18•12 years ago
|
||
So I should be able to verify this fixed for ESR by making a debug shell from latest-mozilla-esr10?
Reporter | ||
Comment 19•12 years ago
|
||
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #18) > So I should be able to verify this fixed for ESR by making a debug shell > from latest-mozilla-esr10? No, unfortunately it's not that easy. The problem is that the -A parameter varies across different versions, even on the same branch. There is no way to verify these bugs except by rerunning the whole testing and hoping it doesn't pop up anymore.
Comment 20•12 years ago
|
||
Marking this qa- given that there does not appear to be any way to reliably verify this fix. Please mark as qa+ if this changes. Thanks Christian.
Whiteboard: [qa-]
Updated•12 years ago
|
Whiteboard: [qa-] → [qa-][advisory-tracking+]
Updated•12 years ago
|
Group: core-security
Reporter | ||
Updated•11 years ago
|
Flags: in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•