Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use holding thread id in AwareLock to avoid orphaned lock crash #107168

Merged
merged 8 commits into from
Dec 10, 2024
Merged

Use holding thread id in AwareLock to avoid orphaned lock crash #107168

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b1e32e1
Check if lock is orphaned
Aug 30, 2024
6094198
Store thread id in sync block and use it in GetThreadOwningMonitorLock
Sep 25, 2024
9de4e06
Add changes in syncblk.inl
eduardo-vp Nov 28, 2024
8511abf
Set threadId to -1
eduardo-vp Nov 28, 2024
95ba6a8
Use m_HoldingThread in AwareLock::LeaveHelper
eduardo-vp Nov 29, 2024
9b4f078
Reset m_HoldingThreadId in AwareLock::LeaveHelper
eduardo-vp Dec 2, 2024
53ccff6
Use thread id to compare in AwareLock::LeaveHelper
eduardo-vp Dec 2, 2024
b0d2a86
PR feedback
eduardo-vp Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions src/coreclr/vm/syncblk.cpp
View file
Open in desktop
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();
eduardo-vp marked this conversation as resolved.
Show resolved Hide resolved
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();
eduardo-vp marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to remove m_HoldingThread altogether. It would probably need a change to get the Thread pointer from the thread ID in the DAC. Maybe this could be cleaned up in a separate PR, for now at least the field shouldn't be dereferenced.

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
Open in desktop
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 @@ -683,6 +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
eduardo-vp marked this conversation as resolved.
Show resolved Hide resolved
if (m_HoldingThread != pCurThread)
return AwareLock::LeaveHelperAction_Error;

Expand Down
Loading