1744520 - Crash [@ js::jit::MBasicBlock::insertBefore] with OOM

Status: VERIFIED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20211206-422c7594bf65 (debug build, run with --fuzzing-safe --differential-testing --ion-warmup-threshold=0 --baseline-warmup-threshold=0 --no-threads):

t = new Uint8ClampedArray;
a = [];
Object.defineProperty(a,0,{
get: (function() {
  try {} catch(e) {}
  Array.prototype.shift.apply(t,[t]);
  t;
})
});
oomTest(Promise.resolve.bind)

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x0000555557b58ae3 in js::jit::MBasicBlock::insertBefore(js::jit::MInstruction*, js::jit::MInstruction*) ()
#1  0x000055555772da49 in js::jit::ArrayMemoryView::visitApplyArray(js::jit::MApplyArray*) ()
#2  0x0000555557734dda in js::jit::EmulateStateOf<js::jit::ArrayMemoryView>::run(js::jit::ArrayMemoryView&) ()
#3  0x00005555577335c1 in js::jit::ScalarReplacement(js::jit::MIRGenerator*, js::jit::MIRGraph&) ()
#4  0x0000555557a76c2c in js::jit::OptimizeMIR(js::jit::MIRGenerator*) ()
#5  0x0000555557a7f28c in js::jit::CompileBackEnd(js::jit::MIRGenerator*, js::jit::WarpSnapshot*) ()
#6  0x0000555557a809cc in js::jit::Compile(JSContext*, JS::Handle<JSScript*>, js::jit::BaselineFrame*, unsigned char*) ()
#7  0x0000555557a815cc in IonCompileScriptForBaseline(JSContext*, js::jit::BaselineFrame*, unsigned char*) ()
#8  0x00002fb10f745445 in ?? ()
[...]
#26 0x0000000000000000 in ?? ()
rax	0x7ffff5a39f08	140737314529032
rbx	0x0	0
rcx	0x7ffff5a39f28	140737314529064
rdx	0x0	0
rsi	0x7ffff5a3bfe8	140737314537448
rdi	0x7ffff5a3bfe8	140737314537448
rbp	0x7fffffff8910	140737488324880
rsp	0x7fffffff88f0	140737488324848
r8	0x0	0
r9	0x1	1
r10	0x7ffff5a3c480	140737314538624
r11	0x0	0
r12	0x1	1
r13	0x7fffffff8ab0	140737488325296
r14	0x7ffff5a3bfe8	140737314537448
r15	0x7ffff5a39f28	140737314529064
rip	0x555557b58ae3 <js::jit::MBasicBlock::insertBefore(js::jit::MInstruction*, js::jit::MInstruction*)+51>
=> 0x555557b58ae3 <_ZN2js3jit11MBasicBlock12insertBeforeEPNS0_12MInstructionES3_+51>:	movzwl 0x24(%rbx),%ecx
   0x555557b58ae7 <_ZN2js3jit11MBasicBlock12insertBeforeEPNS0_12MInstructionES3_+55>:	cmp    $0x96,%ecx

This looks like a null-deref but since it is a crash in JIT-related code, I'll mark it s-s until triaged by JS devs.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Verified bug as reproducible on mozilla-central 20211206092340-422c7594bf65.
The bug appears to have been introduced in the following build range:

Start: 1bedbf784ec9ecbcaa46956a4edd6cd933b38fec (20211109093712)
End: 333f08065c8c5470603b9d50b4dd45f8da8bfff5 (20211110092453)
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1bedbf784ec9ecbcaa46956a4edd6cd933b38fec&tochange=333f08065c8c5470603b9d50b4dd45f8da8bfff5

Comment 4

[Iain Ireland [:iain]]

This is just missing OOM handling in ScalarReplacement. It is not ss; it will crash immediately with a null deref.

When I wrote ArgumentsReplacer::visitApplyArgsObj, I forgot to handle OOM in MakeCall. When anba added support for ApplyArray and ConstructArray, he followed my code and made the same mistake. It's hard for oomTest to find oom-handling bugs inside Ion, because after the first OOM failure in Ion we cancel the compilation and won't start it again for many more iterations, but this testcase managed to hit the bullseye.

This is unlikely to be an issue in the real world. Small-OOM recovery is rare in telemetry; if we didn't crash here, we would crash somewhere else very soon.

I'll fix all three cases.

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 1744520: Fix OOM handling for variadic instructions r=jandem

Variadic instructions contain a FixedList of operands which is initialized fallibly in MVariadicT::init. This means MFoo::New is fallible for variadic instructions. A fuzz bug found one unhandled OOM in scalar replacement. I did a quick survey of places where we create new variadic nodes, and found a few more latent bugs that I introduced in my patches to scalar-replace arguments. I fixed those bugs and added a comment on MVariadicInstruction in the hopes of avoiding the same mistake in the future.

I'm not adding the fuzz testcase, because OOM tests of Ion internals are incredibly fragile and will stop working as soon as we add or remove one more allocation somewhere.

Comment 6

[BugBot [:suhaib / :marco]]

: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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7e2d5f68894
Fix OOM handling for variadic instructions r=jandem

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/a7e2d5f68894

Comment 9

[Bugmon [:jkratzer for issues]]

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

Comment 10

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1739650