[4] Data race in Range::visitNodesInGCThread during GC

HighWebCore DOMRace

The garbage collector read a DOM node the main thread had just freed

ab48873

High. A garbage-collector marking thread could dereference a Node whose last strong reference the main thread had just released — memory unsafety reachable from any script that holds a live Range and mutates the DOM. Reliability is what pulls it back from Critical: the window is a few instructions wide and reclaiming the freed allocation in time is not a given.

DOM Range objects — the things document.createRange() and getSelection().getRangeAt() return — hold two endpoints that the document keeps up to date as the tree changes underneath them. Each endpoint is a RangeBoundaryPoint storing its container node as a Ref<Node>, a never-null reference-counted pointer whose assignment stores the incoming pointer and releases the outgoing one. Meanwhile JSC's collector traces the heap on dedicated marking threads while the main thread keeps running, and WebCore reports non-JS objects to it as "opaque roots" so their wrappers survive. The assumption that makes that safe is that data a marking visitor reads is either immutable during marking or serialized by a lock.

The angle: a page holding a live Range and mutating the DOM in a tight loop can get a collector thread to read a Node whose memory the main thread just freed.

Add a lock for mutating m_start and m_end.

No new tests since there is no reliable way of testing this data race.

Source/WebCore/dom/Range.h

Source/WebCore/dom/Range.cpp

Source/WebCore/dom/Document.cpp

Patch Details

The change introduces a lock and takes it on both sides of the mutator/collector boundary, plus a collateral annotation change with a matching iteration fix.

A mutable Lock m_boundaryPointLock member is added to WebCore::Range, with <wtf/Lock.h> and <wtf/Locker.h> includes. On the write side, Locker scopes are added in Range::setStart and Range::setEnd (two scopes each — one for the m_start.set(...) / m_end.set(...) call, one for the m_end = m_start / m_start = m_end normalization after the treeOrder comparison), and in Range::collapse, selectNodeContents, nodeChildrenChanged, nodeChildrenWillBeRemoved, nodeWillBeRemoved, textInserted, textRemoved, textNodesMerged, and textNodeSplit. On the read side, the pre-existing Range::visitNodesInGCThread — whose signature is unchanged, appearing in the diff only as context — now acquires the same lock before calling addWebCoreOpaqueRoot(visitor, m_start.container()) / m_end.container().

The collateral: the NODELETE annotation is dropped from the declarations of nodeChildrenChanged, textInserted, and textRemoved in Range.h, and correspondingly Document::textInserted / Document::textRemoved change their iteration over m_ranges from for (auto& range : m_ranges) range.get()... to for (Ref range : m_ranges) range->..., so each Range is ref-protected across the now-non-NODELETE call.

Unsynchronized concurrent read of a reference-counted pointer field whose mutation both rewrites the pointer and releases the outgoing object's last reference.

Background

Live DOM Range. A Range object created from script that holds a start and an end boundary point and is kept up to date by the Document as the tree mutates. Document::m_ranges holds the attached live ranges; DOM mutations such as text insertion call into each registered range (Document::textInserted → Range::textInserted) so boundary offsets and containers can be adjusted.

RangeBoundaryPoint and Ref<T>. RangeBoundaryPoint is the concrete storage behind Range::m_start and Range::m_end; it holds the container node as a Ref<Node> together with an offset and a child-before pointer. Ref<T> is a never-null reference-counted smart pointer; assigning to a Ref stores the incoming pointer and releases the outgoing one, destroying the outgoing object when that was the last reference.

Concurrent marking and opaque roots. JSC's collector runs marking work on dedicated threads while the mutator — the main thread — continues to execute JavaScript and DOM operations. WebCore reports non-JS objects such as a DOM tree's root node to the collector via addWebCoreOpaqueRoot(visitor, node); a wrapper whose opaque root is marked is kept alive.

Lock / Locker. WebKit's mutex and its RAII scope guard; Locker locker { lock } acquires on construction and releases at end of scope.

NODELETE. An annotation on WebCore method declarations marking code paths that must not trigger object destruction.

Analysis

The root cause is that Range::visitNodesInGCThread — whose name and JSC::AbstractSlotVisitor& parameter indicate it runs on a marking thread — read m_start.container() / m_end.container() with no synchronization, while the main thread freely mutated those same members from DOM and Range APIs.

  Main thread (mutator)              JSC marking thread
  ─────────────────────              ──────────────────
                                     load m_start.container()
                                       -> Node* N  ──┐
  m_start.set(newContainer, ...)                     │
    store new pointer                                │
    deref old  -> refcount 0                         │
    ~Node()  -> free(N)                              │
                                     addWebCoreOpaqueRoot(visitor, N)
                                       -> dereferences N  ◄─┘  UAF

Every mutation path the patch now guards — set(), setToBeforeContents(), setToAfterContents(), the m_end = m_start copy assignments in collapse()/setStart()/setEnd(), and the boundary-adjustment helpers boundaryNodeWillBeRemoved, boundaryNodeChildrenWillBeRemoved, boundaryTextInserted, boundaryTextRemoved, boundaryTextNodesMerged, boundaryTextNodesSplit — performs a non-atomic pointer overwrite and drops a strong reference on the previously stored Node. The Ref<Node> assignment is two steps: store the new raw pointer, deref() the old one. A marking thread executing addWebCoreOpaqueRoot(visitor, m_start.container()) can load the outgoing Node* and then use it after the main thread's deref() has already run the destructor — which matters exactly when the boundary point held the last strong reference. addWebCoreOpaqueRoot derives the opaque root from the node rather than merely recording the raw pointer value, so the freed object is dereferenced, not just compared.

A second, weaker failure follows from the same race: the marker can observe a torn boundary pair — an m_start from before a mutation together with an m_end from after it — so the opaque-root set computed for that marking increment corresponds to no consistent snapshot of the Range.

Reaching the racy state is entirely web-content-driven: any script that holds a live Range and mutates the DOM exercises the write side, and the read side runs whenever concurrent marking visits the Range. A plausible trigger sequence: (1) allocate a large number of Range objects whose boundary containers point at nodes in a detached subtree, aiming for a state where RangeBoundaryPoint's Ref<Node> is the last strong reference to those nodes — whether a RangeBoundaryPoint can realistically hold the last strong reference to its container is the load-bearing assumption of this trigger, and it would need to be confirmed empirically; (2) drive allocation pressure so concurrent marking runs for a sustained window; (3) from a tight loop, repeatedly call range.setStart(otherNode, 0) / range.collapse() / range.selectNodeContents(other), or perform removeChild / splitText / text mutation so boundaryNodeWillBeRemoved and boundaryTextNodesSplit rewrite the container — each iteration overwrites the Ref<Node> and releases the outgoing node.

Realising a controlled outcome would require (a) winning a window only a few instructions wide, (b) reclaiming the freed Node allocation with attacker-shaped data before the marker reads it, and (c) surviving the read without an immediate fault. If all three held, the marker could register an attacker-influenced value as an opaque root, which might in turn perturb which wrappers survive the collection. A distinct direction follows from the torn-read variant: because the unsynchronized read can observe a boundary pair that never simultaneously existed, the collector could fail to register the opaque root for the subtree a boundary actually points into, and if that opaque root was the only keep-alive, premature collection of those wrappers could leave the Range pointing into a partially-collapsed object graph. Both directions depend on race timing rather than on a deterministic primitive; no attacker-controlled write is visible in the patched code paths.

Discovery most likely came from pattern auditing of concurrent-GC visit callbacks — going looking for WebCore state that a marking thread reads without synchronization, of which visitNodesInGCThread reading two plain Ref<Node>-backed boundary points is a textbook instance. The shape is equally consistent with crash-report telemetry: sporadic, non-reproducible faults on a marking thread inside opaque-root reporting are exactly what this race produces, and such reports typically prompt targeted review. Fuzzing is an unlikely origin for a race with this window, and the patch adds no test.

This vulnerability weakens memory safety inside the WebContent process by breaking the lifetime contract between the mutator and the concurrent collector: before the fix, the GC thread could dereference a Node whose last strong reference had just been released by main-thread DOM code. The security-model assumption at stake is that data reachable from a concurrent marking visitor is either immutable during marking or protected by synchronization; before the fix, Range::m_start/m_end satisfied neither. An attacker who reliably won this race could obtain reads of freed Node memory from the collector thread and could perturb which objects the collector treats as roots, which under favourable heap conditions would be a stepping stone toward a controllable use-after-free in the renderer rather than a mere crash.

Insight: Ref/RefPtr gives no protection at all for the pointer slot itself — the refcount's atomicity protects the count, not the field, and not the ordering between "store new pointer" and "deref old pointer." Any WebCore member legal to read from a marking thread is effectively shared mutable state, yet nothing in the type system distinguishes such members from ordinary main-thread-only ones. Equally notable is what the fix deliberately does not lock: the plain main-thread accessors and the unlocked makeBoundaryPoint(m_start) read sitting between the two Locker scopes in setStart/setEnd. That asymmetry is sound only while the main thread is the unique writer — an assumption the patch encodes implicitly rather than asserting.

Audit directions

• Object fields that a concurrent collector reads without synchronization while the mutator both rewrites the field and releases the outgoing object's last reference — smart-pointer types make this look safe because the refcount is atomic, while the pointer slot is not. Narrow: grep WebCore for opaque-root reporting calls (addWebCoreOpaqueRoot() and for functions taking JSC::AbstractSlotVisitor& / visitAdditionalChildren, and check for each whether the Ref/RefPtr member being read can be reassigned by main-thread code without a lock — Range is now guarded, but sibling DOM bookkeeping types with visit callbacks (selection, highlight registries, observer registries) deserve the same pass. In code review, a visit*/AbstractSlotVisitor function body that dereferences a non-atomic Ref/RefPtr member with no Locker in scope is the tell. Wider: wherever any background thread (scrolling thread, media/AV callback threads, GPU worker threads, AXIsolatedTree builders) reads a main-thread-owned smart-pointer member — also WeakPtr slots read off-thread and std::unique_ptr members swapped under the reader; the code-search shape is a member declared without Atomic, without a guarded-by annotation, and without a nearby Lock, but referenced from a function whose name or comment names a non-main thread. Widest: a reference-counted handle protects the count, never the slot; any cross-thread reader of a mutable owning-pointer field needs its own synchronization or an atomic handle type — carry into Blink's concurrent marking with TracedReference, Rust's Arc<T> in a plain field mutated through interior mutability, and Java/Go concurrent-mark write barriers, always asking "who can drop the last reference to the value this collector thread just loaded?"

• Verify that the asymmetric locking discipline this patch establishes is actually complete for Range. Narrow: enumerate every writer of Range::m_start/m_end and confirm each is inside an m_boundaryPointLock scope — the diff covers the listed mutators, but Range::updateFromSelection and Range::updateRangeForParentlessNodeMovedToNewDocument are declared in Range.h and do not appear in the diff, so read those two first. In code review, the tell is a call to a non-const RangeBoundaryPoint method (or an assignment to m_start/m_end) in Range.cpp with no Locker visible in the enclosing block. Wider: the same latent breakage exists in any "one writer thread, one reader thread, lock only the writer" design elsewhere in WebCore — look for classes carrying a mutable Lock where only a subset of accessors take it, and ask whether the single-writer assumption is asserted or merely incidental. Widest: partial locking is sound only under an unwritten thread-affinity assumption; if the assumption is not asserted in code, the next refactor will violate it silently — look for an assertIsMainThread()-equivalent (or a Rust Send/!Sync marker) next to every unlocked accessor of a partially-locked field, and treat its absence as the finding.

• Audit the annotation-removal side of this patch as its own signal. NODELETE was dropped from Range::nodeChildrenChanged, Range::textInserted, and Range::textRemoved, and Document::textInserted/textRemoved compensated by iterating m_ranges as for (Ref range : ...). Narrow: search Document.cpp and other WebCore registries for loops over collections of raw or weak elements (m_ranges, marker/observer/listener registries) that invoke a callee which is not NODELETE. In code review, the tell is a range-based for over a registry using auto& plus .get() rather than Ref, where the loop body calls a method that can run arbitrary WebCore code. Wider: "iterating a container while the callee can mutate or destroy its elements" shows up with any observer-notification loop, any copyToVector snapshot that was optimized away, and any for over a WeakHashSet — the shape to notice is a loop that neither snapshots the container nor ref-protects its elements. Widest: notification loops must own either a snapshot of the collection or a strong reference to each element for the duration of the callback — applying to DOM event dispatch, Node's EventEmitter, Qt signal/slot emission, and any Vec<Weak<T>> iterated while handlers can push or drop entries; the carry-forward question is "can the callee re-enter and modify the container I am iterating?"