1620215 - Assertion failure: false (MOZ_ASSERT_UNREACHABLE: Unexpected instruction), at jit/BaselineInspector.cpp:1515

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20200305-2e1a978b09d7 (build with --enable-debug, run with --fuzzing-safe --no-threads):

var height = 267;
function test() {
  for (var j61 = -6; j61 < 7; ++j61)
    for (typeof arguments[0] == "object" ? 0 : arguments = height ;;) {}
}
test();

Backtrace:

received signal SIGSEGV, Segmentation fault.
0x00005555563b4156 in js::jit::BaselineInspector::expectedPropertyAccessInputType(unsigned char*) ()
#0  0x00005555563b4156 in js::jit::BaselineInspector::expectedPropertyAccessInputType(unsigned char*) ()
#1  0x0000555556616801 in js::jit::IonBuilder::maybeUnboxForPropertyAccess(js::jit::MDefinition*) ()
#2  0x00005555565f3842 in js::jit::IonBuilder::jsop_getelem() ()
#3  0x00005555565e1991 in js::jit::IonBuilder::inspectOpcode(JSOp, bool*) ()
#4  0x00005555565ddc04 in js::jit::IonBuilder::traverseBytecode() ()
#5  0x00005555565d5396 in js::jit::IonBuilder::build() ()
#6  0x00005555565c9c0c in js::jit::Compile(JSContext*, JS::Handle<JSScript*>, js::jit::BaselineFrame*, unsigned int, unsigned char*, bool) ()
#7  0x00005555565cadef in IonCompileScriptForBaseline(JSContext*, js::jit::BaselineFrame*, unsigned int, unsigned char*) ()
#8  0x00005555565cb3bd in js::jit::IonCompileScriptForBaselineOSR(JSContext*, js::jit::BaselineFrame*, unsigned int, unsigned char*, js::jit::IonOsrTempData**) ()
#9  0x00003c3003870587 in ?? ()
#10 0x00007fffffffb788 in ?? ()
#11 0x00007fffffffb700 in ?? ()
#12 0x0000000000000000 in ?? ()
rax	0x555556f4e002	93825019469826
rbx	0x7ffff5c14058	140737316470872
rcx	0x555557ef4850	93825035880528
rdx	0x0	0
rsi	0x7ffff6efd770	140737336301424
rdi	0x7ffff6efc540	140737336296768
rbp	0x7fffffffad40	140737488334144
rsp	0x7fffffffad00	140737488334080
r8	0x7ffff6efd770	140737336301424
r9	0x7ffff7f9cd00	140737353731328
r10	0x58	88
r11	0x7ffff6ba47a0	140737332791200
r12	0x555556f47c84	93825019444356
r13	0x7ffff5c17878	140737316485240
r14	0x7fffffffad00	140737488334080
r15	0x7ffff5c17812	140737316485138
rip	0x5555563b4156 <js::jit::BaselineInspector::expectedPropertyAccessInputType(unsigned char*)+838>
=> 0x5555563b4156 <_ZN2js3jit17BaselineInspector31expectedPropertyAccessInputTypeEPh+838>:	movl   $0x5eb,0x0
   0x5555563b4161 <_ZN2js3jit17BaselineInspector31expectedPropertyAccessInputTypeEPh+849>:	callq  0x5555557ef05e <abort>

JIT bug, marking s-s until investigated further.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Nicolas B. Pierron [:nbp] {away}]

André, can you look at this issue?

Comment 3

[Daniel Veditz [:dveditz]]

The assertion and stack looks vaguely like a type confusion type bug: rating accordingly. If our brief skim is wrong and it's not that bad please correct or clear the rating and we'll re-triage.

Comment 4

[André Bargull [:anba]]

The assertion started with 0c0c89b7a859a19ddc9d788a45d16c3e150b1bc8 (bug 1595776), so maybe Jan can take a look?

Slightly reduced test case:

function test() {
  for (var i = 0; i < 10; ++i) {
    // Create an IC specialized for lazy arguments when Baseline runs.
    arguments[0];

    // Overwrite |arguments|, so that Ion assumes it's never lazy arguments.
    arguments = 0;

    for (;;) {}
    // while(true) {} // No error when a |while| loop is used.
  }
}
test();

Interestingly there is no assertion failure when while(true) is used instead of for (;;).

Comment 5

[André Bargull [:anba]]

(If it wasn't for the different outcome when while(true) is used, I'd say all it takes for this bug is to update GetCacheIRExpectedInputType to handle GetPropIRGenerator::tryAttachMagicArgument.)

Comment 6

[Iain Ireland [:iain]]

I've poked at this. It looks like the difference between for (;;) and while(true) is that IonBuilder is only smart enough to realize that the former is an infinite loop. Here is the relevant bytecode from while (;;):

# from Goto @ 00098
00081:  LoopHead (ic: 7, depthHint: 2)  # 
00087:  True                            # true
00088:  IfEq 103 (+15)                  # 
00093:  JumpTarget (ic: 8)              # 
00098:  Goto 81 (-17)                   # 
# from IfEq @ 00088
00103:  JumpTarget (ic: 8)              # 
00108:  GetLocal 0                      # i
00112:  ToNumeric                       # (tonumeric i)
00113:  Inc                             # (inc (tonumeric i))
00114:  SetLocal 0                      # (inc (tonumeric i))
00118:  Pop                             # 
00119:  Goto 12 (-107)                  # 

The goto on line 119 creates a back-edge to the beginning of the outer loop, which generates a phi node for arguments. This triggers the check here, so the script is marked as needsArgsObj.

Compare to the relevant bytecode from for (;;):

00080:  LoopHead (ic: 7, depthHint: 2)  # 
00086:  Goto 80 (-6)                    # 
00091:  JumpTarget (ic: 7)              # !!! UNREACHABLE !!!
00096:  GetLocal 0                      # !!! UNREACHABLE !!!
00100:  ToNumeric                       # !!! UNREACHABLE !!!
00101:  Inc                             # !!! UNREACHABLE !!!
00102:  SetLocal 0                      # !!! UNREACHABLE !!!
00106:  Pop                             # !!! UNREACHABLE !!!
00107:  Goto 12 (-95)                   # !!! UNREACHABLE !!!

The goto on line 80 terminates the block. In this case, there are no pending edges for the jumptarget at offset 91, so we don't start a new block until after the goto on line 107, and ignore all of the unreachable ops. This means that there's no back-edge for the outer loop, which means that there's no phi for arguments, so the script is marked as not needing an args obj.

It looks like IonBuilder::jsop_getelem tries to handle lazy arguments first, before it gets into the code that calls GetCacheIRExpectedInputType, so the correct fix is probably not in GetCacheIRExpectedInputType. I don't know what the correct fix is, though.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Iain Ireland [:iain] from comment #6)

It looks like IonBuilder::jsop_getelem tries to handle lazy arguments first, before it gets into the code that calls GetCacheIRExpectedInputType, so the correct fix is probably not in GetCacheIRExpectedInputType. I don't know what the correct fix is, though.

What happens is that we attach a lazy-args Baseline IC stub for arguments[0], but because this is out-of-bounds we also deoptimize lazy args (JSScript::argumentsOptimizationFailed). So I think we should (a) be handling this in GetCacheIRExpectedInputType (b) not attach an IC stub if OOB.

Comment 8

[Jan de Mooij [:jandem]]

Oh and thanks for analyzing these bugs btw!

Comment 9

[Jan de Mooij [:jandem]]

Not a security bug. Opt builds do the right thing.

Comment 10

[Jan de Mooij [:jandem]]

(Gah, I can't type today.)

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Bug 1620215 - Handle GuardMagicValue in GetCacheIRExpectedInputType. r?iain!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e849fc3f8c3
Handle GuardMagicValue in GetCacheIRExpectedInputType. r=iain

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2e849fc3f8c3

Comment 14

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20200422163521-2203d818a3b4.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.