[JSC] `Heap::clearConcurrentRetainedDataIfPossible()` should not run while concurrent marking is active

c8e53c7

Source/JavaScriptCore/heap/Heap.cpp

@@ -1251,6 +1251,10 @@ void Heap::clearConcurrentRetainedDataIfPossible()
 
     if (!m_possiblyAccessedStringsFromConcurrentThreadsOrGCOwnedDataScope.size())
         return;
+
+    // The mutator needs to be fenced while marking and marker threads can access StringImpl::costDuringGC so we have to keep the Impls alive.
+    if (mutatorShouldBeFenced())
+        return;
 #if ENABLE(JIT)
     auto* worklist = JITWorklist::existingGlobalWorklistOrNull();

JSC runs concurrent marker threads that traverse the object graph in parallel with the mutator. When a marker visits a JSString, it loads the underlying StringImpl pointer via fiberConcurrently() and dereferences it (e.g., in StringImpl::costDuringGC called from JSString::estimatedSize) without incrementing the refcount — the m_possiblyAccessedStringsFromConcurrentThreadsOrGCOwnedDataScope list is the sole mechanism keeping those impls alive during this window. A between-GC cleanup path introduced in 314730@main was designed to prune this list via the IncrementalSweeper timer, but it only guarded against JS execution, active GCOwnedDataScopes, and in-flight JIT compilations — not concurrent marking.

This commit closes that gap by adding a mutatorShouldBeFenced() early-return. mutatorShouldBeFenced() is the canonical flag already used by write barriers to mean "markers may be running." Applying it here means the IncrementalSweeper timer skips the clear whenever a marker could still be dereferencing one of the retained impls.

Mutator (IncrementalSweeper, ~100ms timer)        Collector (marker)

  clearConcurrentRetainedDataIfPossible()           SlotVisitor::drain()
          │                                                   │
          │                                       JSString::visitChildrenImpl()
          │                                                   │
          ▼                                       fiberConcurrently() → StringImpl*  ← no ref held
  retainedList.clear()  ◄────── UAF ─────────────────────────┤
  (StringImpl freed)                              StringImpl::costDuringGC(*impl)  ← dangling read

After fix:
  if (mutatorShouldBeFenced()) return;  // markers active → defer clear to next timer fire

Significance

This is an ASAN-confirmed UAF on the GC collector thread — a class of memory safety bug that is frequently exploitable. The fix itself is a one-liner, but the underlying pattern (GC-retained data accessed ref-free by marker threads, with a separate clearing path added later) is a template for similar oversights elsewhere in the heap subsystem. The fix only defers the clear; the timer re-fires every 100ms, so if marking runs long or is triggered rapidly, the retained list grows unboundedly during the deferral window.

Audit directions

Audit commit 314730@main — the "between-GC clearing" feature this fixes — for any other data structures accessed ref-free by marker threads whose clearing paths do not check mutatorShouldBeFenced(). Then sweep for similar between-GC cleanups added elsewhere in Source/JavaScriptCore/heap/ that operate on data the concurrent marker may still be traversing.
JSString::fiberConcurrently() is the mechanism that hands a raw StringImpl pointer to a marker without a ref. Audit all its call sites and the assumptions each one makes about impl lifetime.
The original guard list (JS not running, no GCOwnedDataScope, no JIT inflight) is a useful checklist. Verify no other condition was missed besides mutatorShouldBeFenced() — and probe whether an attacker can keep the retained list large enough during the deferral window to cause memory pressure or influence GC timing in security-relevant ways.