Store thread id in sync block and use it in GetThreadOwningMonitorLock

dotnet · Sep 26, 2024 · 76fc67d · 76fc67d
1 parent b7ddaaa
commit 76fc67d
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 6 deletions.
diff --git a/src/coreclr/vm/syncblk.cpp b/src/coreclr/vm/syncblk.cpp
@@ -1816,15 +1816,22 @@ BOOL ObjHeader::GetThreadOwningMonitorLock(DWORD *pThreadId, DWORD *pAcquisition
 
             _ASSERTE(psb->GetMonitor() != NULL);
             Thread* pThread = psb->GetMonitor()->GetHoldingThread();
-            if(pThread == NULL || pThread == (Thread*) -1) // -1 means the lock is orphaned
+            // If the lock is orphaned during sync block creation, pThread would be assigned -1.
+            // Otherwise pThread would point to the owning thread if there was one or NULL if there wasn't.
+            if(pThread == NULL || pThread == (Thread*) -1)
             {
                 *pThreadId = 0;
                 *pAcquisitionCount = 0;
                 return FALSE;
             }
             else
             {
-                *pThreadId = pThread->GetThreadId();
+                // However, the lock might get orphaned after the sync block is created.
+                // Therefore accessing pThread is not safe and pThread->GetThreadId() shouldn't be used.
+                // The thread id can be obtained from the monitor, which would be the id of the thread that owned the lock.
+                // Notice this id now could have been reused for a different thread,
+                // but this way we avoid crashing the process and orphaned locks shouldn't be expected to work correctly anyway.
+                *pThreadId = psb->GetMonitor()->GetHoldingThreadId();
                 *pAcquisitionCount = psb->GetMonitor()->GetRecursionLevel();
                 return TRUE;
             }
@@ -2190,6 +2197,7 @@ SyncBlock *ObjHeader::GetSyncBlock()
                             _ASSERTE(lockThreadId != 0);
 
                             Thread *pThread = g_pThinLockThreadIdDispenser->IdToThreadWithValidation(lockThreadId);
+                            DWORD threadId = 0;
                             SIZE_T osThreadId;
 
                             if (pThread == NULL)
@@ -2200,10 +2208,11 @@ SyncBlock *ObjHeader::GetSyncBlock()
                             }
                             else
                             {
+                                threadId = pThread->GetThreadId();
                                 osThreadId = pThread->GetOSThreadId64();
                             }
 
-                            syncBlock->InitState(recursionLevel + 1, pThread, osThreadId);
+                            syncBlock->InitState(recursionLevel + 1, pThread, threadId, osThreadId);
                         }
                     }
                     else if ((bits & BIT_SBLK_IS_HASHCODE) != 0)
@@ -2367,6 +2376,7 @@ void AwareLock::Enter()
         {
             // We get here if we successfully acquired the mutex.
             m_HoldingThread = pCurThread;
+            m_HoldingThreadId = pCurThread->GetThreadId();
             m_HoldingOSThreadId = pCurThread->GetOSThreadId64();
             m_Recursion = 1;
 
@@ -2430,6 +2440,7 @@ BOOL AwareLock::TryEnter(INT32 timeOut)
         {
             // We get here if we successfully acquired the mutex.
             m_HoldingThread = pCurThread;
+            m_HoldingThreadId = pCurThread->GetThreadId();
             m_HoldingOSThreadId = pCurThread->GetOSThreadId64();
             m_Recursion = 1;
 
@@ -2709,6 +2720,7 @@ BOOL AwareLock::EnterEpilogHelper(Thread* pCurThread, INT32 timeOut)
     }
 
     m_HoldingThread = pCurThread;
+    m_HoldingThreadId = pCurThread->GetThreadId();
     m_HoldingOSThreadId = pCurThread->GetOSThreadId64();
     m_Recursion = 1;
 

diff --git a/src/coreclr/vm/syncblk.h b/src/coreclr/vm/syncblk.h
@@ -436,6 +436,7 @@ class AwareLock
 
     ULONG           m_Recursion;
     PTR_Thread      m_HoldingThread;
+    DWORD           m_HoldingThreadId;
     SIZE_T          m_HoldingOSThreadId;
 
     LONG            m_TransientPrecious;
@@ -459,6 +460,7 @@ class AwareLock
 // PreFAST has trouble with initializing a NULL PTR_Thread.
           m_HoldingThread(NULL),
 #endif // DACCESS_COMPILE
+          m_HoldingThreadId(0),
           m_HoldingOSThreadId(0),
           m_TransientPrecious(0),
           m_dwSyncIndex(indx),
@@ -523,19 +525,26 @@ class AwareLock
         return m_HoldingThread;
     }
 
+    DWORD GetHoldingThreadId() const
+    {
+        LIMITED_METHOD_CONTRACT;
+        return m_HoldingThreadId;
+    }
+
 private:
     void ResetWaiterStarvationStartTime();
     void RecordWaiterStarvationStartTime();
     bool ShouldStopPreemptingWaiters() const;
 
 private: // friend access is required for this unsafe function
-    void InitializeToLockedWithNoWaiters(ULONG recursionLevel, PTR_Thread holdingThread, SIZE_T holdingOSThreadId)
+    void InitializeToLockedWithNoWaiters(ULONG recursionLevel, PTR_Thread holdingThread, DWORD holdingThreadId, SIZE_T holdingOSThreadId)
     {
         WRAPPER_NO_CONTRACT;
 
         m_lockState.InitializeToLockedWithNoWaiters();
         m_Recursion = recursionLevel;
         m_HoldingThread = holdingThread;
+        m_HoldingThreadId = holdingThreadId;
         m_HoldingOSThreadId = holdingOSThreadId;
     }
 
@@ -1270,10 +1279,10 @@ class SyncBlock
     // This should ONLY be called when initializing a SyncBlock (i.e. ONLY from
     // ObjHeader::GetSyncBlock()), otherwise we'll have a race condition.
     // </NOTE>
-    void InitState(ULONG recursionLevel, PTR_Thread holdingThread, SIZE_T holdingOSThreadId)
+    void InitState(ULONG recursionLevel, PTR_Thread holdingThread, DWORD holdingThreadId, SIZE_T holdingOSThreadId)
     {
         WRAPPER_NO_CONTRACT;
-        m_Monitor.InitializeToLockedWithNoWaiters(recursionLevel, holdingThread, holdingOSThreadId);
+        m_Monitor.InitializeToLockedWithNoWaiters(recursionLevel, holdingThread, holdingThreadId, holdingOSThreadId);
     }
 
 #if defined(ENABLE_CONTRACTS_IMPL)