Changeset 180602 in webkit

Timestamp:

Feb 24, 2015 6:28:23 PM (9 years ago)

Author:

mark.lam@apple.com

Message:

MachineThreads::Thread clean up has a use after free race condition.
<​https://webkit.org/b/141990>

Reviewed by Michael Saboff.

MachineThreads::Thread clean up relies on the clean up mechanism
implemented in _pthread_tsd_cleanup_key(), which looks like this:

void _pthread_tsd_cleanup_key(pthread_t self, pthread_key_t key)
{

void (*destructor)(void *);
if (_pthread_key_get_destructor(key, &destructor)) {

void ptr = &self->tsd[key];
void *value = *ptr;

At this point, this thread has cached "destructor" and "value"
(which is a MachineThreads*). If the VM gets destructed (along
with its MachineThreads registry) by another thread, then this
thread will have no way of knowing that the MachineThreads* is
now pointing to freed memory. Calling the destructor below will
therefore result in a use after free scenario when it tries to
access the MachineThreads' data members.

if (value) {

*ptr = NULL;
if (destructor) {

destructor(value);

}

}

}

}

The solution is simply to change MachineThreads from a per VM thread
registry to a process global singleton thread registry i.e. the
MachineThreads registry is now immortal and we cannot have a use after
free scenario since we never free it.

The cost of this change is that all VM instances will have to scan
stacks of all threads ever touched by a VM, and not just those that
touched a specific VM. However, stacks tend to be shallow. Hence,
those additional scans will tend to be cheap.

Secondly, it is not common for there to be multiple JSC VMs in use
concurrently on multiple threads. Hence, this cost should rarely
manifest in real world applications.

• heap/Heap.cpp:

(JSC::Heap::Heap):
(JSC::Heap::machineThreads):
(JSC::Heap::gatherStackRoots):

• heap/Heap.h:

(JSC::Heap::machineThreads): Deleted.

• heap/MachineStackMarker.cpp:

(JSC::MachineThreads::MachineThreads):
(JSC::MachineThreads::~MachineThreads):
(JSC::MachineThreads::addCurrentThread):

• heap/MachineStackMarker.h:
• runtime/JSLock.cpp:

(JSC::JSLock::didAcquireLock):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• heap/Heap.cpp (modified) (3 diffs)
• heap/Heap.h (modified) (2 diffs)
• heap/MachineStackMarker.cpp (modified) (2 diffs)
• heap/MachineStackMarker.h (modified) (3 diffs)
• runtime/JSLock.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2015-02-24  Mark Lam  <mark.lam@apple.com>
+
+        MachineThreads::Thread clean up has a use after free race condition.
+        <https://webkit.org/b/141990>
+
+        Reviewed by Michael Saboff.
+
+        MachineThreads::Thread clean up relies on the clean up mechanism
+        implemented in _pthread_tsd_cleanup_key(), which looks like this:
+
+        void _pthread_tsd_cleanup_key(pthread_t self, pthread_key_t key)
+        {
+            void (*destructor)(void *);
+            if (_pthread_key_get_destructor(key, &destructor)) {
+                void **ptr = &self->tsd[key];
+                void *value = *ptr;
+
+                // At this point, this thread has cached "destructor" and "value"
+                // (which is a MachineThreads*).  If the VM gets destructed (along
+                // with its MachineThreads registry) by another thread, then this
+                // thread will have no way of knowing that the MachineThreads* is
+                // now pointing to freed memory.  Calling the destructor below will
+                // therefore result in a use after free scenario when it tries to
+                // access the MachineThreads' data members.
+
+                if (value) {
+                    *ptr = NULL;
+                    if (destructor) {
+                        destructor(value);
+                    }
+                }
+            }
+        }
+
+        The solution is simply to change MachineThreads from a per VM thread
+        registry to a process global singleton thread registry i.e. the
+        MachineThreads registry is now immortal and we cannot have a use after
+        free scenario since we never free it.
+
+        The cost of this change is that all VM instances will have to scan
+        stacks of all threads ever touched by a VM, and not just those that
+        touched a specific VM.  However, stacks tend to be shallow.  Hence,
+        those additional scans will tend to be cheap.
+
+        Secondly, it is not common for there to be multiple JSC VMs in use
+        concurrently on multiple threads.  Hence, this cost should rarely
+        manifest in real world applications.
+
+        * heap/Heap.cpp:
+        (JSC::Heap::Heap):
+        (JSC::Heap::machineThreads):
+        (JSC::Heap::gatherStackRoots):
+        * heap/Heap.h:
+        (JSC::Heap::machineThreads): Deleted.
+        * heap/MachineStackMarker.cpp:
+        (JSC::MachineThreads::MachineThreads):
+        (JSC::MachineThreads::~MachineThreads):
+        (JSC::MachineThreads::addCurrentThread):
+        * heap/MachineStackMarker.h:
+        * runtime/JSLock.cpp:
+        (JSC::JSLock::didAcquireLock):
+
 2015-02-24  Myles C. Maxfield  <mmaxfield@apple.com>
 

trunk/Source/JavaScriptCore/heap/Heap.cpp

@@ -312 +312 @@
     , m_storageSpace(this)
     , m_extraMemoryUsage(0)
-    , m_machineThreads(this)
     , m_sharedData(vm)
     , m_slotVisitor(m_sharedData)
@@ -350 +349 @@
 }
 
+MachineThreads& Heap::machineThreads()
+{
+    static std::once_flag initializeMachineThreadsOnceFlag;
+    static MachineThreads* machineThreads = nullptr;
+
+    std::call_once(initializeMachineThreadsOnceFlag, [] {
+        machineThreads = new MachineThreads();
+    });
+
+    return *machineThreads;
+}
+    
 bool Heap::isPagedOut(double deadline)
 {
@@ -588 +599 @@
     GCPHASE(GatherStackRoots);
     m_jitStubRoutines.clearMarks();
-    m_machineThreads.gatherConservativeRoots(roots, m_jitStubRoutines, m_codeBlocks, dummy, registers);
+    machineThreads().gatherConservativeRoots(roots, m_jitStubRoutines, m_codeBlocks, dummy, registers);
 }
 

trunk/Source/JavaScriptCore/heap/Heap.h

@@ -120 +120 @@
     VM* vm() const { return m_vm; }
     MarkedSpace& objectSpace() { return m_objectSpace; }
-    MachineThreads& machineThreads() { return m_machineThreads; }
+    static MachineThreads& machineThreads();
 
     const SlotVisitor& slotVisitor() const { return m_slotVisitor; }
@@ -356 +356 @@
     std::unique_ptr<HashSet<MarkedArgumentBuffer*>> m_markListSet;
 
-    MachineThreads m_machineThreads;
-    
     GCThreadSharedData m_sharedData;
     SlotVisitor m_slotVisitor;

trunk/Source/JavaScriptCore/heap/MachineStackMarker.cpp

@@ -116 +116 @@
 };
 
-MachineThreads::MachineThreads(Heap* heap)
+MachineThreads::MachineThreads()
     : m_registeredThreads(0)
     , m_threadSpecific(0)
-#if !ASSERT_DISABLED
-    , m_heap(heap)
-#endif
-{
-    UNUSED_PARAM(heap);
+{
     threadSpecificKeyCreate(&m_threadSpecific, removeThread);
 }
 
-MachineThreads::~MachineThreads()
-{
-    threadSpecificKeyDelete(m_threadSpecific);
-
-    MutexLocker registeredThreadsLock(m_registeredThreadsMutex);
-    for (Thread* t = m_registeredThreads; t;) {
-        Thread* next = t->next;
-        delete t;
-        t = next;
-    }
+NO_RETURN_DUE_TO_CRASH MachineThreads::~MachineThreads()
+{
+    RELEASE_ASSERT_NOT_REACHED();
 }
 
@@ -161 +150 @@
 }
 
-void MachineThreads::addCurrentThread()
-{
-    ASSERT(!m_heap->vm()->hasExclusiveThread() || m_heap->vm()->exclusiveThread() == std::this_thread::get_id());
+void MachineThreads::addCurrentThread(VM* vm)
+{
+    ASSERT_UNUSED(vm, !vm->hasExclusiveThread() || vm->exclusiveThread() == std::this_thread::get_id());
 
     if (threadSpecificGet(m_threadSpecific)) {

trunk/Source/JavaScriptCore/heap/MachineStackMarker.h

@@ -34 +34 @@
     class Heap;
     class JITStubRoutineSet;
+    class VM;
 
     class MachineThreads {
@@ -40 +41 @@
         typedef jmp_buf RegisterState;
 
-        MachineThreads(Heap*);
+        MachineThreads();
         ~MachineThreads();
 
         void gatherConservativeRoots(ConservativeRoots&, JITStubRoutineSet&, CodeBlockSet&, void* stackCurrent, RegisterState& registers);
 
-        JS_EXPORT_PRIVATE void addCurrentThread(); // Only needs to be called by clients that can use the same heap from multiple threads.
+        JS_EXPORT_PRIVATE void addCurrentThread(VM*); // Only needs to be called by clients that can use the same heap from multiple threads.
 
     private:
@@ -61 +62 @@
         Thread* m_registeredThreads;
         WTF::ThreadSpecificKey m_threadSpecific;
-#if !ASSERT_DISABLED
-        Heap* m_heap;
-#endif
     };
 

trunk/Source/JavaScriptCore/runtime/JSLock.cpp

@@ -145 +145 @@
     ASSERT(m_entryAtomicStringTable);
 
-    m_vm->heap.machineThreads().addCurrentThread();
+    m_vm->heap.machineThreads().addCurrentThread(m_vm);
 }