1816158 - (CVE-2023-29543) js::gc::detail::CellHasStoreBuffer (cell=0xe5e5e5e5e5e5e5e5)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[lukas.bernhard]

Attachment: crash2.js

Steps to reproduce:

On git commit b25ff1fab82c2d3a91531ad3735e50422407b163, the attached sample crashes with a stack trace indicating some kind of GC issue (or a debugger issue?). The sample is extremely flaky and I failed to minimize it (sorry about that). I'm also attaching a python script that runs rr record in a loop until the crash is detected. The script is supposed to be placed at the root of the firefox source folder, alongside with the crash2.js file. Please check the script before running; it is rm'ing the rr output directory (/tmp/BBB) unless there is a crash detected. The script runs for some time, it might take a couple of hundred executions.

#0  0x000055baeb129ab5 in js::gc::detail::CellHasStoreBuffer (cell=0xe5e5e5e5e5e5e5e5)
    at obj-x86_64-pc-linux-gnu/dist/include/js/HeapAPI.h:594
#1  0x000055baeb129a94 in js::gc::IsInsideNursery (cell=0xe5e5e5e5e5e5e5e5)
    at obj-x86_64-pc-linux-gnu/dist/include/js/HeapAPI.h:601
#2  0x000055baeb1299b5 in js::gc::Cell::isTenured (this=0xe5e5e5e5e5e5e5e5)
    at js/src/gc/Cell.h:195
#3  0x000055baeb14f107 in js::gc::ReadBarrierImpl (thing=0xe5e5e5e5e5e5e5e5)
    at js/src/gc/Cell.h:563
#4  0x000055baeb14f020 in js::gc::ReadBarrier<JSObject> (thing=0xe5e5e5e5e5e5e5e5)
    at js/src/gc/Cell.h:539
#5  0x000055baeb14eff5 in js::InternalBarrierMethods<JSObject*, void>::readBarrier (
    v=0xe5e5e5e5e5e5e5e5) at js/src/gc/Barrier.h:351
#6  0x000055baebe8bc05 in js::InternalBarrierMethods<js::Debugger*, void>::readBarrier (
    dbg=0x26c22b106300) at js/src/debugger/Debugger.h:1279
#7  0x000055baebe8bbd8 in js::ReadBarriered<js::Debugger*>::read (this=0x26c22b1ee2b0)
    at js/src/gc/Barrier.h:836
#8  0x000055baebe8bb82 in js::WeakHeapPtr<js::Debugger*>::get (this=0x26c22b1ee2b0)
    at js/src/gc/Barrier.h:889
#9  0x000055baebe1aa15 in js::WeakHeapPtr<js::Debugger*>::operator js::Debugger* const& (
    this=0x26c22b1ee2b0) at js/src/gc/Barrier.h:898
#10 0x000055baebde3215 in js::Debugger::forEachOnStackDebuggerFrame<js::DebugAPI::slowPathOnNewGenerator(JSContext*, js::AbstractFramePtr, JS::Handle<js:
:AbstractGeneratorObject*>)::$_14>(js::AbstractFramePtr, js::DebugAPI::slowPathOnNewGenerator(JSContext*, js::AbstractFramePtr, JS::Handle<js::AbstractGe
neratorObject*>)::$_14) (frame=..., fn=...) at js/src/debugger/Debugger.cpp:3284
#11 0x000055baebde30c0 in js::DebugAPI::slowPathOnNewGenerator (cx=0x7fdc3762f100, frame=...,
    genObj=...) at js/src/debugger/Debugger.cpp:1261
#12 0x000055baeb5c954c in js::DebugAPI::onNewGenerator (cx=0x7fdc3762f100, frame=..., genObj=...)
    at js/src/debugger/DebugAPI-inl.h:88
#13 0x000055baeb5c8a79 in js::AbstractGeneratorObject::createFromFrame (cx=0x7fdc3762f100, frame=...)
    at js/src/vm/GeneratorObject.cpp:81
#14 0x000055baeb2e93f6 in Interpret (cx=0x7fdc3762f100, state=...) at js/src/vm/Interpreter.cpp:4287
#15 0x000055baeb2d1980 in js::RunScript (cx=0x7fdc3762f100, state=...) at js/src/vm/Interpreter.cpp:431
#16 0x000055baeb2f096c in js::ExecuteKernel (cx=0x7fdc3762f100, script=..., envChainArg=..., evalInFrame=..., result=...) at js/src/vm/Interpreter.cpp:81
2
#17 0x000055baeb2f1215 in js::Execute (cx=0x7fdc3762f100, script=..., envChain=..., rval=...)
    at js/src/vm/Interpreter.cpp:844

Comment 1

[lukas.bernhard]

Attachment: reproduce.py

Comment 2

[lukas.bernhard]

Attachment: .mozconfig.clean

Comment 3

[Daniel Veditz [:dveditz]]

It looks like this testcase calls the Debugger in at least one place, but I don't know if that's involved in this crash. If web scripts can't trigger this then we can lower the security severity.

Comment 4

[Jon Coppeard (:jonco)]

From the stack trace this is likely to be a debugger issue.

Comment 5

[Jon Coppeard (:jonco)]

I haven't managed to reproduce this locally. The repro script does seem to crash rr maybe one in twenty times though. I'm getting "Syscall mmap failed with errno ENOMEM". Maybe there is some config I need to change on my system? It's not running out of physical memory.

Comment 6

[Jon Coppeard (:jonco)]

The problem is Debugger::forEachOnStackDebuggerFrame is not safe if the function it calls can GC. It uses a ranged for loop over a global's debugger vector, which can be mutated by GC.

What's happening here is that a GC is triggered and deletes one of the debuggers in the vector, but due to the way ranged for loops evaluate the begin and end iterators at the start, the for loop still subsequently iterates over it.

Comment 7

[Andrew McCreight [:mccr8]]

I'll mark this sec-moderate as it sounds like it requires the debugging API.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: Bug 1816158 - Part 1: Disallow GC while iterating global's debugger vector r?sfink

GC can mutate this vector so don't allow that while we are iterating. I think
it would be safe to use index-based iteration but it's safer to just ban it
entirely.

This fixes the crash produced by the testcase.

Comment 9

[Jon Coppeard (:jonco)]

Attachment: Bug 1816158 - Part 2: Require no GC when giving out references to the realm's debugger vector r?sfink

To prevent any other instances of this problme we can update the getDebuggers()
methods on the global and the realm to require no GC.

Depends on D169701

Comment 10

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

Part 1: Disallow GC while iterating global's debugger vector r=sfink
https://hg.mozilla.org/integration/autoland/rev/760275af660b20d35ace80f4d245c4ad9c4a3869
https://hg.mozilla.org/mozilla-central/rev/760275af660b

Part 2: Require no GC when giving out references to the realm's debugger vector r=sfink
https://hg.mozilla.org/integration/autoland/rev/eb7a5d363182c55a363fe46defe670f060928e76
https://hg.mozilla.org/mozilla-central/rev/eb7a5d363182

apply code formatting via Lando
https://hg.mozilla.org/integration/autoland/rev/9b87c9adfacceee1a275e2f11256ee7c1e76410b
https://hg.mozilla.org/mozilla-central/rev/9b87c9adfacc

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

For more information, please visit auto_nag documentation.

Comment 12

[Jon Coppeard (:jonco)]

I don't think there's any evidence that this is causing significant problems. It's debugger only and even then unlikely to trigger. We can let this ride the trains.

Comment 13

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt