From 0f9a15635600ecd8fa74f718d0c8cabdd8334fb7 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Mon, 23 Sep 2019 17:13:20 +0200 Subject: [PATCH] Fix issue with locals overlapping out of scope GCFrame (dotnet/coreclr#26763) * Fix issue with locals overlapping out of scope GCFrame More aggressive C/C++ optimizations done by VS2019 are breaking fragile assumptions of the CoreCLR "manually managed code". Unwinding of Frame chains accesses stack local variables after the stack frame has been unwound, but it depends on their content to be left intact. The new compiler is breaking this assumption by stack-packing a different variable over it. This change fixes the problem by adding a destructor to GCFrame that pops the frame from the per-thread Frame list. I also had to refactor two functions where the compiler was complaining about mixing SEH and C++ EH in single function. With these changes applied, there was still a problem that I've discovered when running CoreCLR tests with GCStress 3. When the ExceptionTracker::m_pInitialExplicitFrame was still pointing to a frame that was already removed from the explicit Frame chain. When the ExceptionTracker::HasFrameBeenUnwoundByAnyActiveException was walking the frame chain starting at m_pInitialExplicitFrame to figure out if a given frame was already unwound, it has walked to the destroyed GCFrame and crashed. To fix that, the chain from the initial explicit frame is updated so that it stays valid (effectively, the destroyed GCFrame is removed from it). It was also necessary to handle the case when the destroyed GCFrame was the ExceptionTracker::m_pLimitFrame. The limit frame needs to be updated to the next frame so that it can be reached on the chain from the initial explicit frame. * Reflect PR feedback Change the switching to coop mode in ~GCFrame from holder based to manual. Commit migrated from https://github.com/dotnet/coreclr/commit/6059e75e13593b0820e178f8baaace32c09aca6e --- src/coreclr/src/vm/assembly.cpp | 110 ++++++++++++----------- src/coreclr/src/vm/exceptionhandling.cpp | 7 ++ src/coreclr/src/vm/exceptionhandling.h | 2 + src/coreclr/src/vm/frames.cpp | 91 +++++++++++++++++++ src/coreclr/src/vm/frames.h | 16 +++- src/coreclr/src/vm/syncblk.cpp | 62 ++++++------- 6 files changed, 204 insertions(+), 84 deletions(-) diff --git a/src/coreclr/src/vm/assembly.cpp b/src/coreclr/src/vm/assembly.cpp index 0187dfd36b6..ede93721105 100644 --- a/src/coreclr/src/vm/assembly.cpp +++ b/src/coreclr/src/vm/assembly.cpp @@ -1462,6 +1462,62 @@ void ValidateMainMethod(MethodDesc * pFD, CorEntryPointType *pType) } } +struct Param +{ + MethodDesc *pFD; + short numSkipArgs; + INT32 *piRetVal; + PTRARRAYREF *stringArgs; + CorEntryPointType EntryType; + DWORD cCommandArgs; + LPWSTR *wzArgs; +} param; + +static void RunMainInternal(Param* pParam) +{ + MethodDescCallSite threadStart(pParam->pFD); + + PTRARRAYREF StrArgArray = NULL; + GCPROTECT_BEGIN(StrArgArray); + + // Build the parameter array and invoke the method. + if (pParam->EntryType == EntryManagedMain) { + if (pParam->stringArgs == NULL) { + // Allocate a COM Array object with enough slots for cCommandArgs - 1 + StrArgArray = (PTRARRAYREF) AllocateObjectArray((pParam->cCommandArgs - pParam->numSkipArgs), g_pStringClass); + + // Create Stringrefs for each of the args + for (DWORD arg = pParam->numSkipArgs; arg < pParam->cCommandArgs; arg++) { + STRINGREF sref = StringObject::NewString(pParam->wzArgs[arg]); + StrArgArray->SetAt(arg - pParam->numSkipArgs, (OBJECTREF) sref); + } + } + else + StrArgArray = *pParam->stringArgs; + } + + ARG_SLOT stackVar = ObjToArgSlot(StrArgArray); + + if (pParam->pFD->IsVoid()) + { + // Set the return value to 0 instead of returning random junk + *pParam->piRetVal = 0; + threadStart.Call(&stackVar); + } + else + { + *pParam->piRetVal = (INT32)threadStart.Call_RetArgSlot(&stackVar); + SetLatchedExitCode(*pParam->piRetVal); + } + + GCPROTECT_END(); + + // + // When we get mainCRTStartup from the C++ then this should be able to go away. + fflush(stdout); + fflush(stderr); +} + /* static */ HRESULT RunMain(MethodDesc *pFD , short numSkipArgs, @@ -1506,16 +1562,8 @@ HRESULT RunMain(MethodDesc *pFD , ETWFireEvent(Main_V1); - struct Param - { - MethodDesc *pFD; - short numSkipArgs; - INT32 *piRetVal; - PTRARRAYREF *stringArgs; - CorEntryPointType EntryType; - DWORD cCommandArgs; - LPWSTR *wzArgs; - } param; + Param param; + param.pFD = pFD; param.numSkipArgs = numSkipArgs; param.piRetVal = piRetVal; @@ -1526,47 +1574,7 @@ HRESULT RunMain(MethodDesc *pFD , EX_TRY_NOCATCH(Param *, pParam, ¶m) { - MethodDescCallSite threadStart(pParam->pFD); - - PTRARRAYREF StrArgArray = NULL; - GCPROTECT_BEGIN(StrArgArray); - - // Build the parameter array and invoke the method. - if (pParam->EntryType == EntryManagedMain) { - if (pParam->stringArgs == NULL) { - // Allocate a COM Array object with enough slots for cCommandArgs - 1 - StrArgArray = (PTRARRAYREF) AllocateObjectArray((pParam->cCommandArgs - pParam->numSkipArgs), g_pStringClass); - - // Create Stringrefs for each of the args - for (DWORD arg = pParam->numSkipArgs; arg < pParam->cCommandArgs; arg++) { - STRINGREF sref = StringObject::NewString(pParam->wzArgs[arg]); - StrArgArray->SetAt(arg - pParam->numSkipArgs, (OBJECTREF) sref); - } - } - else - StrArgArray = *pParam->stringArgs; - } - - ARG_SLOT stackVar = ObjToArgSlot(StrArgArray); - - if (pParam->pFD->IsVoid()) - { - // Set the return value to 0 instead of returning random junk - *pParam->piRetVal = 0; - threadStart.Call(&stackVar); - } - else - { - *pParam->piRetVal = (INT32)threadStart.Call_RetArgSlot(&stackVar); - SetLatchedExitCode(*pParam->piRetVal); - } - - GCPROTECT_END(); - - // - // When we get mainCRTStartup from the C++ then this should be able to go away. - fflush(stdout); - fflush(stderr); + RunMainInternal(pParam); } EX_END_NOCATCH diff --git a/src/coreclr/src/vm/exceptionhandling.cpp b/src/coreclr/src/vm/exceptionhandling.cpp index cd95a791897..48984c73f8e 100644 --- a/src/coreclr/src/vm/exceptionhandling.cpp +++ b/src/coreclr/src/vm/exceptionhandling.cpp @@ -4020,6 +4020,13 @@ void ExceptionTracker::ResetLimitFrame() m_pLimitFrame = m_pThread->GetFrame(); } +void ExceptionTracker::ResetInitialExplicitFrame() +{ + LIMITED_METHOD_CONTRACT; + + m_pInitialExplicitFrame = m_pThread->GetFrame(); +} + // // static void ExceptionTracker::ResumeExecution( diff --git a/src/coreclr/src/vm/exceptionhandling.h b/src/coreclr/src/vm/exceptionhandling.h index 3cdea906a02..c99f4db52f5 100644 --- a/src/coreclr/src/vm/exceptionhandling.h +++ b/src/coreclr/src/vm/exceptionhandling.h @@ -299,6 +299,8 @@ public: return m_pInitialExplicitFrame; } + void ResetInitialExplicitFrame(); + #ifdef FEATURE_PAL // Reset the range of explicit frames, the limit frame and the scanned // stack range before unwinding a sequence of native frames. These frames diff --git a/src/coreclr/src/vm/frames.cpp b/src/coreclr/src/vm/frames.cpp index bb968f25ee9..ebb16bf840c 100644 --- a/src/coreclr/src/vm/frames.cpp +++ b/src/coreclr/src/vm/frames.cpp @@ -1001,6 +1001,97 @@ VOID GCFrame::Pop() #endif } +#ifndef CROSSGEN_COMPILE +// GCFrame destructor removes the GCFrame from the current thread's explicit frame list. +// This prevents issues in functions that have HELPER_METHOD_FRAME_BEGIN / END around +// GCPROTECT_BEGIN / END and where the C++ compiler places some local variables over +// the stack location of the GCFrame local variable after the variable goes out of scope. +GCFrame::~GCFrame() +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + MODE_ANY; + } + CONTRACTL_END; + + if (m_Next != NULL) + { + // Do a manual switch to the GC cooperative mode instead of using the GCX_COOP_THREAD_EXISTS + // macro so that this function isn't slowed down by having to deal with FS:0 chain on x86 Windows. + BOOL wasCoop = m_pCurThread->PreemptiveGCDisabled(); + if (!wasCoop) + { + m_pCurThread->DisablePreemptiveGC(); + } + + // When the frame is destroyed, make sure it is no longer in the + // frame chain managed by the Thread. + + Pop(); + +#if defined(FEATURE_EH_FUNCLETS) && !defined(FEATURE_PAL) + PTR_ExceptionTracker pCurrentTracker = m_pCurThread->GetExceptionState()->GetCurrentExceptionTracker(); + if (pCurrentTracker != NULL) + { + if (pCurrentTracker->GetLimitFrame() == this) + { + // The current frame that was just popped was the EH limit frame. We need to reset the EH limit frame + // to the current frame so that it stays on the frame chain from initial explicit frame. + // The ExceptionTracker::HasFrameBeenUnwoundByAnyActiveException needs that to correctly detect + // frames that were unwound. + pCurrentTracker->ResetLimitFrame(); + } + + PTR_Frame frame = pCurrentTracker->GetInitialExplicitFrame(); + if (frame != NULL) + { + while ((frame != FRAME_TOP) && (frame != this)) + { + PTR_Frame nextFrame = frame->PtrNextFrame(); + if (nextFrame == this) + { + // Repair frame chain from the initial explicit frame to the current frame, + // skipping the current GCFrame that was destroyed + frame->m_Next = m_pCurThread->m_pFrame; + break; + } + frame = nextFrame; + } + } + } +#endif // FEATURE_EH_FUNCLETS && !FEATURE_PAL + + if (!wasCoop) + { + m_pCurThread->EnablePreemptiveGC(); + } + + } +} + +ExceptionFilterFrame::~ExceptionFilterFrame() +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + MODE_ANY; + } + CONTRACTL_END; + + if (m_Next != NULL) + { + GCX_COOP(); + // When the frame is destroyed, make sure it is no longer in the + // frame chain managed by the Thread. + Pop(); + } +} + +#endif // !CROSSGEN_COMPILE + #ifdef FEATURE_INTERPRETER // Methods of IntepreterFrame. InterpreterFrame::InterpreterFrame(Interpreter* interp) diff --git a/src/coreclr/src/vm/frames.h b/src/coreclr/src/vm/frames.h index 7c614407cca..90944ca1e28 100644 --- a/src/coreclr/src/vm/frames.h +++ b/src/coreclr/src/vm/frames.h @@ -409,6 +409,7 @@ public: class Frame : public FrameBase { friend class CheckAsmOffsets; + friend class GCFrame; #ifdef DACCESS_COMPILE friend void Thread::EnumMemoryRegions(CLRDataEnumMemoryFlags flags); #endif @@ -2530,7 +2531,11 @@ private: BOOL m_MaybeInterior; // Keep as last entry in class - DEFINE_VTABLE_GETTER_AND_DTOR(GCFrame) + DEFINE_VTABLE_GETTER(GCFrame) + +#if !defined(DACCESS_COMPILE) && !defined(CROSSGEN_COMPILE) + ~GCFrame(); +#endif }; #ifdef FEATURE_INTERPRETER @@ -3260,11 +3265,16 @@ public: *m_pShadowSP |= ICodeManager::SHADOW_SP_FILTER_DONE; } } + +#ifndef CROSSGEN_COMPILE + ~ExceptionFilterFrame(); +#endif + #endif private: // Keep as last entry in class - DEFINE_VTABLE_GETTER_AND_CTOR_AND_DTOR(ExceptionFilterFrame) + DEFINE_VTABLE_GETTER_AND_CTOR(ExceptionFilterFrame) }; #ifdef _DEBUG @@ -3582,7 +3592,7 @@ public: #define GCPROTECT_END() \ DEBUG_ASSURE_NO_RETURN_END(GCPROTECT) } \ - __gcframe.Pop(); } while(0) + } while(0) #else // #ifndef DACCESS_COMPILE diff --git a/src/coreclr/src/vm/syncblk.cpp b/src/coreclr/src/vm/syncblk.cpp index 58511554a24..a1941ce6c5f 100644 --- a/src/coreclr/src/vm/syncblk.cpp +++ b/src/coreclr/src/vm/syncblk.cpp @@ -2613,50 +2613,52 @@ BOOL AwareLock::EnterEpilogHelper(Thread* pCurThread, INT32 timeOut) for (;;) { - // We might be interrupted during the wait (Thread.Interrupt), so we need an - // exception handler round the call. - struct Param - { - AwareLock *pThis; - INT32 timeOut; - DWORD ret; - } param; - param.pThis = this; - param.timeOut = timeOut; - // Measure the time we wait so that, in the case where we wake up // and fail to acquire the mutex, we can adjust remaining timeout // accordingly. ULONGLONG start = CLRGetTickCount64(); - EE_TRY_FOR_FINALLY(Param *, pParam, ¶m) - { - pParam->ret = pParam->pThis->m_SemEvent.Wait(pParam->timeOut, TRUE); - _ASSERTE((pParam->ret == WAIT_OBJECT_0) || (pParam->ret == WAIT_TIMEOUT)); - } - EE_FINALLY + // It is likely the case that An APC threw an exception, for instance Thread.Interrupt(). The wait subsystem + // guarantees that if a signal to the event being waited upon is observed by the woken thread, that thread's + // wait will return WAIT_OBJECT_0. So in any race between m_SemEvent being signaled and the wait throwing an + // exception, a thread that is woken by an exception would not observe the signal, and the signal would wake + // another thread as necessary. + + // We must decrement the waiter count in the case an exception happened. This holder takes care of that + class UnregisterWaiterHolder { - if (GOT_EXCEPTION()) + LockState* m_pLockState; + public: + UnregisterWaiterHolder(LockState* pLockState) : m_pLockState(pLockState) { - // It is likely the case that An APC threw an exception, for instance Thread.Interrupt(). The wait subsystem - // guarantees that if a signal to the event being waited upon is observed by the woken thread, that thread's - // wait will return WAIT_OBJECT_0. So in any race between m_SemEvent being signaled and the wait throwing an - // exception, a thread that is woken by an exception would not observe the signal, and the signal would wake - // another thread as necessary. - - // We must decrement the waiter count. - m_lockState.InterlockedUnregisterWaiter(); } - } EE_END_FINALLY; - ret = param.ret; + ~UnregisterWaiterHolder() + { + if (m_pLockState != NULL) + { + m_pLockState->InterlockedUnregisterWaiter(); + } + } + + void SuppressRelease() + { + m_pLockState = NULL; + } + } unregisterWaiterHolder(&m_lockState); + + ret = m_SemEvent.Wait(timeOut, TRUE); + _ASSERTE((ret == WAIT_OBJECT_0) || (ret == WAIT_TIMEOUT)); + if (ret != WAIT_OBJECT_0) { - // We timed out, decrement waiter count. - m_lockState.InterlockedUnregisterWaiter(); + // We timed out + // (the holder unregisters the waiter here) break; } + unregisterWaiterHolder.SuppressRelease(); + // Spin a bit while trying to acquire the lock. This has a few benefits: // - Spinning helps to reduce waiter starvation. Since other non-waiter threads can take the lock while there are // waiters (see LockState::InterlockedTryLock()), once a waiter wakes it will be able to better compete -- 2.34.1