Heap::clearConcurrentRetainedDataIfPossible() must not run while concurrent marking is active

JSC HeapRace

Component: JSC Heap | c8e53c7

Source/JavaScriptCore/heap/Heap.cpp

 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's concurrent garbage collector lets marker threads walk the heap graph while the mutator — the JS execution thread — keeps running. To let markers safely dereference a JSString's StringImpl without taking a ref on every access, JSC maintains a "possibly accessed from concurrent threads" retained list that keeps those impls alive for the duration of a marking pass; this is the invariant that write barriers and mutatorShouldBeFenced() are built around. The between-GC clearing path — added in e69c479 above, to stop the list growing unboundedly between collections — periodically empties that list off a sweeper timer.

This commit adds the missing guard: clearConcurrentRetainedDataIfPossible() now bails while mutatorShouldBeFenced() is true, i.e. while concurrent marker threads may be running. Previously it only guarded against JS execution, a live GCOwnedDataScope, and in-flight JIT compilations, missing the concurrent-marking case entirely.

Significance

This fixes an ASan-confirmed use-after-free where the incremental sweeper timer could free a StringImpl that a marker thread was concurrently reading via fiberConcurrently()/costDuringGC. It is a real memory-safety bug in JSC's concurrent collector, not a hypothetical — and it is the direct sibling of security fix #2 above, where the same retained list was the object of the original lifetime rework. Adding a between-GC drain path to a structure whose whole purpose is cross-thread retention introduced a new reader the drain condition did not enumerate.

Audit directions

This is a textbook mutator-vs-marker race and a good template for finding siblings: any clear-or-free path that can run while concurrent marking is in flight and does not check mutatorShouldBeFenced() (or an equivalent fence) is a UAF candidate. Narrow: audit the other periodic and timer-driven cleanup paths in Heap.cpp and IncrementalSweeper for the same missing check, and confirm there is no window between the mutatorShouldBeFenced() check and the actual clear where marking could start — a TOCTOU on the fence flag. In code review, the tell is a free or clear() in a timer callback whose guard conditions enumerate mutator states (entryScope, compilation counts) but never marking state. Wider: the same shape covers any background-thread cleanup in JSC whose preconditions were written against one set of concurrent readers and later inherited another — the sweeper, the JIT worklist's stub cleanup, and any Options-gated reclamation timer. Widest: when a "safe to free" predicate is a conjunction of "no reader of type X is active" clauses, adding a new reader type requires revisiting every such predicate — the carry-forward question is which readers were enumerated when this condition was written, and which have been added since.