From b1e32e1466438e2c896e3e994678cbd14d3b8048 Mon Sep 17 00:00:00 2001 From: Eduardo Manuel Velarde Polar Date: Thu, 29 Aug 2024 17:21:44 -0700 Subject: [PATCH 1/8] Check if lock is orphaned --- src/coreclr/vm/syncblk.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/syncblk.cpp b/src/coreclr/vm/syncblk.cpp index 868fffca0c055..a1ed22f2c0040 100644 --- a/src/coreclr/vm/syncblk.cpp +++ b/src/coreclr/vm/syncblk.cpp @@ -1816,7 +1816,7 @@ BOOL ObjHeader::GetThreadOwningMonitorLock(DWORD *pThreadId, DWORD *pAcquisition _ASSERTE(psb->GetMonitor() != NULL); Thread* pThread = psb->GetMonitor()->GetHoldingThread(); - if(pThread == NULL) + if(pThread == NULL || pThread == (Thread*) -1) // -1 means the lock is orphaned { *pThreadId = 0; *pAcquisitionCount = 0; From 6094198943d1eb04a84b12f538fc2e7695c2f7c3 Mon Sep 17 00:00:00 2001 From: Eduardo Manuel Velarde Polar Date: Wed, 25 Sep 2024 14:10:41 -0700 Subject: [PATCH 2/8] Store thread id in sync block and use it in GetThreadOwningMonitorLock --- src/coreclr/vm/syncblk.cpp | 18 +++++++++++++++--- src/coreclr/vm/syncblk.h | 15 ++++++++++++--- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/coreclr/vm/syncblk.cpp b/src/coreclr/vm/syncblk.cpp index a1ed22f2c0040..71a196b371c1f 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 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) From 9de4e06f5010c0b9bcbbba19860b9a5a4331fee1 Mon Sep 17 00:00:00 2001 From: Eduardo Velarde Date: Thu, 28 Nov 2024 14:50:39 -0800 Subject: [PATCH 3/8] Add changes in syncblk.inl --- src/coreclr/vm/syncblk.inl | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/syncblk.inl b/src/coreclr/vm/syncblk.inl index 97b479168f146..629751bd3efde 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; @@ -683,7 +687,7 @@ FORCEINLINE AwareLock::LeaveHelperAction AwareLock::LeaveHelper(Thread* pCurThre MODE_ANY; } CONTRACTL_END; - if (m_HoldingThread != pCurThread) + if (m_HoldingThreadId != pCurThread->GetThreadId()) return AwareLock::LeaveHelperAction_Error; _ASSERTE(m_lockState.VolatileLoadWithoutBarrier().IsLocked()); From 8511abff2836acb748215d416a581f14f087e347 Mon Sep 17 00:00:00 2001 From: Eduardo Velarde Date: Thu, 28 Nov 2024 14:57:50 -0800 Subject: [PATCH 4/8] Set threadId to -1 --- src/coreclr/vm/syncblk.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/vm/syncblk.cpp b/src/coreclr/vm/syncblk.cpp index 71a196b371c1f..3d58b12d8dee6 100644 --- a/src/coreclr/vm/syncblk.cpp +++ b/src/coreclr/vm/syncblk.cpp @@ -2204,6 +2204,7 @@ SyncBlock *ObjHeader::GetSyncBlock() { // The lock is orphaned. pThread = (Thread*) -1; + threadId = -1; osThreadId = (SIZE_T)-1; } else From 95ba6a8d47cf53cfeeea9a4ea7a6755763ffb427 Mon Sep 17 00:00:00 2001 From: Eduardo Velarde Date: Fri, 29 Nov 2024 12:29:22 -0800 Subject: [PATCH 5/8] Use m_HoldingThread in AwareLock::LeaveHelper --- src/coreclr/vm/syncblk.inl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/syncblk.inl b/src/coreclr/vm/syncblk.inl index 629751bd3efde..dfa8e5e2d462c 100644 --- a/src/coreclr/vm/syncblk.inl +++ b/src/coreclr/vm/syncblk.inl @@ -687,7 +687,8 @@ FORCEINLINE AwareLock::LeaveHelperAction AwareLock::LeaveHelper(Thread* pCurThre MODE_ANY; } CONTRACTL_END; - if (m_HoldingThreadId != pCurThread->GetThreadId()) + // if (m_HoldingThreadId != pCurThread->GetThreadId()) some tests failed when using this line instead + if (m_HoldingThread != pCurThread) return AwareLock::LeaveHelperAction_Error; _ASSERTE(m_lockState.VolatileLoadWithoutBarrier().IsLocked()); From 9b4f078510720f96ea6f5ef0c25d24c54fa05920 Mon Sep 17 00:00:00 2001 From: Eduardo Velarde Date: Mon, 2 Dec 2024 12:17:53 -0800 Subject: [PATCH 6/8] Reset m_HoldingThreadId in AwareLock::LeaveHelper --- src/coreclr/vm/syncblk.cpp | 2 +- src/coreclr/vm/syncblk.inl | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/syncblk.cpp b/src/coreclr/vm/syncblk.cpp index 3d58b12d8dee6..3fb9089bf31c5 100644 --- a/src/coreclr/vm/syncblk.cpp +++ b/src/coreclr/vm/syncblk.cpp @@ -1818,7 +1818,7 @@ BOOL ObjHeader::GetThreadOwningMonitorLock(DWORD *pThreadId, DWORD *pAcquisition Thread* pThread = psb->GetMonitor()->GetHoldingThread(); // 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) + if (pThread == NULL || pThread == (Thread*) -1) { *pThreadId = 0; *pAcquisitionCount = 0; diff --git a/src/coreclr/vm/syncblk.inl b/src/coreclr/vm/syncblk.inl index dfa8e5e2d462c..8f17c4e42cbc8 100644 --- a/src/coreclr/vm/syncblk.inl +++ b/src/coreclr/vm/syncblk.inl @@ -704,6 +704,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 From 53ccff628f6af54b6b5084d7ce34a6b792d3f966 Mon Sep 17 00:00:00 2001 From: Eduardo Velarde Date: Mon, 2 Dec 2024 14:12:17 -0800 Subject: [PATCH 7/8] Use thread id to compare in AwareLock::LeaveHelper --- src/coreclr/vm/syncblk.inl | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/vm/syncblk.inl b/src/coreclr/vm/syncblk.inl index 8f17c4e42cbc8..fee274263174b 100644 --- a/src/coreclr/vm/syncblk.inl +++ b/src/coreclr/vm/syncblk.inl @@ -687,8 +687,7 @@ FORCEINLINE AwareLock::LeaveHelperAction AwareLock::LeaveHelper(Thread* pCurThre MODE_ANY; } CONTRACTL_END; - // if (m_HoldingThreadId != pCurThread->GetThreadId()) some tests failed when using this line instead - if (m_HoldingThread != pCurThread) + if (m_HoldingThreadId != pCurThread->GetThreadId()) return AwareLock::LeaveHelperAction_Error; _ASSERTE(m_lockState.VolatileLoadWithoutBarrier().IsLocked()); From b0d2a860b99d90c6b2745631bf1a960f6f19a049 Mon Sep 17 00:00:00 2001 From: Eduardo Velarde Date: Tue, 10 Dec 2024 13:33:17 -0800 Subject: [PATCH 8/8] PR feedback --- src/coreclr/vm/syncblk.inl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/syncblk.inl b/src/coreclr/vm/syncblk.inl index fee274263174b..4cad064c8149d 100644 --- a/src/coreclr/vm/syncblk.inl +++ b/src/coreclr/vm/syncblk.inl @@ -687,7 +687,7 @@ FORCEINLINE AwareLock::LeaveHelperAction AwareLock::LeaveHelper(Thread* pCurThre MODE_ANY; } CONTRACTL_END; - if (m_HoldingThreadId != pCurThread->GetThreadId()) + if (m_HoldingThread != pCurThread) return AwareLock::LeaveHelperAction_Error; _ASSERTE(m_lockState.VolatileLoadWithoutBarrier().IsLocked());