Last Comment Bug 745158 - Crash [@ Decompile] with let expressions
: Crash [@ Decompile] with let expressions
Status: VERIFIED FIXED
[js:p1][sg:high][advisory-tracking+]
: crash, sec-high, testcase, valgrind
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
: -- critical (vote)
: mozilla17
Assigned To: David Mandelin [:dmandelin]
:
Mentors:
Depends on:
Blocks: langfuzz
  Show dependency treegraph
 
Reported: 2012-04-13 04:41 PDT by Christian Holler (:decoder)
Modified: 2014-02-26 11:51 PST (History)
9 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
verified
15+
fixed


Attachments
Patch (1.08 KB, patch)
2012-06-29 13:45 PDT, David Mandelin [:dmandelin]
luke: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-04-13 04:41:05 PDT
The following test crashes on mozilla-central revision 3fa30b0edd15 (options -m -n -a), 32 bit opt build:


let ([] = 1) 3; 
let (i) new [i][print[i]];


Valgrind Errors:

==59269== Conditional jump or move depends on uninitialised value(s)
==59269==    at 0x80F970B: Decompile(SprintStack*, unsigned char*, int) (jsopcode.cpp:5077)
==59269==    by 0x81004DD: DecompileCode(JSPrinter*, JSScript*, unsigned char*, unsigned int, unsigned int) (jsopcode.cpp:5404)
==59269==    by 0x81006F0: DecompileExpression(JSContext*, JSScript*, JSFunction*, unsigned char*) (jsopcode.cpp:5810)
==59269==    by 0x81009C1: js_DecompileValueGenerator (jsopcode.cpp:5699)
==59269==    by 0x807E97C: js_ReportValueErrorFlags(JSContext*, unsigned int, unsigned int, int, JS::Value const&, JSString*, char const*, char const*) (jsopcode.h:401)
==59269==    by 0x809A7BF: js_ReportIsNotFunction(JSContext*, JS::Value const*, unsigned int) (jsfun.cpp:1383)
==59269==    by 0x80CDAC5: js::InvokeConstructorKernel(JSContext*, js::CallArgs const&) (jsinterp.cpp:605)
==59269==    by 0x80C1FF8: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2682)
==59269==    by 0x80CD563: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:472)
==59269==    by 0x80CE129: js::Execute(JSContext*, JSScript*, JSObject&, JS::Value*) (jsinterp.cpp:670)
==59269==    by 0x806160C: JS_ExecuteScript (jsapi.cpp:5247)
==59269==    by 0x805030D: Process(JSContext*, JSObject*, char const*, bool) (js.cpp:478)

(twice)

==59269== Invalid read of size 1
==59269==    at 0x80F9700: Decompile(SprintStack*, unsigned char*, int) (jsopcode.cpp:5077)
==59269==    by 0x81004DD: DecompileCode(JSPrinter*, JSScript*, unsigned char*, unsigned int, unsigned int) (jsopcode.cpp:5404)
==59269==    by 0x81006F0: DecompileExpression(JSContext*, JSScript*, JSFunction*, unsigned char*) (jsopcode.cpp:5810)
==59269==    by 0x81009C1: js_DecompileValueGenerator (jsopcode.cpp:5699)
==59269==    by 0x807E97C: js_ReportValueErrorFlags(JSContext*, unsigned int, unsigned int, int, JS::Value const&, JSString*, char const*, char const*) (jsopcode.h:401)
==59269==    by 0x809A7BF: js_ReportIsNotFunction(JSContext*, JS::Value const*, unsigned int) (jsfun.cpp:1383)
==59269==    by 0x80CDAC5: js::InvokeConstructorKernel(JSContext*, js::CallArgs const&) (jsinterp.cpp:605)
==59269==    by 0x80C1FF8: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2682)
==59269==    by 0x80CD563: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:472)
==59269==    by 0x80CE129: js::Execute(JSContext*, JSScript*, JSObject&, JS::Value*) (jsinterp.cpp:670)
==59269==    by 0x806160C: JS_ExecuteScript (jsapi.cpp:5247)
==59269==    by 0x805030D: Process(JSContext*, JSObject*, char const*, bool) (js.cpp:478)
==59269==  Address 0x6ba1a88 is 0 bytes after a block of size 64 alloc'd
==59269==    at 0x48D1876: malloc (vg_replace_malloc.c:236)
==59269==    by 0x80F3D5F: InitSprintStack(JSContext*, SprintStack*, JSPrinter*, unsigned int) (Utility.h:173)
==59269==    by 0x8100480: DecompileCode(JSPrinter*, JSScript*, unsigned char*, unsigned int, unsigned int) (jsopcode.cpp:5379)
==59269==    by 0x81006F0: DecompileExpression(JSContext*, JSScript*, JSFunction*, unsigned char*) (jsopcode.cpp:5810)
==59269==    by 0x81009C1: js_DecompileValueGenerator (jsopcode.cpp:5699)
==59269==    by 0x807E97C: js_ReportValueErrorFlags(JSContext*, unsigned int, unsigned int, int, JS::Value const&, JSString*, char const*, char const*) (jsopcode.h:401)
==59269==    by 0x809A7BF: js_ReportIsNotFunction(JSContext*, JS::Value const*, unsigned int) (jsfun.cpp:1383)
==59269==    by 0x80CDAC5: js::InvokeConstructorKernel(JSContext*, js::CallArgs const&) (jsinterp.cpp:605)
==59269==    by 0x80C1FF8: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2682)
==59269==    by 0x80CD563: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:472)
==59269==    by 0x80CE129: js::Execute(JSContext*, JSScript*, JSObject&, JS::Value*) (jsinterp.cpp:670)
==59269==    by 0x806160C: JS_ExecuteScript (jsapi.cpp:5247)


The test does not always crash, but it has an 80-90% chance to do so. Marking s-s because this seems to be an out-of-bounds read. Assigned to luke for now, as he has been looking on this issue already (and provided the smaller/more reliable testcase :D).
Comment 1 David Mandelin [:dmandelin] 2012-04-26 17:40:26 PDT
This crashes here:


              case JSOP_ENDINIT:
              {
                JSBool inArray;

                op = JSOP_NOP;           /* turn off parens */
                rval = PopStr(ss, op, &rvalpc);
                sn = js_GetSrcNote(jp->script, pc);

                /* Skip any #n= prefix to find the opening bracket. */
                for (xval = rval; *xval != '[' && *xval != '{'; xval++)
                    continue;

with an invalid read of *xval. rval ends up pointing to an empty string, which this loop reads past, so we can crash or read data we're not supposed to. I verified that we can step on the security bug by adding a bounds check to the loop. That leaves a bug, which is that we print out the wrong thing, but I'm not sure that we care that much about invalid decompiler messages for obscure code.
Comment 2 David Mandelin [:dmandelin] 2012-06-28 16:40:25 PDT
I can't seem to reproduce this any more. Gary, if you can show me how, I want to put in the bounds check and knock it out, otherwise we're working on removing this code so it should be taken care of that way.
Comment 3 David Mandelin [:dmandelin] 2012-06-29 13:45:47 PDT
Created attachment 637999 [details] [diff] [review]
Patch

Decompiler's hopefully going away soon, so let's just close the hole. Christian, do you think we need a cover bug? It only reads data, and not necessarily anything interesting, but the bug is really obvious.
Comment 4 David Mandelin [:dmandelin] 2012-07-11 18:48:21 PDT
Terrence, can you sneak this into one of your landings in the next couple of days as cover?
Comment 5 Terrence Cole [:terrence] 2012-07-17 11:48:59 PDT
This slipped for 16.  I had 5 patches out with 3 different people in the week leading up to the split and all of them (and some new ones) are still waiting for review.
Comment 6 David Mandelin [:dmandelin] 2012-07-17 15:02:07 PDT
(In reply to Terrence Cole [:terrence] from comment #5)
> This slipped for 16.  I had 5 patches out with 3 different people in the
> week leading up to the split and all of them (and some new ones) are still
> waiting for review.

No biggie--it can still land for 16 later, and it was too late for 14 before I talked to you. Please do land it when it's convenient--it looks like decompiler removal has yet more problems to work through.
Comment 7 Terrence Cole [:terrence] 2012-07-20 10:44:09 PDT
Pushed to 17 in in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/189816733310
Comment 9 David Mandelin [:dmandelin] 2012-07-25 17:14:03 PDT
Comment on attachment 637999 [details] [diff] [review]
Patch

I'm not sure if it's worth landing this on branches or not. It's only an sg:high (reads past the end of a buffer) and I don't know how likely it is to allow sensitive data to be read. Landing it there also exposes the bug more (we used a cover bug for inbound/central), so it may increase (hypothetical) risk on release branches. But it's a pretty simple, low-risk patch at least.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): decompiler
User impact if declined: leaves a read-past-the-buffer bug in the decompiler -- possible risk of data read by attackers
Testing completed (on m-c, etc.): landed on m-c a few days ago
Risk to taking this patch (and alternatives if risky): minimal
String or UUID changes made by this patch:
Comment 10 Alex Keybl [:akeybl] 2012-07-27 16:45:50 PDT
Comment on attachment 637999 [details] [diff] [review]
Patch

[Triage Comment]
Low risk, sg:high fix. Please also prepare a patch for the ESR10 if affected. Approved for branches.
Comment 11 Terrence Cole [:terrence] 2012-07-30 13:08:41 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/2f7d9cc63a6a
Comment 12 Terrence Cole [:terrence] 2012-07-30 13:33:49 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/90641bda34ec
Comment 13 David Mandelin [:dmandelin] 2012-07-30 18:52:06 PDT
Thanks for landing, Terrence!
Comment 14 David Mandelin [:dmandelin] 2012-07-30 18:53:07 PDT
Comment on attachment 637999 [details] [diff] [review]
Patch

ESR10 is affected, same patch applies.
Comment 15 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-01 17:43:53 PDT
Comment on attachment 637999 [details] [diff] [review]
Patch

approving for ESR, please land in the next week to ensure this goes out alongside the fix with 15.
Comment 16 David Mandelin [:dmandelin] 2012-08-01 17:55:18 PDT
http://hg.mozilla.org/releases/mozilla-esr10/rev/53882eeb9637
Comment 17 Christian Holler (:decoder) 2012-08-01 18:41:56 PDT
JSBugMon: This bug has been automatically verified fixed.
Comment 18 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-19 10:41:51 PDT
What was the target milestone for this bug, Firefox 17?
Comment 19 Gary Kwong [:gkw] [:nth10sd] 2012-09-19 11:17:50 PDT
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #18)
> What was the target milestone for this bug, Firefox 17?

Yes, see comment 6 and comment 7.

http://hg.mozilla.org/mozilla-central/rev/139a8f2a8538
Comment 20 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-19 11:32:47 PDT
Thanks Gary.
Comment 21 Christian Holler (:decoder) 2013-01-19 13:51:23 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.