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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Christian Holler (:decoder)]

NI on :efaust because he maintains Classes.

Comment 2

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Tracking for 43 as it affects Nightly.

Comment 3

[Fuzzing Team]

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.

Comment 4

[Andrew McCreight [:mccr8]]

Eric, are you going to be able to fix this? Or maybe Jason?

Comment 5

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision d01dd42e654b).

Comment 6

[Eric Faust [:efaust]]

Attachment: Fix

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.

Comment 7

[Eric Faust [:efaust]]

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 8

[Eric Faust [:efaust]]

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

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Any updates on a patch here?

Comment 10

[Eric Faust [:efaust]]

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.

Comment 11

[Matt Wobensmith [:mwobensmith][:matt:]]

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

Comment 12

[Andrew McCreight [:mccr8]]

Eric, can you fix this? We should disable ES6 classes on Nightly if 2 month old sec-critical bugs aren't being fixed.

Comment 13

[Benjamin Bouvier [:bbouvier] (afk until August 17th)]

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

Comment 14

[Eric Faust [:efaust]]

Attachment: Part 1: Change SuperElemOptions to EmitElemOptions and use it for BytecodeEmitter::emitElemOperands

This is just a preface for the first part. EmitElem_IncDec to be used in part 2.

Comment 15

[Eric Faust [:efaust]]

Attachment: Part 2: Fix

These two can land together, but part 1 seemed a nice side piece to look at trivially.

Comment 16

[Jan de Mooij [:jandem]]

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 };

Comment 17

[Jan de Mooij [:jandem]]

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?

Comment 18

[Eric Faust [:efaust]]

Attachment: Part 2: Spellchecked Fix

Comment 19

[Jan de Mooij [:jandem]]

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;

Comment 20

[Eric Faust [:efaust]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cf6e88dc6495
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c8e47cf8d379

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cf6e88dc6495
https://hg.mozilla.org/mozilla-central/rev/c8e47cf8d379

Comment 22

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.