Closed
Bug 1368360
Opened 8 years ago
Closed 7 years ago
Crash [@ js::jit::TypeSetIncludes]
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla56
People
(Reporter: decoder, Assigned: tcampbell)
References
Details
(5 keywords, Whiteboard: [jsbugmon:update][adv-main56-])
Crash Data
Attachments
(1 file)
(deleted),
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•8 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 1•8 years ago
|
||
JSBugMon: Bisection requested, failed due to error (try manually).
Updated•8 years ago
|
Flags: needinfo?(jdemooij)
Comment 2•8 years ago
|
||
(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
Comment 3•8 years ago
|
||
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
Comment 5•7 years ago
|
||
(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)
Updated•7 years ago
|
status-firefox53:
--- → wontfix
status-firefox54:
--- → wontfix
status-firefox-esr52:
--- → affected
Comment 7•7 years ago
|
||
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
Reporter | ||
Comment 8•7 years ago
|
||
(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.
Comment 9•7 years ago
|
||
Perhaps both are related to using reallocated memory to get the type from
Comment 10•7 years ago
|
||
Yes it's a different crash from the one on crash-stats.
Ted, would you mind investigating this one?
Flags: needinfo?(jdemooij) → needinfo?(tcampbell)
Assignee | ||
Comment 11•7 years ago
|
||
Yep, will look at it today.
Updated•7 years ago
|
Keywords: csectype-wildptr,
sec-critical
Assignee | ||
Comment 12•7 years ago
|
||
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.
Comment 13•7 years ago
|
||
Agreed, but it would still be interesting to understand why Ion thinks it can be UninitializedLexical (see comment 5) :)
Assignee | ||
Comment 14•7 years ago
|
||
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)
Assignee | ||
Comment 15•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → tcampbell
Assignee | ||
Comment 16•7 years ago
|
||
Updated•7 years ago
|
Group: core-security → javascript-core-security
Comment 17•7 years ago
|
||
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+
Comment 18•7 years ago
|
||
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.
Assignee | ||
Comment 19•7 years ago
|
||
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.
Comment 20•7 years ago
|
||
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
Group: javascript-core-security → core-security-release
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Comment 21•7 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•7 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-min56-]
Updated•7 years ago
|
Whiteboard: [jsbugmon:update][adv-min56-] → [jsbugmon:update][adv-main56-]
Updated•7 years ago
|
Group: core-security-release
Comment 22•6 years ago
|
||
> 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.
Description
•