621e3bf30e1d188679b927a6ea34c0c8971e020c  Data race in Range::visitNodesConcurrently during GC, leading to a use-after-free of RangeBoundaryPoint container nodes

The bug at a glance

This is the follow-up that closes the residual boundary-point data race left behind by commit ab488735a4: the first patch mutated m_start and m_end under two separate lock scopes with an unlocked treeOrder read in between, so the concurrent GC visit could still observe a half-updated boundary pair. Reachability and impact are identical to the parent bug — an unprivileged web page holding a live Range and provoking GC can drive a cross-thread race whose consequence is a use-after-free of container Node objects in WebContent — so the assigned High / CVSS 8.1 carries over. Read this together with ab488735a4: it does not fix a new bug so much as finish fixing the old one.

The first patch bolted a lock onto every Range boundary mutator, but setStart and setEnd cheated: they took the lock, wrote m_start, released it, then re-read m_start and m_end unlocked to decide whether the range had inverted, then took the lock again to fix up the other endpoint. Between those two locked windows the range was, briefly, a validly-lockable but semantically half-updated pair — exactly the inconsistency the GC thread was never supposed to see. This unreviewed cleanup computes the ordering decision up front from the incoming container/offset, then does both endpoint writes inside a single lock hold, and threads the Locker token through every boundary helper so the compiler documents that the lock is held. It is the difference between ‘we take the lock’ and ‘we hold the lock for the whole transaction.’

Root cause

In commit ab488735a4, Range::setStart looked like: take m_boundaryPointLock, call m_start.set(…), release; then, unlocked, evaluate !is_lteq(treeOrder(makeBoundaryPoint(m_start), makeBoundaryPoint(m_end))); then, if inverted, take the lock again and assign m_end = m_start. setEnd was symmetric. This left two defects. First, the treeOrder comparison reads m_start and m_end while holding no lock. Second and worse, the two writes (m_start, then possibly m_end) happen in two disjoint critical sections, so there is a window where m_start has been updated but m_end has not, and the lock is free — a concurrent Range::visitNodesInGCThread can acquire the lock in that gap and root a mismatched (m_start, m_end) pair, re-opening the very GC race the parent patch meant to close.

This patch restructures both functions so the ordering decision is computed before the lock and both endpoint writes occur inside one critical section. setStart now computes bool shouldAlsoSetEnd = !is_lteq(treeOrder(BoundaryPoint(container.copyRef(), offset), makeBoundaryPoint(m_end))) from the incoming container and offset (via copyRef(), not from the not-yet-written m_start), then, under a single Locker locker { m_boundaryPointLock }, does m_start.set(...) and, if shouldAlsoSetEnd, m_end = m_start. setEnd is the mirror image with shouldAlsoSetStart. The GC thread therefore only ever observes the pair either fully before or fully after the update — never the transient inverted/half-written state.

Computing treeOrder before the lock is safe precisely because m_start and m_end are only ever written on the main thread; the GC thread only reads. So reading m_end (or the incoming boundary) on the main thread to make the ordering decision cannot race with anything, while the actual publication of the new boundary values — the part the GC thread does observe — is now atomic under one lock.

The second half of the patch is a lock-discipline hardening: the static boundary helpers boundaryNodeChildrenChanged, boundaryNodeChildrenWillBeRemoved, boundaryNodeWillBeRemoved, boundaryTextInserted, boundaryTextRemoved, boundaryTextNodesMerged and boundaryTextNodesSplit each gain a leading Locker<Lock>& parameter. Their callers (Range::nodeChildrenChanged etc.) already hold the lock and now pass their locker token through. This changes no runtime behaviour but makes ’the boundary lock is held here’ a checkable part of each helper’s signature, preventing a future caller from mutating a boundary point without the lock.

Key code

setStart: ordering decided before the lock, both endpoints written atomically inside one Locker (Range.cpp)

ExceptionOr<void> Range::setStart(Ref<Node>&& container, unsigned offset)
{
    ...
    if (childNode.hasException())
        return childNode.releaseException();

    bool shouldAlsoSetEnd = !is_lteq(treeOrder(BoundaryPoint(container.copyRef(), offset), makeBoundaryPoint(m_end)));
    {
        Locker locker { m_boundaryPointLock };
        m_start.set(WTF::move(container), offset, childNode.releaseReturnValue());
        if (shouldAlsoSetEnd)
            m_end = m_start;
    }

    updateAssociatedSelection();
    updateDocument();
    updateAssociatedHighlight();
    ...
}

Patch walkthrough

• Source/WebCore/dom/Range.cpp (setStart / setEnd) — Replaces the two-lock-scope shape with a single critical section. The inversion decision is hoisted above the lock and computed from the incoming boundary — BoundaryPoint(container.copyRef(), offset) compared via treeOrder against makeBoundaryPoint(m_end) (setStart) or makeBoundaryPoint(m_start) (setEnd). Inside one Locker scope it writes m_start.set(…) and, when shouldAlsoSetEnd, m_end = m_start (mirror for setEnd). This makes the two-endpoint update atomic with respect to the GC thread’s visit, closing the half-updated-pair window.
• Source/WebCore/dom/Range.cpp (boundary* helpers + callbacks) — Adds a Locker<Lock>& first parameter to boundaryNodeChildrenChanged, boundaryNodeChildrenWillBeRemoved, boundaryNodeWillBeRemoved, boundaryTextInserted, boundaryTextRemoved, boundaryTextNodesMerged and boundaryTextNodesSplit, and updates nodeChildrenChanged, nodeChildrenWillBeRemoved, nodeWillBeRemoved, textInserted, textRemoved, textNodesMerged and textNodeSplit to pass their held locker through. This is a compile-time proof-of-lock-held convention with no behavioural change; the callers already acquired m_boundaryPointLock in the parent patch.

Background

Atomic multi-field publication — When two fields (m_start and m_end) form one invariant (start <= end) and another thread reads both, both must be written inside a single critical section; updating them in separate lock scopes lets a reader observe a transiently inconsistent pair.

treeOrder / BoundaryPoint — treeOrder returns the document-order relationship between two boundary points; makeBoundaryPoint builds one from a RangeBoundaryPoint. The fix constructs a BoundaryPoint from the incoming container.copyRef()/offset so the ordering test does not depend on m_start having already been written.

Locker token threading — Passing Locker<Lock>& into a helper is a WebKit idiom that encodes ‘caller holds this lock’ in the type signature, so a helper cannot be called on a shared field without proof the lock is held — a lightweight compile-time lock-discipline guard.

Single-writer / concurrent-reader model — m_start/m_end are written only on the main thread and read by the main thread and the GC thread. That is why reading them unlocked for a decision on the writer thread is safe, but publishing new values must be synchronised with the concurrent reader.

Vulnerability window

1. Parent fix applied — Commit ab488735a4 added m_boundaryPointLock but implemented setStart/setEnd as two separate locked writes with an unlocked treeOrder read between them.
2. Half-update window opens — During setStart the main thread writes m_start under the lock, releases it, and evaluates treeOrder while unlocked — leaving m_start updated but m_end stale, with the lock free.
3. GC thread slips in — A JSC marking thread acquires m_boundaryPointLock in that gap and roots the mismatched (new m_start, old m_end) pair — the inconsistent state the lock was supposed to hide.
4. Residual race / UAF — The concurrent read of a transiently inconsistent boundary pair reopens the container-node race, with the same freed-container-node consequence as the parent bug.
5. This fix closes it — Ordering is computed pre-lock from the incoming boundary; both m_start and m_end are written inside one Locker scope, so the GC thread only ever sees a fully-before or fully-after pair, and helper signatures now require the held lock.

Proof of concept

Grounded in the exact diff: the residual race lived in setStart/setEnd’s inversion fixup, so this PoC deliberately drives the inverting branch (setting the endpoints out of order) while generating GC pressure, maximising the chance the marking thread visits the range during the half-updated window. Like its parent, the race has no deterministic trigger and no script-visible result — a hit is a WebContent TSan/ASan report on m_start/m_end or a container Node, reproducible only against a build without this commit.

// Same non-deterministic nature as the parent bug: the residual race is only
// hittable in the transient window between the two locked writes of the pre-fix
// setStart/setEnd. This targets that window specifically.
const r = document.createRange();
const a = document.body.appendChild(document.createElement('div'));
const b = document.body.appendChild(document.createElement('div'));
b.appendChild(document.createElement('span'));

function churn() {
  for (let i = 0; i < 20000; i++) {
    // Force the inverting branch: set start after end, then before, so the
    // pre-fix code repeatedly hits the 'write m_start, unlock, re-order, relock,
    // write m_end' path where the pair is momentarily inconsistent.
    r.setStart(b, 1);
    r.setEnd(a, 0);   // end now before start -> inversion fixups run
    r.setStart(a, 0);
    r.setEnd(b, 1);
    if ((i & 0x3ff) === 0) { let junk = []; for (let j = 0; j < 5000; j++) junk.push({x:j}); }
  }
  requestAnimationFrame(churn);
}
churn();

Exploitation

1. Hit the two-write gap — Against a build with only the parent patch, the attacker must land the GC visit inside the brief unlocked interval between the m_start write and the m_end fixup — a narrower window than the fully-unsynchronised original, so even harder to hit and steer.
2. Convert to a freed container — As with the parent bug, turning the observed inconsistent pair into a controlled free of a specific container Node depends on precise interleaving of the collector’s rooting with refcount changes, offering little control over the victim object.
3. Standard renderer UAF follow-through — If a container Node is freed while referenced, generic reclaim-and-corrupt technique applies in WebContent, but the doubly-conditional and non-deterministic trigger makes this residual variant impractical to weaponise reliably; its main value is closing the correctness gap flagged in PR review.

Detection & hunting

For defenders and SOC / detection engineers:

• TSan race isolated to setStart/setEnd fixup — On a parent-patched build, ThreadSanitizer races where the main-thread stack is inside Range::setStart/setEnd (the inversion fixup) concurrent with a GC-thread visit pinpoint this residual window specifically.
• Inconsistent boundary pair observed on GC thread — Assertions or logging that m_start <= m_end at the moment of visitNodesInGCThread would flag the transient inverted state this patch eliminates.
• Missing-lock helper calls — After this patch, any call to a boundary* helper without a Locker<Lock>& argument fails to compile; a fuzzer/build guard can treat re-introduction of a token-less overload as a regression signal.

Audit directions

• Multi-field updates under one lock — Hunt other WebCore mutators that update a pair of GC-visited fields in separate lock scopes or with an unlocked read between writes; the m_start/m_end split here is the archetype to grep for.
• Locker-token coverage — Verify every function that mutates m_start/m_end or their RangeBoundaryPoint internals either takes m_boundaryPointLock or a Locker<Lock>& token; any residual path that touches boundaries without the token is a candidate re-opening of the race.
• Sibling GC-visited DOM types — Apply the same ‘compute decision unlocked, publish atomically under one lock’ audit to other concurrently-visited objects (Selection, highlights) whose invariants span multiple fields.