Closed Bug 1202784 Opened 5 years ago Closed 5 years ago

Assertion failure: index < length_, at js/src/jit/FixedList.h:82 with ES6 Classes

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla45
Tracking Status
firefox42 --- disabled
firefox43 + disabled
firefox44 --- disabled
firefox45 --- verified
firefox-esr38 --- unaffected

People

(Reporter: decoder, Assigned: efaust)

References

Details

(5 keywords, Whiteboard: [jsbugmon:update,ignore])

Attachments

(2 files, 2 obsolete files)

The following testcase crashes on mozilla-central revision 5fe9ed3edd68 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --baseline-eager):

class base {
    constructor() { }
}
class derived extends base {
    constructor() { }
    testDeleteElem() {
        --super[(object >>> 0)];
    }
}
var d = new derived();
d.testDeleteElem();



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x00000000004585b4 in js::jit::FixedList<js::jit::StackValue>::operator[] (index=<optimized out>, this=0x7fffffffc790) at js/src/jit/FixedList.h:82
#0  0x00000000004585b4 in js::jit::FixedList<js::jit::StackValue>::operator[] (index=<optimized out>, this=0x7fffffffc790) at js/src/jit/FixedList.h:82
#1  0x00000000008babc3 in operator[] (index=<optimized out>, this=0x7fffffffc790) at js/src/jit/BaselineCompiler.cpp:3338
#2  peek (index=-1, this=0x7fffffffc780) at js/src/jit/BaselineFrameInfo.h:223
#3  js::jit::BaselineCompiler::emit_JSOP_TOID (this=this@entry=0x7fffffffbd40) at js/src/jit/BaselineCompiler.cpp:3316
#4  0x00000000008fbb21 in js::jit::BaselineCompiler::emitBody (this=this@entry=0x7fffffffbd40) at js/src/jit/BaselineCompiler.cpp:985
#5  0x000000000090f58c in js::jit::BaselineCompiler::compile (this=this@entry=0x7fffffffbd40) at js/src/jit/BaselineCompiler.cpp:102
#6  0x00000000009108dd in js::jit::BaselineCompile (cx=cx@entry=0x7ffff6907000, script=0x7ffff7e623d0, forceDebugInstrumentation=<optimized out>) at js/src/jit/BaselineJIT.cpp:264
#7  0x0000000000911049 in CanEnterBaselineJIT (cx=cx@entry=0x7ffff6907000, script=..., script@entry=..., osrFrame=osrFrame@entry=0x0) at js/src/jit/BaselineJIT.cpp:303
#8  0x0000000000911351 in js::jit::CanEnterBaselineMethod (cx=cx@entry=0x7ffff6907000, state=...) at js/src/jit/BaselineJIT.cpp:371
#9  0x00000000006bf63b in js::RunScript (cx=cx@entry=0x7ffff6907000, state=...) at js/src/vm/Interpreter.cpp:690
#10 0x00000000006bfe46 in js::Invoke (cx=cx@entry=0x7ffff6907000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:781
#11 0x00000000006b21b0 in Interpret (cx=cx@entry=0x7ffff6907000, state=...) at js/src/vm/Interpreter.cpp:3067
#12 0x00000000006bf55b in js::RunScript (cx=cx@entry=0x7ffff6907000, state=...) at js/src/vm/Interpreter.cpp:704
#13 0x00000000006c9cd9 in js::ExecuteKernel (cx=cx@entry=0x7ffff6907000, script=..., script@entry=..., scopeChainArg=..., thisv=..., newTargetValue=..., type=type@entry=js::EXECUTE_GLOBAL, evalInFrame=evalInFrame@entry=..., result=result@entry=0x0) at js/src/vm/Interpreter.cpp:978
#14 0x00000000006cbf53 in js::Execute (cx=cx@entry=0x7ffff6907000, script=script@entry=..., scopeChainArg=..., rval=rval@entry=0x0) at js/src/vm/Interpreter.cpp:1012
#15 0x0000000000af0106 in ExecuteScript (cx=cx@entry=0x7ffff6907000, scope=..., script=..., rval=rval@entry=0x0) at js/src/jsapi.cpp:4356
#16 0x0000000000af027b in JS_ExecuteScript (cx=cx@entry=0x7ffff6907000, scriptArg=..., scriptArg@entry=...) at js/src/jsapi.cpp:4387
#17 0x00000000004287db in RunFile (compileOnly=false, file=0x7ffff69a6800, filename=0x7fffffffe054 "min.js", cx=0x7ffff6907000) at js/src/shell/js.cpp:461
#18 Process (cx=cx@entry=0x7ffff6907000, filename=0x7fffffffe054 "min.js", forceTTY=forceTTY@entry=false) at js/src/shell/js.cpp:579
#19 0x000000000047aaa6 in ProcessArgs (op=0x7fffffffdb00, cx=0x7ffff6907000) at js/src/shell/js.cpp:5832
#20 Shell (envp=<optimized out>, op=0x7fffffffdb00, cx=0x7ffff6907000) at js/src/shell/js.cpp:6121
#21 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:6470
rax	0x0	0
rbx	0x7fffffffbd40	140737488338240
rcx	0x7ffff6ca53cd	140737333842893
rdx	0x0	0
rsi	0x7ffff6f7a9d0	140737336814032
rdi	0x7ffff6f791c0	140737336807872
rbp	0x7fffffffb850	140737488336976
rsp	0x7fffffffb850	140737488336976
r8	0x7ffff7fe0780	140737354008448
r9	0x6372732f736a2f6c	7165916604736876396
r10	0x7fffffffb610	140737488336400
r11	0x7ffff6c27960	140737333328224
r12	0x7fffffffbd58	140737488338264
r13	0x7fffffffc780	140737488340864
r14	0xe1	225
r15	0x7fffffffb860	140737488336992
rip	0x4585b4 <js::jit::FixedList<js::jit::StackValue>::operator[](unsigned long) const+28>
=> 0x4585b4 <js::jit::FixedList<js::jit::StackValue>::operator[](unsigned long) const+28>:	movl   $0x52,0x0
   0x4585bf <js::jit::FixedList<js::jit::StackValue>::operator[](unsigned long) const+39>:	callq  0x49c770 <abort()>


I'm marking this s-s because the assertion looks like a range violation to me. However, this should only affect Nightly due to ES6 Classes being disabled on other builds.
NI on :efaust because he maintains Classes.
Flags: needinfo?(efaustbmo)
Tracking for 43 as it affects Nightly.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/a3f6306dd05e
user:        Eric Faust
date:        Wed Apr 08 17:41:01 2015 -0700
summary:     Bug 1141862 - Part 6: Implement ES6 SuperProperty and SuperMember. (r=jorendorff)

This iteration took 162.222 seconds to run.
Group: core-security → javascript-core-security
Eric, are you going to be able to fix this? Or maybe Jason?
Flags: needinfo?(jorendorff)
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision d01dd42e654b).
Attached patch Fix (obsolete) — Splinter Review
The claim that this no longer reproduces is an out and out lie. I have added a simplified test.

The bug was that the TOID operation once needed to verify that the object was coerscible. It doesn't anymore. It can just do TOID and nobody will mind.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Flags: needinfo?(jorendorff)
Flags: needinfo?(efaustbmo)
Attachment #8672164 - Flags: review?(jwalden+bmo)
This was the bug because the TOID operation is also used with super[expr]--, which doesn't have to enforce object coerscibility, and doesn't have an object on the stack where we used to look for it, before the patch.
Comment on attachment 8672164 [details] [diff] [review]
Fix

Review of attachment 8672164 [details] [diff] [review]:
-----------------------------------------------------------------

This patch is wrong. We must check that the the value is coerscible to object /before/ doing any ToPropertyId actions. This will throw errors out of order for |({}).foo[{ valueOf() { throw "oops"; }]|

Blast. Was hoping to get away with one :P
Attachment #8672164 - Flags: review?(jwalden+bmo)
Any updates on a patch here?
Flags: needinfo?(efaustbmo)
Good news here is that the bug is nightly only. The problem only arises with super accesses, which are locked behind the classes flag.

Patch has gotten temporarily waylaid. I'll take a look again.
Flags: needinfo?(efaustbmo)
(In reply to Eric Faust [:efaust] from comment #10)
> Good news here is that the bug is nightly only. The problem only arises with
> super accesses, which are locked behind the classes flag.
> 
> Patch has gotten temporarily waylaid. I'll take a look again.

Hi Eric - did you get a chance to take a look?
Flags: needinfo?(efaustbmo)
Eric, can you fix this? We should disable ES6 classes on Nightly if 2 month old sec-critical bugs aren't being fixed.
Other test cases (it doesn't seem to involve inheritance)

// #1
function base() { }

base.prototype = {
    crash() {
        --super[1];
    }
}

var d = new base();
d.crash();

// #2
class base {
    crash() {
        --super[1];
    }
}

var d = new base();
d.crash()
This is just a preface for the first part. EmitElem_IncDec to be used in part 2.
Attachment #8672164 - Attachment is obsolete: true
Flags: needinfo?(efaustbmo)
Attachment #8696779 - Flags: review?(jdemooij)
Attached patch Part 2: Fix (obsolete) — Splinter Review
These two can land together, but part 1 seemed a nice side piece to look at trivially.
Attachment #8696781 - Flags: review?(jdemooij)
Comment on attachment 8696779 [details] [diff] [review]
Part 1: Change SuperElemOptions to EmitElemOptions and use it for BytecodeEmitter::emitElemOperands

Review of attachment 8696779 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.h
@@ +508,5 @@
>  
>      // Emit bytecode to put operands for a JSOP_GETELEM/CALLELEM/SETELEM/DELELEM
>      // opcode onto the stack in the right order. In the case of SETELEM, the
>      // value to be assigned must already be pushed.
> +    enum EmitElemOptions { EmitElem_Get, EmitElem_Set, EmitElem_Call, EmitElem_IncDec };

Nit: I think singular "option" is more common (see VarEmitOption for instance) and it'd be nice to use an enum class instead of the prefix:

enum class EmitElemOption { Get, Set, Call, IncDec };
Attachment #8696779 - Flags: review?(jdemooij) → review+
Comment on attachment 8696781 [details] [diff] [review]
Part 2: Fix

Review of attachment 8696781 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/MIR.h
@@ +5375,3 @@
>      public BoxInputsPolicy::Data
>  {
> +    MToId(MDefinition* index)

Nit: explicit

@@ +13335,5 @@
> +class MCheckObjCoersce
> +  : public MUnaryInstruction,
> +    public BoxInputsPolicy::Data
> +{
> +    MCheckObjCoersce(MDefinition* toCheck)

And here.

::: js/src/vm/Interpreter-inl.h
@@ -402,5 @@
>          res.set(idval);
>          return true;
>      }
>  
> -    JSObject* obj = ToObjectFromStack(cx, objval);

Really nice to get rid of this, finally.

::: js/src/vm/Opcodes.h
@@ +1480,5 @@
> +     *   Type: Object
> +     *   Operands:
> +     *   Stack: val => val
> +     */ \
> +    macro(JSOP_CHECKOBJCOERSCE, 145, "checkobjcoersce", NULL, 1,  1,  1, JOF_BYTE) \

s/coersce/coerce/ everywhere. Maybe CHECKOBJCOERCIBLE?
Attachment #8696781 - Flags: review?(jdemooij)
Attachment #8696781 - Attachment is obsolete: true
Attachment #8696818 - Flags: review?(jdemooij)
Comment on attachment 8696818 [details] [diff] [review]
Part 2: Spellchecked Fix

Review of attachment 8696818 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Interpreter.cpp
@@ +3880,5 @@
>  
> +CASE(JSOP_CHECKOBJCOERCIBLE)
> +{
> +    ReservedRooted<Value> checkVal(&rootValue0, REGS.sp[-1]);
> +    if (!ToObjectFromStack(cx, checkVal))

Sorry I missed this the first time, but if checkVal.isPrimitive(), this will allocate a new object that's then never used. It's also different from what the JITs do. The easiest fix would be to change the `if` to:

if (checkVal.isNullOrUndefined() && !ToObjectFromStack(cx, checkVal))
    goto error;
Attachment #8696818 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/cf6e88dc6495
https://hg.mozilla.org/mozilla-central/rev/c8e47cf8d379
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
Duplicate of this bug: 1235160
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.