Skip to content

Commit

Permalink
Use holding thread id in AwareLock to avoid orphaned lock crash (dotn…
Browse files Browse the repository at this point in the history 
…et#107168)
  • Loading branch information
@eduardo-vp
eduardo-vp authored Dec 10, 2024
1 parent ca5cf41 commit 938f057
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 6 deletions.
19 changes: 16 additions & 3 deletions src/coreclr/vm/syncblk.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1816,15 +1816,22 @@ 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;
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;
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;

Expand Down
15 changes: 12 additions & 3 deletions src/coreclr/vm/syncblk.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,7 @@ class AwareLock

ULONG m_Recursion;
PTR_Thread m_HoldingThread;
DWORD m_HoldingThreadId;
SIZE_T m_HoldingOSThreadId;

LONG m_TransientPrecious;
Expand All @@ -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),
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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)
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/vm/syncblk.inl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 938f057

Please sign in to comment.