Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Commit 6f7bd12

Browse files
janvorlijanvorli
committed
Fix handling of embedded scopes with multiple exception holders
During the exception handling pass 1 on Unix, we only find one holder for each frame. But for the case where there are multiple scopes embedded in each other, each of them having their own exception holder, this is not correct and we need to call filters for all holders on such frame, starting from the inner-most one. This change fixes that. In addition, it fixes one usage of the EX_CATCH_CPP_ONLY in the src/vm/threads.cpp where the presence of the exception holder in the EX_TRY is not correct and causes the exception handler pass 1 to consider the managed exception handled at that place. The fix is to create a new EX_TRY_CPP_ONLY that doesn't contain the holder and use it at that place. In fact, the comment at EX_CATCH_IMPL_CPP_ONLY was suggesting that a separate EX_TRY clone would be a better solution anyways, since it eliminates one try / catch level in the EX_CATCH_IMPL_CPP_ONLY.
1 parent 018e85f commit 6f7bd12

File tree

6 files changed

+65
-59
lines changed

6 files changed

+65
-59
lines changed

‎src/inc/ex.h‎

Lines changed: 40 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -968,6 +968,8 @@ Exception *ExThrowWithInnerHelper(Exception *inner);
968968

969969
#define EX_TRY EX_TRY_CUSTOM(Exception::HandlerState, , DelegatingException /* was SEHException*/)
970970

971+
#define EX_TRY_CPP_ONLY EX_TRY_CUSTOM_CPP_ONLY(Exception::HandlerState, , DelegatingException /* was SEHException*/)
972+
971973
#ifndef INCONTRACT
972974
#ifdef ENABLE_CONTRACTS
973975
#define INCONTRACT(x) x
@@ -1031,40 +1033,44 @@ Exception *ExThrowWithInnerHelper(Exception *inner);
10311033

10321034
#define EX_CATCH_IMPL EX_CATCH_IMPL_EX(Exception)
10331035

1034-
//
1035-
// What we really need a different version of EX_TRY with one less try scope, but this
1036-
// gets us what we need for now...
1037-
//
1038-
#define EX_CATCH_IMPL_CPP_ONLY \
1039-
DEBUG_ASSURE_NO_RETURN_END(EX_TRY) \
1040-
} \
1041-
SCAN_EHMARKER_END_TRY(); \
1042-
} \
1043-
PAL_CPP_CATCH_DERIVED (Exception, __pExceptionRaw) \
1044-
{ \
1045-
SCAN_EHMARKER_CATCH(); \
1046-
__state.SetCaughtCxx(); \
1047-
__state.m_pExceptionPtr = __pExceptionRaw; \
1048-
SCAN_EHMARKER_END_CATCH(); \
1049-
SCAN_IGNORE_THROW_MARKER; \
1050-
PAL_CPP_RETHROW; \
1051-
} \
1052-
PAL_CPP_ENDTRY \
1053-
SCAN_EHMARKER_END_TRY(); \
1054-
} \
1055-
PAL_CPP_CATCH_EXCEPTION_NOARG \
1056-
{ \
1057-
SCAN_EHMARKER_CATCH(); \
1058-
VALIDATE_BACKOUT_STACK_CONSUMPTION; \
1059-
__defaultException_t __defaultException; \
1060-
CHECK::ResetAssert(); \
1061-
ExceptionHolder __pException(__state.m_pExceptionPtr); \
1062-
/* work around unreachable code warning */ \
1063-
if (true) { \
1064-
DEBUG_ASSURE_NO_RETURN_BEGIN(EX_CATCH) \
1065-
/* don't embed file names in retail to save space and avoid IP */ \
1066-
/* a findstr /n will allow you to locate it in a pinch */ \
1067-
__state.SetupCatch(INDEBUG_COMMA(__FILE__) __LINE__); \
1036+
#define EX_TRY_CUSTOM_CPP_ONLY(STATETYPE, STATEARG, DEFAULT_EXCEPTION_TYPE) \
1037+
{ \
1038+
STATETYPE __state STATEARG; \
1039+
typedef DEFAULT_EXCEPTION_TYPE __defaultException_t; \
1040+
SCAN_EHMARKER(); \
1041+
PAL_CPP_TRY \
1042+
{ \
1043+
SCAN_EHMARKER_TRY(); \
1044+
CAutoTryCleanup<STATETYPE> __autoCleanupTry(__state); \
1045+
/* prevent annotations from being dropped by optimizations in debug */ \
1046+
INDEBUG(static bool __alwayszero;) \
1047+
INDEBUG(VolatileLoad(&__alwayszero);) \
1048+
{ \
1049+
/* this is necessary for Rotor exception handling to work */ \
1050+
DEBUG_ASSURE_NO_RETURN_BEGIN(EX_TRY) \
1051+
1052+
#define EX_CATCH_IMPL_CPP_ONLY \
1053+
DEBUG_ASSURE_NO_RETURN_END(EX_TRY) \
1054+
} \
1055+
SCAN_EHMARKER_END_TRY(); \
1056+
} \
1057+
PAL_CPP_CATCH_DERIVED (Exception, __pExceptionRaw) \
1058+
{ \
1059+
SCAN_EHMARKER_CATCH(); \
1060+
__state.SetCaughtCxx(); \
1061+
__state.m_pExceptionPtr = __pExceptionRaw; \
1062+
SCAN_EHMARKER_END_CATCH(); \
1063+
SCAN_IGNORE_THROW_MARKER; \
1064+
VALIDATE_BACKOUT_STACK_CONSUMPTION; \
1065+
__defaultException_t __defaultException; \
1066+
CHECK::ResetAssert(); \
1067+
ExceptionHolder __pException(__state.m_pExceptionPtr); \
1068+
/* work around unreachable code warning */ \
1069+
if (true) { \
1070+
DEBUG_ASSURE_NO_RETURN_BEGIN(EX_CATCH) \
1071+
/* don't embed file names in retail to save space and avoid IP */ \
1072+
/* a findstr /n will allow you to locate it in a pinch */ \
1073+
__state.SetupCatch(INDEBUG_COMMA(__FILE__) __LINE__); \
10681074

10691075

10701076
// Here we finally define the EX_CATCH* macros that will be used throughout the system.

‎src/pal/inc/pal.h‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6766,11 +6766,9 @@ class NativeExceptionHolderBase : CatchHardwareExceptionHolder
67666766
// with return value optimization (in CreateHolder).
67676767
void Push();
67686768

6769-
// Given the locals stack range find the next holder starting with this one
6770-
NativeExceptionHolderBase *FindNextHolder(void *frameLowAddress, void *frameHighAddress);
6771-
6772-
// Given the locals stack range find the holder
6773-
static NativeExceptionHolderBase *FindHolder(void *frameLowAddress, void *frameHighAddress);
6769+
// Given the currentHolder and locals stack range find the next holder starting with this one
6770+
// To find the first holder, pass nullptr as the currentHolder.
6771+
static NativeExceptionHolderBase *FindNextHolder(NativeExceptionHolderBase *currentHolder, void *frameLowAddress, void *frameHighAddress);
67746772
};
67756773

67766774
//

‎src/pal/src/exception/seh.cpp‎

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -321,9 +321,9 @@ NativeExceptionHolderBase::Push()
321321
}
322322

323323
NativeExceptionHolderBase *
324-
NativeExceptionHolderBase::FindNextHolder(void *stackLowAddress, void *stackHighAddress)
324+
NativeExceptionHolderBase::FindNextHolder(NativeExceptionHolderBase *currentHolder, void *stackLowAddress, void *stackHighAddress)
325325
{
326-
NativeExceptionHolderBase *holder = this;
326+
NativeExceptionHolderBase *holder = (currentHolder == nullptr) ? t_nativeExceptionHolderHead : currentHolder->m_next;
327327

328328
while (holder != nullptr)
329329
{
@@ -338,15 +338,4 @@ NativeExceptionHolderBase::FindNextHolder(void *stackLowAddress, void *stackHigh
338338
return nullptr;
339339
}
340340

341-
NativeExceptionHolderBase *
342-
NativeExceptionHolderBase::FindHolder(void *stackLowAddress, void *stackHighAddress)
343-
{
344-
NativeExceptionHolderBase *head = t_nativeExceptionHolderHead;
345-
if (head == nullptr)
346-
{
347-
return nullptr;
348-
}
349-
return head->FindNextHolder(stackLowAddress, stackHighAddress);
350-
}
351-
352341
#include "seh-unwind.cpp"

‎src/vm/clrex.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -823,6 +823,10 @@ class EEFileLoadException : public EEException
823823
#define EX_TRY \
824824
EX_TRY_CUSTOM(CLRException::HandlerState, (::GetThreadNULLOk()), CLRLastThrownObjectException)
825825

826+
#undef EX_TRY_CPP_ONLY
827+
#define EX_TRY_CPP_ONLY \
828+
EX_TRY_CUSTOM_CPP_ONLY(CLRException::HandlerState, (::GetThreadNULLOk()), CLRLastThrownObjectException)
829+
826830
// Faster version with thread, skipping GetThread call
827831
#define EX_TRY_THREAD(pThread) \
828832
EX_TRY_CUSTOM(CLRException::HandlerState, (pThread, CLRException::HandlerState::ThreadIsNotNull), CLRLastThrownObjectException)

‎src/vm/exceptionhandling.cpp‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4633,13 +4633,22 @@ VOID DECLSPEC_NORETURN UnwindManagedExceptionPass1(PAL_SEHException& ex)
46334633

46344634
UINT_PTR parentSp = GetSP(&frameContext);
46354635

4636-
NativeExceptionHolderBase* holder = NativeExceptionHolderBase::FindHolder((void*)sp, (void*)parentSp);
4637-
if ((holder != NULL) && (holder->InvokeFilter(ex) == EXCEPTION_EXECUTE_HANDLER))
4636+
// Find all holders on this frame that are in scopes embedded in each other and call their filters.
4637+
NativeExceptionHolderBase* holder = nullptr;
4638+
while ((holder = NativeExceptionHolderBase::FindNextHolder(holder, (void*)sp, (void*)parentSp)) != nullptr)
46384639
{
4639-
// Switch to pass 2
4640-
ex.TargetFrameSp = sp;
4641-
UnwindManagedExceptionPass2(ex, &unwindStartContext);
4642-
}
4640+
EXCEPTION_DISPOSITION disposition = holder->InvokeFilter(ex);
4641+
if (disposition == EXCEPTION_EXECUTE_HANDLER)
4642+
{
4643+
// Switch to pass 2
4644+
ex.TargetFrameSp = sp;
4645+
UnwindManagedExceptionPass2(ex, &unwindStartContext);
4646+
UNREACHABLE();
4647+
}
4648+
4649+
// The EXCEPTION_CONTINUE_EXECUTION is not supported and should never be returned by a filter
4650+
_ASSERTE(disposition == EXCEPTION_CONTINUE_SEARCH);
4651+
}
46434652
}
46444653

46454654
} while (IsSpInStackLimits(GetSP(&frameContext), stackLowAddress, stackHighAddress));

‎src/vm/threads.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10105,7 +10105,7 @@ static void ManagedThreadBase_DispatchMiddle(ManagedThreadCallState *pCallState)
1010510105
// also invokes SO_INTOLERANT code.
1010610106
BEGIN_SO_INTOLERANT_CODE(GetThread());
1010710107

10108-
EX_TRY
10108+
EX_TRY_CPP_ONLY
1010910109
{
1011010110
// During an unwind, we have some cleanup:
1011110111
//

0 commit comments

Comments
 (0)