Closed Bug 1700616 Opened 4 years ago Closed 4 years ago

Assertion failure: phiType != MIRType::None, at jit/IonAnalysis.cpp:1680 or Crash [@ js::jit::LIRGenerator::visitUnbox]

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
89 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox86 --- unaffected
firefox87 --- unaffected
firefox88 --- unaffected
firefox89 --- verified

People

(Reporter: decoder, Assigned: iain)

References

(Regression)

Details

(4 keywords, Whiteboard: [bugmon:update,bisected,confirmed])

Crash Data

Attachments

(3 files)

The following testcase crashes on mozilla-central revision 20210324-768e04aaea52 (debug build, run with --fuzzing-safe --ion-offthread-compile=off):

function a() {
    while (10)
        for (b = 2; b;) {
            d = {
                1: ""
            }
            for (f in d) {
                const g = this
            }
        }
}
a()

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x000055555789a753 in (anonymous namespace)::TypeAnalyzer::analyze() ()
#1  0x000055555788f151 in js::jit::ApplyTypeInformation(js::jit::MIRGenerator*, js::jit::MIRGraph&) ()
#2  0x0000555557888a18 in js::jit::OptimizeMIR(js::jit::MIRGenerator*) ()
#3  0x00005555578921fc in js::jit::CompileBackEnd(js::jit::MIRGenerator*, js::jit::WarpSnapshot*) ()
#4  0x0000555557893ae2 in js::jit::Compile(JSContext*, JS::Handle<JSScript*>, js::jit::BaselineFrame*, unsigned char*) ()
#5  0x0000555557894502 in IonCompileScriptForBaseline(JSContext*, js::jit::BaselineFrame*, unsigned char*) ()
#6  0x0000555557894d1a in js::jit::IonCompileScriptForBaselineOSR(JSContext*, js::jit::BaselineFrame*, unsigned int, unsigned char*, js::jit::IonOsrTempData**) ()
#7  0x00002875717ee8c7 in ?? ()
[...]
#11 0x0000000000000000 in ?? ()
rax	0x5555557b0ead	93824994709165
rbx	0x7ffff60eb068	140737321545832
rcx	0x555557fff3b8	93825036972984
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffffb200	140737488335360
rsp	0x7fffffffb170	140737488335216
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f99840	140737353717824
r10	0x0	0
r11	0x0	0
r12	0x7fffffffb210	140737488335376
r13	0x7ffff60ea930	140737321543984
r14	0x13	19
r15	0x7ffff60eaf80	140737321545600
rip	0x55555789a753 <(anonymous namespace)::TypeAnalyzer::analyze()+9187>
=> 0x55555789a753 <_ZN12_GLOBAL__N_112TypeAnalyzer7analyzeEv+9187>:	movl   $0x690,0x0
   0x55555789a75e <_ZN12_GLOBAL__N_112TypeAnalyzer7analyzeEv+9198>:	callq  0x555556a8180f <abort>
Attached file Testcase (deleted) —

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20210324084848-3be60f42358a.
The bug appears to have been introduced in the following build range:

Start: 177159d72cd3fd7cff403260f2df51524a2cc385 (20210323222054)
End: ab7497109e62f2122bf0ac3babc1b1942727cd2c (20210323222145)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=177159d72cd3fd7cff403260f2df51524a2cc385&tochange=ab7497109e62f2122bf0ac3babc1b1942727cd2c

Whiteboard: [bugmon:update,bisect] → [bugmon:update,bisected,confirmed]

Not a security bug: this will (and does!) crash safely in non-debug builds. In debug builds we crash in adjustPhiInputs when we see MIRType::None; in non-debug builds we instead insert an unbox to None, then crash later when we try to lower that unbox.

The problem is that the change to guessPhiType in this patch didn't go far enough. We can handle phis that have two OsrValue operands, but we crash if we have a cycle of phis where all the operands are either OsrValues or other phis in the cycle.

Assignee: nobody → iireland
Group: javascript-core-security

The new implementation of branch pruning in bug 1697696 can remove the path from the entry block to the OSR preheader. In this case, phis in the preheader only have one argument and can be removed, which means that phis elsewhere can have multiple OsrValues as operands.

In the original pruning patches, I handled the case where a phi only had OsrValue operands by forcing its type to MIRType::Value, but I didn't cover the case where we have a cycle of phis that only depend on each other and on OsrValue operands. This patch fixes that.

I initially tried adding a new MIRType to represent this case during phi specialization, but decided that changing phi->type() == MIRType::None to phi->type() == MIRType::None || phi->type() == MIRType::OsrValue everywhere in TypeAnalyzer wasn't actually an improvement.

Note that when we specialize these phis, we are not propagating MIRType::Value to their uses. In an earlier version of this patch I tried that, but realized it's unnecessary. If the use has a non-Value type, then adjustPhiInputs will add an optimistic unbox. (The testcase includes a case where we specialize incorrectly and bail out, to verify that this works.)

Out of an abundance of caution, I'm turning branch pruning off by default so that we can fuzz it for a day or so.

While I'm here, I renamed the option from --ion-pgo to --ion-pruning because it's not really PGO anymore.

Depends on D109686

Pushed by iireland@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e8691d90fb57 Specialize phis that only depend on OsrValues r=jandem https://hg.mozilla.org/integration/autoland/rev/ee10e8481d49 Turn branch pruning off by default r=jandem

Change the status for beta to have the same as nightly and release.
For more information, please visit auto_nag documentation.

Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 89 Branch

This bug has a known regressor, which only landed in 89 (and was backed out immediately). More than 97%+ of all crashes with this signature occurred in a single build (20210324084848).

Any crashes with similar signatures in other releases are unrelated.

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

Status: RESOLVED → VERIFIED
Keywords: bugmon

:iain, since this bug contains a bisection range, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Flags: needinfo?(iireland)
Flags: needinfo?(iireland)
Regressed by: 1697696
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: