<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[180602] trunk/Source/JavaScriptCore</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/180602">180602</a></dd>
<dt>Author</dt> <dd>mark.lam@apple.com</dd>
<dt>Date</dt> <dd>2015-02-24 18:28:23 -0800 (Tue, 24 Feb 2015)</dd>
</dl>
<h3>Log Message</h3>
<pre>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):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreheapHeapcpp">trunk/Source/JavaScriptCore/heap/Heap.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreheapHeaph">trunk/Source/JavaScriptCore/heap/Heap.h</a></li>
<li><a href="#trunkSourceJavaScriptCoreheapMachineStackMarkercpp">trunk/Source/JavaScriptCore/heap/MachineStackMarker.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreheapMachineStackMarkerh">trunk/Source/JavaScriptCore/heap/MachineStackMarker.h</a></li>
<li><a href="#trunkSourceJavaScriptCoreruntimeJSLockcpp">trunk/Source/JavaScriptCore/runtime/JSLock.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (180601 => 180602)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2015-02-25 01:49:59 UTC (rev 180601)
+++ trunk/Source/JavaScriptCore/ChangeLog 2015-02-25 02:28:23 UTC (rev 180602)
</span><span class="lines">@@ -1,3 +1,65 @@
</span><ins>+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):
+
</ins><span class="cx"> 2015-02-24 Myles C. Maxfield <mmaxfield@apple.com>
</span><span class="cx">
</span><span class="cx"> [Mac] [iOS] Parsing support for -apple-trailing-word
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreheapHeapcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/heap/Heap.cpp (180601 => 180602)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/heap/Heap.cpp 2015-02-25 01:49:59 UTC (rev 180601)
+++ trunk/Source/JavaScriptCore/heap/Heap.cpp 2015-02-25 02:28:23 UTC (rev 180602)
</span><span class="lines">@@ -311,7 +311,6 @@
</span><span class="cx"> , m_objectSpace(this)
</span><span class="cx"> , m_storageSpace(this)
</span><span class="cx"> , m_extraMemoryUsage(0)
</span><del>- , m_machineThreads(this)
</del><span class="cx"> , m_sharedData(vm)
</span><span class="cx"> , m_slotVisitor(m_sharedData)
</span><span class="cx"> , m_copyVisitor(m_sharedData)
</span><span class="lines">@@ -349,6 +348,18 @@
</span><span class="cx"> {
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+MachineThreads& Heap::machineThreads()
+{
+ static std::once_flag initializeMachineThreadsOnceFlag;
+ static MachineThreads* machineThreads = nullptr;
+
+ std::call_once(initializeMachineThreadsOnceFlag, [] {
+ machineThreads = new MachineThreads();
+ });
+
+ return *machineThreads;
+}
+
</ins><span class="cx"> bool Heap::isPagedOut(double deadline)
</span><span class="cx"> {
</span><span class="cx"> return m_objectSpace.isPagedOut(deadline) || m_storageSpace.isPagedOut(deadline);
</span><span class="lines">@@ -587,7 +598,7 @@
</span><span class="cx"> {
</span><span class="cx"> GCPHASE(GatherStackRoots);
</span><span class="cx"> m_jitStubRoutines.clearMarks();
</span><del>- m_machineThreads.gatherConservativeRoots(roots, m_jitStubRoutines, m_codeBlocks, dummy, registers);
</del><ins>+ machineThreads().gatherConservativeRoots(roots, m_jitStubRoutines, m_codeBlocks, dummy, registers);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void Heap::gatherJSStackRoots(ConservativeRoots& roots)
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreheapHeaph"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/heap/Heap.h (180601 => 180602)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/heap/Heap.h 2015-02-25 01:49:59 UTC (rev 180601)
+++ trunk/Source/JavaScriptCore/heap/Heap.h 2015-02-25 02:28:23 UTC (rev 180602)
</span><span class="lines">@@ -119,7 +119,7 @@
</span><span class="cx">
</span><span class="cx"> VM* vm() const { return m_vm; }
</span><span class="cx"> MarkedSpace& objectSpace() { return m_objectSpace; }
</span><del>- MachineThreads& machineThreads() { return m_machineThreads; }
</del><ins>+ static MachineThreads& machineThreads();
</ins><span class="cx">
</span><span class="cx"> const SlotVisitor& slotVisitor() const { return m_slotVisitor; }
</span><span class="cx">
</span><span class="lines">@@ -355,8 +355,6 @@
</span><span class="cx"> Vector<Vector<ValueStringPair, 0, UnsafeVectorOverflow>*> m_tempSortingVectors;
</span><span class="cx"> std::unique_ptr<HashSet<MarkedArgumentBuffer*>> m_markListSet;
</span><span class="cx">
</span><del>- MachineThreads m_machineThreads;
-
</del><span class="cx"> GCThreadSharedData m_sharedData;
</span><span class="cx"> SlotVisitor m_slotVisitor;
</span><span class="cx"> CopyVisitor m_copyVisitor;
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreheapMachineStackMarkercpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/heap/MachineStackMarker.cpp (180601 => 180602)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/heap/MachineStackMarker.cpp 2015-02-25 01:49:59 UTC (rev 180601)
+++ trunk/Source/JavaScriptCore/heap/MachineStackMarker.cpp 2015-02-25 02:28:23 UTC (rev 180602)
</span><span class="lines">@@ -115,27 +115,16 @@
</span><span class="cx"> void* stackBase;
</span><span class="cx"> };
</span><span class="cx">
</span><del>-MachineThreads::MachineThreads(Heap* heap)
</del><ins>+MachineThreads::MachineThreads()
</ins><span class="cx"> : m_registeredThreads(0)
</span><span class="cx"> , m_threadSpecific(0)
</span><del>-#if !ASSERT_DISABLED
- , m_heap(heap)
-#endif
</del><span class="cx"> {
</span><del>- UNUSED_PARAM(heap);
</del><span class="cx"> threadSpecificKeyCreate(&m_threadSpecific, removeThread);
</span><span class="cx"> }
</span><span class="cx">
</span><del>-MachineThreads::~MachineThreads()
</del><ins>+NO_RETURN_DUE_TO_CRASH MachineThreads::~MachineThreads()
</ins><span class="cx"> {
</span><del>- threadSpecificKeyDelete(m_threadSpecific);
-
- MutexLocker registeredThreadsLock(m_registeredThreadsMutex);
- for (Thread* t = m_registeredThreads; t;) {
- Thread* next = t->next;
- delete t;
- t = next;
- }
</del><ins>+ RELEASE_ASSERT_NOT_REACHED();
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> static inline PlatformThread getCurrentPlatformThread()
</span><span class="lines">@@ -160,9 +149,9 @@
</span><span class="cx"> #endif
</span><span class="cx"> }
</span><span class="cx">
</span><del>-void MachineThreads::addCurrentThread()
</del><ins>+void MachineThreads::addCurrentThread(VM* vm)
</ins><span class="cx"> {
</span><del>- ASSERT(!m_heap->vm()->hasExclusiveThread() || m_heap->vm()->exclusiveThread() == std::this_thread::get_id());
</del><ins>+ ASSERT_UNUSED(vm, !vm->hasExclusiveThread() || vm->exclusiveThread() == std::this_thread::get_id());
</ins><span class="cx">
</span><span class="cx"> if (threadSpecificGet(m_threadSpecific)) {
</span><span class="cx"> ASSERT(threadSpecificGet(m_threadSpecific) == this);
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreheapMachineStackMarkerh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/heap/MachineStackMarker.h (180601 => 180602)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/heap/MachineStackMarker.h 2015-02-25 01:49:59 UTC (rev 180601)
+++ trunk/Source/JavaScriptCore/heap/MachineStackMarker.h 2015-02-25 02:28:23 UTC (rev 180602)
</span><span class="lines">@@ -33,18 +33,19 @@
</span><span class="cx"> class ConservativeRoots;
</span><span class="cx"> class Heap;
</span><span class="cx"> class JITStubRoutineSet;
</span><ins>+ class VM;
</ins><span class="cx">
</span><span class="cx"> class MachineThreads {
</span><span class="cx"> WTF_MAKE_NONCOPYABLE(MachineThreads);
</span><span class="cx"> public:
</span><span class="cx"> typedef jmp_buf RegisterState;
</span><span class="cx">
</span><del>- MachineThreads(Heap*);
</del><ins>+ MachineThreads();
</ins><span class="cx"> ~MachineThreads();
</span><span class="cx">
</span><span class="cx"> void gatherConservativeRoots(ConservativeRoots&, JITStubRoutineSet&, CodeBlockSet&, void* stackCurrent, RegisterState& registers);
</span><span class="cx">
</span><del>- JS_EXPORT_PRIVATE void addCurrentThread(); // Only needs to be called by clients that can use the same heap from multiple threads.
</del><ins>+ JS_EXPORT_PRIVATE void addCurrentThread(VM*); // Only needs to be called by clients that can use the same heap from multiple threads.
</ins><span class="cx">
</span><span class="cx"> private:
</span><span class="cx"> class Thread;
</span><span class="lines">@@ -60,9 +61,6 @@
</span><span class="cx"> Mutex m_registeredThreadsMutex;
</span><span class="cx"> Thread* m_registeredThreads;
</span><span class="cx"> WTF::ThreadSpecificKey m_threadSpecific;
</span><del>-#if !ASSERT_DISABLED
- Heap* m_heap;
-#endif
</del><span class="cx"> };
</span><span class="cx">
</span><span class="cx"> } // namespace JSC
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreruntimeJSLockcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/runtime/JSLock.cpp (180601 => 180602)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/runtime/JSLock.cpp 2015-02-25 01:49:59 UTC (rev 180601)
+++ trunk/Source/JavaScriptCore/runtime/JSLock.cpp 2015-02-25 02:28:23 UTC (rev 180602)
</span><span class="lines">@@ -144,7 +144,7 @@
</span><span class="cx"> m_entryAtomicStringTable = threadData.setCurrentAtomicStringTable(m_vm->atomicStringTable());
</span><span class="cx"> ASSERT(m_entryAtomicStringTable);
</span><span class="cx">
</span><del>- m_vm->heap.machineThreads().addCurrentThread();
</del><ins>+ m_vm->heap.machineThreads().addCurrentThread(m_vm);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void JSLock::unlock()
</span></span></pre>
</div>
</div>
</body>
</html>