Closed Bug 1368360 Opened 8 years ago Closed 7 years ago

Crash [@ js::jit::TypeSetIncludes]

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla56
Tracking Status
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- verified

People

(Reporter: decoder, Assigned: tcampbell)

References

Details

(5 keywords, Whiteboard: [jsbugmon:update][adv-main56-])

Crash Data

Attachments

(1 file)

The following testcase crashes on mozilla-central revision ebad93e11770 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe --ion-eager): function T(a) this.actual = a; function r(actual) new T(actual); r(0); r(0); evaluate(` for (let [k, map = r(map)] of map) {} `); Backtrace: received signal SIGSEGV, Segmentation fault. js::jit::TypeSetIncludes (types=0x7ffff431d378, input=js::jit::MIRType::MagicUninitializedLexical, inputTypes=0x7ffff450e0a8) at js/src/jit/MIR.cpp:2637 #0 js::jit::TypeSetIncludes (types=0x7ffff431d378, input=js::jit::MIRType::MagicUninitializedLexical, inputTypes=0x7ffff450e0a8) at js/src/jit/MIR.cpp:2637 #1 0x00000000006c5924 in js::jit::CanWriteProperty (implicitType=js::jit::MIRType::None, value=0x7ffff450e030, property=..., constraints=0x7ffff450a120, alloc=...) at js/src/jit/MIR.cpp:6590 #2 js::jit::PropertyWriteNeedsTypeBarrier (alloc=..., constraints=0x7ffff450a120, current=0x7ffff4518728, pobj=pobj@entry=0x7fffffffb600, name=name@entry=0x7ffff46a0ec0, pvalue=pvalue@entry=0x7fffffffb5f8, canModify=true, implicitType=js::jit::MIRType::None) at js/src/jit/MIR.cpp:6627 #3 0x0000000000656f07 in js::jit::IonBuilder::jsop_setprop (this=this@entry=0x7fffffffb890, name=0x7ffff46a0ec0) at js/src/jit/IonBuilder.cpp:11366 #4 0x000000000064fca8 in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7fffffffb890, op=op@entry=JSOP_SETPROP) at js/src/jit/IonBuilder.cpp:2181 #5 0x0000000000651044 in js::jit::IonBuilder::visitBlock (this=this@entry=0x7fffffffb890, cfgblock=cfgblock@entry=0x7ffff4328240, mblock=<optimized out>) at js/src/jit/IonBuilder.cpp:1529 #6 0x00000000006513bf in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7fffffffb890) at js/src/jit/IonBuilder.cpp:1450 #7 0x0000000000651e9b in js::jit::IonBuilder::buildInline (this=this@entry=0x7fffffffb890, callerBuilder=callerBuilder@entry=0x7fffffffbfa0, callerResumePoint=callerResumePoint@entry=0x7ffff4518438, callInfo=...) at js/src/jit/IonBuilder.cpp:1007 #8 0x0000000000652308 in js::jit::IonBuilder::inlineScriptedCall (this=this@entry=0x7fffffffbfa0, callInfo=..., target=<optimized out>) at js/src/jit/IonBuilder.cpp:3694 #9 0x00000000006528e0 in js::jit::IonBuilder::inlineSingleCall (this=this@entry=0x7fffffffbfa0, callInfo=..., targetArg=targetArg@entry=0x7ffff46a3540) at js/src/jit/IonBuilder.cpp:4222 #10 0x0000000000653c36 in js::jit::IonBuilder::inlineCallsite (this=this@entry=0x7fffffffbfa0, targets=..., callInfo=...) at js/src/jit/IonBuilder.cpp:4276 #11 0x0000000000653f37 in js::jit::IonBuilder::jsop_call (this=this@entry=0x7fffffffbfa0, argc=1, constructing=<optimized out>, ignoresReturnValue=<optimized out>) at js/src/jit/IonBuilder.cpp:5275 #12 0x000000000064fb64 in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7fffffffbfa0, op=op@entry=JSOP_NEW) at js/src/jit/IonBuilder.cpp:2026 #13 0x0000000000651044 in js::jit::IonBuilder::visitBlock (this=this@entry=0x7fffffffbfa0, cfgblock=cfgblock@entry=0x7ffff4328150, mblock=<optimized out>) at js/src/jit/IonBuilder.cpp:1529 #14 0x00000000006513bf in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7fffffffbfa0) at js/src/jit/IonBuilder.cpp:1450 #15 0x0000000000651e9b in js::jit::IonBuilder::buildInline (this=this@entry=0x7fffffffbfa0, callerBuilder=callerBuilder@entry=0x7ffff450a190, callerResumePoint=callerResumePoint@entry=0x7ffff45177f0, callInfo=...) at js/src/jit/IonBuilder.cpp:1007 #16 0x0000000000652308 in js::jit::IonBuilder::inlineScriptedCall (this=this@entry=0x7ffff450a190, callInfo=..., target=<optimized out>) at js/src/jit/IonBuilder.cpp:3694 #17 0x00000000006528e0 in js::jit::IonBuilder::inlineSingleCall (this=this@entry=0x7ffff450a190, callInfo=..., targetArg=targetArg@entry=0x7ffff46a3580) at js/src/jit/IonBuilder.cpp:4222 #18 0x0000000000653c36 in js::jit::IonBuilder::inlineCallsite (this=this@entry=0x7ffff450a190, targets=..., callInfo=...) at js/src/jit/IonBuilder.cpp:4276 #19 0x0000000000653f37 in js::jit::IonBuilder::jsop_call (this=this@entry=0x7ffff450a190, argc=1, constructing=<optimized out>, ignoresReturnValue=<optimized out>) at js/src/jit/IonBuilder.cpp:5275 #20 0x000000000064fb64 in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7ffff450a190, op=op@entry=JSOP_CALL) at js/src/jit/IonBuilder.cpp:2026 #21 0x0000000000651044 in js::jit::IonBuilder::visitBlock (this=this@entry=0x7ffff450a190, cfgblock=cfgblock@entry=0x7ffff4328628, mblock=<optimized out>) at js/src/jit/IonBuilder.cpp:1529 #22 0x00000000006513bf in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7ffff450a190) at js/src/jit/IonBuilder.cpp:1450 #23 0x0000000000651900 in js::jit::IonBuilder::build (this=this@entry=0x7ffff450a190) at js/src/jit/IonBuilder.cpp:845 #24 0x0000000000430c7c in js::jit::IonCompile (cx=cx@entry=0x7ffff6924000, script=<optimized out>, baselineFrame=baselineFrame@entry=0x0, osrPc=osrPc@entry=0x0, recompile=<optimized out>, optimizationLevel=optimizationLevel@entry=js::jit::OptimizationLevel::Normal) at js/src/jit/Ion.cpp:2176 #25 0x0000000000662e1e in js::jit::Compile (cx=cx@entry=0x7ffff6924000, script=script@entry=..., osrFrame=osrFrame@entry=0x0, osrPc=osrPc@entry=0x0, forceRecompile=forceRecompile@entry=false) at js/src/jit/Ion.cpp:2440 #26 0x0000000000662f74 in js::jit::CanEnter (cx=cx@entry=0x7ffff6924000, state=...) at js/src/jit/Ion.cpp:2537 #27 0x0000000000503853 in js::RunScript (cx=0x7ffff6924000, state=...) at js/src/vm/Interpreter.cpp:386 #28 0x0000000000505ca5 in js::ExecuteKernel (result=0x7fffffffd1a8, evalInFrame=..., newTargetValue=..., envChainArg=..., script=..., cx=0x7ffff6924000) at js/src/vm/Interpreter.cpp:699 #29 js::Execute (cx=cx@entry=0x7ffff6924000, script=script@entry=..., envChainArg=..., rval=rval@entry=0x7fffffffd1a8) at js/src/vm/Interpreter.cpp:732 #30 0x0000000000797f11 in ExecuteScript (cx=cx@entry=0x7ffff6924000, scope=scope@entry=..., script=script@entry=..., rval=rval@entry=0x7fffffffd1a8) at js/src/jsapi.cpp:4559 #31 0x000000000079fd4b in JS_ExecuteScript (cx=cx@entry=0x7ffff6924000, scriptArg=scriptArg@entry=..., rval=rval@entry=...) at js/src/jsapi.cpp:4585 #32 0x0000000000453013 in Evaluate (cx=0x7ffff6924000, argc=<optimized out>, vp=0x7fffffffd1a8) at js/src/shell/js.cpp:1655 #33 0x0000000000503ac7 in js::CallJSNative (args=..., native=0x452ba0 <Evaluate(JSContext*, unsigned int, JS::Value*)>, cx=0x7ffff6924000) at js/src/jscntxtinlines.h:293 #34 js::InternalCallOrConstruct (cx=cx@entry=0x7ffff6924000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:470 #35 0x00000000005047d5 in InternalCall (args=..., cx=0x7ffff6924000) at js/src/vm/Interpreter.cpp:515 #36 js::CallFromStack (cx=cx@entry=0x7ffff6924000, args=...) at js/src/vm/Interpreter.cpp:521 #37 0x00000000005a976e in js::jit::DoCallFallback (cx=0x7ffff6924000, frame=0x7fffffffd1f8, stub_=0x7ffff4327250, argc=1, vp=0x7fffffffd1a8, res=...) at js/src/jit/BaselineIC.cpp:2455 [...] #60 0x0000000000619ba1 in js::jit::CreateMIRRootList (builder=...) at js/src/jit/IonAnalysis.cpp:4775 #61 0x0000000000000000 in ?? () rax 0x1b16580 28403072 rbx 0xe0c845 14731333 rcx 0xe 14 rdx 0x7ffff450e0a8 140737292329128 rsi 0xe 14 rdi 0x7ffff431d378 140737290294136 rbp 0x7fffffffb5f8 140737488336376 rsp 0x7fffffffb470 140737488335984 r8 0x7ffff46a0ec0 140737293979328 r9 0x7fffffffb5f8 140737488336376 r10 0x7fffffffb500 140737488336128 r11 0x7fffffffb4c0 140737488336064 r12 0x0 0 r13 0x7ffff450e030 140737292329008 r14 0x7ffff4518428 140737292370984 r15 0x1 1 rip 0x6c4ab2 <js::jit::TypeSetIncludes(js::TypeSet*, js::jit::MIRType, js::TypeSet*)+290> => 0x6c4ab2 <js::jit::TypeSetIncludes(js::TypeSet*, js::jit::MIRType, js::TypeSet*)+290>: movl $0x0,0x0 0x6c4abd <js::jit::TypeSetIncludes(js::TypeSet*, js::jit::MIRType, js::TypeSet*)+301>: ud2 Looks like a safe release-abort, but a debug build doesn't trigger any assertion or MOZ_CRASH message.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, failed due to error (try manually).
Flags: needinfo?(jdemooij)
(In reply to Christian Holler (:decoder) from comment #0) > Looks like a safe release-abort, but a debug build doesn't trigger any > assertion or MOZ_CRASH message. With --no-threads --ion-eager I get Hit MOZ_CRASH(Bad input type) at js/src/jit/MIR.cpp:2637
The problem is the last line of this testcase: -- function T(a) { this.actual = a; } function r(actual) { new T(actual); } r(0); r(0); evaluate("for (let [x = r(x)] of x) {}"); -- For |x| passed to r(x), we emit a JSOP_GETLOCAL but no lexical check. Then Ion gets confused when it sees MIRType::MagicUninitializedLexical where it doesn't expect it. Shu, any thoughts? Is the frontend correct?
Flags: needinfo?(jdemooij) → needinfo?(shu)
Due to skipped revisions, the first bad revision could be any of: changeset: https://hg.mozilla.org/mozilla-central/rev/cb6fc6d38f8d user: Shu-yu Guo date: Thu Aug 25 01:28:47 2016 -0700 summary: Bug 1263355 - Rewrite the frontend: bindings. (r=jorendorff,Waldo) changeset: https://hg.mozilla.org/mozilla-central/rev/18bec78f348e user: Shu-yu Guo date: Thu Aug 25 01:28:47 2016 -0700 summary: Bug 1263355 - Report memory metrics for Scopes. (r=njn) Perhaps bug 1263355 is related?
Blocks: 1263355
(In reply to Jan de Mooij [:jandem] from comment #3) > The problem is the last line of this testcase: > -- > function T(a) { > this.actual = a; > } > function r(actual) { > new T(actual); > } > r(0); > r(0); > evaluate("for (let [x = r(x)] of x) {}"); > -- > For |x| passed to r(x), we emit a JSOP_GETLOCAL but no lexical check. Then > Ion gets confused when it sees MIRType::MagicUninitializedLexical where it > doesn't expect it. Shu, any thoughts? Is the frontend correct? |for (let-decl of expr) {}| creates a lexical scope that encompasses |expr|. The initial few bytecodes for the example above are: 00000: uninitialized # UNINITIALIZED 00001: initlexical 0 # UNINITIALIZED 00005: pop # 00006: checklexical 0 # 00010: getlocal 0 # x ... So there's a JSOP_CHECKLEXICAL that dominates the entire rest of the code, which is why there's no JSOP_CHECKLEXICAL for the |x| that gets passed to |r(x)|. This code also unconditionally throws a TDZ error. How does the MagicUninitializedLexical get into r(x)'s typeset?
Flags: needinfo?(shu)
(ni? jan for shu's question)
Flags: needinfo?(jdemooij)
Marking sec-critical because of a 54b11 crash with an EXEC crash with a wild address. https://crash-stats.mozilla.com/report/index/67376fbe-4161-4802-bb8a-f0c7d0170530
Group: core-security
(In reply to Randell Jesup [:jesup] from comment #7) > Marking sec-critical because of a 54b11 crash with an EXEC crash with a wild > address. > https://crash-stats.mozilla.com/report/index/67376fbe-4161-4802-bb8a- > f0c7d0170530 Pretty sure this is a different crash. The crash here hits the MOZ_CRASH (release abort) case where `input` is none of the types specified in the switch case. The crash-stats crash hits the MIRType::Value case. But I'll let JS developers decide on this.
Perhaps both are related to using reallocated memory to get the type from
Yes it's a different crash from the one on crash-stats. Ted, would you mind investigating this one?
Flags: needinfo?(jdemooij) → needinfo?(tcampbell)
Yep, will look at it today.
Looks like Ion determines |x| will be UninitializedLexical, but tries to inline the |r(x)| call, resulting in a SETPROP using UninitializedLexical as a rhs. Having something like PropertyTypeIncludes return false on UninitializedLexical (instead of calling down to TypeSetIncludes) would solve. I'll poke around for best place to apply fix and see if there are any obvious related bugs.
Agreed, but it would still be interesting to understand why Ion thinks it can be UninitializedLexical (see comment 5) :)
In BytecodeEmitter::emitForOf, we use a single TDZCheckCache for both the iterator expression and the body. The argument being roughly that the header dominates the body. The problem is that the LexicalEnvironment for the iterator expression is distinct from the body and TDZCheckCache shouldn't be reused. This has not been a problem in past, because any TDZCheck emitted during the iterator expression will throw a TDZ exception and the body will never execute (so we don't notice we have less-than-ideal bytecode for the body). The Ion crash doesn't have TDZ in the TypeSet, but attempts to check if a TDZ is compatible with the (unknown()) TypeSet. Normally, a CHECKLEXICAL is between these. I propose leaving Ion as is, and have frontend be more precise about bytecode by using a distinct TDZCheckCache. In normal code this isn't used, but it makes these degenerate cases better behaved. > --- a/js/src/frontend/BytecodeEmitter.cpp > +++ b/js/src/frontend/BytecodeEmitter.cpp > @@ -7240,7 +7240,7 @@ BytecodeEmitter::emitForOf(ParseNode* forOfLoop, EmitterScope* headLexicalEmitte > } > > // Evaluate the expression being iterated. > - if (!emitTree(forHeadExpr)) // ITERABLE > + if (!emitTreeInBranch(forHeadExpr)) // ITERABLE > return false; > if (iterKind == IteratorKind::Async) { > if (!emitAsyncIterator()) // ITER I'll cleanup the patch and test it.
Flags: needinfo?(tcampbell)
Use separate TDZCheckCache for for-of/for-in iteration expression. This generates more correct bytecode instead of relying on the script throwing before slot would be reset to TDZ.
Attachment #8877679 - Flags: review?(shu)
Assignee: nobody → tcampbell
Group: core-security → javascript-core-security
Comment on attachment 8877679 [details] [diff] [review] 0001-Bug-1368360-Use-distinct-TDZCheckCache-for-for-of-fo.patch Review of attachment 8877679 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the explanation on IRC. Good find. ::: js/src/frontend/BytecodeEmitter.cpp @@ +7239,5 @@ > allowSelfHostedIter = true; > } > > + // Evaluate the expression being iterated. The forHeadExpr should use a > + // distinct TDZCheckCache to evaluate since (abstractly) it runs in it's Nit: its
Attachment #8877679 - Flags: review?(shu) → review+
For context: I had originally thought, per comment 5, that the subsequent TDZ checks were omitted because they were dominated by the header TDZ check. But this is incorrect -- the optimization the frontend was doing was that, since the header expression of for-of/for-in loops don't change, subsequent TDZ checks would've been dead code. This tripped up an Ion sanity check.
https://hg.mozilla.org/integration/mozilla-inbound/rev/29d1a9af2655 Final assessment is that MOZ_CRASH is more of a sanity check and not indicative of corrupted state. (Typesets are intact.) This doesn't occur in the wild so marking everything but nightly as wontfix.
Keywords: sec-other
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Group: javascript-core-security → core-security-release
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-min56-]
Whiteboard: [jsbugmon:update][adv-min56-] → [jsbugmon:update][adv-main56-]
Group: core-security-release
> the explanation on IRC. for future reference, here's the log https://mozilla.logbot.info/jsapi/20170614#c848252 I'll add some more comment about the reasoning, in bug 1456404 patch.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: