Last Comment Bug 756600 - OOM Testing: Glibc Abort due to invalid free in js::TokenStream::~TokenStream
: OOM Testing: Glibc Abort due to invalid free in js::TokenStream::~TokenStream
Status: RESOLVED FIXED
[qa-][advisory-tracking+]
: crash, sec-critical, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86_64 Linux
: -- critical (vote)
: mozilla15
Assigned To: Jan de Mooij [:jandem]
:
Mentors:
: 756616 (view as bug list)
Depends on:
Blocks: 624094
  Show dependency treegraph
 
Reported: 2012-05-18 13:45 PDT by Christian Holler (:decoder)
Modified: 2013-03-11 06:27 PDT (History)
10 users (show)
choller: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
fixed
+
fixed
14+
fixed


Attachments
Patch (1.41 KB, patch)
2012-05-22 07:06 PDT, Jan de Mooij [:jandem]
n.nethercote: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Review

Description Christian Holler (:decoder) 2012-05-18 13:45:14 PDT
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
Comment 1 Christian Holler (:decoder) 2012-05-18 13:49:36 PDT
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.
Comment 2 Christian Holler (:decoder) 2012-05-19 07:20:59 PDT
*** Bug 756616 has been marked as a duplicate of this bug. ***
Comment 3 Christian Holler (:decoder) 2012-05-19 07:27:13 PDT
Still valid with ballast patch, same parameters.
Comment 4 Jan de Mooij [:jandem] 2012-05-22 07:06:43 PDT
Created attachment 625994 [details] [diff] [review]
Patch

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().
Comment 5 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-22 17:32:24 PDT
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.
Comment 6 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-22 18:27:31 PDT
> 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.
Comment 8 Jan de Mooij [:jandem] 2012-05-23 06:22:37 PDT
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.
Comment 9 Ed Morley [:emorley] 2012-05-24 10:58:41 PDT
https://hg.mozilla.org/mozilla-central/rev/cb27363cfba1
Comment 10 Alex Keybl [:akeybl] 2012-05-24 16:34:22 PDT
(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.
Comment 11 Alex Keybl [:akeybl] 2012-05-29 10:57:05 PDT
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).
Comment 12 Jan de Mooij [:jandem] 2012-05-29 12:30:55 PDT
Pushed to aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/f38511c875c8
Comment 13 [On PTO until 6/29] 2012-06-08 17:21:57 PDT
Christian, can you verify that this is fixed on trunk?
Comment 14 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-14 15:06:31 PDT
Comment on attachment 625994 [details] [diff] [review]
Patch

[Triage Comment]
Time to get this landed for release alongside FF14.
Comment 15 Jan de Mooij [:jandem] 2012-06-15 01:11:16 PDT
(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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-21 13:56:12 PDT
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?
Comment 17 David Anderson [:dvander] 2012-06-21 13:57:33 PDT
Whoops, this was not an IonMonkey bug, so you shouldn't have to worry about dealing with that branch.
Comment 18 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-21 14:00:46 PDT
So I should be able to verify this fixed for ESR by making a debug shell from latest-mozilla-esr10?
Comment 19 Christian Holler (:decoder) 2012-06-21 14:02:32 PDT
(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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-21 14:37:46 PDT
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.

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