[5] Subscriber: teardown callbacks freed while a GC thread walks the snapshot

MediumWebCore DOM Observables — SubscriberRace

db743cc

Medium. The lock was scoped to producing the snapshot, not to using it — so the returned raw pointers carried exactly zero of the lifetime guarantee the lock existed to provide. No reliable reproduction accompanies the fix, which caps practical exploitability at winning an unsynchronised main-thread/GC-thread race.

JavaScriptCore marks the heap on dedicated GC threads that run alongside the main thread, so any C++ object those threads dereference during marking must stay alive for the whole dereference window — normally guaranteed either by the GC's own object lifetimes or by a lock that also serialises destruction. WebKit's DOM bindings participate in that marking through custom mark functions: for wrappers whose C++ side holds JS values the generated bindings cannot see, a visitAdditionalChildrenInGCThread implementation walks those values and reports them. Code reached from a GC thread must not touch refcounts of main-thread-owned objects, so these functions deliberately borrow raw pointers instead — which means something other than refcounting has to keep the referents alive for the duration.

The angle: a page that drops a subscription's teardown callbacks at the moment a GC thread is marking that Subscriber can have the collector read freed memory and feed whatever occupies it to the slot visitor.

This PR fixes a race condition in JSSubscriber::visitAdditionalChildren which 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 VoidCallback while a GC thread is calling visitJSFunction on that VoidCallback. No new tests since there is no reliable reproduction.

Source/WebCore/dom/Subscriber.cpp

Source/WebCore/bindings/js/JSSubscriberCustom.cpp

Source/WebCore/dom/Subscriber.h

Patch Details

The patch rewrites how JSSubscriber's custom mark function reaches the Subscriber's script-visible children. Previously JSSubscriber::visitAdditionalChildrenInGCThread called wrapped().teardownCallbacksConcurrently(), which took m_teardownsLock, snapshotted m_teardowns into a Vector<VoidCallback*> of raw pointers via m_teardowns.map([](auto& callback) { return callback.ptr(); }), released the lock on return, and only then iterated the snapshot calling teardown->visitJSFunctionInGCThread(visitor).

The patch deletes teardownCallbacksConcurrently() and observerConcurrently() entirely and replaces the non-template Subscriber::visitAdditionalChildrenInGCThread(JSC::AbstractSlotVisitor&) with a template<typename Visitor> version that holds Locker locker { m_teardownsLock } across the whole iteration, walking m_teardowns in place instead of copying pointers out. The observer visit moves outside the locked scope and goes directly through m_observer->visitAdditionalChildrenInGCThread(visitor). DEFINE_VISIT_ADDITIONAL_CHILDREN_IN_GC_THREAD(Subscriber) instantiates the template for both visitor types, and Subscriber.h narrows the public surface to the single template method. SUPPRESS_UNCOUNTED_LOCAL / SUPPRESS_UNCOUNTED_ARG / SUPPRESS_UNRETAINED_ARG annotations plus the comment "Do not ref anything in this function, which runs in a GC thread concurrently to the main thread" keep WebKit's static ref-checker quiet about deliberately not ref'ing objects from a GC thread.

Lock scope covering the snapshot of a collection but not the dereference of the borrowed, non-owning pointers taken from it.

Background

Observables and Subscriber. The DOM Observable API hands a Subscriber to the subscribe callback; script calls subscriber.addTeardown(fn) to register cleanup callbacks, which WebCore stores as Vector<Ref<VoidCallback>> m_teardowns. A subscription is torn down when it completes, errors, or its AbortSignal fires, and also when the ActiveDOMObject is stopped at document teardown or navigation.

VoidCallback. A generated WebIDL callback wrapper — a RefCounted C++ object that owns a reference to the underlying JS function object.

Custom mark functions. For wrappers whose C++ side holds JS values the generated bindings cannot see, WebKit defines visitAdditionalChildren / visitAdditionalChildrenInGCThread in a *Custom.cpp file. DEFINE_VISIT_ADDITIONAL_CHILDREN_IN_GC_THREAD(X) instantiates the template for both visitor types.

Concurrent marking. JSC marks the heap on dedicated GC threads that run alongside the main thread. AbstractSlotVisitor is the visitor type used on those threads; SlotVisitor is the main-thread variant. Code reached from a GC thread must not touch refcounts of main-thread-owned objects, which is why WebKit annotates these paths with SUPPRESS_UNCOUNTED_ARG / SUPPRESS_UNRETAINED_ARG to silence its static ref-checker.

Ref<T> versus T*. Ref<T> owns a reference and keeps the object alive; T::ptr() yields a bare pointer with no ownership. Clearing a Vector<Ref<T>> releases every reference it held.

Lock, Locker, WTF_GUARDED_BY_LOCK. WTF's mutex, its RAII scoped acquisition, and a static annotation declaring which lock guards a field.

Analysis

The root cause is a lock-scope error producing a use-after-free. teardownCallbacksConcurrently() used the lock only to build a snapshot, not to protect the use of that snapshot. It converted Ref<VoidCallback> elements into bare VoidCallback*, returned them by value, and the lock was released at function exit. The refcount was deliberately never incremented — ref'ing from a GC thread is unsafe in WebKit's model — so the returned vector carried zero lifetime guarantee. The caller then dereferenced each pointer outside any lock.

  GC thread                          Main thread
  ─────────                          ───────────
  lock m_teardownsLock
  copy N raw VoidCallback*
  unlock  ────────────────────┐
                              │      lock m_teardownsLock
                              │      stop(): m_teardowns.clear()
                              │        └─ last Ref dropped
                              │             └─ ~VoidCallback()
                              │      unlock
  ┌───────────────────────────┘
  ▼
  teardown->visitJSFunctionInGCThread(visitor)   ← UAF read of freed object

m_teardowns is Vector<Ref<VoidCallback>>, so the Subscriber holds the owning reference to every teardown callback. Two main-thread paths destroy those refs, both under m_teardownsLock: Subscriber::stop() does m_teardowns.clear(), and Subscriber::close() iterates and invokes teardowns with stop() called immediately after. Concurrent GC marking runs the custom mark function on a GC thread while the main thread is not stopped for the whole marking phase, which opens the interleaving above. The call reads the callback's stored JS function slot out of a destroyed heap object and hands the resulting value to the slot visitor — the exact body of visitJSFunctionInGCThread is inferred from its name and the general WebIDL-callback idiom, since VoidCallback.h/.cpp is not in the supplied context.

Holding the lock for the duration of the loop restores the invariant that every element of m_teardowns observed by the GC thread stays alive for the entire visit, because every destroy path also serialises on m_teardownsLock. The m_observer half was never a lifetime problem — m_observer is declared const Ref<InternalObserver>, so it is neither reassignable nor releasable for the lifetime of the Subscriber; removing observerConcurrently() is cleanup, not a fix.

This vulnerability weakens memory safety inside the WebContent process at the boundary between the main thread and JSC's concurrent GC threads. The security model assumption at stake is that any object a GC thread dereferences during marking remains alive for the whole dereference window. Before the fix, that guarantee was absent for teardown callbacks, so an attacker who wins the race could have a GC thread read a freed VoidCallback and feed whatever occupies that memory into the slot visitor as a JS cell reference. Beyond the immediate crash, an attacker who groomed the freed slot could plausibly influence what the collector treats as a live object-graph node, which would be a corruption-class outcome rather than a mere availability one — though that step is speculation about post-free heap state rather than something the diff establishes.

The deleted helper is a textbook example of a lock that looks correct at the call site and is useless in practice: teardownCallbacksConcurrently() acquires m_teardownsLock, so a reviewer scanning it sees synchronisation — but the value it returns is a vector of non-owning pointers whose validity is exactly what the lock was protecting. The moment the lock is scoped to the accessor rather than the use, the returned data is stale by construction. WebKit's own ref-checker annotations arguably contributed: the author correctly knew not to ref from a GC thread and silenced the analyzer, but silencing the analyzer removed the one signal that would have prompted the question "then what keeps this alive?" Any borrow-a-raw-pointer-because-we-cannot-take-a-reference pattern needs the lock, or some other keep-alive, to extend to the last dereference, and that is precisely the shape the fix imposes.

Audit directions

• Accessors that lock to produce a snapshot of non-owning handles and release before the caller dereferences them. The lock protects the read of the container, not the lifetime of its contents. Narrow: grep WebCore for methods returning Vector<T*> (or T*) built with .map([](auto& x) { return x.ptr(); }) or .get() from a Vector<Ref<T>>/HashMap<..., Ref<T>> member guarded by WTF_GUARDED_BY_LOCK, and check whether every caller re-takes the lock — start with other *Custom.cpp binding files under Source/WebCore/bindings/js. Wider: the same class appears with any borrow-then-release mechanism, not just Locker — a function returning an iterator, std::span, or StringView into a locked container, or one returning a raw pointer after a Ref local goes out of scope. In code review, the tell is a function whose return type is non-owning while its body contains a scoped lock or a scoped owning local. Widest: a borrow may not outlive the scope that guaranteed it — audit for it in any codebase with explicit lock scopes and non-owning views (Chromium's base::AutoLock returning raw_ptr/base::span out of a locked block, Rust code laundering a borrow through a raw pointer to escape the guard, Java collections returning views of a synchronised map). The question to carry: what keeps the returned thing alive after this function returns, and is it the same thing the lock was protecting?

• Every WebCore visitAdditionalChildrenInGCThread / visitAdditionalChildren implementation, for main-thread-mutable state read without serialisation. The pattern is GC-thread code that must not touch refcounts and therefore borrows raw pointers, while the main thread is free to destroy the referents concurrently. Narrow: grep Source/WebCore/bindings/js/*Custom.cpp and the corresponding DOM classes for visitJSFunctionInGCThread / visitAdditionalChildrenInGCThread callers and check, for each dereferenced member, whether it is const Ref<...> — immortal for the object's lifetime, like Subscriber::m_observer — or a mutable container like m_teardowns; mutable containers must be visited under the same lock that guards every mutation path. Wider: the same shape covers any WebKit code annotated SUPPRESS_UNRETAINED_ARG / SUPPRESS_UNCOUNTED_ARG / SUPPRESS_UNCOUNTED_LOCAL; treat each annotation as an explicit assertion by the author that lifetime is guaranteed by something other than refcounting, and verify what that something is — often it is "the object is const Ref" (fine) or "we took a lock somewhere" (needs checking). Match tell: an annotation suppressing the ref-checker on a member that is not const Ref/const UniqueRef and not dereferenced under a lock. This rung is bound to WebKit's annotation idiom and JSC's concurrent-marking model; there is no direct out-of-WebKit analog, so the audit ceiling is WebKit's binding layer.

• Destroy-side symmetry for every lock claimed to protect a GC-visited collection. A lock is only a lifetime guarantee if all release paths for the contained owning references also take it. Narrow: for Subscriber, both stop() and close() serialise correctly — apply the same enumeration to other ActiveDOMObject subclasses whose stop() clears a container a custom mark function reads, by grepping Source/WebCore for void stop() final bodies that clear or reassign members. Wider: the same check applies to any container mutated from destructors, contextDestroyed(), suspend()/resume(), or abort-algorithm callbacks — enumerate all writers to the guarded member, not just the obvious setter, and confirm each takes the guard. Widest: a lock protects a lifetime only if the set of writers taking it is complete — carry it into any system where a reader assumes a mutex implies liveness (Go maps under an RWMutex holding pointers freed elsewhere, C++ shared_ptr caches where one eviction path bypasses the mutex). Match tell: any write to a WTF_GUARDED_BY_LOCK member whose enclosing scope does not construct a Locker for that exact lock — the static annotation catches most, but assignments hidden behind helper methods or move-out patterns slip through.