1841082 - Assertion failure: icEntry->firstStub() == stub, at jit/BaselineIC.cpp:469

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

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

class c {
    constructor() {
        b();
        super[1];
    }
}
function b() {
    new c;
}
b();

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x58837ee0 in js::jit::ICFallbackStub::unlinkStub(JS::Zone*, js::jit::ICEntry*, js::jit::ICCacheIRStub*, js::jit::ICCacheIRStub*) ()
#1  0x58d15f4b in WarpScriptOracle::maybeInlineCall(mozilla::LinkedList<js::jit::WarpOpSnapshot>&, js::BytecodeLocation, js::jit::ICCacheIRStub*, js::jit::ICFallbackStub*, unsigned char*) ()
#2  0x58d14a5d in WarpScriptOracle::maybeInlineIC(mozilla::LinkedList<js::jit::WarpOpSnapshot>&, js::BytecodeLocation) ()
#3  0x58d11406 in WarpScriptOracle::createScriptSnapshot() ()
#4  0x58d15d94 in WarpScriptOracle::maybeInlineCall(mozilla::LinkedList<js::jit::WarpOpSnapshot>&, js::BytecodeLocation, js::jit::ICCacheIRStub*, js::jit::ICFallbackStub*, unsigned char*) ()
#5  0x58d14a5d in WarpScriptOracle::maybeInlineIC(mozilla::LinkedList<js::jit::WarpOpSnapshot>&, js::BytecodeLocation) ()
#6  0x58d11406 in WarpScriptOracle::createScriptSnapshot() ()
#7  0x58d15d94 in WarpScriptOracle::maybeInlineCall(mozilla::LinkedList<js::jit::WarpOpSnapshot>&, js::BytecodeLocation, js::jit::ICCacheIRStub*, js::jit::ICFallbackStub*, unsigned char*) ()
#8  0x58d14a5d in WarpScriptOracle::maybeInlineIC(mozilla::LinkedList<js::jit::WarpOpSnapshot>&, js::BytecodeLocation) ()
#9  0x58d11406 in WarpScriptOracle::createScriptSnapshot() ()
#10 0x58d15d94 in WarpScriptOracle::maybeInlineCall(mozilla::LinkedList<js::jit::WarpOpSnapshot>&, js::BytecodeLocation, js::jit::ICCacheIRStub*, js::jit::ICFallbackStub*, unsigned char*) ()
#11 0x58d14a5d in WarpScriptOracle::maybeInlineIC(mozilla::LinkedList<js::jit::WarpOpSnapshot>&, js::BytecodeLocation) ()
#12 0x58d11406 in WarpScriptOracle::createScriptSnapshot() ()
#13 0x58d15d94 in WarpScriptOracle::maybeInlineCall(mozilla::LinkedList<js::jit::WarpOpSnapshot>&, js::BytecodeLocation, js::jit::ICCacheIRStub*, js::jit::ICFallbackStub*, unsigned char*) ()
#14 0x58d14a5d in WarpScriptOracle::maybeInlineIC(mozilla::LinkedList<js::jit::WarpOpSnapshot>&, js::BytecodeLocation) ()
#15 0x58d11406 in WarpScriptOracle::createScriptSnapshot() ()
#16 0x58d10d02 in js::jit::WarpOracle::createSnapshot() ()
#17 0x58c8a47c in js::jit::CreateWarpSnapshot(JSContext*, js::jit::MIRGenerator*, JS::Handle<JSScript*>) ()
#18 0x58c83043 in js::jit::Compile(JSContext*, JS::Handle<JSScript*>, js::jit::BaselineFrame*, unsigned char*) ()
#19 0x58c83ea6 in IonCompileScriptForBaseline(JSContext*, js::jit::BaselineFrame*, unsigned char*) ()
#20 0x58c83970 in js::jit::IonCompileScriptForBaselineAtEntry(JSContext*, js::jit::BaselineFrame*) ()
#21 0x5e746b4e in ?? ()
[...]
#127 0x5e76e394 in ?? ()
eax	0x566c3d3b	1449934139
ebx	0x595248d0	1498564816
ecx	0x595264d4	1498571988
edx	0x0	0
esi	0xf3242010	-215736304
edi	0x595248d0	1498564816
ebp	0xfffa2a78	4294584952
esp	0xfffa2a50	4294584912
eip	0x58837ee0 <js::jit::ICFallbackStub::unlinkStub(JS::Zone*, js::jit::ICEntry*, js::jit::ICCacheIRStub*, js::jit::ICCacheIRStub*)+352>
=> 0x58837ee0 <_ZN2js3jit14ICFallbackStub10unlinkStubEPN2JS4ZoneEPNS0_7ICEntryEPNS0_13ICCacheIRStubES8_+352>:	movl   $0x1d5,0x0
   0x58837eea <_ZN2js3jit14ICFallbackStub10unlinkStubEPN2JS4ZoneEPNS0_7ICEntryEPNS0_13ICCacheIRStubES8_+362>:	call   0x57bd069f <abort>

Reproduces on 32-bit only for me. Marking s-s because it is an assert in JIT internals.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20230629153145-e784085dfb50.
Unable to bisect testcase (Unable to launch the start build!):

Start: 65e579f52525f9dc53be7ace5fd4452d4f835588 (20220630095519)
End: c93a9e0ad90d7eca1077e5b52a84b577549bc02e (20230629085424)
BuildFlags: BuildFlags(asan=False, tsan=False, debug=True, fuzzing=False, coverage=False, valgrind=False, no_opt=False, fuzzilli=False, nyx=False)

Comment 4

[Iain Ireland [:iain]]

This is x86-specific, due to this code.

Two functions are calling each other recursively, b and c. Neither function has a polymorphic or megamorphic IC, so during trial inlining we decide to inline them monomorphically. When we go to build a snapshot for c, we inline b. This recursively makes a snapshot of b, which inlines c. The process continues until we hit the maximum depth (8), at which point we make a regular call instead of an inlined call. We continue building the innermost snapshot. The next interesting op is a GetElemSuper. Because Ion on x86 doesn't have enough registers for GetElemSuper, we abort the snapshot, disable Ion compilation for c, and clean up. As part of the process, we unlink the call stub.

Then we return to an outer copy of c, which aborts for the same reason. The caller of that copy of c also tries to clean up. However, we've already unlinked the call stub, so this assertion fails. This can only happen with monomorphic inlining. With trial inlining, it's impossible to see the same IC more than once, because we create private ICScripts per-callee.

The next line is a no-op: it will set icEntry->firstStub_, which currently points to the fallback stub, to stub->next(), which is also the fallback stub. In a release build, this will all end up working out correctly, and there's no security concern. However, after that we decrement numOptimizedStubs_, which can underflow. I don't actually see anything that can go wrong if we underflow there, but it's at least a little concerning.

I've split the test/comments/assertions out into a separate patch to be extra-careful, although it's likely that it will turn out this is unexploitable.

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 1841082: Don't unlink empty IC chain r=jandem!

Comment 6

[Iain Ireland [:iain]]

Attachment: Bug 1841082: Add testcase and comment r=jandem!

Depends on D182530

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 8

[Andrew McCreight [:mccr8]]

Is there more of a security impact here than "I don't actually see anything that can go wrong if we underflow there, but it's at least a little concerning."? Do you think this should be sec-high or is maybe sec-moderate better if this is like something kind of going wrong but there's no clear exploitability? Thanks.

Comment 9

[Iain Ireland [:iain]]

I think sec-moderate is sufficient.

I've done a second pass over the uses of numOptimizedStubs_, and it is still the case that the only things that depend on it are assertions and heuristics. Our bookkeeping of the number of stubs in the IC chain is wrong, but the IC chain itself remains in a correct state. "Something kind of going wrong but there's no clear exploitability" is an accurate description.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Bugmon [:jkratzer for issues]]

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

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:iain, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox116 to wontfix.

For more information, please visit BugBot documentation.

Comment 14

[Iain Ireland [:iain]]

This only affects 32-bit x86 and there's no clear exploitability. I think it can ride the trains.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Given the support lifecycle ahead of us for the branch, I think it might make sense to take this on ESR115 still. Please nominate for approval if you agree. It grafts cleanly.

Comment 16

[Iain Ireland [:iain]]

Comment on attachment 9341846 [details]
Bug 1841082: Don't unlink empty IC chain r=jandem!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Sec-moderate bug where things go wrong, but there's no clear exploitability. Given the long support lifecycle for ESR115, maybe safest to uplift.
• User impact if declined: Possible x86-specific exploit if we missed something exploitable.
• Fix Landed on Version: 117
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small fix, validated in Nightly.

Alternative is to not uplift.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9341846 [details]
Bug 1841082: Don't unlink empty IC chain r=jandem!

Approved for 115.2esr.

Comment 18

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/d04c00458180

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/176b6edd9b70
Add testcase and comment r=jandem

Comment 20

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/176b6edd9b70