Changeset 117340 in webkit

Timestamp:

May 16, 2012 2:06:53 PM (12 years ago)

Author:

ggaren@apple.com

Message:

GC is not thread-safe when moving values between C stacks
​https://bugs.webkit.org/show_bug.cgi?id=86672

Reviewed by Phil Pizlo.

GC pauses thread A while marking thread A, and then B while marking B,
which isn't safe against A and B moving values between each others'
stacks.

This is a theoretical bug -- I haven't been able to reproduce it
in the wild.

• heap/MachineStackMarker.cpp:

(JSC::MachineThreads::gatherFromOtherThread):
(JSC::MachineThreads::gatherConservativeRoots): Pause all C stacks for the
duration of stack marking, to avoid missing values that might be moving
between C stacks.

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• heap/MachineStackMarker.cpp (modified) (4 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2012-05-16  Geoffrey Garen  <ggaren@apple.com>
+
+        GC is not thread-safe when moving values between C stacks
+        https://bugs.webkit.org/show_bug.cgi?id=86672
+
+        Reviewed by Phil Pizlo.
+
+        GC pauses thread A while marking thread A, and then B while marking B,
+        which isn't safe against A and B moving values between each others'
+        stacks.
+
+        This is a theoretical bug -- I haven't been able to reproduce it
+        in the wild.
+
+        * heap/MachineStackMarker.cpp:
+        (JSC::MachineThreads::gatherFromOtherThread):
+        (JSC::MachineThreads::gatherConservativeRoots): Pause all C stacks for the
+        duration of stack marking, to avoid missing values that might be moving
+        between C stacks.
+
 2012-05-15  Mark Hahnenberg  <mhahnenberg@apple.com>
 

trunk/Source/JavaScriptCore/heap/MachineStackMarker.cpp

@@ -453 +453 @@
 void MachineThreads::gatherFromOtherThread(ConservativeRoots& conservativeRoots, Thread* thread)
 {
-    suspendThread(thread->platformThread);
-
     PlatformThreadRegisters regs;
     size_t regSize = getPlatformThreadRegisters(thread->platformThread, regs);
@@ -464 +462 @@
     swapIfBackwards(stackPointer, stackBase);
     conservativeRoots.add(stackPointer, stackBase);
-
-    resumeThread(thread->platformThread);
 
     freePlatformThreadRegisters(regs);
@@ -485 +481 @@
         fastMallocForbid();
 #endif
+        for (Thread* thread = m_registeredThreads; thread; thread = thread->next) {
+            if (!equalThread(thread->platformThread, currentPlatformThread))
+                suspendThread(thread->platformThread);
+        }
+
         // It is safe to access the registeredThreads list, because we earlier asserted that locks are being held,
         // and since this is a shared heap, they are real locks.
@@ -491 +492 @@
                 gatherFromOtherThread(conservativeRoots, thread);
         }
+
+        for (Thread* thread = m_registeredThreads; thread; thread = thread->next) {
+            if (!equalThread(thread->platformThread, currentPlatformThread))
+                resumeThread(thread->platformThread);
+        }
+
 #ifndef NDEBUG
         fastMallocAllow();