From c132a3fc5a52b324740bd853c09636a1774a1cad Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 18 Jul 2023 16:56:04 -0700 Subject: [PATCH 01/20] Include the official asan interface header when using ASAN --- eng/native/configurecompiler.cmake | 21 +++++++++++++++++---- src/native/minipal/utils.h | 9 +-------- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/eng/native/configurecompiler.cmake b/eng/native/configurecompiler.cmake index bb734c59d5557..483456e882928 100644 --- a/eng/native/configurecompiler.cmake +++ b/eng/native/configurecompiler.cmake @@ -140,6 +140,20 @@ elseif (CLR_CMAKE_HOST_UNIX) endif(MSVC) if (CLR_CMAKE_ENABLE_SANITIZERS) + enable_language(C) + if (CMAKE_C_COMPILER_ID MATCHES "Clang") + execute_process( + COMMAND ${CMAKE_C_COMPILER} -print-resource-dir + OUTPUT_VARIABLE compilerResourceDir + OUTPUT_STRIP_TRAILING_WHITESPACE) + include_directories(SYSTEM ${compilerResourceDir}/include) + + # Work around https://gitlab.kitware.com/cmake/cmake/-/issues/19227 since CoreCLR passes -nostdinc + # and CMake will filter out the resource dir if -nostdinc is passed. + list(REMOVE_ITEM CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES "${compilerResourceDir}/include") + list(REMOVE_ITEM CMAKE_C_IMPLICIT_INCLUDE_DIRECTORIES "${compilerResourceDir}/include") + endif() + set (CLR_CMAKE_BUILD_SANITIZERS "") set (CLR_CMAKE_SANITIZER_RUNTIMES "") string(FIND "${CLR_CMAKE_ENABLE_SANITIZERS}" "address" __ASAN_POS) @@ -157,12 +171,11 @@ if (CLR_CMAKE_ENABLE_SANITIZERS) # The rest of our platforms use statically-linked ASAN so this isn't a concern for those platforms. if (CLR_CMAKE_TARGET_OSX OR CLR_CMAKE_TARGET_MACCATALYST) function(getSanitizerRuntimeDirectory output) - enable_language(C) execute_process( - COMMAND ${CMAKE_C_COMPILER} -print-resource-dir - OUTPUT_VARIABLE compilerResourceDir + COMMAND ${CMAKE_C_COMPILER} -print-runtime-dir + OUTPUT_VARIABLE compilerRuntimeDir OUTPUT_STRIP_TRAILING_WHITESPACE) - set(${output} "${compilerResourceDir}/lib/darwin/" PARENT_SCOPE) + set(${output} "${compilerResourceDir}" PARENT_SCOPE) endfunction() getSanitizerRuntimeDirectory(sanitizerRuntimeDirectory) find_library(ASAN_RUNTIME clang_rt.asan_osx_dynamic PATHS ${sanitizerRuntimeDirectory}) diff --git a/src/native/minipal/utils.h b/src/native/minipal/utils.h index 9dcb4a8b6620c..30754bfcb75ad 100644 --- a/src/native/minipal/utils.h +++ b/src/native/minipal/utils.h @@ -45,14 +45,7 @@ #endif #if defined(HAS_ADDRESS_SANITIZER) -# ifdef __cplusplus - extern "C" - { -# endif - void SANITIZER_INTERFACE_CALLCONV __asan_handle_no_return(void); -# ifdef __cplusplus - } -# endif +#include #elif defined(__llvm__) # pragma clang diagnostic push # ifdef COMPILER_SUPPORTS_W_RESERVED_IDENTIFIER From e1cfdb7aa4c9aabaee92b046408c56eded9cee77 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 19 Jul 2023 11:51:24 -0700 Subject: [PATCH 02/20] Enable use-after-return checking in ASAN Fixes #89133 --- eng/native/configurecompiler.cmake | 12 ++++---- src/coreclr/vm/stackwalk.cpp | 16 +++++------ src/coreclr/vm/threads.cpp | 22 ++++++++++++-- src/coreclr/vm/threads.h | 46 ++++++++++++++++++++++++++---- 4 files changed, 74 insertions(+), 22 deletions(-) diff --git a/eng/native/configurecompiler.cmake b/eng/native/configurecompiler.cmake index 483456e882928..9379d055ebe85 100644 --- a/eng/native/configurecompiler.cmake +++ b/eng/native/configurecompiler.cmake @@ -208,11 +208,13 @@ if (CLR_CMAKE_ENABLE_SANITIZERS) # define the preprocessor define ourselves. add_compile_definitions($<$:HAS_ADDRESS_SANITIZER>) - # Disable the use-after-return check for ASAN on Clang. This is because we have a lot of code that - # depends on the fact that our locals are not saved in a parallel stack, so we can't enable this today. - # If we ever have a way to detect a parallel stack and track its bounds, we can re-enable this check. - add_compile_options($<$:-fsanitize-address-use-after-return=never>) - add_compile_options($<$:-fsanitize-address-use-after-return=never>) + # Disable the use-after-return check for ASAN on release builds. We track the fake-stack used by use-after-return + # checking only on Debug/Checked builds. + # On Debug and Checked builds, always enable the use-after-return check to reduce code-size. + add_compile_options($<$AND:$,$>:-fsanitize-address-use-after-return=never>) + add_compile_options($<$AND:$,$>:-fsanitize-address-use-after-return=never>) + add_compile_options($<$AND:$,$>:-fsanitize-address-use-after-return=always>) + add_compile_options($<$AND:$,$>:-fsanitize-address-use-after-return=always>) endif() endif() diff --git a/src/coreclr/vm/stackwalk.cpp b/src/coreclr/vm/stackwalk.cpp index 6abe943c47b4c..4dd6c510f099b 100644 --- a/src/coreclr/vm/stackwalk.cpp +++ b/src/coreclr/vm/stackwalk.cpp @@ -433,7 +433,7 @@ void ExInfoWalker::WalkToPosition( SUPPORTS_DAC; while (m_pExInfo && - ((GetSPFromContext() < taMinimum) || + (m_pThread->IsStackPointerBefore(GetSPFromContext(), taMinimum) || (GetSPFromContext() == NULL)) ) { // Try the next ExInfo, if there is one. @@ -1351,7 +1351,7 @@ BOOL StackFrameIterator::ResetRegDisp(PREGDISPLAY pRegDisp, while (m_crawl.pFrame != FRAME_TOP) { // this check is sufficient on WIN64 - if (dac_cast(m_crawl.pFrame) >= curSP) + if (m_pThread->IsStackPointerBefore(curSP, dac_cast(m_crawl.pFrame))) { #if defined(TARGET_X86) // check the IP @@ -2418,7 +2418,7 @@ StackWalkAction StackFrameIterator::NextRaw(void) // FaultingExceptionFrame is special case where it gets // pushed on the stack after the frame is running _ASSERTE((m_crawl.pFrame == FRAME_TOP) || - ((TADDR)GetRegdisplaySP(m_crawl.pRD) < dac_cast(m_crawl.pFrame)) || + m_pThread->IsStackPointerBefore((TADDR)GetRegdisplaySP(m_crawl.pRD), dac_cast(m_crawl.pFrame)) || (m_crawl.pFrame->GetVTablePtr() == FaultingExceptionFrame::GetMethodFrameVPtr())); #endif // !defined(ELIMINATE_FEF) @@ -2524,9 +2524,9 @@ StackWalkAction StackFrameIterator::NextRaw(void) PTR_VOID newSP = PTR_VOID((TADDR)GetRegdisplaySP(m_crawl.pRD)); #ifndef NO_FIXED_STACK_LIMIT - FAIL_IF_SPECULATIVE_WALK(m_crawl.pThread->IsExecutingOnAltStack() || newSP >= m_crawl.pThread->GetCachedStackLimit()); + FAIL_IF_SPECULATIVE_WALK(m_crawl.pThread->IsExecutingOnAltStack() || m_crawl.pThread->IsStackPointerBefore(dac_cast(m_crawl.pThread->GetCachedStackLimit()), dac_cast(newSP))); #endif // !NO_FIXED_STACK_LIMIT - FAIL_IF_SPECULATIVE_WALK(m_crawl.pThread->IsExecutingOnAltStack() || newSP < m_crawl.pThread->GetCachedStackBase()); + FAIL_IF_SPECULATIVE_WALK(m_crawl.pThread->IsExecutingOnAltStack() || m_crawl.pThread->IsStackPointerBefore(dac_cast(newSP), dac_cast(m_crawl.pThread->GetCachedStackBase()))); #undef FAIL_IF_SPECULATIVE_WALK @@ -2829,7 +2829,7 @@ void StackFrameIterator::ProcessCurrentFrame(void) // the pContext. // There are still a few cases in which a FaultingExceptionFrame is linked in. If // the next frame is one of them, we don't want to override it. THIS IS PROBABLY BAD!!! - if ( (pContextSP < dac_cast(m_crawl.pFrame)) && + if ( m_pThread->IsStackPointerBefore(pContextSP, dac_cast(m_crawl.pFrame)) && ((m_crawl.GetFrame() == FRAME_TOP) || (m_crawl.GetFrame()->GetVTablePtr() != FaultingExceptionFrame::GetMethodFrameVPtr() ) ) ) { @@ -2980,7 +2980,7 @@ BOOL StackFrameIterator::CheckForSkippedFrames(void) #endif // PROCESS_EXPLICIT_FRAME_BEFORE_MANAGED_FRAME if ( !( (m_crawl.pFrame != FRAME_TOP) && - (dac_cast(m_crawl.pFrame) < pvReferenceSP) ) + m_pThread->IsStackPointerBefore(dac_cast(m_crawl.pFrame), pvReferenceSP) ) ) { return FALSE; @@ -2991,7 +2991,7 @@ BOOL StackFrameIterator::CheckForSkippedFrames(void) // We might have skipped past some Frames. // This happens with InlinedCallFrames. while ( (m_crawl.pFrame != FRAME_TOP) && - (dac_cast(m_crawl.pFrame) < pvReferenceSP) + m_pThread->IsStackPointerBefore(dac_cast(m_crawl.pFrame), pvReferenceSP) ) { BOOL fReportInteropMD = diff --git a/src/coreclr/vm/threads.cpp b/src/coreclr/vm/threads.cpp index edbd6a011f33b..bdd6db15e0933 100644 --- a/src/coreclr/vm/threads.cpp +++ b/src/coreclr/vm/threads.cpp @@ -47,6 +47,8 @@ #include "asmconstants.h" #endif +#include + static const PortableTailCallFrame g_sentinelTailCallFrame = { NULL, NULL }; TailCallTls::TailCallTls() @@ -238,7 +240,7 @@ void Thread::SetFrame(Frame *pFrame) if (g_pConfig->fAssertOnFailFast() == false) return; - Frame* espVal = (Frame*)GetCurrentSP(); + void* espVal = (void*)GetCurrentSP(); while (pFrame != (Frame*) -1) { @@ -246,8 +248,14 @@ void Thread::SetFrame(Frame *pFrame) if (pFrame == stopFrame) _ASSERTE(!"SetFrame frame == stopFrame"); - _ASSERTE(IsExecutingOnAltStack() || espVal < pFrame); - _ASSERTE(IsExecutingOnAltStack() || pFrame < m_CacheStackBase); + if (!IsExecutingOnAltStack()) + { +#if defined(DEBUG) && defined(HAS_ADDRESS_SANITIZER) + _ASSERTE(__asan_addr_is_in_fake_stack(m_fakeStack, pFrame, nullptr, nullptr) || (espVal < pFrame && pFrame < m_CacheStackBase)); +#else + _ASSERTE(espVal < pFrame && pFrame < m_CacheStackBase); +#endif + } _ASSERTE(pFrame->GetFrameType() < Frame::TYPE_COUNT); pFrame = pFrame->m_Next; @@ -6334,6 +6342,10 @@ BOOL Thread::SetStackLimits(SetStackLimitScope scope) } CONTRACTL_END; +#if defined(DEBUG) && defined(HAS_ADDRESS_SANITIZER) + m_fakeStack = __asan_get_current_fake_stack(); +#endif + if (scope == fAll) { m_CacheStackBase = GetStackUpperBound(); @@ -7674,7 +7686,11 @@ Frame * Thread::NotifyFrameChainOfExceptionUnwind(Frame* pStartFrame, LPVOID pvL while (pFrame < pvLimitSP) { CONSISTENCY_CHECK(pFrame != PTR_NULL); +#if defined(DEBUG) && defined(HAS_ADDRESS_SANITIZER) + CONSISTENCY_CHECK(__asan_addr_is_in_fake_stack(m_fakeStack, pFrame, nullptr, nullptr) || (pFrame) > static_cast((LPVOID)GetCurrentSP())); +#else CONSISTENCY_CHECK((pFrame) > static_cast((LPVOID)GetCurrentSP())); +#endif pFrame->ExceptionUnwind(); pFrame = pFrame->Next(); } diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index 1665565c5e640..c57f6e77cdd0a 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -1652,9 +1652,7 @@ class Thread WRAPPER_NO_CONTRACT; if (this == GetThreadNULLOk()) { - void* curSP; - curSP = (void *)GetCurrentSP(); - _ASSERTE(IsExecutingOnAltStack() || (curSP <= m_pFrame && m_pFrame < m_CacheStackBase) || m_pFrame == (Frame*) -1); + _ASSERTE(IsExecutingOnAltStack() || IsAddressInCurrentStack(m_pFrame) || m_pFrame == (Frame*) -1); } #endif @@ -1672,9 +1670,7 @@ class Thread WRAPPER_NO_CONTRACT; if (this == GetThreadNULLOk()) { - void* curSP; - curSP = (void *)GetCurrentSP(); - _ASSERTE((m_pGCFrame == NULL) || (curSP <= m_pGCFrame && m_pGCFrame < m_CacheStackBase)); + _ASSERTE((m_pGCFrame == NULL) || IsAddressInCurrentStack(m_pGCFrame)); } #endif @@ -2928,6 +2924,13 @@ class Thread PTR_VOID m_CacheStackLimit; UINT_PTR m_CacheStackSufficientExecutionLimit; UINT_PTR m_CacheStackStackAllocNonRiskyExecutionLimit; +#ifdef DEBUG + // A pointer that represents the fake stack for the current thread. + // Currently used only by AddressSanitizer when use-after-return validation is enabled. + // We include it on all debug builds to ensure that the DAC's implementation of Thread has the same layout + // as the runtime's implementation. + PTR_VOID m_fakeStack; +#endif #define HARD_GUARD_REGION_SIZE GetOsPageSize() @@ -2983,6 +2986,12 @@ class Thread BOOL IsAddressInStack (PTR_VOID addr) const { +#if defined(DEBUG) && defined(HAS_ADDRESS_SANITIZER) + if (__asan_addr_is_in_fake_stack(m_fakeStack, addr, nullptr, nullptr)) + { + return TRUE; + } +#endif // HAS_ADDRESS_SANITIZER LIMITED_METHOD_DAC_CONTRACT; _ASSERTE(m_CacheStackBase != NULL); _ASSERTE(m_CacheStackLimit != NULL); @@ -2999,12 +3008,37 @@ class Thread return FALSE; } +#if defined(DEBUG) && defined(HAS_ADDRESS_SANITIZER) + if (__asan_addr_is_in_fake_stack(m_fakeStack, addr, nullptr, nullptr)) + { + return TRUE; + } +#endif // HAS_ADDRESS_SANITIZER + PTR_VOID sp = dac_cast(GetCurrentSP()); _ASSERTE(currentThread->m_CacheStackBase != NULL); _ASSERTE(sp < currentThread->m_CacheStackBase); return sp < addr && addr <= currentThread->m_CacheStackBase; } + BOOL IsStackPointerBefore(TADDR sp1, TADDR sp2) + { +#if defined(DEBUG) && defined(HAS_ADDRESS_SANITIZER) + // Resolve any fake stack pointers to their addresses on the real stack. + void* realSp1 = __asan_addr_is_in_fake_stack(m_fakeStack, sp1, nullptr, nullptr); + void* realSp2 = __asan_addr_is_in_fake_stack(m_fakeStack, sp2, nullptr, nullptr); + if (realSp1 != nullptr) + { + sp1 = dac_cast(realSp1); + } + if (realSp2 != nullptr) + { + sp2 = dac_cast(realSp2); + } +#endif + return sp1 < sp2; + } + // DetermineIfGuardPagePresent returns TRUE if the thread's stack contains a proper guard page. This function // makes a physical check of the stack, rather than relying on whether or not the CLR is currently processing a // stack overflow exception. From c3c7771139d24fa16c2a611dc5411c447f09a63d Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 19 Jul 2023 12:02:16 -0700 Subject: [PATCH 03/20] Remove spaces --- eng/native/configurecompiler.cmake | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/eng/native/configurecompiler.cmake b/eng/native/configurecompiler.cmake index 9379d055ebe85..8827f3d2928a8 100644 --- a/eng/native/configurecompiler.cmake +++ b/eng/native/configurecompiler.cmake @@ -211,10 +211,10 @@ if (CLR_CMAKE_ENABLE_SANITIZERS) # Disable the use-after-return check for ASAN on release builds. We track the fake-stack used by use-after-return # checking only on Debug/Checked builds. # On Debug and Checked builds, always enable the use-after-return check to reduce code-size. - add_compile_options($<$AND:$,$>:-fsanitize-address-use-after-return=never>) - add_compile_options($<$AND:$,$>:-fsanitize-address-use-after-return=never>) - add_compile_options($<$AND:$,$>:-fsanitize-address-use-after-return=always>) - add_compile_options($<$AND:$,$>:-fsanitize-address-use-after-return=always>) + add_compile_options($<$AND:$,$>:-fsanitize-address-use-after-return=never>) + add_compile_options($<$AND:$,$>:-fsanitize-address-use-after-return=never>) + add_compile_options($<$AND:$,$>:-fsanitize-address-use-after-return=always>) + add_compile_options($<$AND:$,$>:-fsanitize-address-use-after-return=always>) endif() endif() From be90def9f2303d1fc04d2e95b98c586a0fc40bbf Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 19 Jul 2023 14:00:36 -0700 Subject: [PATCH 04/20] pThread isn't available in ExInfoWallker --- src/coreclr/vm/stackwalk.cpp | 4 ++-- src/coreclr/vm/threads.h | 23 +++++++++++------------ 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/src/coreclr/vm/stackwalk.cpp b/src/coreclr/vm/stackwalk.cpp index 4dd6c510f099b..06cf34fa51995 100644 --- a/src/coreclr/vm/stackwalk.cpp +++ b/src/coreclr/vm/stackwalk.cpp @@ -433,7 +433,7 @@ void ExInfoWalker::WalkToPosition( SUPPORTS_DAC; while (m_pExInfo && - (m_pThread->IsStackPointerBefore(GetSPFromContext(), taMinimum) || + ((GetSPFromContext() < taMinimum) || (GetSPFromContext() == NULL)) ) { // Try the next ExInfo, if there is one. @@ -1206,7 +1206,7 @@ BOOL StackFrameIterator::Init(Thread * pThread, // Walk the ExInfo chain, past any specified starting frame. m_exInfoWalk.Init(&(pThread->GetExceptionState()->m_currentExInfo)); // false means don't reset UseExInfoForStackwalk - m_exInfoWalk.WalkToPosition(dac_cast(m_pStartFrame), false); + m_exInfoWalk.WalkToPosition(pThread->GetRealStackPointer(m_pStartFrame), false); #endif // ELIMINATE_FEF // diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index c57f6e77cdd0a..a39dbf74feeb2 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -3022,21 +3022,20 @@ class Thread } BOOL IsStackPointerBefore(TADDR sp1, TADDR sp2) + { + sp1 = dac_cast(GetRealStackPointer(dac_cast(sp1))); + sp2 = dac_cast(GetRealStackPointer(dac_cast(sp2))); + return sp1 < sp2; + } + + PTR_VOID GetRealStackPointer(PTR_VOID addr) { #if defined(DEBUG) && defined(HAS_ADDRESS_SANITIZER) - // Resolve any fake stack pointers to their addresses on the real stack. - void* realSp1 = __asan_addr_is_in_fake_stack(m_fakeStack, sp1, nullptr, nullptr); - void* realSp2 = __asan_addr_is_in_fake_stack(m_fakeStack, sp2, nullptr, nullptr); - if (realSp1 != nullptr) - { - sp1 = dac_cast(realSp1); - } - if (realSp2 != nullptr) - { - sp2 = dac_cast(realSp2); - } + void* realAddr = __asan_addr_is_in_fake_stack(m_fakeStack, sp1, nullptr, nullptr); + return realAddr ? realAddr : addr; +#else + return addr; #endif - return sp1 < sp2; } // DetermineIfGuardPagePresent returns TRUE if the thread's stack contains a proper guard page. This function From 2c157c961f05869dd59781cd77f1cdb2d0681a0b Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 19 Jul 2023 16:01:05 -0700 Subject: [PATCH 05/20] Add missing cast. --- src/coreclr/vm/stackwalk.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/stackwalk.cpp b/src/coreclr/vm/stackwalk.cpp index 06cf34fa51995..14c4ed12a334f 100644 --- a/src/coreclr/vm/stackwalk.cpp +++ b/src/coreclr/vm/stackwalk.cpp @@ -1206,7 +1206,7 @@ BOOL StackFrameIterator::Init(Thread * pThread, // Walk the ExInfo chain, past any specified starting frame. m_exInfoWalk.Init(&(pThread->GetExceptionState()->m_currentExInfo)); // false means don't reset UseExInfoForStackwalk - m_exInfoWalk.WalkToPosition(pThread->GetRealStackPointer(m_pStartFrame), false); + m_exInfoWalk.WalkToPosition(dac_cast(pThread->GetRealStackPointer(m_pStartFrame)), false); #endif // ELIMINATE_FEF // From f179ce15a3728db8616656aae700645e4c38c177 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 20 Jul 2023 10:47:14 -0700 Subject: [PATCH 06/20] Fix typo in generator expression --- eng/native/configurecompiler.cmake | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/eng/native/configurecompiler.cmake b/eng/native/configurecompiler.cmake index 8827f3d2928a8..ad66a4f9ced3e 100644 --- a/eng/native/configurecompiler.cmake +++ b/eng/native/configurecompiler.cmake @@ -211,10 +211,10 @@ if (CLR_CMAKE_ENABLE_SANITIZERS) # Disable the use-after-return check for ASAN on release builds. We track the fake-stack used by use-after-return # checking only on Debug/Checked builds. # On Debug and Checked builds, always enable the use-after-return check to reduce code-size. - add_compile_options($<$AND:$,$>:-fsanitize-address-use-after-return=never>) - add_compile_options($<$AND:$,$>:-fsanitize-address-use-after-return=never>) - add_compile_options($<$AND:$,$>:-fsanitize-address-use-after-return=always>) - add_compile_options($<$AND:$,$>:-fsanitize-address-use-after-return=always>) + add_compile_options($<$,$>:-fsanitize-address-use-after-return=never>) + add_compile_options($<$,$>:-fsanitize-address-use-after-return=never>) + add_compile_options($<$,$>:-fsanitize-address-use-after-return=always>) + add_compile_options($<$,$>:-fsanitize-address-use-after-return=always>) endif() endif() From c7108785820d405d49b61a53637782d1aee81b6b Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 8 Aug 2023 11:29:45 -0700 Subject: [PATCH 07/20] Fix accessing instance member in static --- src/coreclr/vm/threads.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index a39dbf74feeb2..18834e6a8fd3f 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -3009,7 +3009,7 @@ class Thread } #if defined(DEBUG) && defined(HAS_ADDRESS_SANITIZER) - if (__asan_addr_is_in_fake_stack(m_fakeStack, addr, nullptr, nullptr)) + if (__asan_addr_is_in_fake_stack(currentThread->m_fakeStack, addr, nullptr, nullptr)) { return TRUE; } From 18d18fa8847bbc4676462a338ef3bf09b9469212 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 9 Aug 2023 16:34:38 -0700 Subject: [PATCH 08/20] Fix typo --- src/coreclr/vm/threads.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index 18834e6a8fd3f..84bf19096a5bb 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -3031,7 +3031,7 @@ class Thread PTR_VOID GetRealStackPointer(PTR_VOID addr) { #if defined(DEBUG) && defined(HAS_ADDRESS_SANITIZER) - void* realAddr = __asan_addr_is_in_fake_stack(m_fakeStack, sp1, nullptr, nullptr); + void* realAddr = __asan_addr_is_in_fake_stack(m_fakeStack, addr, nullptr, nullptr); return realAddr ? realAddr : addr; #else return addr; From e07f30c3ee676fe593102856b4e9f685c8d14ca5 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 9 Aug 2023 16:56:54 -0700 Subject: [PATCH 09/20] Fix DAC type system problems --- src/coreclr/vm/threads.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index 84bf19096a5bb..e7ccc67f53095 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -3031,8 +3031,8 @@ class Thread PTR_VOID GetRealStackPointer(PTR_VOID addr) { #if defined(DEBUG) && defined(HAS_ADDRESS_SANITIZER) - void* realAddr = __asan_addr_is_in_fake_stack(m_fakeStack, addr, nullptr, nullptr); - return realAddr ? realAddr : addr; + void* realAddr = __asan_addr_is_in_fake_stack(m_fakeStack, (void*)PTR_TO_TADDR(addr), nullptr, nullptr); + return realAddr ? dac_cast(realAddr) : addr; #else return addr; #endif From f2ae69f5950025b643cc312ee9adc4921ed4f185 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 9 Aug 2023 17:41:22 -0700 Subject: [PATCH 10/20] More fixes --- src/coreclr/vm/threads.cpp | 6 +----- src/coreclr/vm/threads.h | 4 ++-- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/coreclr/vm/threads.cpp b/src/coreclr/vm/threads.cpp index bdd6db15e0933..313231cc6f76f 100644 --- a/src/coreclr/vm/threads.cpp +++ b/src/coreclr/vm/threads.cpp @@ -7686,11 +7686,7 @@ Frame * Thread::NotifyFrameChainOfExceptionUnwind(Frame* pStartFrame, LPVOID pvL while (pFrame < pvLimitSP) { CONSISTENCY_CHECK(pFrame != PTR_NULL); -#if defined(DEBUG) && defined(HAS_ADDRESS_SANITIZER) - CONSISTENCY_CHECK(__asan_addr_is_in_fake_stack(m_fakeStack, pFrame, nullptr, nullptr) || (pFrame) > static_cast((LPVOID)GetCurrentSP())); -#else - CONSISTENCY_CHECK((pFrame) > static_cast((LPVOID)GetCurrentSP())); -#endif + CONSISTENCY_CHECK(IsStackPointerBefore(static_cast((LPVOID)GetCurrentSP()), pFrame)); pFrame->ExceptionUnwind(); pFrame = pFrame->Next(); } diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index e7ccc67f53095..47a3aeb2b91ad 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -2987,7 +2987,7 @@ class Thread BOOL IsAddressInStack (PTR_VOID addr) const { #if defined(DEBUG) && defined(HAS_ADDRESS_SANITIZER) - if (__asan_addr_is_in_fake_stack(m_fakeStack, addr, nullptr, nullptr)) + if (__asan_addr_is_in_fake_stack(m_fakeStack, (void*)PTR_TO_TADDR(addr), nullptr, nullptr)) { return TRUE; } @@ -3009,7 +3009,7 @@ class Thread } #if defined(DEBUG) && defined(HAS_ADDRESS_SANITIZER) - if (__asan_addr_is_in_fake_stack(currentThread->m_fakeStack, addr, nullptr, nullptr)) + if (__asan_addr_is_in_fake_stack(currentThread->m_fakeStack, (void*)PTR_TO_TADDR(addr), nullptr, nullptr)) { return TRUE; } From 7450bb4f111364ddea6d71267bcf730c80192387 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 10 Aug 2023 11:34:51 -0700 Subject: [PATCH 11/20] Fix consistency check --- src/coreclr/vm/threads.cpp | 2 +- src/coreclr/vm/threads.h | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/vm/threads.cpp b/src/coreclr/vm/threads.cpp index 313231cc6f76f..e8a4e4e6cf38a 100644 --- a/src/coreclr/vm/threads.cpp +++ b/src/coreclr/vm/threads.cpp @@ -7686,7 +7686,7 @@ Frame * Thread::NotifyFrameChainOfExceptionUnwind(Frame* pStartFrame, LPVOID pvL while (pFrame < pvLimitSP) { CONSISTENCY_CHECK(pFrame != PTR_NULL); - CONSISTENCY_CHECK(IsStackPointerBefore(static_cast((LPVOID)GetCurrentSP()), pFrame)); + CONSISTENCY_CHECK(IsStackPointerBefore(reinterpret_cast(static_cast((LPVOID)GetCurrentSP())), reinterpret_cast(pFrame))); pFrame->ExceptionUnwind(); pFrame = pFrame->Next(); } diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index 47a3aeb2b91ad..6ab02ab7e4af1 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -2987,7 +2987,7 @@ class Thread BOOL IsAddressInStack (PTR_VOID addr) const { #if defined(DEBUG) && defined(HAS_ADDRESS_SANITIZER) - if (__asan_addr_is_in_fake_stack(m_fakeStack, (void*)PTR_TO_TADDR(addr), nullptr, nullptr)) + if (__asan_addr_is_in_fake_stack((void*)PTR_TO_TADDR(m_fakeStack), (void*)PTR_TO_TADDR(addr), nullptr, nullptr)) { return TRUE; } @@ -3009,7 +3009,7 @@ class Thread } #if defined(DEBUG) && defined(HAS_ADDRESS_SANITIZER) - if (__asan_addr_is_in_fake_stack(currentThread->m_fakeStack, (void*)PTR_TO_TADDR(addr), nullptr, nullptr)) + if (__asan_addr_is_in_fake_stack((void*)PTR_TO_TADDR(currentThread->m_fakeStack), (void*)PTR_TO_TADDR(addr), nullptr, nullptr)) { return TRUE; } @@ -3031,7 +3031,7 @@ class Thread PTR_VOID GetRealStackPointer(PTR_VOID addr) { #if defined(DEBUG) && defined(HAS_ADDRESS_SANITIZER) - void* realAddr = __asan_addr_is_in_fake_stack(m_fakeStack, (void*)PTR_TO_TADDR(addr), nullptr, nullptr); + void* realAddr = __asan_addr_is_in_fake_stack((void*)PTR_TO_TADDR(m_fakeStack), (void*)PTR_TO_TADDR(addr), nullptr, nullptr); return realAddr ? dac_cast(realAddr) : addr; #else return addr; From e6759976754728cd30accf23fb229236030fddc6 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 14 Aug 2023 10:17:24 -0700 Subject: [PATCH 12/20] Use IsStackPointerBefore in Frame::Push --- src/coreclr/vm/frames.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/frames.cpp b/src/coreclr/vm/frames.cpp index 4cccc1e6dfc42..4839283cab9b7 100644 --- a/src/coreclr/vm/frames.cpp +++ b/src/coreclr/vm/frames.cpp @@ -390,7 +390,7 @@ VOID Frame::Push(Thread *pThread) // with multiple Frames in coreclr.dll _ASSERTE((pThread->IsExecutingOnAltStack() || (m_Next == FRAME_TOP) || - (PBYTE(m_Next) + (2 * GetOsPageSize())) > PBYTE(this)) && + pThread->IsStackPointerBefore(PBYTE(this), (PBYTE(m_Next) + (2 * GetOsPageSize())))) && "Pushing a frame out of order ?"); _ASSERTE(// If AssertOnFailFast is set, the test expects to do stack overrun From eadfc17b802573e9192b2ff21494833c19febeb4 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 14 Aug 2023 10:31:33 -0700 Subject: [PATCH 13/20] Add some casts. --- src/coreclr/vm/frames.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/frames.cpp b/src/coreclr/vm/frames.cpp index 4839283cab9b7..57c033928114e 100644 --- a/src/coreclr/vm/frames.cpp +++ b/src/coreclr/vm/frames.cpp @@ -390,7 +390,7 @@ VOID Frame::Push(Thread *pThread) // with multiple Frames in coreclr.dll _ASSERTE((pThread->IsExecutingOnAltStack() || (m_Next == FRAME_TOP) || - pThread->IsStackPointerBefore(PBYTE(this), (PBYTE(m_Next) + (2 * GetOsPageSize())))) && + pThread->IsStackPointerBefore(dac_cast(PBYTE(this)), dac_cast((PBYTE(m_Next) + (2 * GetOsPageSize()))))) && "Pushing a frame out of order ?"); _ASSERTE(// If AssertOnFailFast is set, the test expects to do stack overrun From 84d933938949e1cafcbc2f1539bd6ec7dcd9a02d Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 21 Aug 2023 17:16:00 -0700 Subject: [PATCH 14/20] Fix stack-pointer comparison to do calculations after the real stack pointer adjustment, not before. --- src/coreclr/vm/frames.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/frames.cpp b/src/coreclr/vm/frames.cpp index 57c033928114e..49760a2ecb270 100644 --- a/src/coreclr/vm/frames.cpp +++ b/src/coreclr/vm/frames.cpp @@ -390,7 +390,7 @@ VOID Frame::Push(Thread *pThread) // with multiple Frames in coreclr.dll _ASSERTE((pThread->IsExecutingOnAltStack() || (m_Next == FRAME_TOP) || - pThread->IsStackPointerBefore(dac_cast(PBYTE(this)), dac_cast((PBYTE(m_Next) + (2 * GetOsPageSize()))))) && + dac_cast(pThread->GetRealStackPointer(dac_cast(this))) < dac_cast(pThread->GetRealStackPointer(dac_cast(m_Next))) + (2 * GetOsPageSize())) && "Pushing a frame out of order ?"); _ASSERTE(// If AssertOnFailFast is set, the test expects to do stack overrun @@ -988,7 +988,7 @@ void GCFrame::Push(Thread* pThread) // So GetOsPageSize() is a guess of the maximum stack frame size of any method // with multiple GCFrames in coreclr.dll _ASSERTE(((m_Next == NULL) || - (PBYTE(m_Next) + (2 * GetOsPageSize())) > PBYTE(this)) && + dac_cast(pThread->GetRealStackPointer(dac_cast(this))) < dac_cast(pThread->GetRealStackPointer(dac_cast(m_Next))) + (2 * GetOsPageSize())) && "Pushing a GCFrame out of order ?"); pThread->SetGCFrame(this); From f3066b23801a00424a16e0b3940f7f528d37e54b Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 23 Aug 2023 11:27:01 -0700 Subject: [PATCH 15/20] Fix more comparisons to use the fake stack and add a TODO for a place where we need to figure out a good solution. --- src/coreclr/pal/src/exception/seh-unwind.cpp | 4 +++ src/coreclr/vm/excep.cpp | 2 +- src/coreclr/vm/exceptionhandling.cpp | 27 ++++++++++---------- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/src/coreclr/pal/src/exception/seh-unwind.cpp b/src/coreclr/pal/src/exception/seh-unwind.cpp index 8afe1ffb3db6f..f3727a3f13e52 100644 --- a/src/coreclr/pal/src/exception/seh-unwind.cpp +++ b/src/coreclr/pal/src/exception/seh-unwind.cpp @@ -651,6 +651,10 @@ void GetContextPointers(unw_cursor_t *cursor, unw_context_t *unwContext, KNONVOL #ifndef HOST_WINDOWS +// TODO-ASAN: These "offsets to a local in the frame" constructs do not work if we are using any instrumentation +// that uses a fake stack. We need to either diable instrumentation in the functions that set these variables +// or find a way to make them work with fake stacks. + // Frame pointer relative offset of a local containing a pointer to the windows style context of a location // where a hardware exception occurred. int g_hardware_exception_context_locvar_offset = 0; diff --git a/src/coreclr/vm/excep.cpp b/src/coreclr/vm/excep.cpp index 5be645ad496d7..35d6e1492cff3 100644 --- a/src/coreclr/vm/excep.cpp +++ b/src/coreclr/vm/excep.cpp @@ -3429,7 +3429,7 @@ void UnwindFrameChain(Thread* pThread, LPVOID pvLimitSP) CONTRACTL_END; Frame* pFrame = pThread->m_pFrame; - if (pFrame < pvLimitSP) + if (pThread->IsStackPointerBefore(dac_cast(pFrame), dac_cast(pvLimitSP))) { GCX_COOP_THREAD_EXISTS(pThread); diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index 0621ae03efe4f..e1bd5f6a788e2 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -337,7 +337,8 @@ StackWalkAction UpdateObjectRefInResumeContextCallback(CrawlFrame* pCF, LPVOID p if (pFrame->NeedsUpdateRegDisplay()) { - CONSISTENCY_CHECK(pFrame >= pState->pHighestFrameWithRegisters); + CONSISTENCY_CHECK(pCF->GetThread()->IsStackPointerBefore(dac_cast(pState->pHighestFrameWithRegisters), dac_cast(pFrame)) || + pState->pHighestFrameWithRegisters == pFrame); pState->pHighestFrameWithRegisters = pFrame; // Is this an InlinedCallFrame? @@ -393,7 +394,7 @@ bool ExceptionTracker::FindNonvolatileRegisterPointers(Thread* pThread, UINT_PTR Frame *pHighestFrameWithRegisters = NULL; Frame *pFrame = pThread->GetFrame(); - while ((UINT_PTR)pFrame < uOriginalSP) + while (pThread->IsStackPointerBefore(dac_cast(pFrame), uOriginalSP)) { if (pFrame->NeedsUpdateRegDisplay()) pHighestFrameWithRegisters = pFrame; @@ -1191,10 +1192,10 @@ ProcessCLRException(IN PEXCEPTION_RECORD pExceptionRecord, // Note that we should only fail to fix up for SO. bool fFixedUp = FixNonvolatileRegisters(uOriginalSP, pThread, pContextRecord, fAborting); - _ASSERTE(fFixedUp || (pExceptionRecord->ExceptionCode == STATUS_STACK_OVERFLOW)); + _ASSERTE(fFixedUp || (pExceptionRecord->ExceptionCode == STATUS_STACK_OVERFLOW)); // TODO-ASAN: Cache the exception code beforehand. Otherwise we get a heap-use-after-free when the assert fails. - CONSISTENCY_CHECK(pLimitFrame > dac_cast(GetSP(pContextRecord))); + CONSISTENCY_CHECK(pThread->IsStackPointerBefore(dac_cast(GetSP(pContextRecord)), dac_cast(pLimitFrame))); if (pICFSetAsLimitFrame != NULL) { _ASSERTE(pICFSetAsLimitFrame == pLimitFrame); @@ -1789,7 +1790,7 @@ CLRUnwindStatus ExceptionTracker::ProcessOSExceptionNotification( cfThisFrame.SetCurGSCookie(Frame::SafeGetGSCookiePtr(pFrame)); } - while (((UINT_PTR)pFrame) < uCallerSP) + while (pThread->IsStackPointerBefore(dac_cast(pFrame), uCallerSP)) { // InlinedCallFrames (ICF) are allocated, initialized and linked to the Frame chain // by the code generated by the JIT for a method containing a PInvoke. @@ -1853,8 +1854,8 @@ CLRUnwindStatus ExceptionTracker::ProcessOSExceptionNotification( // // 1) ICF address is higher than the current frame's SP (which we get from DispatcherContext), AND // 2) ICF address is below callerSP. - if ((GetSP(pDispatcherContext->ContextRecord) < (TADDR)pICF) && - ((UINT_PTR)pICF < uCallerSP)) + if (pThread->IsStackPointerBefore(GetSP(pDispatcherContext->ContextRecord), (TADDR)pICF) && + pThread->IsStackPointerBefore((TADDR)pICF, uCallerSP)) { pICFForUnwindTarget = pFrame; @@ -3265,7 +3266,7 @@ CLRUnwindStatus ExceptionTracker::ProcessManagedCallFrame( #undef OPTIONAL_SO_CLEANUP_UNWIND -#define OPTIONAL_SO_CLEANUP_UNWIND(pThread, pFrame) if (pThread->GetFrame() < pFrame) { UnwindFrameChain(pThread, pFrame); } +#define OPTIONAL_SO_CLEANUP_UNWIND(pThread, pFrame) if (pThread->IsStackPointerBefore(dac_cast(pThread->GetFrame()), dac_cast(pFrame))) { UnwindFrameChain(pThread, pFrame); } typedef DWORD_PTR (HandlerFn)(UINT_PTR uStackFrame, Object* pExceptionObj); @@ -3958,7 +3959,7 @@ ExceptionTracker* ExceptionTracker::GetOrCreateTracker( { // In the second pass, there's a possibility that UMThunkUnwindFrameChainHandler() has // popped some frames off the frame chain underneath us. Check for this case here. - if (pTracker->m_pLimitFrame < pThread->GetFrame()) + if (pThread->IsStackPointerBefore(dac_cast(pTracker->m_pLimitFrame), dac_cast(pThread->GetFrame()))) { pTracker->ResetLimitFrame(); } @@ -3966,7 +3967,7 @@ ExceptionTracker* ExceptionTracker::GetOrCreateTracker( } } - _ASSERTE(pTracker->m_pLimitFrame >= pThread->GetFrame()); + _ASSERTE(!pThread->IsStackPointerBefore(dac_cast(pTracker->m_pLimitFrame), dac_cast(pThread->GetFrame()))); RETURN pTracker; } @@ -5807,7 +5808,7 @@ BOOL IsSafeToUnwindFrameChain(Thread* pThread, LPVOID MemoryStackFpForFrameChain { // Look for the last Frame to be removed that marks a managed-to-unmanaged transition Frame* pLastFrameOfInterest = FRAME_TOP; - for (Frame* pf = pThread->m_pFrame; pf < MemoryStackFpForFrameChain; pf = pf->PtrNextFrame()) + for (Frame* pf = pThread->m_pFrame; pThread->IsStackPointerBefore(dac_cast(pf), dac_cast(MemoryStackFpForFrameChain)); pf = pf->PtrNextFrame()) { PCODE retAddr = pf->GetReturnAddress(); if (retAddr != NULL && ExecutionManager::IsManagedCode(retAddr)) @@ -5862,9 +5863,9 @@ void CleanUpForSecondPass(Thread* pThread, bool fIsSO, LPVOID MemoryStackFpForFr // incorrectly removed a transition frame (more details in IsSafeToUnwindFrameChain) // [Do not perform the IsSafeToUnwindFrameChain() check in the SO case, since // IsSafeToUnwindFrameChain() requires a large amount of stack space.] - _ASSERTE(fIsSO || IsSafeToUnwindFrameChain(pThread, (Frame*)MemoryStackFpForFrameChain)); + _ASSERTE(fIsSO || IsSafeToUnwindFrameChain(pThread, MemoryStackFpForFrameChain)); - UnwindFrameChain(pThread, (Frame*)MemoryStackFpForFrameChain); + UnwindFrameChain(pThread, MemoryStackFpForFrameChain); // Only pop the trackers if this is not an SO. It's not safe to pop the trackers during EH for an SO. // Instead, we rely on the END_SO_TOLERANT_CODE macro to call ClearExceptionStateAfterSO(). Of course, From b019ad4b728e2ac0a3fd97a3bfa44276e85b3c4a Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 13 Feb 2024 11:47:21 -0800 Subject: [PATCH 16/20] Resolve TODOs and extract frame offset calculations to use non-intstrumented helper methods. --- .../pal/src/exception/machexception.cpp | 6 ++- src/coreclr/pal/src/exception/seh-unwind.cpp | 8 +-- src/coreclr/pal/src/exception/signal.cpp | 49 +++++++++++++------ src/coreclr/vm/exceptionhandling.cpp | 4 +- 4 files changed, 42 insertions(+), 25 deletions(-) diff --git a/src/coreclr/pal/src/exception/machexception.cpp b/src/coreclr/pal/src/exception/machexception.cpp index 50db83248fe7a..264fbe29e8ab3 100644 --- a/src/coreclr/pal/src/exception/machexception.cpp +++ b/src/coreclr/pal/src/exception/machexception.cpp @@ -366,14 +366,16 @@ RestoreCompleteContext( ); #endif -__attribute__((noinline)) +__attribute__((noinline)) DISABLE_ASAN static void PAL_DispatchExceptionInner(PCONTEXT pContext, PEXCEPTION_RECORD pExRecord) { // Stash the inner context record into a local in a frame other than PAL_DispatchException // to ensure we have a compiler-defined callee stack frame state to record the context record // local before we call SEHProcessException. The instrumentation introduced by native sanitizers // doesn't interface that well with the fake caller frames we define for PAL_DispatchException, - // but they work fine for any callees of PAL_DispatchException. + // but they work fine for any callees of PAL_DispatchException. However, we still need to disable + // sanitizers for this function to avoid using any "fake stack" features + // that would break the "frame offset" calculations done below. CONTEXT *contextRecord = pContext; g_hardware_exception_context_locvar_offset = (int)((char*)&contextRecord - (char*)__builtin_frame_address(0)); diff --git a/src/coreclr/pal/src/exception/seh-unwind.cpp b/src/coreclr/pal/src/exception/seh-unwind.cpp index f3727a3f13e52..ce8936b3f792e 100644 --- a/src/coreclr/pal/src/exception/seh-unwind.cpp +++ b/src/coreclr/pal/src/exception/seh-unwind.cpp @@ -651,10 +651,6 @@ void GetContextPointers(unw_cursor_t *cursor, unw_context_t *unwContext, KNONVOL #ifndef HOST_WINDOWS -// TODO-ASAN: These "offsets to a local in the frame" constructs do not work if we are using any instrumentation -// that uses a fake stack. We need to either diable instrumentation in the functions that set these variables -// or find a way to make them work with fake stacks. - // Frame pointer relative offset of a local containing a pointer to the windows style context of a location // where a hardware exception occurred. int g_hardware_exception_context_locvar_offset = 0; @@ -688,8 +684,8 @@ BOOL PAL_VirtualUnwind(CONTEXT *context, KNONVOLATILE_CONTEXT_POINTERS *contextP // cannot cross on some systems. if ((void*)curPc == g_InvokeActivationHandlerReturnAddress) { - CONTEXT* activationContext = (CONTEXT*)(CONTEXTGetFP(context) + g_inject_activation_context_locvar_offset); - memcpy_s(context, sizeof(CONTEXT), activationContext, sizeof(CONTEXT)); + CONTEXT** activationContext = (CONTEXT**)(CONTEXTGetFP(context) + g_inject_activation_context_locvar_offset); + memcpy_s(context, sizeof(CONTEXT), *activationContext, sizeof(CONTEXT)); return TRUE; } diff --git a/src/coreclr/pal/src/exception/signal.cpp b/src/coreclr/pal/src/exception/signal.cpp index 0da7f05001b90..ca61abf0b881c 100644 --- a/src/coreclr/pal/src/exception/signal.cpp +++ b/src/coreclr/pal/src/exception/signal.cpp @@ -790,6 +790,20 @@ static void InvokeActivationHandler(CONTEXT *pWinContext) g_activationFunction(pWinContext); } +DIABLE_ASAN +static void HoldContextAndInvokeActivationHandler(CONTEXT* pWinContext) +{ + // We need to disable ASAN for this function as we use the frame offset to find the context when stackwalking. + // Some ASAN features use fake stacks, which is incompatible with this design. + CONTEXT* winContext = pWinContext; + g_inject_activation_context_locvar_offset = (int)((char*)&winContext - (char*)__builtin_frame_address(0)); + int savedErrNo = errno; // Make sure that errno is not modified + InvokeActivationHandler(&winContext); + errno = savedErrNo; + // Activation function may have modified the context, so update it. + CONTEXTToNativeContext(&winContext, ucontext); +} + /*++ Function : inject_activation_handler @@ -835,13 +849,7 @@ static void inject_activation_handler(int code, siginfo_t *siginfo, void *contex if (g_safeActivationCheckFunction(CONTEXTGetPC(&winContext), /* checkingCurrentThread */ TRUE)) { - g_inject_activation_context_locvar_offset = (int)((char*)&winContext - (char*)__builtin_frame_address(0)); - int savedErrNo = errno; // Make sure that errno is not modified - InvokeActivationHandler(&winContext); - errno = savedErrNo; - - // Activation function may have modified the context, so update it. - CONTEXTToNativeContext(&winContext, ucontext); + HoldContextAndInvokeActivationHandler(&winContext); } } else @@ -945,6 +953,21 @@ void PAL_IgnoreProfileSignal(int signalNum) #endif } +DISABLE_ASAN +static void CallSEHProcessException(CONTEXT* pContext, PAL_SEHException* pException) +{ + // We need to disable ASAN for this function as we use the frame offset to find the context when stackwalking. + // Some ASAN features use fake stacks, which is incompatible with this design. + CONTEXT* winContext = pContext; + g_hardware_exception_context_locvar_offset = (int)((char*)&winContext - (char*)__builtin_frame_address(0)); + if (SEHProcessException(pException)) + { + // Exception handling may have modified the context, so update it. + CONTEXTToNativeContext(&winContext, ucontext); + return true; + } + return false; +} /*++ Function : @@ -970,12 +993,10 @@ static bool common_signal_handler(int code, siginfo_t *siginfo, void *sigcontext #if !HAVE_MACH_EXCEPTIONS sigset_t signal_set; CONTEXT signalContextRecord; - CONTEXT* signalContextRecordPtr = &signalContextRecord; EXCEPTION_RECORD exceptionRecord; native_context_t *ucontext; ucontext = (native_context_t *)sigcontext; - g_hardware_exception_context_locvar_offset = (int)((char*)&signalContextRecordPtr - (char*)__builtin_frame_address(0)); if (code == (SIGSEGV | StackOverflowFlag)) { @@ -1028,14 +1049,10 @@ static bool common_signal_handler(int code, siginfo_t *siginfo, void *sigcontext // The exception object takes ownership of the exceptionRecord and contextRecord PAL_SEHException exception(&exceptionRecord, &signalContextRecord, true); - if (SEHProcessException(&exception)) - { - // Exception handling may have modified the context, so update it. - CONTEXTToNativeContext(exception.ExceptionPointers.ContextRecord, ucontext); - return true; - } -#endif // !HAVE_MACH_EXCEPTIONS + return CallSEHProcessException(&signalContextRecord, &exception); +#else // !HAVE_MACH_EXCEPTIONS return false; +#endif // !HAVE_MACH_EXCEPTIONS } /*++ diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index e1bd5f6a788e2..652d19e3b112f 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -1190,9 +1190,11 @@ ProcessCLRException(IN PEXCEPTION_RECORD pExceptionRecord, // INDEBUG(pTracker = (ExceptionTracker*)POISONC); + INDEBUG(DWORD exceptionCode = pExceptionRecord->ExceptionCode); + // Note that we should only fail to fix up for SO. bool fFixedUp = FixNonvolatileRegisters(uOriginalSP, pThread, pContextRecord, fAborting); - _ASSERTE(fFixedUp || (pExceptionRecord->ExceptionCode == STATUS_STACK_OVERFLOW)); // TODO-ASAN: Cache the exception code beforehand. Otherwise we get a heap-use-after-free when the assert fails. + _ASSERTE(fFixedUp || (exceptionCode == STATUS_STACK_OVERFLOW)); CONSISTENCY_CHECK(pThread->IsStackPointerBefore(dac_cast(GetSP(pContextRecord)), dac_cast(pLimitFrame))); From 204cc43780b56f2bedd7a2e6ed0c0e6b0d8a64a0 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 13 Feb 2024 14:34:26 -0800 Subject: [PATCH 17/20] Fix linux build --- src/coreclr/pal/src/exception/signal.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/coreclr/pal/src/exception/signal.cpp b/src/coreclr/pal/src/exception/signal.cpp index ca61abf0b881c..0f4402214e80a 100644 --- a/src/coreclr/pal/src/exception/signal.cpp +++ b/src/coreclr/pal/src/exception/signal.cpp @@ -790,7 +790,7 @@ static void InvokeActivationHandler(CONTEXT *pWinContext) g_activationFunction(pWinContext); } -DIABLE_ASAN +DISABLE_ASAN static void HoldContextAndInvokeActivationHandler(CONTEXT* pWinContext) { // We need to disable ASAN for this function as we use the frame offset to find the context when stackwalking. @@ -800,8 +800,6 @@ static void HoldContextAndInvokeActivationHandler(CONTEXT* pWinContext) int savedErrNo = errno; // Make sure that errno is not modified InvokeActivationHandler(&winContext); errno = savedErrNo; - // Activation function may have modified the context, so update it. - CONTEXTToNativeContext(&winContext, ucontext); } /*++ @@ -850,6 +848,8 @@ static void inject_activation_handler(int code, siginfo_t *siginfo, void *contex if (g_safeActivationCheckFunction(CONTEXTGetPC(&winContext), /* checkingCurrentThread */ TRUE)) { HoldContextAndInvokeActivationHandler(&winContext); + // Activation function may have modified the context, so update it. + CONTEXTToNativeContext(&winContext, ucontext); } } else @@ -954,7 +954,7 @@ void PAL_IgnoreProfileSignal(int signalNum) } DISABLE_ASAN -static void CallSEHProcessException(CONTEXT* pContext, PAL_SEHException* pException) +static bool CallSEHProcessException(CONTEXT* pContext, PAL_SEHException* pException, ucontext_t* ucontext) { // We need to disable ASAN for this function as we use the frame offset to find the context when stackwalking. // Some ASAN features use fake stacks, which is incompatible with this design. @@ -963,7 +963,7 @@ static void CallSEHProcessException(CONTEXT* pContext, PAL_SEHException* pExcept if (SEHProcessException(pException)) { // Exception handling may have modified the context, so update it. - CONTEXTToNativeContext(&winContext, ucontext); + CONTEXTToNativeContext(winContext, ucontext); return true; } return false; @@ -1049,7 +1049,7 @@ static bool common_signal_handler(int code, siginfo_t *siginfo, void *sigcontext // The exception object takes ownership of the exceptionRecord and contextRecord PAL_SEHException exception(&exceptionRecord, &signalContextRecord, true); - return CallSEHProcessException(&signalContextRecord, &exception); + return CallSEHProcessException(&signalContextRecord, &exception, ucontext); #else // !HAVE_MACH_EXCEPTIONS return false; #endif // !HAVE_MACH_EXCEPTIONS From 7aecf81a5f22a87662b6fd44b20e14ad185de828 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 13 Feb 2024 15:33:33 -0800 Subject: [PATCH 18/20] Fix invocation --- src/coreclr/pal/src/exception/signal.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/pal/src/exception/signal.cpp b/src/coreclr/pal/src/exception/signal.cpp index 0f4402214e80a..790054c4e2a9a 100644 --- a/src/coreclr/pal/src/exception/signal.cpp +++ b/src/coreclr/pal/src/exception/signal.cpp @@ -798,7 +798,7 @@ static void HoldContextAndInvokeActivationHandler(CONTEXT* pWinContext) CONTEXT* winContext = pWinContext; g_inject_activation_context_locvar_offset = (int)((char*)&winContext - (char*)__builtin_frame_address(0)); int savedErrNo = errno; // Make sure that errno is not modified - InvokeActivationHandler(&winContext); + InvokeActivationHandler(winContext); errno = savedErrNo; } From 533e331a4b2b2e26425cc6a2956c975dbde61ce9 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 15 Feb 2024 15:13:16 -0800 Subject: [PATCH 19/20] Add some no-inlines to assist in the unwinding --- src/coreclr/pal/src/exception/signal.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/pal/src/exception/signal.cpp b/src/coreclr/pal/src/exception/signal.cpp index 790054c4e2a9a..a7e3c0af08a56 100644 --- a/src/coreclr/pal/src/exception/signal.cpp +++ b/src/coreclr/pal/src/exception/signal.cpp @@ -790,6 +790,7 @@ static void InvokeActivationHandler(CONTEXT *pWinContext) g_activationFunction(pWinContext); } +__attribute__((noinline)) DISABLE_ASAN static void HoldContextAndInvokeActivationHandler(CONTEXT* pWinContext) { @@ -953,6 +954,7 @@ void PAL_IgnoreProfileSignal(int signalNum) #endif } +__attribute__((noinline)) DISABLE_ASAN static bool CallSEHProcessException(CONTEXT* pContext, PAL_SEHException* pException, ucontext_t* ucontext) { From 4a2dfe404565aa8963652b9a91a0fb4743bd8d4f Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 15 Feb 2024 15:14:40 -0800 Subject: [PATCH 20/20] Adjust activation context copying to match the exception context copying. --- src/coreclr/pal/src/exception/seh-unwind.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/pal/src/exception/seh-unwind.cpp b/src/coreclr/pal/src/exception/seh-unwind.cpp index ce8936b3f792e..046549b0c7589 100644 --- a/src/coreclr/pal/src/exception/seh-unwind.cpp +++ b/src/coreclr/pal/src/exception/seh-unwind.cpp @@ -684,8 +684,8 @@ BOOL PAL_VirtualUnwind(CONTEXT *context, KNONVOLATILE_CONTEXT_POINTERS *contextP // cannot cross on some systems. if ((void*)curPc == g_InvokeActivationHandlerReturnAddress) { - CONTEXT** activationContext = (CONTEXT**)(CONTEXTGetFP(context) + g_inject_activation_context_locvar_offset); - memcpy_s(context, sizeof(CONTEXT), *activationContext, sizeof(CONTEXT)); + CONTEXT* activationContext = *(CONTEXT**)(CONTEXTGetFP(context) + g_inject_activation_context_locvar_offset); + memcpy_s(context, sizeof(CONTEXT), activationContext, sizeof(CONTEXT)); return TRUE; }