diff --git a/src/coreclr/vm/syncblk.cpp b/src/coreclr/vm/syncblk.cpp index 868fffca0c055..3fb9089bf31c5 100644 --- a/src/coreclr/vm/syncblk.cpp +++ b/src/coreclr/vm/syncblk.cpp @@ -1816,7 +1816,9 @@ BOOL ObjHeader::GetThreadOwningMonitorLock(DWORD *pThreadId, DWORD *pAcquisition _ASSERTE(psb->GetMonitor() != NULL); Thread* pThread = psb->GetMonitor()->GetHoldingThread(); - if(pThread == NULL) + // 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; @@ -1824,7 +1826,12 @@ BOOL ObjHeader::GetThreadOwningMonitorLock(DWORD *pThreadId, DWORD *pAcquisition } 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,20 +2197,23 @@ SyncBlock *ObjHeader::GetSyncBlock() _ASSERTE(lockThreadId != 0); Thread *pThread = g_pThinLockThreadIdDispenser->IdToThreadWithValidation(lockThreadId); + DWORD threadId = 0; SIZE_T osThreadId; if (pThread == NULL) { // The lock is orphaned. pThread = (Thread*) -1; + threadId = -1; osThreadId = (SIZE_T)-1; } 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 +2377,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 +2441,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 +2721,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 index 7cdf92cdda878..aa76cd3056b88 100644 --- 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. // - 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) diff --git a/src/coreclr/vm/syncblk.inl b/src/coreclr/vm/syncblk.inl index 97b479168f146..4cad064c8149d 100644 --- a/src/coreclr/vm/syncblk.inl +++ b/src/coreclr/vm/syncblk.inl @@ -479,6 +479,7 @@ FORCEINLINE bool AwareLock::TryEnterHelper(Thread* pCurThread) if (m_lockState.InterlockedTryLock()) { m_HoldingThread = pCurThread; + m_HoldingThreadId = pCurThread->GetThreadId(); m_HoldingOSThreadId = pCurThread->GetOSThreadId64(); m_Recursion = 1; return true; @@ -525,6 +526,7 @@ FORCEINLINE AwareLock::EnterHelperResult AwareLock::TryEnterBeforeSpinLoopHelper // Lock was acquired and the spinner was not registered m_HoldingThread = pCurThread; + m_HoldingThreadId = pCurThread->GetThreadId(); m_HoldingOSThreadId = pCurThread->GetOSThreadId64(); m_Recursion = 1; return EnterHelperResult_Entered; @@ -557,6 +559,7 @@ FORCEINLINE AwareLock::EnterHelperResult AwareLock::TryEnterInsideSpinLoopHelper // Lock was acquired and spinner was unregistered m_HoldingThread = pCurThread; + m_HoldingThreadId = pCurThread->GetThreadId(); m_HoldingOSThreadId = pCurThread->GetOSThreadId64(); m_Recursion = 1; return EnterHelperResult_Entered; @@ -580,6 +583,7 @@ FORCEINLINE bool AwareLock::TryEnterAfterSpinLoopHelper(Thread *pCurThread) // Spinner was unregistered and the lock was acquired m_HoldingThread = pCurThread; + m_HoldingThreadId = pCurThread->GetThreadId(); m_HoldingOSThreadId = pCurThread->GetOSThreadId64(); m_Recursion = 1; return true; @@ -699,6 +703,7 @@ FORCEINLINE AwareLock::LeaveHelperAction AwareLock::LeaveHelper(Thread* pCurThre if (--m_Recursion == 0) { m_HoldingThread = NULL; + m_HoldingThreadId = 0; m_HoldingOSThreadId = 0; // Clear lock bit and determine whether we must signal a waiter to wake