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.
<rdar://problem/19739959>
Created attachment 246146 [details] the patch.
Comment on attachment 246146 [details] the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=246146&action=review > Source/JavaScriptCore/API/tests/testapi.mm:507 > + while (!mainThreadIsReadyToJoin) > + sleep(1); This feels super shady.
Comment on attachment 246146 [details] the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=246146&action=review > Source/JavaScriptCore/API/tests/testapi.mm:1427 > + sleep(10); Eeek ... this is not supposed to be here. Leftover debugging code. Will remove.
Comment on attachment 246146 [details] the patch. r=me but reduce the sleep(1) to something shorter, like a usleep(10000)
Thanks for the review. Reduced the sleep time as suggested. Landed in r179753: <http://trac.webkit.org/r179753>.
Comment on attachment 246146 [details] the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=246146&action=review > Source/JavaScriptCore/heap/Heap.cpp:350 > + // 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(); This is super weird. (1) The comment claims to be about the main thread, but this code runs on any thread. (2) A class that never called addCurrentThread calls removeCurrentThread, seemingly over-releasing a resource. > Source/JavaScriptCore/heap/MachineStackMarker.h:67 > + uint64_t m_magicNumber; // Only used for detecting use after free. Why is this better than using MallocScribble or libgmalloc?
Comment on attachment 246146 [details] the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=246146&action=review >> Source/JavaScriptCore/heap/MachineStackMarker.h:67 >> + uint64_t m_magicNumber; // Only used for detecting use after free. > > Why is this better than using MallocScribble or libgmalloc? Presumably lower cost in a debug build?
After reading more into how pthread_key_delete() and _pthread_tsd_cleanup() works, we determined that this fix is invalid. Before the fix, there was indeed a race condition with a very small window that could result in a use after free scenario. I will roll out the patch and implement the fix from scratch with a different approach.
r179753 was rolled out in r180591: <http://trac.webkit.org/r180591>.