745158 - Crash [@ Decompile] with let expressions

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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]]

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]]

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]]

Attachment: 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]]

Terrence, can you sneak this into one of your landings in the next couple of days as cover?

Comment 5

[Terrence Cole [:terrence]]

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]]

(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]]

Pushed to 17 in in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/189816733310

Comment 8

[Terrence Cole [:terrence]]

Backed out in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/166cae54928d
Re-landed in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/139a8f2a8538

Comment 9

[David Mandelin [:dmandelin]]

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]]

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]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2f7d9cc63a6a

Comment 12

[Terrence Cole [:terrence]]

https://hg.mozilla.org/releases/mozilla-beta/rev/90641bda34ec

Comment 13

[David Mandelin [:dmandelin]]

Thanks for landing, Terrence!

Comment 14

[David Mandelin [:dmandelin]]

Comment on attachment 637999 [details] [diff] [review]
Patch

ESR10 is affected, same patch applies.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

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]]

http://hg.mozilla.org/releases/mozilla-esr10/rev/53882eeb9637

Comment 17

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 18

[u279076]

What was the target milestone for this bug, Firefox 17?

Comment 19

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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

[u279076]

Thanks Gary.

Comment 21

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

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