From 93d7c4631663b4d6547d04a94688d8cfe6074246 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Tue, 25 Jun 2019 21:32:47 +0200 Subject: Fix StackFrameIterator::IsValid check race (#25359) * Fix StackFrameIterator::IsValid check race During GC stress >= 4, there could be a race when we would compute bRedirectedPinvoke as false, but before the condition of the following _ASSERTE is evaluated, the thread that is being walked pushes a ResumableFrame to the explicit frames stack of that thread in the GC marker handler. The fix to prevent this race is to evaluate all the conditions that formed the bRedirectedPinvoke after the conditions in the _ASSERTE. --- src/vm/stackwalk.cpp | 40 ++++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-) (limited to 'src/vm') diff --git a/src/vm/stackwalk.cpp b/src/vm/stackwalk.cpp index 97e83ee7d3..0eb0952648 100644 --- a/src/vm/stackwalk.cpp +++ b/src/vm/stackwalk.cpp @@ -1545,28 +1545,32 @@ BOOL StackFrameIterator::IsValid(void) // Try to ensure that the frame chain did not change underneath us. // In particular, is thread's starting frame the same as it was when // we started? - //DevDiv 168789: In GCStress >= 4 two threads could race on triggering GC; - // if the one that just made p/invoke call is second and hits the trap instruction - // before call to syncronize with GC, it will push a frame [ResumableFrame on Unix - // and RedirectedThreadFrame on Windows] concurrently with GC stackwalking. - // In normal case (no GCStress), after p/invoke, IL_STUB will check if GC is in progress and syncronize. - BOOL bRedirectedPinvoke = FALSE; + BOOL bIsRealStartFrameUnchanged = + (m_pStartFrame != NULL) || + (m_flags & POPFRAMES) || + (m_pRealStartFrame == m_pThread->GetFrame()); #ifdef FEATURE_HIJACK - bRedirectedPinvoke = ((GCStress::IsEnabled()) && - (m_pRealStartFrame != NULL) && - (m_pRealStartFrame != FRAME_TOP) && - (m_pRealStartFrame->GetVTablePtr() == InlinedCallFrame::GetMethodFrameVPtr()) && - (m_pThread->GetFrame() != NULL) && - (m_pThread->GetFrame() != FRAME_TOP) && - ((m_pThread->GetFrame()->GetVTablePtr() == ResumableFrame::GetMethodFrameVPtr()) || - (m_pThread->GetFrame()->GetVTablePtr() == RedirectedThreadFrame::GetMethodFrameVPtr()))); + // In GCStress >= 4 two threads could race on triggering GC; + // if the one that just made p/invoke call is second and hits the trap instruction + // before call to synchronize with GC, it will push a frame [ResumableFrame on Unix + // and RedirectedThreadFrame on Windows] concurrently with GC stackwalking. + // In normal case (no GCStress), after p/invoke, IL_STUB will check if GC is in progress and synchronize. + // NOTE: This condition needs to be evaluated after the previous one to prevent a subtle race condition + // (https://github.com/dotnet/coreclr/issues/21581) + bIsRealStartFrameUnchanged = bIsRealStartFrameUnchanged || + ((GCStress::IsEnabled()) && + (m_pRealStartFrame != NULL) && + (m_pRealStartFrame != FRAME_TOP) && + (m_pRealStartFrame->GetVTablePtr() == InlinedCallFrame::GetMethodFrameVPtr()) && + (m_pThread->GetFrame() != NULL) && + (m_pThread->GetFrame() != FRAME_TOP) && + ((m_pThread->GetFrame()->GetVTablePtr() == ResumableFrame::GetMethodFrameVPtr()) || + (m_pThread->GetFrame()->GetVTablePtr() == RedirectedThreadFrame::GetMethodFrameVPtr()))); #endif // FEATURE_HIJACK - _ASSERTE( (m_pStartFrame != NULL) || - (m_flags & POPFRAMES) || - (m_pRealStartFrame == m_pThread->GetFrame()) || - (bRedirectedPinvoke)); + _ASSERTE(bIsRealStartFrameUnchanged); + #endif //_DEBUG return FALSE; -- cgit v1.2.3