db743cc339  Race condition in JSSubscriber::visitAdditionalChildren during GC

The bug at a glance

A data race and consequent use-after-free of VoidCallback objects in JSSubscriber’s concurrent GC visitation: the GC thread snapshotted teardown callbacks under a lock but then released the lock and visited them, so the main thread could destroy a VoidCallback while the GC thread called visitJSFunctionInGCThread on it. Medium reflects a timing-dependent race in Observable/Subscriber binding code, reachable via the Observable API, with crash-class impact rather than a shown controlled primitive.

Subscriber::teardownCallbacksConcurrently grabbed m_teardownsLock only long enough to copy raw VoidCallback* pointers into a Vector, then returned. The GC thread visited those raw pointers after the lock was released, so the main thread could free a VoidCallback (or the observer) during visitation - a race and use-after-free.

Root cause

JSSubscriber::visitAdditionalChildrenInGCThread runs on a concurrent GC marking thread and must report the subscriber’s teardown callbacks and its observer to the visitor. The old design split this across helpers: teardownCallbacksConcurrently() took m_teardownsLock, copied m_teardowns into a Vector<VoidCallback*> of raw pointers, and released the lock on return; observerConcurrently() returned a raw InternalObserver*. visitAdditionalChildrenInGCThread then iterated the returned raw-pointer vector and called teardown->visitJSFunctionInGCThread(visitor) on each, and dereferenced the observer - all outside the lock.

Because the lock was dropped before the callbacks were actually visited, the main thread could concurrently mutate m_teardowns and destroy a VoidCallback while the GC thread was mid-visit on that same object, and similarly race on the observer. The result is a data race and a use-after-free of VoidCallback objects during marking.

The patch collapses the logic into a single Subscriber::visitAdditionalChildrenInGCThread(Visitor&) that holds m_teardownsLock across the entire iteration: inside the locker it loops for (auto& teardown : m_teardowns) teardown->visitJSFunctionInGCThread(visitor), so callbacks are visited while still protected. It then visits m_observer via m_observer->visitAdditionalChildrenInGCThread(visitor). The comment explicitly warns not to ref anything here because it runs on a GC thread; the SUPPRESS_UNCOUNTED_LOCAL/SUPPRESS_UNCOUNTED_ARG/SUPPRESS_UNRETAINED_ARG annotations acknowledge the deliberate use of unref’d locals under the checker. The now-unnecessary teardownCallbacksConcurrently()/observerConcurrently() helpers are deleted, and JSSubscriber::visitAdditionalChildrenInGCThread simply forwards to wrapped().visitAdditionalChildrenInGCThread(visitor). Templating the method (DEFINE_VISIT_ADDITIONAL_CHILDREN_IN_GC_THREAD) lets it serve both AbstractSlotVisitor and SlotVisitor.

Key code

Visit teardown callbacks while still holding the lock

template<typename Visitor>
void Subscriber::visitAdditionalChildrenInGCThread(Visitor& visitor)
{
    // Do not ref anything in this function, which runs in a GC thread concurrently to the main thread.
    {
        Locker locker { m_teardownsLock };
        SUPPRESS_UNCOUNTED_LOCAL for (auto& teardown : m_teardowns)
            SUPPRESS_UNCOUNTED_ARG teardown->visitJSFunctionInGCThread(visitor);
    }

    SUPPRESS_UNRETAINED_ARG m_observer->visitAdditionalChildrenInGCThread(visitor);
}

Patch walkthrough

• Source/WebCore/bindings/js/JSSubscriberCustom.cpp — Replaces the two-step visitation (looping over teardownCallbacksConcurrently() then observerConcurrently()->visit…) with a single delegation to wrapped().visitAdditionalChildrenInGCThread(visitor), moving all locking into Subscriber.
• Source/WebCore/dom/Subscriber.cpp — Deletes teardownCallbacksConcurrently() (which released the lock after only copying pointers) and observerConcurrently(). Adds a templated Subscriber::visitAdditionalChildrenInGCThread that holds m_teardownsLock across the whole loop visiting each teardown’s JS function, then visits m_observer, and uses DEFINE_VISIT_ADDITIONAL_CHILDREN_IN_GC_THREAD to instantiate it.
• Source/WebCore/dom/Subscriber.h — Removes the teardownCallbacksConcurrently()/observerConcurrently()/non-templated visit declarations and replaces them with template<typename Visitor> void visitAdditionalChildrenInGCThread(Visitor&).

Background

Observable / Subscriber — Subscriber backs the Observable API’s subscription. It holds teardown callbacks (VoidCallback) registered via addTeardown and an InternalObserver. Its JS wrapper JSSubscriber must keep those JS functions alive during GC by visiting them from visitAdditionalChildrenInGCThread.

Concurrent GC visitation constraint — visitAdditionalChildrenInGCThread runs on a GC marking thread concurrently with the main thread. Refing (taking a strong reference to) WebCore objects on this thread is unsafe, so the code visits objects without ref’ing them - which makes correct locking, not ref-counting, the mechanism that must prevent concurrent destruction.

m_teardownsLock — A lock guarding Subscriber::m_teardowns. The bug was that the lock protected only the pointer snapshot, not the subsequent visitation. The fix widens the critical section so each VoidCallback is visited while the lock is held, preventing the main thread from destroying it mid-visit.

SUPPRESS_UNCOUNTED/UNRETAINED annotations — WebKit’s static checkers flag raw/un-ref’d pointer use. These SUPPRESS_ macros mark deliberately un-ref’d local iterations and arguments that are safe here specifically because the lock (and single-threaded destruction contract) protects them, documenting intent to the checker.

Vulnerability window

1. Subscribe — An Observable subscription creates a Subscriber with teardown VoidCallbacks and an InternalObserver, each backed by JS functions.
2. GC visit — A concurrent marking thread calls JSSubscriber::visitAdditionalChildrenInGCThread; pre-patch it obtained a raw-pointer snapshot under lock, then released the lock before visiting.
3. Race — After the lock is released, the main thread destroys a VoidCallback (or the observer) while the GC thread is calling visitJSFunctionInGCThread on that raw pointer.
4. UAF — The GC thread dereferences the freed VoidCallback - a data race and use-after-free during marking.
5. Fix — The consolidated visitAdditionalChildrenInGCThread holds m_teardownsLock across the entire teardown-visit loop, so callbacks cannot be destroyed while being visited.

Triggering

The commit states ‘No new tests since there is no reliable reproduction.’ Conceptual trigger: create Observables with teardown callbacks, then churn subscriptions (unsubscribe / drop subscribers so the main thread destroys VoidCallbacks) while forcing concurrent garbage collection, so a marking thread visits a teardown callback as the main thread frees it.

Exploitation

1. Race window — Requires a concurrent GC marking pass over a JSSubscriber to overlap with main-thread destruction of a teardown VoidCallback or the observer.
2. Use — The GC thread calls visitJSFunctionInGCThread on a freed VoidCallback; impact is corruption/crash during marking. No controlled primitive is demonstrated, and there is no reliable reproduction, so this is crash-class requiring precise timing.

Detection & hunting

For defenders and SOC / detection engineers:

• ASan UAF on VoidCallback during GC marking —
• TSan race on Subscriber::m_teardowns —

Audit directions

• Locks that protect only snapshots —
• Other custom visitAdditionalChildrenInGCThread implementations —
• VoidCallback / callback lifetime vs GC —