[6] Race-condition UAF in JSSubscriber GC marking

Severity: Medium | Component: WebCore DOM Observable/Subscriber | db743cc

Rated Medium because the diff fixes a genuine race in which the GC thread can dereference a freed VoidCallback via a stale raw pointer; escalation to a usable UAF primitive requires reliably winning a narrow main-thread-versus-GC-thread timing window and reclaiming the freed allocation, neither of which the diff establishes.

This PR fixes a race condition in JSSubscriber::visitAdditionalChildren that results in a use-after-free of VoidCallback objects. While Subscriber::teardownCallbacksConcurrently grabs a lock and creates a Vector of VoidCallback*, the main thread can still go ahead and destroy a VoidCallback while a GC thread calls visitJSFunction on it. No new tests, since there is no reliable reproduction.

Source/WebCore/dom/Subscriber.cpp

-Vector<VoidCallback*> Subscriber::teardownCallbacksConcurrently()
-{
-    Locker locker { m_teardownsLock };
-    return m_teardowns.map([](auto& callback) {
-        return callback.ptr();
-    });
-}
-
-void Subscriber::visitAdditionalChildrenInGCThread(JSC::AbstractSlotVisitor& visitor)
-{
-    // We cannot ref `teardown` here as this may get called from a GC thread.
-    SUPPRESS_UNRETAINED_ARG for (auto* teardown : teardownCallbacksConcurrently())
-        teardown->visitJSFunctionInGCThread(visitor);
-    ...
-}
+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);
+}

Source/WebCore/dom/Subscriber.h

// Vector<Ref<VoidCallback>> m_teardowns WTF_GUARDED_BY_LOCK(m_teardownsLock);

// void stop() final { Locker locker { m_teardownsLock }; m_teardowns.clear(); }

Patch Details

The patch deletes teardownCallbacksConcurrently() and observerConcurrently() and consolidates a single templated Subscriber::visitAdditionalChildrenInGCThread into Subscriber.cpp. The new method holds Locker locker { m_teardownsLock } across the entire iteration over m_teardowns (a Vector<Ref<VoidCallback>>), calling teardown->visitJSFunctionInGCThread(visitor) while the lock is held, then visits m_observer after the locked block. JSSubscriber::visitAdditionalChildrenInGCThread now simply forwards to wrapped().visitAdditionalChildrenInGCThread(visitor).

Use-after-free from snapshotting refcounted objects into raw pointers under a lock, then dereferencing them after the lock is released.

Background

The Observable API exposes a Subscriber to JS; subscriber.addTeardown(callback) registers VoidCallback JS functions that run when the subscription ends, stored as Vector<Ref<VoidCallback>> m_teardowns guarded by m_teardownsLock. JSC uses a concurrent garbage collector: marking can run on a separate GC thread at the same time the main thread executes JS, so custom mark hooks (the visitAdditionalChildrenInGCThread family) must be thread-safe and must not call ref()/deref() — refcount churn from the collector thread is unsafe — which is why the code uses raw access guarded only by a lock. ActiveDOMObject::stop() is the lifecycle teardown the runtime invokes (e.g. on context shutdown) and here clears m_teardowns under the same lock. Ref<VoidCallback> is a sole-ownership smart pointer; clearing the vector drops the last reference and destroys the callback.

Analysis

This is a race-condition use-after-free of the classic snapshot-then-use-after-unlock (TOCTOU) shape. Before the fix, the lock protecting m_teardowns was held only long enough to copy the callbacks into a Vector<VoidCallback*> of unowned raw pointers; the lock was then dropped before the GC thread dereferenced those pointers via visitJSFunctionInGCThread. The invariant that must hold — every VoidCallback* visited by the GC thread is still alive — was enforced only at snapshot time, not at use time. Because the GC thread deliberately does not ref() the callbacks, nothing kept them alive across the gap.

The main thread can destroy those VoidCallback objects in the window between lock release and GC-thread iteration: Subscriber::stop() takes m_teardownsLock and calls m_teardowns.clear(), dropping the last Ref<VoidCallback> for each entry and running the destructor. The GC thread, holding only stale raw pointers, then calls visitJSFunctionInGCThread on freed memory. Whether stop() actually executes concurrently with GC marking in practice — rather than the race being purely theoretical — is not directly demonstrable from the diff, and the commit's note of "no reliable reproduction" reflects that. An attacker who reliably wins the race could read or be served reclaimed/reallocated heap memory through the visiting path, which under controlled heap conditions could be developed toward a use-after-free primitive in the renderer; that reliable timing and reclamation is the speculative part.

This vulnerability weakens memory safety inside the WebContent process by allowing a main-thread object destruction to race against concurrent GC marking. The model assumes any object reachable from a custom GC mark hook stays alive for the duration of that hook; before the fix that held only at the moment the raw-pointer snapshot was taken. The fix holds m_teardownsLock across the whole visit, so stop()'s clear() cannot run concurrently with the visit and the Ref<VoidCallback> entries remain alive for its duration. This is the danger of "snapshot under lock, use after unlock" when the snapshot deliberately strips ownership — the non-ref requirement of concurrent GC marking removes the usual safety net, so the lock must cover the entire dereference window.

Note: Whether the freeing path actually races GC marking in practice, and whether the race is reliably winnable for a usable primitive, are inferred; the lock-scope defect and its fix are directly supported by the diff.

Audit directions

Lock-guarded refcounted containers snapshotted into raw pointers. A lock-guarded container of refcounted objects is snapshotted into raw pointers under the lock, the lock released, then the raw pointers dereferenced. Grep for visitAdditionalChildrenInGCThread and visitJSFunctionInGCThread implementations and verify the guarding Locker spans the whole visit loop, not just a .map(...)/.ptr() snapshot.
Concurrent GC code that intentionally avoids ref(). Code marked with SUPPRESS_UNRETAINED_ARG, SUPPRESS_UNCOUNTED_ARG, or NODELETE relies entirely on a lock for liveness. Review each site and confirm the corresponding mutator-side mutation (clears, removals, destructors) takes the same lock; cross-reference WTF_GUARDED_BY_LOCK members against the ActiveDOMObject::stop() and destructor paths that mutate them.
Sibling callback registries. Verify other Observable/EventTarget-style callback registries (teardown lists, observer lists) hold their lock across GC visitation. Examine Subscriber.cpp, InternalObserver, and sibling DOM objects with Vector<Ref<...Callback>> members guarded by a lock for the same release-before-use gap.