Skip to content

Commit

Permalink
Fix exception handling in the prestub worker - take 2 (#112666)
Browse files Browse the repository at this point in the history 
* Fix exception handling in the prestub worker

There is a bug in the new exception handling when ThePreStub is called
from CallDescrWorkerInternal and the exception is propagated through
that. One of such cases is when a managed class is being initialized
during JITting and the constructor is in an assembly that's not found.
The bug results in skipping all the native frames upto a managed frame
that called that native chain that lead to the exception. In the
specific case I've mentioned, a lock in the native code is left in
locked state. That later leads to a hang. This was case was observed
with Roslyn invoking an analyzer where one of the dependencies was
missing.

The fix is to ensure that when ThePreStub is called by
CallDescrWorkerInternal, the exception is not caught in the
PreStubWorker. It is left flowing into the native calling code instead.

The same treatment is applied to ExternalMethodFixupWorker and
VSD_ResolveWorker too.

On Windows, we also need to prevent the ProcessCLRException invocation
to call into the managed exception handling code.

* Fix missing ContextFlags setting

I was hitting intermittent crashes in the unhandled exception test
with GC stress C enabled due to this when GetSSP tried to access the
SSP in the extended part of the context.

* Fix couple of issues

* The previous set of changes removed popping of ExInfos too. That's not
  correct though. But it should be done in a different place than the
  ProcessCLRExceptionNew.
* There was a problem (even before the fix) that an exception caught in
  DispatchInfo::InvokeMember was reported (via console log and to the
  debugger) as unhandled.

This change also adds a new flavor of the unhandled exception test that
throws an unhandled exception on a secondary thread to exercise the
related code path.

---------

Co-authored-by: Jan Kotas <[email protected]>
  • Loading branch information
@janvorli @jkotas
janvorli and jkotas authored Feb 20, 2025
1 parent 306ced2 commit 2cb402c
Show file tree
Hide file tree
Showing 17 changed files with 318 additions and 77 deletions.
2 changes: 1 addition & 1 deletion src/coreclr/vm/dispatchinfo.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2164,7 +2164,7 @@ HRESULT DispatchInfo::InvokeMember(SimpleComCallWrapper *pSimpleWrap, DISPID id,
// The sole purpose of having this frame is to tell the debugger that we have a catch handler here
// which may swallow managed exceptions. The debugger needs this in order to send a
// CatchHandlerFound (CHF) notification.
DebuggerU2MCatchHandlerFrame catchFrame;
DebuggerU2MCatchHandlerFrame catchFrame(true /* catchesAllExceptions */);
EX_TRY
{
InvokeMemberDebuggerWrapper(pDispMemberInfo,
Expand Down
17 changes: 10 additions & 7 deletions src/coreclr/vm/excep.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3053,14 +3053,17 @@ void StackTraceInfo::AppendElement(OBJECTHANDLE hThrowable, UINT_PTR currentIP,
// This is a workaround to fix the generation of stack traces from exception objects so that
// they point to the line that actually generated the exception instead of the line
// following.
if (pCf->IsIPadjusted())
if (pCf != NULL)
{
stackTraceElem.flags |= STEF_IP_ADJUSTED;
}
else if (!pCf->HasFaulted() && stackTraceElem.ip != 0)
{
stackTraceElem.ip -= STACKWALK_CONTROLPC_ADJUST_OFFSET;
stackTraceElem.flags |= STEF_IP_ADJUSTED;
if (pCf->IsIPadjusted())
{
stackTraceElem.flags |= STEF_IP_ADJUSTED;
}
else if (!pCf->HasFaulted() && stackTraceElem.ip != 0)
{
stackTraceElem.ip -= STACKWALK_CONTROLPC_ADJUST_OFFSET;
stackTraceElem.flags |= STEF_IP_ADJUSTED;
}
}

#ifndef TARGET_UNIX // Watson is supported on Windows only
Expand Down
38 changes: 24 additions & 14 deletions src/coreclr/vm/exceptionhandling.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -932,14 +932,13 @@ ProcessCLRExceptionNew(IN PEXCEPTION_RECORD pExceptionRecord,

Thread* pThread = GetThread();

if (pThread->HasThreadStateNC(Thread::TSNC_ProcessedUnhandledException))
// Skip native frames of asm helpers that have the ProcessCLRException set as their personality routine.
// There is nothing to do for those with the new exception handling.
// Also skip all frames when processing unhandled exceptions. That allows them to reach the host app
// level and let 3rd party the chance to handle them.
if (!ExecutionManager::IsManagedCode((PCODE)pDispatcherContext->ControlPc) ||
pThread->HasThreadStateNC(Thread::TSNC_ProcessedUnhandledException))
{
if ((pExceptionRecord->ExceptionFlags & EXCEPTION_UNWINDING))
{
GCX_COOP();
PopExplicitFrames(pThread, (void*)pDispatcherContext->EstablisherFrame, (void*)GetSP(pDispatcherContext->ContextRecord));
ExInfo::PopExInfos(pThread, (void*)pDispatcherContext->EstablisherFrame);
}
return ExceptionContinueSearch;
}

Expand Down Expand Up @@ -6147,6 +6146,7 @@ BOOL IsSafeToUnwindFrameChain(Thread* pThread, LPVOID MemoryStackFpForFrameChain
// Otherwise "unwind" to managed method
REGDISPLAY rd;
CONTEXT ctx;
ctx.ContextFlags = CONTEXT_CONTROL;
SetIP(&ctx, 0);
SetSP(&ctx, 0);
FillRegDisplay(&rd, &ctx);
Expand Down Expand Up @@ -6192,9 +6192,16 @@ void CleanUpForSecondPass(Thread* pThread, bool fIsSO, LPVOID MemoryStackFpForFr
// Instead, we rely on the END_SO_TOLERANT_CODE macro to call ClearExceptionStateAfterSO(). Of course,
// we may leak in the UMThunkStubCommon() case where we don't have this macro lower on the stack
// (stack grows up).
if (!fIsSO && !g_isNewExceptionHandlingEnabled)
if (!fIsSO)
{
ExceptionTracker::PopTrackerIfEscaping(MemoryStackFp);
if (g_isNewExceptionHandlingEnabled)
{
ExInfo::PopExInfos(pThread, MemoryStackFp);
}
else
{
ExceptionTracker::PopTrackerIfEscaping(MemoryStackFp);
}
}
}

Expand Down Expand Up @@ -8522,7 +8529,7 @@ static StackWalkAction MoveToNextNonSkippedFrame(StackFrameIterator* pStackFrame
return retVal;
}

extern "C" size_t CallDescrWorkerInternalReturnAddressOffset;
bool IsCallDescrWorkerInternalReturnAddress(PCODE pCode);

extern "C" bool QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollideClauseIdx, bool* fUnwoundReversePInvoke, bool* pfIsExceptionIntercepted)
{
Expand Down Expand Up @@ -8577,8 +8584,7 @@ extern "C" bool QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollideCla
}
else
{
size_t CallDescrWorkerInternalReturnAddress = (size_t)CallDescrWorkerInternal + CallDescrWorkerInternalReturnAddressOffset;
if (GetIP(pThis->m_crawl.GetRegisterSet()->pCallerContext) == CallDescrWorkerInternalReturnAddress)
if (IsCallDescrWorkerInternalReturnAddress(GetIP(pThis->m_crawl.GetRegisterSet()->pCallerContext)))
{
invalidRevPInvoke = true;
}
Expand All @@ -8601,8 +8607,12 @@ extern "C" bool QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollideCla
_ASSERTE(pThis->GetFrameState() != StackFrameIterator::SFITER_SKIPPED_FRAME_FUNCTION);

pFrame = pThis->m_crawl.GetFrame();
// Check if there are any further managed frames on the stack, if not, the exception is unhandled.
if ((pFrame == FRAME_TOP) || IsTopmostDebuggerU2MCatchHandlerFrame(pFrame))

// Check if there are any further managed frames on the stack or a catch for all exceptions in native code (marked by
// DebuggerU2MCatchHandlerFrame with CatchesAllExceptions() returning true).
// If not, the exception is unhandled.
if ((pFrame == FRAME_TOP) ||
(IsTopmostDebuggerU2MCatchHandlerFrame(pFrame) && !((DebuggerU2MCatchHandlerFrame*)pFrame)->CatchesAllExceptions()))
{
if (pTopExInfo->m_passNumber == 1)
{
Expand Down
16 changes: 14 additions & 2 deletions src/coreclr/vm/exceptmacros.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -277,15 +277,22 @@ VOID DECLSPEC_NORETURN UnwindAndContinueRethrowHelperAfterCatch(Frame* pEntryFra
#ifdef TARGET_UNIX
VOID DECLSPEC_NORETURN DispatchManagedException(PAL_SEHException& ex, bool isHardwareException);

#define INSTALL_MANAGED_EXCEPTION_DISPATCHER \
#define INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX \
PAL_SEHException exCopy; \
bool hasCaughtException = false; \
try {

#define UNINSTALL_MANAGED_EXCEPTION_DISPATCHER \
#define INSTALL_MANAGED_EXCEPTION_DISPATCHER \
INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX

#define UNINSTALL_MANAGED_EXCEPTION_DISPATCHER_EX(nativeRethrow) \
} \
catch (PAL_SEHException& ex) \
{ \
if (nativeRethrow) \
{ \
throw; \
} \
exCopy = std::move(ex); \
hasCaughtException = true; \
} \
Expand All @@ -294,6 +301,9 @@ VOID DECLSPEC_NORETURN DispatchManagedException(PAL_SEHException& ex, bool isHar
DispatchManagedException(exCopy, false);\
}

#define UNINSTALL_MANAGED_EXCEPTION_DISPATCHER \
UNINSTALL_MANAGED_EXCEPTION_DISPATCHER_EX(false)

// Install trap that catches unhandled managed exception and dumps its stack
#define INSTALL_UNHANDLED_MANAGED_EXCEPTION_TRAP \
try {
Expand All @@ -315,7 +325,9 @@ VOID DECLSPEC_NORETURN DispatchManagedException(PAL_SEHException& ex, bool isHar
#else // TARGET_UNIX

#define INSTALL_MANAGED_EXCEPTION_DISPATCHER
#define INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX
#define UNINSTALL_MANAGED_EXCEPTION_DISPATCHER
#define UNINSTALL_MANAGED_EXCEPTION_DISPATCHER_EX

#define INSTALL_UNHANDLED_MANAGED_EXCEPTION_TRAP
#define UNINSTALL_UNHANDLED_MANAGED_EXCEPTION_TRAP
Expand Down
16 changes: 14 additions & 2 deletions src/coreclr/vm/frames.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2497,13 +2497,15 @@ class DebuggerU2MCatchHandlerFrame : public Frame
{
public:
#ifndef DACCESS_COMPILE
DebuggerU2MCatchHandlerFrame() : Frame(FrameIdentifier::DebuggerU2MCatchHandlerFrame)
DebuggerU2MCatchHandlerFrame(bool catchesAllExceptions) : Frame(FrameIdentifier::DebuggerU2MCatchHandlerFrame),
m_catchesAllExceptions(catchesAllExceptions)
{
WRAPPER_NO_CONTRACT;
Frame::Push();
}

DebuggerU2MCatchHandlerFrame(Thread * pThread) : Frame(FrameIdentifier::DebuggerU2MCatchHandlerFrame)
DebuggerU2MCatchHandlerFrame(Thread * pThread, bool catchesAllExceptions) : Frame(FrameIdentifier::DebuggerU2MCatchHandlerFrame),
m_catchesAllExceptions(catchesAllExceptions)
{
WRAPPER_NO_CONTRACT;
Frame::Push(pThread);
Expand All @@ -2515,6 +2517,16 @@ class DebuggerU2MCatchHandlerFrame : public Frame
LIMITED_METHOD_DAC_CONTRACT;
return TT_U2M;
}

bool CatchesAllExceptions()
{
LIMITED_METHOD_DAC_CONTRACT;
return m_catchesAllExceptions;
}

private:
// The catch handled marked by the DebuggerU2MCatchHandlerFrame catches all exceptions.
bool m_catchesAllExceptions;
};

// Frame for the Reverse PInvoke (i.e. UnmanagedCallersOnlyAttribute).
Expand Down
114 changes: 73 additions & 41 deletions src/coreclr/vm/prestub.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2538,6 +2538,20 @@ Stub * MakeInstantiatingStubWorker(MethodDesc *pMD)
}
#endif // defined(FEATURE_SHARE_GENERIC_CODE)

extern "C" size_t CallDescrWorkerInternalReturnAddressOffset;

bool IsCallDescrWorkerInternalReturnAddress(PCODE pCode)
{
LIMITED_METHOD_CONTRACT;
#ifdef FEATURE_EH_FUNCLETS
size_t CallDescrWorkerInternalReturnAddress = (size_t)CallDescrWorkerInternal + CallDescrWorkerInternalReturnAddressOffset;

return pCode == CallDescrWorkerInternalReturnAddress;
#else // FEATURE_EH_FUNCLETS
return false;
#endif // FEATURE_EH_FUNCLETS
}

//=============================================================================
// This function generates the real code when from Preemptive mode.
// It is specifically designed to work with the UnmanagedCallersOnlyAttribute.
Expand Down Expand Up @@ -2633,60 +2647,76 @@ extern "C" PCODE STDCALL PreStubWorker(TransitionBlock* pTransitionBlock, Method

pPFrame->Push(CURRENT_THREAD);

INSTALL_MANAGED_EXCEPTION_DISPATCHER;
INSTALL_UNWIND_AND_CONTINUE_HANDLER;
EX_TRY
{
bool propagateExceptionToNativeCode = IsCallDescrWorkerInternalReturnAddress(pTransitionBlock->m_ReturnAddress);

// Make sure the method table is restored, and method instantiation if present
pMD->CheckRestore();
CONSISTENCY_CHECK(GetAppDomain()->CheckCanExecuteManagedCode(pMD));
INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX;
INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX;

MethodTable* pDispatchingMT = NULL;
if (pMD->IsVtableMethod())
{
OBJECTREF curobj = pPFrame->GetThis();
// Make sure the method table is restored, and method instantiation if present
pMD->CheckRestore();
CONSISTENCY_CHECK(GetAppDomain()->CheckCanExecuteManagedCode(pMD));

if (curobj != NULL) // Check for virtual function called non-virtually on a NULL object
MethodTable* pDispatchingMT = NULL;
if (pMD->IsVtableMethod())
{
pDispatchingMT = curobj->GetMethodTable();
OBJECTREF curobj = pPFrame->GetThis();

if (pDispatchingMT->IsIDynamicInterfaceCastable())
if (curobj != NULL) // Check for virtual function called non-virtually on a NULL object
{
MethodTable* pMDMT = pMD->GetMethodTable();
TypeHandle objectType(pDispatchingMT);
TypeHandle methodType(pMDMT);
pDispatchingMT = curobj->GetMethodTable();

GCStress<cfg_any>::MaybeTrigger();
INDEBUG(curobj = NULL); // curobj is unprotected and CanCastTo() can trigger GC
if (!objectType.CanCastTo(methodType))
if (pDispatchingMT->IsIDynamicInterfaceCastable())
{
// Apparently IDynamicInterfaceCastable magic was involved when we chose this method to be called
// that's why we better stick to the MethodTable it belongs to, otherwise
// DoPrestub() will fail not being able to find implementation for pMD in pDispatchingMT.
MethodTable* pMDMT = pMD->GetMethodTable();
TypeHandle objectType(pDispatchingMT);
TypeHandle methodType(pMDMT);

GCStress<cfg_any>::MaybeTrigger();
INDEBUG(curobj = NULL); // curobj is unprotected and CanCastTo() can trigger GC
if (!objectType.CanCastTo(methodType))
{
// Apparently IDynamicInterfaceCastable magic was involved when we chose this method to be called
// that's why we better stick to the MethodTable it belongs to, otherwise
// DoPrestub() will fail not being able to find implementation for pMD in pDispatchingMT.

pDispatchingMT = pMDMT;
pDispatchingMT = pMDMT;
}
}
}

// For value types, the only virtual methods are interface implementations.
// Thus pDispatching == pMT because there
// is no inheritance in value types. Note the BoxedEntryPointStubs are shared
// between all sharable generic instantiations, so the == test is on
// canonical method tables.
// For value types, the only virtual methods are interface implementations.
// Thus pDispatching == pMT because there
// is no inheritance in value types. Note the BoxedEntryPointStubs are shared
// between all sharable generic instantiations, so the == test is on
// canonical method tables.
#ifdef _DEBUG
MethodTable* pMDMT = pMD->GetMethodTable(); // put this here to see what the MT is in debug mode
_ASSERTE(!pMD->GetMethodTable()->IsValueType() ||
(pMD->IsUnboxingStub() && (pDispatchingMT->GetCanonicalMethodTable() == pMDMT->GetCanonicalMethodTable())));
MethodTable* pMDMT = pMD->GetMethodTable(); // put this here to see what the MT is in debug mode
_ASSERTE(!pMD->GetMethodTable()->IsValueType() ||
(pMD->IsUnboxingStub() && (pDispatchingMT->GetCanonicalMethodTable() == pMDMT->GetCanonicalMethodTable())));
#endif // _DEBUG
}
}
}

GCX_PREEMP_THREAD_EXISTS(CURRENT_THREAD);
GCX_PREEMP_THREAD_EXISTS(CURRENT_THREAD);
{
pbRetVal = pMD->DoPrestub(pDispatchingMT, CallerGCMode::Coop);
}

UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_EX(propagateExceptionToNativeCode);
UNINSTALL_MANAGED_EXCEPTION_DISPATCHER_EX(propagateExceptionToNativeCode);
}
EX_CATCH
{
pbRetVal = pMD->DoPrestub(pDispatchingMT, CallerGCMode::Coop);
if (g_isNewExceptionHandlingEnabled)
{
OBJECTHANDLE ohThrowable = CURRENT_THREAD->LastThrownObjectHandle();
_ASSERTE(ohThrowable);
StackTraceInfo::AppendElement(ohThrowable, 0, (UINT_PTR)pTransitionBlock, pMD, NULL);
}
EX_RETHROW;
}

UNINSTALL_UNWIND_AND_CONTINUE_HANDLER;
UNINSTALL_MANAGED_EXCEPTION_DISPATCHER;
EX_END_CATCH(SwallowAllExceptions)

{
HardwareExceptionHolder;
Expand Down Expand Up @@ -3156,8 +3186,10 @@ EXTERN_C PCODE STDCALL ExternalMethodFixupWorker(TransitionBlock * pTransitionBl

pEMFrame->Push(CURRENT_THREAD); // Push the new ExternalMethodFrame onto the frame stack

INSTALL_MANAGED_EXCEPTION_DISPATCHER;
INSTALL_UNWIND_AND_CONTINUE_HANDLER;
bool propagateExceptionToNativeCode = IsCallDescrWorkerInternalReturnAddress(pTransitionBlock->m_ReturnAddress);

INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX;
INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX;

bool fVirtual = false;
MethodDesc * pMD = NULL;
Expand Down Expand Up @@ -3399,8 +3431,8 @@ EXTERN_C PCODE STDCALL ExternalMethodFixupWorker(TransitionBlock * pTransitionBl
}
// Ready to return

UNINSTALL_UNWIND_AND_CONTINUE_HANDLER;
UNINSTALL_MANAGED_EXCEPTION_DISPATCHER;
UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_EX(propagateExceptionToNativeCode);
UNINSTALL_MANAGED_EXCEPTION_DISPATCHER_EX(propagateExceptionToNativeCode);

pEMFrame->Pop(CURRENT_THREAD); // Pop the ExternalMethodFrame from the frame stack

Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/vm/threads.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7206,7 +7206,7 @@ static void ManagedThreadBase_DispatchOuter(ManagedThreadCallState *pCallState)
// The sole purpose of having this frame is to tell the debugger that we have a catch handler here
// which may swallow managed exceptions. The debugger needs this in order to send a
// CatchHandlerFound (CHF) notification.
DebuggerU2MCatchHandlerFrame catchFrame;
DebuggerU2MCatchHandlerFrame catchFrame(false /* catchesAllExceptions */);

TryParam param(pCallState);
param.pFrame = &catchFrame;
Expand Down
Loading

0 comments on commit 2cb402c

Please sign in to comment.