<!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>[180716] 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/180716">180716</a></dd>
<dt>Author</dt> <dd>mark.lam@apple.com</dd>
<dt>Date</dt> <dd>2015-02-26 17:25:21 -0800 (Thu, 26 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 Filip Pizlo.
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;
// === Start of window for the bug to manifest =================
// 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) {
// === End of window for the bug to manifest ==================
destructor(value);
}
}
}
}
The fix is to add each active MachineThreads to an ActiveMachineThreadsManager,
and always check if the manager still contains that MachineThreads object
before we call removeCurrentThread() on it. When MachineThreads is destructed,
it will remove itself from the manager. The add, remove, and checking
operations are all synchronized on the manager's lock, thereby ensuring that
the MachineThreads object, if found in the manager, will remain alive for the
duration of time we call removeCurrentThread() on it.
There's also possible for the MachineThreads object to already be destructed
and another one happened to have been instantiated at the same address.
Hence, we should only remove the exiting thread if it is found in the
MachineThreads object.
There is no test for this issue because this bug requires a race condition
between 2 threads where:
1. Thread B, which had previously used the VM, exiting and
getting to the bug window shown in _pthread_tsd_cleanup_key() above.
2. Thread A destructing the VM (and its MachineThreads object)
within that window of time before Thread B calls the destructor.
It is not possible to get a reliable test case without invasively
instrumenting _pthread_tsd_cleanup_key() or MachineThreads::removeCurrentThread()
to significantly increase that window of opportunity.
* heap/MachineStackMarker.cpp:
(JSC::ActiveMachineThreadsManager::Locker::Locker):
(JSC::ActiveMachineThreadsManager::add):
(JSC::ActiveMachineThreadsManager::remove):
(JSC::ActiveMachineThreadsManager::contains):
(JSC::ActiveMachineThreadsManager::ActiveMachineThreadsManager):
(JSC::activeMachineThreadsManager):
(JSC::MachineThreads::MachineThreads):
(JSC::MachineThreads::~MachineThreads):
(JSC::MachineThreads::removeThread):
(JSC::MachineThreads::removeThreadIfFound):
(JSC::MachineThreads::removeCurrentThread): Deleted.
* heap/MachineStackMarker.h:</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</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>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (180715 => 180716)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2015-02-27 01:15:23 UTC (rev 180715)
+++ trunk/Source/JavaScriptCore/ChangeLog 2015-02-27 01:25:21 UTC (rev 180716)
</span><span class="lines">@@ -1,3 +1,80 @@
</span><ins>+2015-02-26 Mark Lam <mark.lam@apple.com>
+
+ MachineThreads::Thread clean up has a use after free race condition.
+ <https://webkit.org/b/141990>
+
+ Reviewed by Filip Pizlo.
+
+ 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;
+
+ // === Start of window for the bug to manifest =================
+
+ // 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) {
+
+ // === End of window for the bug to manifest ==================
+
+ destructor(value);
+ }
+ }
+ }
+ }
+
+ The fix is to add each active MachineThreads to an ActiveMachineThreadsManager,
+ and always check if the manager still contains that MachineThreads object
+ before we call removeCurrentThread() on it. When MachineThreads is destructed,
+ it will remove itself from the manager. The add, remove, and checking
+ operations are all synchronized on the manager's lock, thereby ensuring that
+ the MachineThreads object, if found in the manager, will remain alive for the
+ duration of time we call removeCurrentThread() on it.
+
+ There's also possible for the MachineThreads object to already be destructed
+ and another one happened to have been instantiated at the same address.
+ Hence, we should only remove the exiting thread if it is found in the
+ MachineThreads object.
+
+ There is no test for this issue because this bug requires a race condition
+ between 2 threads where:
+ 1. Thread B, which had previously used the VM, exiting and
+ getting to the bug window shown in _pthread_tsd_cleanup_key() above.
+ 2. Thread A destructing the VM (and its MachineThreads object)
+ within that window of time before Thread B calls the destructor.
+
+ It is not possible to get a reliable test case without invasively
+ instrumenting _pthread_tsd_cleanup_key() or MachineThreads::removeCurrentThread()
+ to significantly increase that window of opportunity.
+
+ * heap/MachineStackMarker.cpp:
+ (JSC::ActiveMachineThreadsManager::Locker::Locker):
+ (JSC::ActiveMachineThreadsManager::add):
+ (JSC::ActiveMachineThreadsManager::remove):
+ (JSC::ActiveMachineThreadsManager::contains):
+ (JSC::ActiveMachineThreadsManager::ActiveMachineThreadsManager):
+ (JSC::activeMachineThreadsManager):
+ (JSC::MachineThreads::MachineThreads):
+ (JSC::MachineThreads::~MachineThreads):
+ (JSC::MachineThreads::removeThread):
+ (JSC::MachineThreads::removeThreadIfFound):
+ (JSC::MachineThreads::removeCurrentThread): Deleted.
+ * heap/MachineStackMarker.h:
+
</ins><span class="cx"> 2015-02-26 Joseph Pecoraro <pecoraro@apple.com>
</span><span class="cx">
</span><span class="cx"> Web Inspector: Save Console Evaluations into Command Line variables $1-$99 ($n)
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreheapMachineStackMarkercpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/heap/MachineStackMarker.cpp (180715 => 180716)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/heap/MachineStackMarker.cpp 2015-02-27 01:15:23 UTC (rev 180715)
+++ trunk/Source/JavaScriptCore/heap/MachineStackMarker.cpp 2015-02-27 01:25:21 UTC (rev 180716)
</span><span class="lines">@@ -1,5 +1,5 @@
</span><span class="cx"> /*
</span><del>- * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
</del><ins>+ * Copyright (C) 2003-2009, 2015 Apple Inc. All rights reserved.
</ins><span class="cx"> * Copyright (C) 2007 Eric Seidel <eric@webkit.org>
</span><span class="cx"> * Copyright (C) 2009 Acision BV. All rights reserved.
</span><span class="cx"> *
</span><span class="lines">@@ -88,6 +88,65 @@
</span><span class="cx"> #endif
</span><span class="cx"> #endif
</span><span class="cx">
</span><ins>+class ActiveMachineThreadsManager;
+static ActiveMachineThreadsManager& activeMachineThreadsManager();
+
+class ActiveMachineThreadsManager {
+ WTF_MAKE_NONCOPYABLE(ActiveMachineThreadsManager);
+public:
+
+ class Locker {
+ public:
+ Locker(ActiveMachineThreadsManager& manager)
+ : m_locker(manager.m_lock)
+ {
+ }
+
+ private:
+ MutexLocker m_locker;
+ };
+
+ void add(MachineThreads* machineThreads)
+ {
+ MutexLocker managerLock(m_lock);
+ m_set.add(machineThreads);
+ }
+
+ void remove(MachineThreads* machineThreads)
+ {
+ MutexLocker managerLock(m_lock);
+ auto recordedMachineThreads = m_set.take(machineThreads);
+ RELEASE_ASSERT(recordedMachineThreads = machineThreads);
+ }
+
+ bool contains(MachineThreads* machineThreads)
+ {
+ return m_set.contains(machineThreads);
+ }
+
+private:
+ typedef HashSet<MachineThreads*> MachineThreadsSet;
+
+ ActiveMachineThreadsManager() { }
+
+ Mutex m_lock;
+ MachineThreadsSet m_set;
+
+ friend ActiveMachineThreadsManager& activeMachineThreadsManager();
+};
+
+static ActiveMachineThreadsManager& activeMachineThreadsManager()
+{
+ static std::once_flag initializeManagerOnceFlag;
+ static ActiveMachineThreadsManager* manager = nullptr;
+
+ std::call_once(initializeManagerOnceFlag, [] {
+ manager = new ActiveMachineThreadsManager();
+ });
+ return *manager;
+}
+
+
</ins><span class="cx"> class MachineThreads::Thread {
</span><span class="cx"> WTF_MAKE_FAST_ALLOCATED;
</span><span class="cx"> public:
</span><span class="lines">@@ -124,10 +183,12 @@
</span><span class="cx"> {
</span><span class="cx"> UNUSED_PARAM(heap);
</span><span class="cx"> threadSpecificKeyCreate(&m_threadSpecific, removeThread);
</span><ins>+ activeMachineThreadsManager().add(this);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> MachineThreads::~MachineThreads()
</span><span class="cx"> {
</span><ins>+ activeMachineThreadsManager().remove(this);
</ins><span class="cx"> threadSpecificKeyDelete(m_threadSpecific);
</span><span class="cx">
</span><span class="cx"> MutexLocker registeredThreadsLock(m_registeredThreadsMutex);
</span><span class="lines">@@ -180,15 +241,24 @@
</span><span class="cx">
</span><span class="cx"> void MachineThreads::removeThread(void* p)
</span><span class="cx"> {
</span><del>- static_cast<MachineThreads*>(p)->removeCurrentThread();
</del><ins>+ auto& manager = activeMachineThreadsManager();
+ ActiveMachineThreadsManager::Locker lock(manager);
+ auto machineThreads = static_cast<MachineThreads*>(p);
+ if (manager.contains(machineThreads)) {
+ // There's a chance that the MachineThreads registry that this thread
+ // was registered with was already destructed, and another one happened
+ // to be instantiated at the same address. Hence, this thread may or
+ // may not be found in this MachineThreads registry. We only need to
+ // do a removal if this thread is found in it.
+ machineThreads->removeThreadIfFound(getCurrentPlatformThread());
+ }
</ins><span class="cx"> }
</span><span class="cx">
</span><del>-void MachineThreads::removeCurrentThread()
</del><ins>+template<typename PlatformThread>
+void MachineThreads::removeThreadIfFound(PlatformThread platformThread)
</ins><span class="cx"> {
</span><del>- PlatformThread currentPlatformThread = getCurrentPlatformThread();
-
</del><span class="cx"> MutexLocker lock(m_registeredThreadsMutex);
</span><del>- if (equalThread(currentPlatformThread, m_registeredThreads->platformThread)) {
</del><ins>+ if (equalThread(platformThread, m_registeredThreads->platformThread)) {
</ins><span class="cx"> Thread* t = m_registeredThreads;
</span><span class="cx"> m_registeredThreads = m_registeredThreads->next;
</span><span class="cx"> delete t;
</span><span class="lines">@@ -196,13 +266,12 @@
</span><span class="cx"> Thread* last = m_registeredThreads;
</span><span class="cx"> Thread* t;
</span><span class="cx"> for (t = m_registeredThreads->next; t; t = t->next) {
</span><del>- if (equalThread(t->platformThread, currentPlatformThread)) {
</del><ins>+ if (equalThread(t->platformThread, platformThread)) {
</ins><span class="cx"> last->next = t->next;
</span><span class="cx"> break;
</span><span class="cx"> }
</span><span class="cx"> last = t;
</span><span class="cx"> }
</span><del>- ASSERT(t); // If t is NULL, we never found ourselves in the list.
</del><span class="cx"> delete t;
</span><span class="cx"> }
</span><span class="cx"> }
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreheapMachineStackMarkerh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/heap/MachineStackMarker.h (180715 => 180716)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/heap/MachineStackMarker.h 2015-02-27 01:15:23 UTC (rev 180715)
+++ trunk/Source/JavaScriptCore/heap/MachineStackMarker.h 2015-02-27 01:25:21 UTC (rev 180716)
</span><span class="lines">@@ -55,8 +55,10 @@
</span><span class="cx"> bool tryCopyOtherThreadStacks(MutexLocker&, void*, size_t capacity, size_t*);
</span><span class="cx">
</span><span class="cx"> static void removeThread(void*);
</span><del>- void removeCurrentThread();
</del><span class="cx">
</span><ins>+ template<typename PlatformThread>
+ void removeThreadIfFound(PlatformThread);
+
</ins><span class="cx"> Mutex m_registeredThreadsMutex;
</span><span class="cx"> Thread* m_registeredThreads;
</span><span class="cx"> WTF::ThreadSpecificKey m_threadSpecific;
</span></span></pre>
</div>
</div>
</body>
</html>