8d71b72447  Potential use-after-free under JSAttr::visitAdditionalChildren()

The bug at a glance

A concurrent garbage-collection thread could dereference an Attr’s owner Element after the main thread freed it, a genuine use-after-free/data race in core DOM binding code that is exercised by ordinary attribute manipulation plus GC. It is rated medium because it is a timing-dependent race between the GC marking thread and main-thread DOM mutation rather than a deterministically controllable corruption, and the observed impact is reading a freed Element to compute an opaque root.

GC marking runs concurrently with the main thread. JSAttr::visitAdditionalChildrenInGCThread read Attr::m_element (a WeakPtr) to compute the element’s opaque root, but the main thread can detach and destroy that Element at the same moment, so the marking thread races on and can dereference a freed Element.

Root cause

During concurrent garbage collection, JSAttr::visitAdditionalChildrenInGCThread was implemented as if (auto* element = wrapped().ownerElement()) addWebCoreOpaqueRoot(visitor, *element). That reads Attr::m_element and dereferences the resulting Element on a GC marking thread. Nothing synchronized this read with the main thread, which can concurrently run Attr::detachFromElementWithValue (nulling m_element) or otherwise destroy the owner Element.

Because m_element was a WeakPtr<Element>, the marking thread and the main thread could interleave such that the GC thread observed a non-null Element pointer and then called addWebCoreOpaqueRoot on it after the Element had begun or completed destruction, producing a use-after-free and an unsynchronized data race on the m_element field itself.

The patch introduces a mutable Lock m_elementLockForGC and funnels all access to m_element through it. detachFromElementWithValue and attachToElement now take the locker before writing m_element, and a new Attr::visitOwnerElementInGCThread(Visitor&) takes the same locker before reading m_element and calling addWebCoreOpaqueRoot. JSAttr::visitAdditionalChildrenInGCThread now simply calls wrapped().visitOwnerElementInGCThread(visitor). m_element is also changed from WeakPtr to CheckedPtr<Element>, documenting that it is only nulled by detachFromElementWithValue and that the lock, not weak-pointer semantics, now guards concurrent access. Explicit template instantiations are emitted for AbstractSlotVisitor and SlotVisitor.

Key code

Locked owner-element visitation added to Attr

template<typename Visitor>
void Attr::visitOwnerElementInGCThread(Visitor& visitor)
{
    Locker locker { m_elementLockForGC };
    if (m_element)
        addWebCoreOpaqueRoot(visitor, *m_element);
}

template void Attr::visitOwnerElementInGCThread(JSC::AbstractSlotVisitor&);
template void Attr::visitOwnerElementInGCThread(JSC::SlotVisitor&);

Patch walkthrough

• Source/WebCore/bindings/js/JSAttrCustom.cpp — Removes the direct ownerElement() read and addWebCoreOpaqueRoot call (and the WebCoreOpaqueRootInlines include) from visitAdditionalChildrenInGCThread, replacing them with a single call to wrapped().visitOwnerElementInGCThread(visitor) so the locked access lives in Attr.
• Source/WebCore/dom/Attr.cpp — Adds the WebCoreOpaqueRootInlines include and forward declarations of JSC::AbstractSlotVisitor/SlotVisitor. detachFromElementWithValue and attachToElement now update m_element inside a Locker on m_elementLockForGC. Adds visitOwnerElementInGCThread, which takes that locker and only then reads m_element and calls addWebCoreOpaqueRoot, plus explicit template instantiations for both visitor types.
• Source/WebCore/dom/Attr.h — Includes <wtf/Lock.h>, declares template<typename Visitor> void visitOwnerElementInGCThread(Visitor&), changes m_element from WeakPtr<Element, WeakPtrImplWithEventTargetData> to CheckedPtr<Element>, and adds the mutable Lock m_elementLockForGC guarding it.

Background

Concurrent GC marking (visitAdditionalChildren) — JSC marks the object graph on dedicated GC threads that run concurrently with the main thread. Custom wrappers implement visitAdditionalChildrenInGCThread to report extra roots. Because this executes off the main thread, any WebCore state it touches must be safe against concurrent main-thread mutation - refing objects here is forbidden, so reads must be otherwise synchronized.

Opaque roots — addWebCoreOpaqueRoot registers a native object (here the owner Element) as a root so the GC keeps the wrapper subgraph alive. Computing the opaque root requires dereferencing the Element, which is why a freed Element causes a use-after-free during marking.

Attr and its owner element — An Attr node wraps either an element/name pair or a standalone name/value pair. Attr::m_element points to the owning Element while attached and is nulled by detachFromElementWithValue when the attribute becomes standalone. The comment in Attr.h notes m_element/m_standaloneValue are the mutually exclusive halves.

WeakPtr vs CheckedPtr here — The field was a WeakPtr, whose thread-safe nulling gives a false sense of safety but does not synchronize the read-then-dereference against concurrent Element destruction. The patch switches to CheckedPtr<Element> and an explicit Lock, making the invariant (only detachFromElementWithValue nulls it, all cross-thread access is locked) explicit and enforced.

Vulnerability window

1. Attach — attachToElement sets m_element to the owning Element for a normal attribute node.
2. GC starts — A concurrent marking thread runs JSAttr::visitAdditionalChildrenInGCThread and reads m_element to compute the owner Element’s opaque root.
3. Race — Simultaneously the main thread detaches or destroys the owner Element (e.g. removing the attribute), freeing the Element while the GC thread still holds/derefs the pointer.
4. UAF — The GC thread calls addWebCoreOpaqueRoot on the freed Element - a use-after-free plus an unsynchronized data race on m_element.
5. Fix — m_elementLockForGC serializes all reads/writes of m_element; visitOwnerElementInGCThread reads under the lock, so the GC thread never observes a half-destroyed Element.

Triggering

No test was added; concurrent-GC races are not reliably reproducible. Conceptual trigger: create many Attr nodes whose JS wrappers are live, then in a tight loop detach/destroy their owner elements (remove attributes / drop element references) while forcing concurrent garbage collection, so a marking thread reads m_element as the main thread frees the Element.

Exploitation

1. Race window — Requires interleaving a concurrent GC marking pass over a JSAttr wrapper with main-thread destruction of the attribute’s owner Element.
2. Use — The GC thread dereferences the freed Element to compute its opaque root; observed impact is memory corruption/crash during marking. Reliable weaponization would demand controlling the GC scheduling and the freed-Element allocation, which is not shown - treat as crash-class.

Detection & hunting

For defenders and SOC / detection engineers:

• ASan UAF on Element during GC marking —
• TSan data race on Attr::m_element —

Audit directions

• Other visitAdditionalChildrenInGCThread implementations —
• WeakPtr fields touched on GC threads —
• Element back-pointers in DOM node wrappers —