[5] WebCore Range concurrent-GC UAF on lock-free treeOrder

JSC's atomized-string retention queue was scoped to a single GC — but a stack `StringView` can outlive the GC that retired its buffer.

Severity: High | Component: WebCore DOM Range | 621e3bf

Rated High because the diff confirms that Range::setStart/setEnd released m_boundaryPointLock between writing one boundary and reading both for treeOrder, exposing the concurrent GC marker to a torn RangeBoundaryPoint; the commit title attributes this directly to a UAF of container Nodes under Range::visitNodesConcurrently, which is a classic DOM-node UAF primitive in WebContent.

In Range::setStart and Range::setEnd, the patch collapses two separate m_boundaryPointLock critical sections into one. The pre-fix code locked once to write m_start (or m_end), released the lock, re-read both boundary points without the lock to compute treeOrder(...), then re-acquired the lock to fix the ordering by copying one boundary into the other. The new code computes the ordering question first using the parameters and a snapshot of the other boundary, then takes the lock once, writes the updated boundary point, and conditionally writes the second boundary point — all inside the same Locker { m_boundaryPointLock } scope. The seven boundary* static helpers are also rewritten to take a leading Locker<Lock>& parameter, documenting at the type level that the caller must already hold m_boundaryPointLock.

Source/WebCore/dom/Range.cpp

Patch Details

In Range::setStart and Range::setEnd, the pre-fix shape { lock; write_one; } read_unlocked treeOrder; { lock; maybe_write_other; } is replaced with compute_decision_from_args_and_snapshot; { lock; write_one; maybe_write_other; }. The conditional second boundary write now lives inside the same critical section as the first boundary write, so the marker cannot observe a partial update. Seven boundary* static helpers (boundaryNodeChildrenChanged, boundaryNodeChildrenWillBeRemoved, boundaryNodeWillBeRemoved, boundaryTextInserted, boundaryTextRemoved, boundaryTextNodesMerged, boundaryTextNodesSplit) take a leading Locker<Lock>& parameter; their call sites in Range::nodeChildrenChanged, Range::nodeChildrenWillBeRemoved, Range::nodeWillBeRemoved, Range::textInserted, Range::textRemoved, Range::textNodesMerged, and Range::textNodeSplit are updated to forward the existing locker. Only Range.cpp is touched.

Lock-free read of shared multi-word DOM boundary state races a concurrent GC visitor, allowing the marker to retain a stale container Node* past its lifetime.

Background

Range represents a live DOM range — a [start, end] interval over the DOM tree — used by Selection, find-in-page, Highlight, and similar features. Both endpoints are RangeBoundaryPoint values holding a RefPtr<Node> container, an unsigned offset, and a childBefore pointer used to keep the offset coherent across child mutations. The boundary points are multi-word state that cannot be read coherently without synchronization.

JSC's garbage collector runs a concurrent marking phase on a separate thread while JavaScript continues to execute on the main thread. Concurrent visitors call visit*Concurrently overrides on objects that hold native references to GC-managed roots, and these visitors must synchronize with mutator writes to any field they read. Range::visitNodesConcurrently is the per-Range concurrent visitor that traces m_start and m_end's container Nodes. m_boundaryPointLock is the per-Range lock that serializes mutator writes to m_start/m_end against this concurrent visitor.

treeOrder is a comparator that walks the DOM tree to order two boundary points; reading its inputs requires that those inputs be stable for the duration of the call. Locker<Lock>& as a parameter is a WebKit idiom that documents at the function signature that the caller must already hold the named lock — it does not enforce identity, but it forces every call site to materialize a locker, which is mechanically auditable at compile time.

Analysis

The bug is a data race producing a concurrent use-after-free during GC marking. Pre-fix Range::setStart/setEnd exposed two windows in which the GC's concurrent visitor could observe m_start/m_end in an inconsistent or torn state. After updating one boundary inside a lock and releasing it, the code re-read both m_start and m_end without the lock to compute treeOrder(makeBoundaryPoint(m_start), makeBoundaryPoint(m_end)). Because RangeBoundaryPoint is multi-word, that lock-free read races every concurrent writer and the GC marker. Between the first lock release and the conditional second lock re-acquisition, the marker could observe a state where m_start was already advanced past m_end, then the writer would overwrite m_end in a separate critical section.

The downstream consequence, per the commit title, is that the marker could retain a stale pointer to a container Node whose last live reference had just been dropped by the boundary mutation. A torn RangeBoundaryPoint lets the marker record the dropped container; later in marking — or in a downstream consumer of the marker's recorded roots — the marker dereferences that pointer, reading or writing freed memory. The HTML/DOM specification implicitly assumes a live Range's container nodes remain reachable to the GC for as long as the range references them; the pre-fix code violated this invariant whenever a mutator updated boundaries concurrently with the JSC concurrent marker.

The follow-up nature of this commit (Unreviewed. Addressing the review comments in the original PR.) and the seven helpers being retrofitted to take Locker<Lock>& parameters indicate the original race was reported on Bugzilla 311261 and that reviewers pushed for type-level documentation of the lock requirement after the initial fix. The Locker<Lock>& parameter idiom is a cheap mechanical mitigation — every call site must materialize a Locker, so a forgotten lock surfaces at compile time rather than as a sporadic GC crash.

This vulnerability weakens memory safety inside the WebContent renderer. The pre-fix code's shape — {lock; write;} read_unlocked; {lock; maybe_write;} — looks safe to a single-threaded reader but is exactly the shape that breaks under a concurrent marker that takes the same lock. Successful exploitation could yield a use-after-free on a DOM Node in the WebContent process, a class of primitive historically leveraged for renderer RCE.

Audit directions

• Critical sections that release the lock between a write and a follow-up read of the same shared state, on objects also visited by the JSC concurrent marker. Audit other WebCore types whose marker uses visit*Concurrently and a per-object lock (e.g., other DOM types with visitNodesConcurrently/visitAdditionalChildren overrides). Grep Source/WebCore for visitNodesConcurrently, then for each owner check whether every read of a marker-visible field happens inside the same Locker that gates writes.
• Helper functions that mutate marker-visible fields but take the mutated field by reference without any signal that a lock is required. Audit Source/WebCore/dom/ and Source/WebCore/editing/ for inline helpers operating on RangeBoundaryPoint, Position, or similar boundary types that are not of the form static void foo(Locker<Lock>&, ...) and confirm each caller holds the relevant lock. The Locker<Lock>& parameter idiom added here is worth retrofitting elsewhere as both documentation and a forcing function.
• Comparator/ordering calls (treeOrder, comparePositions, etc.) invoked on shared state outside a lock to decide whether a subsequent write is needed. Audit Range.cpp, Selection*.cpp, and Highlight*.cpp for treeOrder(makeBoundaryPoint(...), makeBoundaryPoint(...)) where one or both arguments read marker-visible members without the relevant lock; the fix here demonstrates the safe shape — use the new value plus a snapshot of the other side, then commit the decision inside one critical section.
• GC-visible state held as multi-word records (RefPtr<Node> + offset + childBefore) that cannot be read atomically. Investigate other WebCore objects that expose composite GC roots to a concurrent marker — verify that every reader path either takes the lock or reads through an atomic snapshot.