Changeset 10792 in vbox for trunk

Timestamp:

Jul 21, 2008 10:19:26 PM (16 years ago)

Author:

vboxsync

Message:

IPRT: Fixed issue where RTCreateThread would race against the termination of the new thread, if RTCreateThread didn't manage to insert the thread before the new thread completed, we would be inserting a thread into the tree that was terminated and which native ID should not be in the tree. If the OS then reused the native thread ID immediately (OS/2 and now glibc/linux), we would end up with a duplicate in the tree and get all confused. The fix is to check the state in rtThreadInsert and never insert a terminated thread.

File:

• trunk/src/VBox/Runtime/common/misc/thread.cpp (modified) (7 diffs)

trunk/src/VBox/Runtime/common/misc/thread.cpp

@@ -256 +256 @@
         {
             rtThreadInsert(pThread, NativeThread);
-            pThread->enmState = RTTHREADSTATE_RUNNING;
+            ASMAtomicWriteSize(&pThread->enmState, RTTHREADSTATE_RUNNING);
             rtThreadRelease(pThread);
         }
@@ -370 +370 @@
 
     /*
-     * Before inserting we must check if there is a thread with this id
-     * in the tree already. We're racing parent and child on insert here
-     * so that the handle is valid in both ends when they return / start.
+     * Do not insert a terminated thread.
      *
-     * If it's not ourself we find, it's a dead alien thread and we will
-     * unlink it from the tree. Alien threads will be released at this point.
-     */
-    PRTTHREADINT pThreadOther = (PRTTHREADINT)RTAvlPVGet(&g_ThreadTree, (void *)NativeThread);
-    if (pThreadOther != pThread)
-    {
-        /* remove dead alien if any */
-        if (pThreadOther)
+     * This may happen if the thread finishes before the RTThreadCreate call
+     * gets this far. Since the OS may quickly reuse the native thread ID
+     * it should not be reinserted at this point.
+     */
+    if (pThread->enmState != RTTHREADSTATE_TERMINATED)
+    {
+        /*
+         * Before inserting we must check if there is a thread with this id
+         * in the tree already. We're racing parent and child on insert here
+         * so that the handle is valid in both ends when they return / start.
+         *
+         * If it's not ourself we find, it's a dead alien thread and we will
+         * unlink it from the tree. Alien threads will be released at this point.
+         */
+        PRTTHREADINT pThreadOther = (PRTTHREADINT)RTAvlPVGet(&g_ThreadTree, (void *)NativeThread);
+        if (pThreadOther != pThread)
         {
-            Assert(pThreadOther->fIntFlags & RTTHREADINT_FLAGS_ALIEN);
-            ASMAtomicBitClear(&pThread->fIntFlags, RTTHREADINT_FLAG_IN_TREE_BIT);
-            rtThreadRemoveLocked(pThreadOther);
-            if (pThreadOther->fIntFlags & RTTHREADINT_FLAGS_ALIEN)
-                rtThreadRelease(pThreadOther);
+            /* remove dead alien if any */
+            if (pThreadOther)
+            {
+                AssertMsg(pThreadOther->fIntFlags & RTTHREADINT_FLAGS_ALIEN, ("%p:%s; %p:%s\n", pThread, pThread->szName, pThreadOther, pThreadOther->szName));
+                ASMAtomicBitClear(&pThread->fIntFlags, RTTHREADINT_FLAG_IN_TREE_BIT);
+                rtThreadRemoveLocked(pThreadOther);
+                if (pThreadOther->fIntFlags & RTTHREADINT_FLAGS_ALIEN)
+                    rtThreadRelease(pThreadOther);
+            }
+
+            /* insert the thread */
+            ASMAtomicWritePtr(&pThread->Core.Key, (void *)NativeThread);
+            bool fRc = RTAvlPVInsert(&g_ThreadTree, &pThread->Core);
+            ASMAtomicOrU32(&pThread->fIntFlags, RTTHREADINT_FLAG_IN_TREE);
+
+            AssertReleaseMsg(fRc, ("Lock problem? %p (%RTnthrd) %s\n", pThread, NativeThread, pThread->szName));
+            NOREF(fRc);
         }
-
-        /* insert the thread */
-        pThread->Core.Key = (void *)NativeThread;
-        bool fRc = RTAvlPVInsert(&g_ThreadTree, &pThread->Core);
-        ASMAtomicOrU32(&pThread->fIntFlags, RTTHREADINT_FLAG_IN_TREE);
-
-        AssertReleaseMsg(fRc, ("Lock problem? %p (%RTnthrd) %s\n", pThread, NativeThread, pThread->szName));
-        NOREF(fRc);
     }
 
@@ -544 +554 @@
      * Free resources.
      */
-    pThread->Core.Key   = (void *)NIL_RTTHREAD;
+    ASMAtomicWritePtr(&pThread->Core.Key, (void *)NIL_RTTHREAD);
     pThread->enmType    = RTTHREADTYPE_INVALID;
     RTSemEventMultiDestroy(pThread->EventUser);
@@ -579 +589 @@
      */
     pThread->rc = rc;
-    ASMAtomicXchgSize(&pThread->enmState, RTTHREADSTATE_TERMINATED);
+    ASMAtomicWriteSize(&pThread->enmState, RTTHREADSTATE_TERMINATED);
     ASMAtomicOrU32(&pThread->fIntFlags, RTTHREADINT_FLAGS_TERMINATED);
     if (pThread->EventTerminated != NIL_RTSEMEVENTMULTI)
@@ -625 +635 @@
      * Call thread function and terminate when it returns.
      */
-    pThread->enmState = RTTHREADSTATE_RUNNING;
+    ASMAtomicWriteSize(&pThread->enmState, RTTHREADSTATE_RUNNING);
     rc = pThread->pfnThread(pThread, pThread->pvUser);
 
@@ -1367 +1377 @@
         pThread->uBlockLine     = uLine;
         pThread->uBlockId       = uId;
-        ASMAtomicXchgSize(&pThread->enmState, enmState);
+        ASMAtomicWriteSize(&pThread->enmState, enmState);
 
         /*
@@ -1452 +1462 @@
 {
     if (pThread && pThread->enmState == enmCurState)
-        ASMAtomicXchgSize(&pThread->enmState, RTTHREADSTATE_RUNNING);
+        ASMAtomicWriteSize(&pThread->enmState, RTTHREADSTATE_RUNNING);
 }