<rdar://problem/6467376> Race condition in WTF::currentThread can lead to a thread...
authormrowe@apple.com <mrowe@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 28 Dec 2008 02:54:12 +0000 (02:54 +0000)
committermrowe@apple.com <mrowe@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 28 Dec 2008 02:54:12 +0000 (02:54 +0000)
If a newly-created thread calls WTF::currentThread() before WTF::createThread calls establishIdentifierForPthreadHandle
then more than one identifier will be used for the same thread.  We can avoid this by adding some extra synchronization
during thread creation that delays the execution of the thread function until the thread identifier has been set up, and
an assertion to catch this problem should it reappear in the future.

Reviewed by Alexey Proskuryakov.

* wtf/Threading.cpp: Added.
(WTF::NewThreadContext::NewThreadContext):
(WTF::threadEntryPoint):
(WTF::createThread): Add cross-platform createThread function that delays the execution of the thread function until
after the thread identifier has been set up.
* wtf/Threading.h:
* wtf/ThreadingGtk.cpp:
(WTF::establishIdentifierForThread):
(WTF::createThreadInternal):
* wtf/ThreadingNone.cpp:
(WTF::createThreadInternal):
* wtf/ThreadingPthreads.cpp:
(WTF::establishIdentifierForPthreadHandle):
(WTF::createThreadInternal):
* wtf/ThreadingQt.cpp:
(WTF::identifierByQthreadHandle):
(WTF::establishIdentifierForThread):
(WTF::createThreadInternal):
* wtf/ThreadingWin.cpp:
(WTF::storeThreadHandleByIdentifier):
(WTF::createThreadInternal):

Add Threading.cpp to the build.

* GNUmakefile.am:
* JavaScriptCore.pri:
* JavaScriptCore.scons:
* JavaScriptCore.vcproj/WTF/WTF.vcproj:
* JavaScriptCore.xcodeproj/project.pbxproj:
* JavaScriptCoreSources.bkl:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@39487 268f45cc-cd09-0410-ab3c-d52691b4dbfc

14 files changed:
JavaScriptCore/ChangeLog
JavaScriptCore/GNUmakefile.am
JavaScriptCore/JavaScriptCore.pri
JavaScriptCore/JavaScriptCore.scons
JavaScriptCore/JavaScriptCore.vcproj/WTF/WTF.vcproj
JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj
JavaScriptCore/JavaScriptCoreSources.bkl
JavaScriptCore/wtf/Threading.cpp [new file with mode: 0644]
JavaScriptCore/wtf/Threading.h
JavaScriptCore/wtf/ThreadingGtk.cpp
JavaScriptCore/wtf/ThreadingNone.cpp
JavaScriptCore/wtf/ThreadingPthreads.cpp
JavaScriptCore/wtf/ThreadingQt.cpp
JavaScriptCore/wtf/ThreadingWin.cpp

index d4138ac..f3a0996 100644 (file)
@@ -1,3 +1,45 @@
+2008-12-26  Mark Rowe  <mrowe@apple.com>
+
+        Reviewed by Alexey Proskuryakov.
+
+        <rdar://problem/6467376> Race condition in WTF::currentThread can lead to a thread using two different identifiers during its lifetime
+
+        If a newly-created thread calls WTF::currentThread() before WTF::createThread calls establishIdentifierForPthreadHandle
+        then more than one identifier will be used for the same thread.  We can avoid this by adding some extra synchronization
+        during thread creation that delays the execution of the thread function until the thread identifier has been set up, and
+        an assertion to catch this problem should it reappear in the future.
+
+        * wtf/Threading.cpp: Added.
+        (WTF::NewThreadContext::NewThreadContext):
+        (WTF::threadEntryPoint):
+        (WTF::createThread): Add cross-platform createThread function that delays the execution of the thread function until
+        after the thread identifier has been set up.
+        * wtf/Threading.h:
+        * wtf/ThreadingGtk.cpp:
+        (WTF::establishIdentifierForThread):
+        (WTF::createThreadInternal):
+        * wtf/ThreadingNone.cpp:
+        (WTF::createThreadInternal):
+        * wtf/ThreadingPthreads.cpp:
+        (WTF::establishIdentifierForPthreadHandle):
+        (WTF::createThreadInternal):
+        * wtf/ThreadingQt.cpp:
+        (WTF::identifierByQthreadHandle):
+        (WTF::establishIdentifierForThread):
+        (WTF::createThreadInternal):
+        * wtf/ThreadingWin.cpp:
+        (WTF::storeThreadHandleByIdentifier):
+        (WTF::createThreadInternal):
+
+        Add Threading.cpp to the build.
+
+        * GNUmakefile.am:
+        * JavaScriptCore.pri:
+        * JavaScriptCore.scons:
+        * JavaScriptCore.vcproj/WTF/WTF.vcproj:
+        * JavaScriptCore.xcodeproj/project.pbxproj:
+        * JavaScriptCoreSources.bkl:
+
 2008-12-26  Sam Weinig  <sam@webkit.org>
 
         Reviewed by Alexey Proskuryakov.
index 4702a16..c83b0d3 100644 (file)
@@ -235,6 +235,7 @@ javascriptcore_sources += \
        JavaScriptCore/wtf/TCSpinLock.h \
        JavaScriptCore/wtf/ThreadSpecific.h \
        JavaScriptCore/wtf/Threading.h \
+       JavaScriptCore/wtf/Threading.cpp \
        JavaScriptCore/wtf/ThreadingGtk.cpp \
        JavaScriptCore/wtf/ThreadingPthreads.cpp \
        JavaScriptCore/wtf/UnusedParam.h \
index 98dff80..20f5b52 100644 (file)
@@ -174,6 +174,7 @@ SOURCES += \
     profiler/Profiler.cpp \
     profiler/TreeProfile.cpp \
     wtf/FastMalloc.cpp \
+    wtf/Threading.cpp \
     wtf/ThreadingQt.cpp \
     wtf/qt/MainThreadQt.cpp
 
index 1053fe2..37b9d89 100644 (file)
@@ -144,6 +144,7 @@ sources['wtf'] = [
     'wtf/HashTable.cpp',
     'wtf/RandomNumber.cpp',
     'wtf/RefCountedLeakCounter.cpp',
+    'wtf/Threading.cpp',
     'wtf/dtoa.cpp',
 ]
 sources['wtf/unicode'] = [
index fa269ce..f69608f 100644 (file)
                        >\r
                </File>\r
                <File\r
+                       RelativePath="..\..\wtf\Threading.cpp"\r
+                       >\r
+               </File>\r
+               <File\r
                        RelativePath="..\..\wtf\ThreadingWin.cpp"\r
                        >\r
                </File>\r
index 47e2297..0cfdafc 100644 (file)
@@ -92,6 +92,7 @@
                5D53726F0E1C54880021E549 /* Tracing.h in Headers */ = {isa = PBXBuildFile; fileRef = 5D53726E0E1C54880021E549 /* Tracing.h */; };
                5D5D8AB60E0D0A7200F9C692 /* jsc in Copy Into Framework */ = {isa = PBXBuildFile; fileRef = 932F5BE10822A1C700736975 /* jsc */; };
                5D5D8AD10E0D0EBE00F9C692 /* libedit.dylib in Frameworks */ = {isa = PBXBuildFile; fileRef = 5D5D8AD00E0D0EBE00F9C692 /* libedit.dylib */; };
+               5D6A566B0F05995500266145 /* Threading.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 5D6A566A0F05995500266145 /* Threading.cpp */; };
                5DE6E5B30E1728EC00180407 /* create_hash_table in Headers */ = {isa = PBXBuildFile; fileRef = F692A8540255597D01FF60F7 /* create_hash_table */; settings = {ATTRIBUTES = (Private, ); }; };
                6507D29E0E871E5E00D7D896 /* TypeInfo.h in Headers */ = {isa = PBXBuildFile; fileRef = 6507D2970E871E4A00D7D896 /* TypeInfo.h */; settings = {ATTRIBUTES = (Private, ); }; };
                659126BD0BDD1728001921FB /* AllInOneFile.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 659126BC0BDD1728001921FB /* AllInOneFile.cpp */; };
                5D53726E0E1C54880021E549 /* Tracing.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = Tracing.h; sourceTree = "<group>"; };
                5D53727D0E1C55EC0021E549 /* TracingDtrace.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = TracingDtrace.h; sourceTree = "<group>"; };
                5D5D8AD00E0D0EBE00F9C692 /* libedit.dylib */ = {isa = PBXFileReference; lastKnownFileType = "compiled.mach-o.dylib"; name = libedit.dylib; path = /usr/lib/libedit.dylib; sourceTree = "<absolute>"; };
+               5D6A566A0F05995500266145 /* Threading.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = Threading.cpp; sourceTree = "<group>"; };
                5DA479650CFBCF56009328A0 /* TCPackedCache.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = TCPackedCache.h; sourceTree = "<group>"; };
                5DBD18AF0C5401A700C15EAE /* MallocZoneSupport.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MallocZoneSupport.h; sourceTree = "<group>"; };
                5DE3D0F40DD8DDFB00468714 /* WebKitAvailability.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WebKitAvailability.h; sourceTree = "<group>"; };
                                6541BD7108E80A17002CBEE7 /* TCSystemAlloc.h */,
                                E1B7C8BD0DA3A3360074B0DC /* ThreadSpecific.h */,
                                E1EE79220D6C95CD00FEA3BA /* Threading.h */,
+                               5D6A566A0F05995500266145 /* Threading.cpp */,
                                E1EE793C0D6C9B9200FEA3BA /* ThreadingPthreads.cpp */,
                                935AF46B09E9D9DB00ACD1D8 /* UnusedParam.h */,
                                6592C316098B7DE10003D4F6 /* Vector.h */,
                                930754D008B0F74600AB3056 /* pcre_tables.cpp in Sources */,
                                937013480CA97E0E00FA14D3 /* pcre_ucp_searchfuncs.cpp in Sources */,
                                93E26BD408B1514100F85226 /* pcre_xclass.cpp in Sources */,
+                               5D6A566B0F05995500266145 /* Threading.cpp in Sources */,
                        );
                        runOnlyForDeploymentPostprocessing = 0;
                };
index 96450f7..c5f4d78 100644 (file)
@@ -163,6 +163,7 @@ Source files for JSCore.
         wtf/RandomNumber.cpp
         wtf/RefCountedLeakCounter.cpp
         wtf/TCSystemAlloc.cpp
+        wtf/Threading.cpp
         wtf/ThreadingNone.cpp
         wtf/wx/MainThreadWx.cpp
         wtf/unicode/CollatorDefault.cpp
diff --git a/JavaScriptCore/wtf/Threading.cpp b/JavaScriptCore/wtf/Threading.cpp
new file mode 100644 (file)
index 0000000..0179dcc
--- /dev/null
@@ -0,0 +1,81 @@
+/*
+ * Copyright (C) 2008 Apple Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1.  Redistributions of source code must retain the above copyright
+ *     notice, this list of conditions and the following disclaimer. 
+ * 2.  Redistributions in binary form must reproduce the above copyright
+ *     notice, this list of conditions and the following disclaimer in the
+ *     documentation and/or other materials provided with the distribution. 
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+ * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
+ * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
+ * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include "config.h"
+#include "Threading.h"
+
+namespace WTF {
+
+struct NewThreadContext {
+    NewThreadContext(ThreadFunction entryPoint, void* data)
+        : entryPoint(entryPoint)
+        , data(data)
+    { }
+
+    ThreadFunction entryPoint;
+    void* data;
+
+    Mutex creationMutex;
+};
+
+static void* threadEntryPoint(void* contextData)
+{
+    NewThreadContext* context = reinterpret_cast<NewThreadContext*>(contextData);
+
+    // Block until our creating thread has completed any extra setup work
+    {
+        MutexLocker locker(context->creationMutex);
+    }
+
+    // Grab the info that we need out of the context, then deallocate it.
+    ThreadFunction entryPoint = context->entryPoint;
+    void* data = context->data;
+    delete context;
+
+    return entryPoint(data);
+}
+
+ThreadIdentifier createThread(ThreadFunction entryPoint, void* data, const char* name)
+{
+    NewThreadContext* context = new NewThreadContext(entryPoint, data);
+
+    // Prevent the thread body from executing until we've established the thread identifier
+    MutexLocker locker(context->creationMutex);
+
+    return createThreadInternal(threadEntryPoint, context, name);
+}
+
+#if PLATFORM(MAC) || PLATFORM(WIN)
+
+// This function is deprecated but needs to be kept around for backward
+// compatibility. Use the 3-argument version of createThread above.
+
+ThreadIdentifier createThread(ThreadFunction entryPoint, void* data)
+{
+    return createThread(entryPoint, data, 0);
+}
+#endif
+
+} // namespace WTF
index d1fe252..156013d 100644 (file)
@@ -110,6 +110,7 @@ typedef void* (*ThreadFunction)(void* argument);
 
 // Returns 0 if thread creation failed
 ThreadIdentifier createThread(ThreadFunction, void*, const char* threadName);
+ThreadIdentifier createThreadInternal(ThreadFunction, void*, const char* threadName);
 
 ThreadIdentifier currentThread();
 bool isMainThread();
index b5cf3d3..777d55b 100644 (file)
@@ -82,17 +82,6 @@ static HashMap<ThreadIdentifier, GThread*>& threadMap()
     return map;
 }
 
-static ThreadIdentifier establishIdentifierForThread(GThread*& thread)
-{
-    MutexLocker locker(threadMapMutex());
-
-    static ThreadIdentifier identifierCount = 1;
-
-    threadMap().add(identifierCount, thread);
-
-    return identifierCount++;
-}
-
 static ThreadIdentifier identifierByGthreadHandle(GThread*& thread)
 {
     MutexLocker locker(threadMapMutex());
@@ -106,6 +95,19 @@ static ThreadIdentifier identifierByGthreadHandle(GThread*& thread)
     return 0;
 }
 
+static ThreadIdentifier establishIdentifierForThread(GThread*& thread)
+{
+    ASSERT(!identifierByGthreadHandle(thread));
+
+    MutexLocker locker(threadMapMutex());
+
+    static ThreadIdentifier identifierCount = 1;
+
+    threadMap().add(identifierCount, thread);
+
+    return identifierCount++;
+}
+
 static GThread* threadForIdentifier(ThreadIdentifier id)
 {
     MutexLocker locker(threadMapMutex());
@@ -122,7 +124,7 @@ static void clearThreadForIdentifier(ThreadIdentifier id)
     threadMap().remove(id);
 }
 
-ThreadIdentifier createThread(ThreadFunction entryPoint, void* data, const char*)
+ThreadIdentifier createThreadInternal(ThreadFunction entryPoint, void* data, const char*)
 {
     GThread* thread;
     if (!(thread = g_thread_create(entryPoint, data, TRUE, 0))) {
index ccb27af..832cd0c 100644 (file)
@@ -33,7 +33,7 @@
 namespace WTF {
 
 void initializeThreading() { }
-ThreadIdentifier createThread(ThreadFunction, void*, const char*) { return 0; }
+ThreadIdentifier createThreadInternal(ThreadFunction, void*, const char*) { return 0; }
 int waitForThreadCompletion(ThreadIdentifier, void**) { return 0; }
 void detachThread(ThreadIdentifier) { }
 ThreadIdentifier currentThread() { return 0; }
index 4ea9999..f111fcf 100644 (file)
@@ -86,17 +86,6 @@ static ThreadMap& threadMap()
     return map;
 }
 
-static ThreadIdentifier establishIdentifierForPthreadHandle(pthread_t& pthreadHandle)
-{
-    MutexLocker locker(threadMapMutex());
-
-    static ThreadIdentifier identifierCount = 1;
-
-    threadMap().add(identifierCount, pthreadHandle);
-    
-    return identifierCount++;
-}
-
 static ThreadIdentifier identifierByPthreadHandle(const pthread_t& pthreadHandle)
 {
     MutexLocker locker(threadMapMutex());
@@ -110,6 +99,19 @@ static ThreadIdentifier identifierByPthreadHandle(const pthread_t& pthreadHandle
     return 0;
 }
 
+static ThreadIdentifier establishIdentifierForPthreadHandle(pthread_t& pthreadHandle)
+{
+    ASSERT(!identifierByPthreadHandle(pthreadHandle));
+
+    MutexLocker locker(threadMapMutex());
+
+    static ThreadIdentifier identifierCount = 1;
+
+    threadMap().add(identifierCount, pthreadHandle);
+    
+    return identifierCount++;
+}
+
 static pthread_t pthreadHandleForIdentifier(ThreadIdentifier id)
 {
     MutexLocker locker(threadMapMutex());
@@ -126,7 +128,7 @@ static void clearPthreadHandleForIdentifier(ThreadIdentifier id)
     threadMap().remove(id);
 }
 
-ThreadIdentifier createThread(ThreadFunction entryPoint, void* data, const char*)
+ThreadIdentifier createThreadInternal(ThreadFunction entryPoint, void* data, const char*)
 {
     pthread_t threadHandle;
     if (pthread_create(&threadHandle, NULL, entryPoint, data)) {
@@ -134,19 +136,9 @@ ThreadIdentifier createThread(ThreadFunction entryPoint, void* data, const char*
         return 0;
     }
 
-    ThreadIdentifier threadID = establishIdentifierForPthreadHandle(threadHandle);
-    return threadID;
+    return establishIdentifierForPthreadHandle(threadHandle);
 }
 
-#if PLATFORM(MAC)
-// This function is deprecated but needs to be kept around for backward
-// compatibility. Use the 3-argument version of createThread above instead.
-ThreadIdentifier createThread(ThreadFunction entryPoint, void* data)
-{
-    return createThread(entryPoint, data, 0);
-}
-#endif
-
 int waitForThreadCompletion(ThreadIdentifier threadID, void** result)
 {
     ASSERT(threadID);
@@ -223,7 +215,7 @@ void Mutex::unlock()
     if (pthread_mutex_unlock(&m_mutex) != 0)
         ASSERT(false);
 }
-                
+
 ThreadCondition::ThreadCondition()
 { 
     pthread_cond_init(&m_condition, NULL);
index 68d6a27..0309f2b 100644 (file)
@@ -80,8 +80,23 @@ static HashMap<ThreadIdentifier, QThread*>& threadMap()
     return map;
 }
 
+static ThreadIdentifier identifierByQthreadHandle(QThread*& thread)
+{
+    MutexLocker locker(threadMapMutex());
+
+    HashMap<ThreadIdentifier, QThread*>::iterator i = threadMap().begin();
+    for (; i != threadMap().end(); ++i) {
+        if (i->second == thread)
+            return i->first;
+    }
+
+    return 0;
+}
+
 static ThreadIdentifier establishIdentifierForThread(QThread*& thread)
 {
+    ASSERT(!identifierByQthreadHandle(thread));
+
     MutexLocker locker(threadMapMutex());
 
     static ThreadIdentifier identifierCount = 1;
@@ -100,19 +115,6 @@ static void clearThreadForIdentifier(ThreadIdentifier id)
     threadMap().remove(id);
 }
 
-static ThreadIdentifier identifierByQthreadHandle(QThread*& thread)
-{
-    MutexLocker locker(threadMapMutex());
-
-    HashMap<ThreadIdentifier, QThread*>::iterator i = threadMap().begin();
-    for (; i != threadMap().end(); ++i) {
-        if (i->second == thread)
-            return i->first;
-    }
-
-    return 0;
-}
-
 static QThread* threadForIdentifier(ThreadIdentifier id)
 {
     MutexLocker locker(threadMapMutex());
@@ -145,7 +147,7 @@ void unlockAtomicallyInitializedStaticMutex()
     atomicallyInitializedStaticMutex->unlock();
 }
 
-ThreadIdentifier createThread(ThreadFunction entryPoint, void* data, const char*)
+ThreadIdentifier createThreadInternal(ThreadFunction entryPoint, void* data, const char*)
 {
     ThreadPrivate* thread = new ThreadPrivate(entryPoint, data);
     if (!thread) {
index b1310dd..7b30f76 100644 (file)
@@ -150,6 +150,7 @@ static HashMap<DWORD, HANDLE>& threadMap()
 static void storeThreadHandleByIdentifier(DWORD threadID, HANDLE threadHandle)
 {
     MutexLocker locker(threadMapMutex());
+    ASSERT(!threadMap().contains(threadID));
     threadMap().add(threadID, threadHandle);
 }
 
@@ -188,7 +189,7 @@ static unsigned __stdcall wtfThreadEntryPoint(void* param)
     return reinterpret_cast<unsigned>(result);
 }
 
-ThreadIdentifier createThread(ThreadFunction entryPoint, void* data, const char* threadName)
+ThreadIdentifier createThreadInternal(ThreadFunction entryPoint, void* data, const char* threadName)
 {
     unsigned threadIdentifier = 0;
     ThreadIdentifier threadID = 0;
@@ -208,13 +209,6 @@ ThreadIdentifier createThread(ThreadFunction entryPoint, void* data, const char*
     return threadID;
 }
 
-// This function is deprecated but needs to be kept around for backward
-// compatibility. Use the 3-argument version of createThread above.
-ThreadIdentifier createThread(ThreadFunction entryPoint, void* data)
-{
-    return createThread(entryPoint, data, 0);
-}
-
 int waitForThreadCompletion(ThreadIdentifier threadID, void** result)
 {
     ASSERT(threadID);