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의 concurrent garbage collector는 mutator, 즉 JS 실행 thread가 계속 동작하는 동안 marker thread가 heap graph를 순회할 수 있도록 허용합니다. marker가 접근할 때마다 ref를 잡지 않고도 JSString의 StringImpl을 안전하게 dereference할 수 있도록, JSC는 "concurrent thread에서 접근했을 가능성이 있는" 대상을 모아두는 retained list를 유지합니다. 이 list는 marking pass가 진행되는 동안 해당 impl들의 생존을 보장하며, write barrier와 mutatorShouldBeFenced()가 바로 이 invariant를 전제로 설계되어 있습니다. 위쪽 커밋인 e69c479에서 추가된 between-GC clearing 경로는, GC 사이에 이 list가 무한정 커지는 것을 막기 위해 sweeper timer를 통해 주기적으로 list를 비웁니다.

이번 commit은 빠져 있던 guard를 추가합니다. clearConcurrentRetainedDataIfPossible()는 이제 mutatorShouldBeFenced()가 true인 동안, 즉 concurrent marker thread가 동작 중일 수 있는 동안에는 실행을 중단합니다. 기존에는 JS 실행 여부, 활성 상태인 GCOwnedDataScope, 그리고 진행 중인 JIT compilation만 guard 대상으로 삼았을 뿐, concurrent marking 상황은 아예 고려하지 않았습니다.

Significance

이번 fix는 ASan으로 확인된 use-after-free를 해결합니다. incremental sweeper timer가, marker thread가 fiberConcurrently()/costDuringGC를 통해 동시에 읽고 있는 StringImpl을 해제해버릴 수 있는 상황이었습니다. 이는 JSC의 concurrent collector에 존재하던 실제 memory-safety 버그이며, 가설적인 문제가 아닙니다. 또한 앞서 다룬 security fix #2와 직접적으로 연결되는 형제 격 문제이기도 한데, 해당 fix에서도 같은 retained list가 원래의 lifetime 재설계 대상이었습니다. cross-thread retention이 본래 목적인 구조에 between-GC drain 경로를 새로 추가하면서, drain 조건이 미처 열거하지 못한 새로운 reader가 함께 딸려 들어온 셈입니다.

Audit directions

이번 사례는 mutator와 marker 사이의 전형적인 race이며, 유사 사례를 찾는 좋은 template이 됩니다. concurrent marking이 진행되는 동안 실행될 수 있으면서도 mutatorShouldBeFenced()나 이에 준하는 fence를 확인하지 않는 clear-or-free 경로는 모두 UAF 후보에 해당합니다. 좁게 보면, Heap.cpp와 IncrementalSweeper에 있는 다른 periodic·timer-driven cleanup 경로들에서 같은 check가 빠져 있지 않은지 점검할 필요가 있습니다. 또한 mutatorShouldBeFenced() check와 실제 clear 사이에 marking이 새로 시작될 수 있는 window, 즉 fence flag에 대한 TOCTOU가 존재하지 않는지도 확인해야 합니다. 코드 리뷰에서 눈여겨볼 지점은, timer callback 안의 free나 clear()가 entryScope나 compilation count 같은 mutator 상태만 guard 조건으로 열거하고 marking 상태는 전혀 고려하지 않는 형태입니다. 넓게 보면, JSC 내에서 백그라운드 thread가 수행하는 cleanup 중, 애초에 특정 concurrent reader 집합을 전제로 precondition이 작성되었다가 이후 다른 reader가 추가로 상속된 경우라면 모두 같은 패턴에 해당합니다. sweeper, JIT worklist의 stub cleanup, 그리고 Options로 gate되는 각종 reclamation timer가 여기 포함됩니다. 가장 넓게 보면, "safe to free" predicate가 "type X의 reader가 활성 상태가 아니다"라는 여러 절의 conjunction으로 구성되어 있다면, 새로운 reader type이 추가될 때마다 그런 predicate 전부를 다시 점검해야 한다는 원칙으로 귀결됩니다. 이때 계속 이어가야 할 질문은, 이 조건이 처음 작성될 당시 어떤 reader들이 열거되어 있었고, 그 이후로 어떤 reader가 새로 추가되었는가입니다.