diff --git a/src/coreclr/vm/syncblk.cpp b/src/coreclr/vm/syncblk.cpp index a1ed22f2c00401..71a196b371c1f0 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 || 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; @@ -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,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 index e3eb90b663fd3d..770e024164d151 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)