1904011 - (CVE-2024-7530) Crash [@ js::UninlinedIsCrossCompartmentWrapper(JSObject const*)] with Debugger and GC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

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

f = function() {
    try {
        c()
    } catch (exc) {}
}
gczeal(22, 6)
g = newGlobal({
    newCompartment: true
})
dbg = Debugger(g)
function loop() {}
g.eval(loop.toString())
f()
dbg.collectCoverageInfo = true
g.eval("loop")
f()
dbg.collectCoverageInfo = false

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x000055555763d4f7 in js::UninlinedIsCrossCompartmentWrapper(JSObject const*) ()
#1  0x0000555556e69dde in JSObject::nonCCWRealm() const ()
#2  0x0000555557a109f7 in JS::Zone::clearScriptCounts(JS::Realm*) ()
#3  0x00005555573a8d4b in JS::Realm::updateDebuggerObservesCoverage() ()
#4  0x00005555576d1d05 in js::Debugger::updateObservesCoverageOnDebuggees(JSContext*, js::DebugAPI::IsObserving) ()
#5  0x00005555576d9b51 in js::Debugger::CallData::setCollectCoverageInfo() ()
#6  0x00005555576e97a7 in bool js::Debugger::CallData::ToNative<&js::Debugger::CallData::setCollectCoverageInfo>(JSContext*, unsigned int, JS::Value*) ()
#7  0x0000555557022e25 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) ()
#8  0x00005555570223d9 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) ()
#9  0x0000555557023c69 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) ()
#10 0x00005555570253da in js::CallSetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>) ()
#11 0x00005555573117fe in SetExistingProperty(JSContext*, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<js::NativeObject*>, js::PropertyResult const&, JS::ObjectOpResult&) ()
#12 0x00005555573105ce in bool js::NativeSetProperty<(js::QualifiedBool)1>(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) ()
#13 0x0000555557031055 in js::Interpret(JSContext*, js::RunState&) ()
[...]
#22 0x0000555556e6c4c2 in main ()
rax	0xfffe4b4b4b4b4b4b	-480163195565237
rbx	0x2123e2e4f078	36438014226552
rcx	0x1	1
rdx	0x7ffff3d55724	140737284232996
rsi	0x7ffff3d21400	140737284019200
rdi	0x2123e2e4f078	36438014226552
rbp	0x7fffffffc9c0	140737488341440
rsp	0x7fffffffc9c0	140737488341440
r8	0x7ffff3d55980	140737284233600
r9	0x20	32
r10	0x7ffff3d55780	140737284233088
r11	0x1	1
r12	0x2123e2e500b0	36438014230704
r13	0x7fffffffcb30	140737488341808
r14	0x7fffffffc9f0	140737488341488
r15	0xffffffffffffff	72057594037927935
rip	0x55555763d4f7 <js::UninlinedIsCrossCompartmentWrapper(JSObject const*)+7>
=> 0x55555763d4f7 <_ZN2js34UninlinedIsCrossCompartmentWrapperEPK8JSObject+7>:	testb  $0x30,0x8(%rax)
   0x55555763d4fb <_ZN2js34UninlinedIsCrossCompartmentWrapperEPK8JSObject+11>:	je     0x55555763d501 <_ZN2js34UninlinedIsCrossCompartmentWrapperEPK8JSObject+17>

Involving the debugger, so likely sec-moderate at most.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Bugmon [:jkratzer for issues]]

Unable to reproduce bug 1904011 using build mozilla-central 20240620040816-81bbe3af9834. Without a baseline, bugmon is unable to analyze this bug.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 4

[Iain Ireland [:iain]]

I looked into this one a bit. I think it has to do with the clever tricks we describe in this comment.

I believe the sequence of events is something along these lines:

1. We start collecting code coverage info.
2. We execute an eval, which creates a BaseScript and an associated ScriptSourceObject.
3. When the eval script runs, it calls initScriptCounts, which inserts the BaseScript into the scriptCountsMap.
4. We trigger a GC. During root tracing, we call Zone::traceScriptTableRoots, but we don't trace the scriptCountsMap, because we're doing our clever pseudo-weakmap thing.
5. There are no other edges to the BaseScript or the ScriptSourceObject, so they're both dead.
6. We start sweeping arenas. We sweep OBJECT4 and poison the ScriptSourceObject
7. Before we sweep anything else, gczeal 22 (YieldBeforeSweepingNonObjects) causes us to yield back to the mutator. Our dead BaseScript is still in the scriptCountsMap, because we rely on its finalizer to remove it.
8. The mutator sets collectCoverageInfo to false.
9. This triggers updateObservesCoverageOnDebuggees, which calls clearScriptCounts on the realm, which iterates through the keys of the scriptCountsMap.
10. One of those keys is our undead BaseScript. It points to a poisoned ScriptSourceObject. When we try to read the realm from the ScriptSourceObject, we segfault.

tl;dr: The pseudo-weakmap trick for the scriptCountsMap implicitly depends on the base script being finalized if anything it points to is finalized, but incremental GC breaks that.

I don't know how to untangle this mess. There's some good discussion in this Phabricator review, but the conclusion seems to be that this is a generally hard problem.

Jon, you reviewed that patch. Do you have thoughts?

Comment 5

[Jon Coppeard (:jonco)]

(In reply to Iain Ireland [:iain] from comment #4)
Thanks for the detailed analysis. It sounds like the we need to check for finalized scripts when iterating these tables.

Comment 6

[Jon Coppeard (:jonco)]

I'll mark this as sec-moderate since it requires the debugger.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: Bug 1904011 - Ignore finalized scripts when iterating code covarage tables r?iain

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f13a62f9043a
Ignore finalized scripts when iterating code covarage tables r=iain

Comment 9

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

The bug is marked as tracked for firefox128 (beta) and tracked for firefox129 (nightly). However, the bug still has low severity.

:sdetar, could you please increase the severity for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 11

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

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

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

For more information, please visit BugBot documentation.

Comment 12

[Jon Coppeard (:jonco)]

Unlike to affect users so we can let it ride the trains.

Comment 13

[Tyson Smith [:tsmith]]

Attachment: advisory.txt

Comment 14

[Tyson Smith [:tsmith]]

Attachment: advisory.txt