1870925 - Assertion failure: [barrier verifier] Unmarked edge: JS Script 38d5a84660b0 'baseline-ic-stub-code' edge to JS JitCode 38d5a84672e0, at gc/Verifier.cpp:385

Status: RESOLVED FIXED

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

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

let x = [[]];
for (let i = 0; i < 22; i++) {
  for (let j = 0; j < 21; j++) {
    (function () {
      x[i] | 0;
    })();
  }
}
verifyprebarriers();
bailAfter(1);
schedulegc(1);
y;

Assertion failure: [barrier verifier] Unmarked edge: JS Script 338c74d660b0 'baseline-ic-stub-code' edge to JS JitCode 338c74d672e0, at /home/gen16gx500/trees/mozilla-central/js/src/gc/Verifier.cpp:385
#01: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x277e42e]
#02: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x26b61b2]
#03: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x2682d3a]
#04: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x2683299]
#05: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x26b9c40]
#06: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x1d073c6]
#07: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x215014b]
#08: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x216fc3f]
#09: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x21515a9]
#10: JS_NewStringCopyUTF8Z(JSContext*, JS::ConstUTF8CharsZ)[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x2317335]
#11: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x2327499]
#12: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x1eb8ce5]
#13: JS_ReportErrorNumberUTF8(JSContext*, JSErrorFormatString const* (*)(void*, unsigned int), void*, unsigned int, ...)[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x2303ebe]
#14: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x1f15505]
#15: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x1cf8978]
#16: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x1d08b89]
#17: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x2873b5b]
#18: ??? (???:???)
Hit MOZ_CRASH() at /home/gen16gx500/trees/mozilla-central/js/src/gc/Verifier.cpp:386
#01: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x277e442]
#02: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x26b61b2]
#03: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x2682d3a]
#04: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x2683299]
#05: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x26b9c40]
#06: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x1d073c6]
#07: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x215014b]
#08: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x216fc3f]
#09: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x21515a9]
#10: JS_NewStringCopyUTF8Z(JSContext*, JS::ConstUTF8CharsZ)[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x2317335]
#11: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x2327499]
#12: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x1eb8ce5]
#13: JS_ReportErrorNumberUTF8(JSContext*, JSErrorFormatString const* (*)(void*, unsigned int), void*, unsigned int, ...)[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x2303ebe]
#14: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x1f15505]
#15: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x1cf8978]
#16: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x1d08b89]
#17: ???[/home/gen16gx500/shell-cache/js-dbg-64-linux-x86_64-3fd71c45d9fc/js-dbg-64-linux-x86_64-3fd71c45d9fc +0x2873b5b]
#18: ??? (???:???)
Segmentation fault

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/dd251ff86895
user:        Jan de Mooij
date:        Wed Dec 13 11:53:13 2023 +0000
summary:     Bug 1867193 - Reset more IC state when discarding IC stubs. r=iain

Run with --fuzzing-safe --no-threads --no-ggc, compile with AR=ar sh ../configure --enable-debug --with-ccache --enable-nspr-build --enable-ctypes --enable-debug-symbols --enable-gczeal --enable-rust-simd --disable-tests, tested on m-c rev 3fd71c45d9fc.

Jan, is bug 1867193 a likely regressor? Setting s-s because a previous bug 1804626 with a similar-looking message is marked sec-high.

Comment 1

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

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

Comment 2

[Iain Ireland [:iain]]

It looks like what's happening is something along the lines of:

1. We start verifying barriers. This traces the heap, and records a Script -> JitCode edge. Specifically: Script->ScriptWarmUpData->JitScript->InliningRoot->ICScript->ICEntry->ICCacheIRStub->JitCode, where the ICScript is a trial-inlined script for the anonymous inner function, and the IC stub appears to be the bitwise or.
2. We call bailAfter. This calls ReleaseAllJitCode, which discards the baseline IC stub.
3. We finish verifying barriers. To do so, we trace the heap again. The jitcode is unreachable and unmarked.
4. Oops.

// |jit-test| --no-ggc
let x = [[]];
for (let i = 0; i < 25; i++) {
  for (let j = 0; j < 25; j++) {
    (function () {
      x[i] | 0;
    })();
  }
}

verifyprebarriers();
bailAfter(1);
verifyprebarriers();

I assume the fix is to add a prebarrier of some sort when discarding the ICScript? Maybe we could do something similar to what we do for JitScript here?

I'm not sure how bad this is, security-wise. bailAfter is a testing function, but there are other user-exposed ways to trigger jitcode being discarded. This comment implies that it would be safe to collect the JitCode as long as it is genuinely unreachable, but maybe that's not guaranteed.

Jon, do you have thoughts here?

Comment 3

[Jon Coppeard (:jonco)]

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

I assume the fix is to add a prebarrier of some sort when discarding the ICScript? Maybe we could do something similar to what we do for JitScript here?

Yes that's right.

I'm not sure how bad this is, security-wise. bailAfter is a testing function, but there are other user-exposed ways to trigger jitcode being discarded. This comment implies that it would be safe to collect the JitCode as long as it is genuinely unreachable, but maybe that's not guaranteed.

Jon, do you have thoughts here?

The consequence of a missing prebarrier is potential UAF due to the target not getting marked by incremental marking while still being reachable from some other part of the graph. It's true that this is not possible in every case - we use the snapshot-at-the-beginning invariant which as the comment says is conservative. However the default position is that issues like this should be treated as a security issue and fixed by adding the barrier.

Comment 4

[Andrew McCreight [:mccr8]]

I'll mark this sec-high then, though it isn't clear how easy it is to trigger in practice.

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 1870925: Add prebarriers to ICScript::prepareToDestroy r=jonco!

Comment 6

[Iain Ireland [:iain]]

Attachment: Bug 1870925: Add test r=jonco

Depends on D197098

Comment 7

[Jan de Mooij [:jandem]]

Iain, thanks for looking at this while I was on PTO. Clearing the NI but let me know if there's more to do here.

Comment 8

[Donal Meehan [:dmeehan]]

:iain next week is the final week of beta for Fx122.
Checking this will land and get an uplift request in time?

Comment 9

[Iain Ireland [:iain]]

Comment on attachment 9369900 [details]
Bug 1870925: Add prebarriers to ICScript::prepareToDestroy r=jonco!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It might be possible. I don't think it would be trivial.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: 122
• If not all supported branches, which bug introduced the flaw?: Bug 1867193
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: This is a one-line patch and should apply trivially.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions; it's a small fix adding a prebarrier.
• Is Android affected?: Yes

Comment 10

[Tom Ritter [:tjr]]

Comment on attachment 9369900 [details]
Bug 1870925: Add prebarriers to ICScript::prepareToDestroy r=jonco!

Approved to land

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/99e507782040
Add prebarriers to ICScript::prepareToDestroy r=jonco

Comment 12

[Iain Ireland [:iain]]

Comment on attachment 9369900 [details]
Bug 1870925: Add prebarriers to ICScript::prepareToDestroy r=jonco!

Beta/Release Uplift Approval Request

• User impact if declined: Potentially exploitable UAF
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We are adding a prebarrier, which should not be able to cause any correctness issues.
• String changes made/needed:
• Is Android affected?: Yes

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/99e507782040

Comment 14

[Donal Meehan [:dmeehan]]

Comment on attachment 9369900 [details]
Bug 1870925: Add prebarriers to ICScript::prepareToDestroy r=jonco!

Approved for 122.0b7

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/2ff9eed3a336

Comment 17

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

a month ago, tjr placed a reminder on the bug using the whiteboard tag [reminder-test 2024-03-05] .

iain, please refer to the original comment to better understand the reason for the reminder.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/744a8f21be28
Add test r=jonco

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/744a8f21be28

Comment 20

[Daniel Veditz [:dveditz]]

Making Firefox 122 security bugs public. [bugspam filter string: Pilgarlic-Towers]