1818781 - (CVE-2023-29544) Assertion failure: !hasDelayedMarking(), at js/src/gc/GC.cpp:1370

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P2)

Description

[lukas.bernhard]

Attachment: js.patch

Steps to reproduce:

On git commit 2346ab0fa1665d535677c48353a3b6beb6981d3e a fuzzer found an assertion violation in the js-shell. The crash is flaky and difficult to reproduce but I attached the necessary files that trigger the issue (on my machine).
First of all, there is js.patch which slightly modifies the js-shell such that reprl mode becomes compatible with rr recording.
Then, there is reprl2.py which feeds 4 .js files via the reprl interface to the shell. The script exits if the particular execution didn't trigger the assertion violation and should freeze once the shell crashes.
Then, there are the 4 .js files executed via the reprl interface.
Letting the script run with while true; do python3 reprl2.py; done; should provoke the crash at some point, roughly takes 50-100 runs on my machine. Occasionally, rr encounters an internal error and the loop must be restarted.

#0  js::gc::GCRuntime::assertNoMarkingWork (this=0x3f418b23728)
    at js/src/gc/GC.cpp:1370
#1  0x000055c367db9249 in js::gc::GCRuntime::endSweepingSweepGroup (this=0x3f418b23728, 
    gcx=0x3f418b23740, budget=...) at js/src/gc/Sweeping.cpp:1630
#2  0x000055c367dd6e31 in sweepaction::SweepActionSequence::run (this=0x3f418b06790, args=...)
    at js/src/gc/Sweeping.cpp:2128
#3  0x000055c367dd3260 in sweepaction::SweepActionForEach<js::gc::SweepGroupsIter, JSRuntime*>::run (
    this=0x3f418b1a610, args=...) at js/src/gc/Sweeping.cpp:2163
#4  0x000055c367dbdac2 in js::gc::GCRuntime::performSweepActions (this=0x3f418b23728, budget=...)
    at js/src/gc/Sweeping.cpp:2305
#5  0x000055c367d1e5b9 in js::gc::GCRuntime::incrementalSlice (this=0x3f418b23728, budget=..., 
    reason=JS::GCReason::DEBUG_GC, budgetWasIncreased=<optimized out>)
    at js/src/gc/GC.cpp:3687
#6  0x000055c367d23579 in js::gc::GCRuntime::gcCycle (this=0x3f418b23728, nonincrementalByAPI=false, 
    budgetArg=..., reason=JS::GCReason::DEBUG_GC)
    at js/src/gc/GC.cpp:4198
#7  0x000055c367d243d3 in js::gc::GCRuntime::collect (this=0x3f418b23728, 
    nonincrementalByAPI=<optimized out>, budget=..., reason=JS::GCReason::DEBUG_GC)
    at js/src/gc/GC.cpp:4386
#8  0x000055c36794a006 in GCSlice (cx=<optimized out>, argc=<optimized out>, vp=<optimized out>)
    at js/src/builtin/TestingFunctions.cpp:2638

Comment 1

[lukas.bernhard]

Attachment: reprl2.py

Comment 2

[lukas.bernhard]

Attachment: j1.js

Comment 3

[lukas.bernhard]

Attachment: j2.js

Comment 4

[lukas.bernhard]

Attachment: j3.js

Comment 5

[lukas.bernhard]

Attachment: j4.js

Comment 6

[Nicolas B. Pierron [:nbp]]

Jon / Steve, whether you can reproduce this using rr, can you look at what is going on behind this assertion failure?

Comment 7

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

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

Comment 8

[Jon Coppeard (:jonco)]

Attachment: Bug 1818781 - Mark delayed children when doing background marking during sweep phase r?sfink

The problem is that the parallel marking work moved the delayed marking list
from GCMarkers to the GCRuntime, since there was only one list and there can
now be multiple markers. However the check for marking in
GCRuntime::markDuringSweeping wasn't updated so we skip the delayed marking in
this case.

For this to happen we have to OOM expanding the mark stack during sweep marking
and then run out of marking budget at exactly the point at which the mark stack
is empty. Then on the next slice we will miss the fact that there is delayed
marking work. This is pretty unlikely to occur and hard to arrange, as the bug
description shows.

Comment 9

[Andrew McCreight [:mccr8]]

I'll mark this as sec-moderate because it sounds like you need multiple resources to be exhausted at the right moment, and the initial test case is extremely unreliable, so it sounds very difficult to exploit.

Comment 10

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

Mark delayed children when doing background marking during sweep phase r=sfink
https://hg.mozilla.org/integration/autoland/rev/0fb5928699807be22b8982ab82bda2626e4d69c7
https://hg.mozilla.org/mozilla-central/rev/0fb592869980

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-firefox112 to wontfix.

For more information, please visit auto_nag documentation.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Hi Steve, can you please request Beta approval on this since Jon is away and we're almost out of time this cycle? Thanks!

Comment 13

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9324219 [details]
Bug 1818781 - Mark delayed children when doing background marking during sweep phase r?sfink

Beta/Release Uplift Approval Request

• User impact if declined: potential crash/corruption/UAF in rare OOM cases. Difficult to trigger, difficult to exploit, but not out of the question.
• Is this code covered by automated tests?: No
• 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): It's tricky code, but this is finishing up work that clearly needs to be done in the rare cases where it applies (and is dangerous to leave undone). This fallback path doesn't get exercised a lot, but it does get exercised, and this patch is reverting to previous behavior.
• String changes made/needed:
• Is Android affected?: Yes

Comment 14

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9319729 [details] [diff] [review]
js.patch

Oops, shouldn't have included this patch.

Comment 15

[Dianna Smith [:diannaS]]

Comment on attachment 9324219 [details]
Bug 1818781 - Mark delayed children when doing background marking during sweep phase r?sfink

Approved for 112.0b9

Comment 16

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/61ac53a8b75e

Comment 17

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 18

[Daniel Veditz [:dveditz]]

Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons