Christian Holler (:decoder) Reporter
	Description • 7 years ago

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.

	Fuzzing Team
	Comment 1 • 7 years ago

JSBugMon: Bisection requested, failed due to error (try manually).

	Jan de Mooij [:jandem]
	Comment 2 • 7 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

	Jan de Mooij [:jandem]
	Comment 3 • 7 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?

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 4 • 7 years ago

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?

	Shu-yu Guo [:shu]
	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?

	Mike Taylor [:miketaylr]
	Comment 6 • 7 years ago

(ni? jan for shu's question)

	Randell Jesup [:jesup] (needinfo me)
	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

	Christian Holler (:decoder) 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.

	Randell Jesup [:jesup] (needinfo me)
	Comment 9 • 7 years ago

Perhaps both are related to using reallocated memory to get the type from

	Jan de Mooij [:jandem]
	Comment 10 • 7 years ago

Yes it's a different crash from the one on crash-stats.

Ted, would you mind investigating this one?

	Ted Campbell [:tcampbell] Assignee
	Comment 11 • 7 years ago

Yep, will look at it today.

	Ted Campbell [:tcampbell] 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.

	Jan de Mooij [:jandem]
	Comment 13 • 7 years ago

Agreed, but it would still be interesting to understand why Ion thinks it can be UninitializedLexical (see comment 5) :)

	Ted Campbell [:tcampbell] 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.

	Ted Campbell [: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.

	Ted Campbell [:tcampbell] Assignee
	Comment 16 • 7 years ago

Try in progress: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ea934ffe4fed61fd7bb45d4d0b00ad20c58dbcf

	Shu-yu Guo [:shu]
	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

	Shu-yu Guo [:shu]
	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.

	Ted Campbell [:tcampbell] 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.

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 7 years ago

https://hg.mozilla.org/mozilla-central/rev/29d1a9af2655

	Fuzzing Team
	Comment 21 • 7 years ago

JSBugMon: This bug has been automatically verified fixed.

	Tooru Fujisawa [:arai]
	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.