[2] IPC::Connection SyncMessageState destruction escapes its lock

A lock refactor let a shared object's last reference escape the lock.

Severity: Medium | Component: WebKit IPC layer | 89d4e73

Medium. A locking refactor moved one refcount release out of the critical section that serialized it, turning a hot per-reply teardown into a cross-thread race on shared IPC state. Escalation is gated on an attacker reliably winning that destruction race and then grooming the heap around a ThreadSafeRefCounted free.

Cross-thread reference counting in WebKit's IPC layer is normally kept safe by a single lock that serializes the read-use-drop of shared per-connection state against any concurrent teardown. Every Connection — the object carrying messages between the WebContent, GPU, Networking, and UI processes — processes incoming messages on a dedicated work-queue thread and holds a reference to a SyncMessageState, the per-dispatcher coordinator shared by every connection bound to the same target thread. That shared coordinator is destroyed only when its last reference drops, and the expectation is that the final release runs under the same m_incomingMessagesLock that guards invalidate()'s concurrent release on the dispatcher thread.

The angle: a peer already sitting on one end of an IPC connection can race a WebPage teardown and corrupt the shared SyncMessageState in the receiving process, whose reported teardown path runs through the WebContent process's GPUProcess connection.

316617@main added incomingMessagesLocker.unlockEarly() / waitForMessagesLocker.unlockEarly() to the async-reply-with-dispatcher branch of Connection::processIncomingMessage() so the reply handler would run without the locks held (matching the other branches and avoiding re-entrancy). However, the RefPtr syncState = m_syncState local declared earlier in the function is destroyed before those lockers at the return (reverse declaration order), so this change moved the final SyncMessageState deref — and the ~SyncMessageState() it can trigger — out from under m_incomingMessagesLock on a hot path (every async reply, including GPUProcess sendWithAsyncReply replies). The fix drops the SyncMessageState reference (syncState = nullptr) while still holding m_incomingMessagesLock, before unlockEarly(), in both async-reply branches.

Source/WebKit/Platform/IPC/Connection.cpp

Patch Details

The change touches only IPC::Connection::processIncomingMessage(). In the two async-reply branches — the invalid-destinationID early-return branch and the takeAsyncReplyHandlerWithDispatcherWithLockHeld() branch — it inserts syncState = nullptr; immediately before the existing incomingMessagesLocker.unlockEarly() / waitForMessagesLocker.unlockEarly() calls. This forces the local RefPtr syncState = m_syncState to release its reference while m_incomingMessagesLock is still held, instead of at function return after the lockers have already been early-unlocked. No other logic changes; the reply handler still runs with the locks released.

The bug is purely one of destruction ordering at scope exit:

  Declaration order            Destruction order (reverse)
  ─────────────────            ───────────────────────────
  RefPtr syncState  (early)      Locker incomingMessagesLocker
  Locker incomingMessagesLocker  Locker waitForMessagesLocker
  Locker waitForMessagesLocker   RefPtr syncState   <-- last, and now
                                                        AFTER unlockEarly()

A refcounted resource whose destruction must stay inside a critical section escapes it because reverse-order scope destruction places the owning smart pointer's release after an early lock release.

Background

The four-process model. WebKit runs cross-process message transport between the UIProcess, WebContent, GPUProcess, and Networking process. IPC::Connection is the transport shared across those roles; each connection processes incoming messages on a dedicated work queue (processIncomingMessage() is documented as "Called on the connection work queue").

Dispatchers and shared state. In WebKit's threading model, a SerialFunctionDispatcher is the abstract owner of a run loop or work queue — the thing that decides which thread a piece of code runs on. Each Connection is bound to one dispatcher. SyncMessageState is a per-dispatcher coordinator created once per SerialFunctionDispatcher via SyncMessageState::getOrCreate() and shared by every Connection bound to it; it coordinates message dispatch while a thread waits for a synchronous reply. The dispatcher abstraction exists so multiple connections bound to the same target thread can share coordination state rather than each duplicating it.

SyncMessageState lifetime. SyncMessageState is ThreadSafeRefCounted and kept alive by the m_syncState of every connection bound to its dispatcher (for example, the main UIProcess connection and the RemoteRenderingBackendProxy GPUProcess connection). Its destructor takes syncMessageStateMapLock and removes the dispatcher's entry from a global map. Connection::invalidate() runs on the dispatcher thread and drops its own m_syncState reference under m_incomingMessagesLock — that lock is what serialized the "read m_syncState, use it, drop it" section on the connection work queue against invalidate().

unlockEarly() and destruction order. Locker is a scoped RAII lock guard that normally releases at scope exit; unlockEarly() releases it immediately so subsequent code runs unlocked. WebKit uses unlockEarly() on hot IPC paths deliberately: holding an IPC lock across a reply handler would block other messages queued behind the reply, so the reply handler is meant to run unlocked. Separately, C++ destroys automatic locals in reverse declaration order at scope exit — a language rule, but the load-bearing one here: a RefPtr declared before a Locker is destroyed after that Locker.

Analysis

The root cause is a data race producing a use-after-free on a ThreadSafeRefCounted object, introduced by an interaction between unlockEarly() and C++ scope-exit order. unlockEarly() releases m_incomingMessagesLock before the enclosing scope ends, but syncState, declared above both lockers, is destroyed after them. So the release of syncState at function return — potentially the last reference, triggering ~SyncMessageState() — runs with m_incomingMessagesLock no longer held.

  Work-queue thread                Dispatcher thread
  ─────────────────                ─────────────────
  processIncomingMessage()
    reply handler runs
    unlockEarly()  (lock released)
    ...return...
    ~RefPtr syncState  ─┐          Connection::invalidate()
    last deref          │            drops m_syncState under
    ~SyncMessageState() │            m_incomingMessagesLock
      takes             │            (its own last-ref path)
      syncMessageStateMapLock ◄────► concurrent teardown of the
                                      same shared object -> UAF

With the final deref now unlocked, the work-queue thread's ~SyncMessageState() — which enters syncMessageStateMap() under syncMessageStateMapLock to remove the dispatcher entry — can race invalidate()'s concurrent teardown of the same shared object. The commit message reports this as a heap-use-after-free caught by ASan during WebPage teardown, along the path ~WebPage → ~RemoteRenderingBackendProxy → disconnectGPUProcess → Connection::invalidate().

Exploitability depends on reliably winning the destruction race between the connection work-queue thread and the dispatcher thread's invalidate(). If an attacker could do so, the resulting use-after-free on a ThreadSafeRefCounted object could, under controlled heap conditions, corrupt refcount or map state; realizing that into a memory-corruption primitive would require a full exploit chain and deterministic race control the change does not itself establish.

This vulnerability weakens memory safety within whichever process owns the racing connection (the reported teardown runs in the WebContent process's GPUProcess connection). The security model assumption at stake is that access to and destruction of the shared per-dispatcher SyncMessageState is serialized by m_incomingMessagesLock — the regression violated exactly that assumption.

This is a textbook C++ destruction-order footgun: a smart-pointer owner declared before the lock guard it must release under. unlockEarly() silently breaks the assumption that everything below a Locker runs locked, but automatic locals declared above the Locker still destruct after it. Any function that uses unlockEarly() to run callbacks unlocked must audit every RAII local declared before the lock guard whose destructor has cross-thread side effects. The 316617@main change preserved correctness in the two other branches only because they happened not to hold such an owner past the early unlock.

Audit directions

• Early lock-release exposing reverse-order destruction of RAII locals declared before the lock guard. A destructor with cross-thread side effects escapes the critical section. Grep Source/WebKit/Platform/IPC and the rest of WebKit for unlockEarly(, and for each hit check whether any RefPtr/Ref/UniqueRef or other destructor-bearing local is declared before the Locker. In review, the tell is an owning local higher in the function body than the lock guard whose ~T() touches shared state — any unlockEarly() adjacent to a locally-declared RefPtr deserves a comment spelling out what must be released first. Wider: the same shape appears with manual lock.unlock() followed by still-live locals, or scope-guard .release()/makeScopeExit locals ordered before a lock. Widest: this is the general "RAII resource released after the lock that was supposed to serialize its release" invariant, applying to any codebase mixing early-release locking with RAII ownership (Chromium base::AutoLock with early Release(), Rust drop(guard) before an owned value drops). The portable question: after the early unlock, does anything the lock protected get destroyed or mutated on the current thread before another thread can touch it?
• Shared ThreadSafeRefCounted state whose destructor mutates a global registry. Such destructors rely on an external lock to serialize the last-reference release against a concurrent unregister. Audit SyncMessageState and other per-dispatcher/per-connection shared objects in IPC/Connection.cpp — confirm every path that can drop the final reference does so under m_incomingMessagesLock, matching Connection::invalidate(). Wider: enumerate other getOrCreate-style singletons keyed in a global map whose ~T() removes their entry under a second lock while refcount drops are guarded by a different lock — the two-lock split is the danger. Widest: destruction of shared refcounted state that touches another data structure must be serialized against every concurrent mutator of that structure. The tell is a destructor that acquires lock B while the last-ref-drop is only sometimes guarded by lock A.

Note: The regression is attributed by the commit message to 316617@main, and the heap-use-after-free and teardown path to an ASan report; neither the prior commit nor the report is included in the supplied context, so those attributions are relayed as-is.