Closed
Bug 1202784
Opened 8 years ago
Closed 7 years ago
Assertion failure: index < length_, at js/src/jit/FixedList.h:82 with ES6 Classes
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla45
People
(Reporter: decoder, Assigned: efaust)
References
Details
(5 keywords, Whiteboard: [jsbugmon:update,ignore])
Attachments
(2 files, 2 obsolete files)
5.94 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
26.74 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•8 years ago
|
||
NI on :efaust because he maintains Classes.
Flags: needinfo?(efaustbmo)
Updated•8 years ago
|
status-firefox42:
--- → disabled
status-firefox-esr38:
--- → unaffected
Keywords: csectype-bounds,
sec-critical
Tracking for 43 as it affects Nightly.
tracking-firefox43:
--- → +
Updated•8 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 3•8 years ago
|
||
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.
Updated•8 years ago
|
Group: core-security → javascript-core-security
Updated•8 years ago
|
status-firefox44:
--- → affected
Comment 4•7 years ago
|
||
Eric, are you going to be able to fix this? Or maybe Jason?
Flags: needinfo?(jorendorff)
Updated•7 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
Comment 5•7 years ago
|
||
JSBugMon: The testcase found in this bug no longer reproduces (tried revision d01dd42e654b).
Assignee | ||
Comment 6•7 years ago
|
||
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)
Assignee | ||
Comment 7•7 years ago
|
||
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.
Assignee | ||
Comment 8•7 years ago
|
||
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)
Updated•7 years ago
|
status-firefox45:
--- → affected
Assignee | ||
Comment 10•7 years ago
|
||
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)
Comment 11•7 years ago
|
||
(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)
Comment 12•7 years ago
|
||
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•7 years ago
|
||
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()
Assignee | ||
Comment 14•7 years ago
|
||
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)
Assignee | ||
Comment 15•7 years ago
|
||
These two can land together, but part 1 seemed a nice side piece to look at trivially.
Attachment #8696781 -
Flags: review?(jdemooij)
Comment 16•7 years ago
|
||
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 17•7 years ago
|
||
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)
Assignee | ||
Comment 18•7 years ago
|
||
Attachment #8696781 -
Attachment is obsolete: true
Attachment #8696818 -
Flags: review?(jdemooij)
Comment 19•7 years ago
|
||
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+
Assignee | ||
Comment 20•7 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/cf6e88dc6495 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c8e47cf8d379
Comment 21•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cf6e88dc6495 https://hg.mozilla.org/mozilla-central/rev/c8e47cf8d379
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Comment 22•7 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•7 years ago
|
Group: javascript-core-security → core-security-release
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•