- Revision
- 179753
- Author
- [email protected]
- Date
- 2015-02-06 12:57:45 -0800 (Fri, 06 Feb 2015)
Log Message
MachineThreads should be ref counted.
<https://webkit.org/b/141317>
Reviewed by Filip Pizlo.
The VM's MachineThreads registry object is being referenced from other
threads as a raw pointer. In a scenario where the VM is destructed on
the main thread, there is no guarantee that another thread isn't still
holding a reference to the registry and will eventually invoke
removeThread() on it on thread exit. Hence, there's a possible use
after free scenario here.
The fix is to make MachineThreads ThreadSafeRefCounted, and have all
threads that references keep a RefPtr to it to ensure that it stays
alive until the very last thread is done with it.
* API/tests/testapi.mm:
(useVMFromOtherThread): - Renamed to be more descriptive.
(useVMFromOtherThreadAndOutliveVM):
- Added a test that has another thread which uses the VM outlive the
VM to confirm that there is no crash.
However, I was not actually able to get the VM to crash without this
patch because I wasn't always able to the thread destructor to be
called. With this patch applied, I did verify with some logging that
the MachineThreads registry is only destructed after all threads
have removed themselves from it.
(threadMain): Deleted.
* heap/Heap.cpp:
(JSC::Heap::Heap):
(JSC::Heap::~Heap):
(JSC::Heap::gatherStackRoots):
* heap/Heap.h:
(JSC::Heap::machineThreads):
* heap/MachineStackMarker.cpp:
(JSC::MachineThreads::Thread::Thread):
(JSC::MachineThreads::addCurrentThread):
(JSC::MachineThreads::removeCurrentThread):
* heap/MachineStackMarker.h:
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/API/tests/testapi.mm (179752 => 179753)
--- trunk/Source/_javascript_Core/API/tests/testapi.mm 2015-02-06 20:46:19 UTC (rev 179752)
+++ trunk/Source/_javascript_Core/API/tests/testapi.mm 2015-02-06 20:57:45 UTC (rev 179753)
@@ -472,7 +472,7 @@
return containsClass;
}
-static void* threadMain(void* contextPtr)
+static void* useVMFromOtherThread(void* contextPtr)
{
JSContext *context = (__bridge JSContext*)contextPtr;
@@ -482,6 +482,32 @@
pthread_exit(nullptr);
}
+struct ThreadArgs {
+ JSContext* context;
+ volatile bool* mainThreadIsReadyToJoin;
+ volatile bool* otherThreadIsDoneWithJSWork;
+};
+
+static void* useVMFromOtherThreadAndOutliveVM(void* data)
+{
+ ThreadArgs* args = reinterpret_cast<ThreadArgs*>(data);
+ volatile bool& mainThreadIsReadyToJoin = *args->mainThreadIsReadyToJoin;
+ volatile bool& otherThreadIsDoneWithJSWork = *args->otherThreadIsDoneWithJSWork;
+
+ @autoreleasepool {
+ JSContext *context = args->context;
+
+ // Do something to enter the VM.
+ TestObject *testObject = [TestObject testObject];
+ context[@"testObject"] = testObject;
+ }
+ otherThreadIsDoneWithJSWork = true;
+
+ while (!mainThreadIsReadyToJoin)
+ usleep(10000);
+ pthread_exit(nullptr);
+}
+
void testObjectiveCAPI()
{
NSLog(@"Testing Objective-C API");
@@ -1375,13 +1401,33 @@
JSContext *context = [[JSContext alloc] init];
pthread_t threadID;
- pthread_create(&threadID, NULL, &threadMain, (__bridge void*)context);
+ pthread_create(&threadID, NULL, &useVMFromOtherThread, (__bridge void*)context);
pthread_join(threadID, nullptr);
JSSynchronousGarbageCollectForDebugging([context JSGlobalContextRef]);
checkResult(@"Did not crash after entering the VM from another thread", true);
}
-
+
+ @autoreleasepool {
+ pthread_t threadID;
+ volatile bool mainThreadIsReadyToJoin = false;
+ volatile bool otherThreadIsDoneWithJSWork = false;
+ @autoreleasepool {
+ JSContext *context = [[JSContext alloc] init];
+ ThreadArgs args = { context, &mainThreadIsReadyToJoin, &otherThreadIsDoneWithJSWork };
+
+ pthread_create(&threadID, NULL, &useVMFromOtherThreadAndOutliveVM, &args);
+ JSSynchronousGarbageCollectForDebugging([context JSGlobalContextRef]);
+
+ while (!otherThreadIsDoneWithJSWork)
+ usleep(10000);
+ }
+
+ mainThreadIsReadyToJoin = true;
+ pthread_join(threadID, nullptr);
+ checkResult(@"Did not crash if the VM is destructed before another thread using the VM ends", true);
+ }
+
currentThisInsideBlockGetterTest();
runDateTests();
runJSExportTests();
Modified: trunk/Source/_javascript_Core/ChangeLog (179752 => 179753)
--- trunk/Source/_javascript_Core/ChangeLog 2015-02-06 20:46:19 UTC (rev 179752)
+++ trunk/Source/_javascript_Core/ChangeLog 2015-02-06 20:57:45 UTC (rev 179753)
@@ -1,3 +1,47 @@
+2015-02-06 Mark Lam <[email protected]>
+
+ MachineThreads should be ref counted.
+ <https://webkit.org/b/141317>
+
+ Reviewed by Filip Pizlo.
+
+ The VM's MachineThreads registry object is being referenced from other
+ threads as a raw pointer. In a scenario where the VM is destructed on
+ the main thread, there is no guarantee that another thread isn't still
+ holding a reference to the registry and will eventually invoke
+ removeThread() on it on thread exit. Hence, there's a possible use
+ after free scenario here.
+
+ The fix is to make MachineThreads ThreadSafeRefCounted, and have all
+ threads that references keep a RefPtr to it to ensure that it stays
+ alive until the very last thread is done with it.
+
+ * API/tests/testapi.mm:
+ (useVMFromOtherThread): - Renamed to be more descriptive.
+ (useVMFromOtherThreadAndOutliveVM):
+ - Added a test that has another thread which uses the VM outlive the
+ VM to confirm that there is no crash.
+
+ However, I was not actually able to get the VM to crash without this
+ patch because I wasn't always able to the thread destructor to be
+ called. With this patch applied, I did verify with some logging that
+ the MachineThreads registry is only destructed after all threads
+ have removed themselves from it.
+
+ (threadMain): Deleted.
+
+ * heap/Heap.cpp:
+ (JSC::Heap::Heap):
+ (JSC::Heap::~Heap):
+ (JSC::Heap::gatherStackRoots):
+ * heap/Heap.h:
+ (JSC::Heap::machineThreads):
+ * heap/MachineStackMarker.cpp:
+ (JSC::MachineThreads::Thread::Thread):
+ (JSC::MachineThreads::addCurrentThread):
+ (JSC::MachineThreads::removeCurrentThread):
+ * heap/MachineStackMarker.h:
+
2015-02-06 Commit Queue <[email protected]>
Unreviewed, rolling out r179743.
Modified: trunk/Source/_javascript_Core/heap/Heap.cpp (179752 => 179753)
--- trunk/Source/_javascript_Core/heap/Heap.cpp 2015-02-06 20:46:19 UTC (rev 179752)
+++ trunk/Source/_javascript_Core/heap/Heap.cpp 2015-02-06 20:57:45 UTC (rev 179753)
@@ -311,7 +311,6 @@
, m_objectSpace(this)
, m_storageSpace(this)
, m_extraMemoryUsage(0)
- , m_machineThreads(this)
, m_sharedData(vm)
, m_slotVisitor(m_sharedData)
, m_copyVisitor(m_sharedData)
@@ -340,6 +339,7 @@
, m_delayedReleaseRecursionCount(0)
#endif
{
+ m_machineThreads = adoptRef(new MachineThreads(this));
m_storageSpace.init();
if (Options::verifyHeap())
m_verifier = std::make_unique<HeapVerifier>(this, Options::numberOfGCCyclesToRecordForVerification());
@@ -347,6 +347,9 @@
Heap::~Heap()
{
+ // We need to remove the main thread explicitly here because the main thread
+ // may not terminate for a while though the Heap (and VM) is being shut down.
+ m_machineThreads->removeCurrentThread();
}
bool Heap::isPagedOut(double deadline)
@@ -587,7 +590,7 @@
{
GCPHASE(GatherStackRoots);
m_jitStubRoutines.clearMarks();
- m_machineThreads.gatherConservativeRoots(roots, m_jitStubRoutines, m_codeBlocks, dummy, registers);
+ m_machineThreads->gatherConservativeRoots(roots, m_jitStubRoutines, m_codeBlocks, dummy, registers);
}
void Heap::gatherJSStackRoots(ConservativeRoots& roots)
Modified: trunk/Source/_javascript_Core/heap/Heap.h (179752 => 179753)
--- trunk/Source/_javascript_Core/heap/Heap.h 2015-02-06 20:46:19 UTC (rev 179752)
+++ trunk/Source/_javascript_Core/heap/Heap.h 2015-02-06 20:57:45 UTC (rev 179753)
@@ -119,7 +119,7 @@
VM* vm() const { return m_vm; }
MarkedSpace& objectSpace() { return m_objectSpace; }
- MachineThreads& machineThreads() { return m_machineThreads; }
+ MachineThreads& machineThreads() { return *m_machineThreads; }
const SlotVisitor& slotVisitor() const { return m_slotVisitor; }
@@ -355,7 +355,7 @@
Vector<Vector<ValueStringPair, 0, UnsafeVectorOverflow>*> m_tempSortingVectors;
std::unique_ptr<HashSet<MarkedArgumentBuffer*>> m_markListSet;
- MachineThreads m_machineThreads;
+ RefPtr<MachineThreads> m_machineThreads;
GCThreadSharedData m_sharedData;
SlotVisitor m_slotVisitor;
Modified: trunk/Source/_javascript_Core/heap/MachineStackMarker.cpp (179752 => 179753)
--- trunk/Source/_javascript_Core/heap/MachineStackMarker.cpp 2015-02-06 20:46:19 UTC (rev 179752)
+++ trunk/Source/_javascript_Core/heap/MachineStackMarker.cpp 2015-02-06 20:57:45 UTC (rev 179753)
@@ -91,9 +91,10 @@
class MachineThreads::Thread {
WTF_MAKE_FAST_ALLOCATED;
public:
- Thread(const PlatformThread& platThread, void* base)
+ Thread(PassRefPtr<MachineThreads> machineThreads, const PlatformThread& platThread, void* base)
: platformThread(platThread)
, stackBase(base)
+ , m_machineThreads(machineThreads)
{
#if USE(PTHREADS) && !OS(WINDOWS) && !OS(DARWIN) && defined(SA_RESTART)
// if we have SA_RESTART, enable SIGUSR2 debugging mechanism
@@ -113,6 +114,7 @@
Thread* next;
PlatformThread platformThread;
void* stackBase;
+ RefPtr<MachineThreads> m_machineThreads;
};
MachineThreads::MachineThreads(Heap* heap)
@@ -120,6 +122,7 @@
, m_threadSpecific(0)
#if !ASSERT_DISABLED
, m_heap(heap)
+ , m_magicNumber(0x1234567890abcdef)
#endif
{
UNUSED_PARAM(heap);
@@ -136,6 +139,9 @@
delete t;
t = next;
}
+#if !ASSERT_DISABLED
+ m_magicNumber = 0xbaddbeefdeadbeef;
+#endif
}
static inline PlatformThread getCurrentPlatformThread()
@@ -170,7 +176,7 @@
}
threadSpecificSet(m_threadSpecific, this);
- Thread* thread = new Thread(getCurrentPlatformThread(), wtfThreadData().stack().origin());
+ Thread* thread = new Thread(this, getCurrentPlatformThread(), wtfThreadData().stack().origin());
MutexLocker lock(m_registeredThreadsMutex);
@@ -180,6 +186,7 @@
void MachineThreads::removeThread(void* p)
{
+ ASSERT(static_cast<MachineThreads*>(p)->m_magicNumber == 0x1234567890abcdef);
static_cast<MachineThreads*>(p)->removeCurrentThread();
}
@@ -187,6 +194,21 @@
{
PlatformThread currentPlatformThread = getCurrentPlatformThread();
+ // This thread could be the last entity that holds a RefPtr to the
+ // MachineThreads registry. We don't want the registry to be deleted while
+ // we're deleting the the Thread entry, because:
+ //
+ // 1. ~MachineThread() will attempt to lock its m_registeredThreadsMutex
+ // and we already hold it here. m_registeredThreadsMutex is not
+ // re-entrant.
+ // 2. The MutexLocker will unlock m_registeredThreadsMutex at the end of
+ // this function. We can't let it be destructed until before then.
+ //
+ // Using this RefPtr here will defer destructing the registry till after
+ // MutexLocker releases its m_registeredThreadsMutex.
+ RefPtr<MachineThreads> retainMachineThreadsRegistryUntilDoneRemovingThread = this;
+
+ ASSERT(m_magicNumber == 0x1234567890abcdef);
MutexLocker lock(m_registeredThreadsMutex);
if (equalThread(currentPlatformThread, m_registeredThreads->platformThread)) {
Thread* t = m_registeredThreads;
Modified: trunk/Source/_javascript_Core/heap/MachineStackMarker.h (179752 => 179753)
--- trunk/Source/_javascript_Core/heap/MachineStackMarker.h 2015-02-06 20:46:19 UTC (rev 179752)
+++ trunk/Source/_javascript_Core/heap/MachineStackMarker.h 2015-02-06 20:57:45 UTC (rev 179753)
@@ -24,6 +24,7 @@
#include <setjmp.h>
#include <wtf/Noncopyable.h>
+#include <wtf/ThreadSafeRefCounted.h>
#include <wtf/ThreadSpecific.h>
#include <wtf/ThreadingPrimitives.h>
@@ -34,7 +35,7 @@
class Heap;
class JITStubRoutineSet;
- class MachineThreads {
+ class MachineThreads : public ThreadSafeRefCounted<MachineThreads> {
WTF_MAKE_NONCOPYABLE(MachineThreads);
public:
typedef jmp_buf RegisterState;
@@ -46,6 +47,8 @@
JS_EXPORT_PRIVATE void addCurrentThread(); // Only needs to be called by clients that can use the same heap from multiple threads.
+ void removeCurrentThread();
+
private:
class Thread;
@@ -55,13 +58,13 @@
bool tryCopyOtherThreadStacks(MutexLocker&, void*, size_t capacity, size_t*);
static void removeThread(void*);
- void removeCurrentThread();
Mutex m_registeredThreadsMutex;
Thread* m_registeredThreads;
WTF::ThreadSpecificKey m_threadSpecific;
#if !ASSERT_DISABLED
Heap* m_heap;
+ uint64_t m_magicNumber; // Only used for detecting use after free.
#endif
};
