Enable use-after-return checking in ASAN

eng/native/configurecompiler.cmake

@@ -159,6 +159,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)
@@ -176,12 +190,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})
@@ -214,11 +227,13 @@ if (CLR_CMAKE_ENABLE_SANITIZERS)
       # define the preprocessor define ourselves.
       add_compile_definitions($<$<COMPILE_LANGUAGE:ASM,ASM_MASM>: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($<$<COMPILE_LANG_AND_ID:C,Clang,AppleClang>:-fsanitize-address-use-after-return=never>)
-      add_compile_options($<$<COMPILE_LANG_AND_ID:CXX,Clang,AppleClang>:-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:$<CONFIG:RelWithDebInfo,Release>,$<COMPILE_LANG_AND_ID:C,Clang,AppleClang>>:-fsanitize-address-use-after-return=never>)
+      add_compile_options($<$<AND:$<CONFIG:RelWithDebInfo,Release>,$<COMPILE_LANG_AND_ID:CXX,Clang,AppleClang>>:-fsanitize-address-use-after-return=never>)
+      add_compile_options($<$<AND:$<CONFIG:Debug,Checked>,$<COMPILE_LANG_AND_ID:C,Clang,AppleClang>>:-fsanitize-address-use-after-return=always>)
+      add_compile_options($<$<AND:$<CONFIG:Debug,Checked>,$<COMPILE_LANG_AND_ID:CXX,Clang,AppleClang>>:-fsanitize-address-use-after-return=always>)
     endif()
   endif()
 

src/coreclr/pal/inc/pal.h

@@ -85,7 +85,6 @@ extern "C" {
 
 #include <pal_error.h>
 #include <pal_mstypes.h>
-#include <minipal/utils.h>
 
 // Native system libray handle.
 // On Unix systems, NATIVE_LIBRARY_HANDLE type represents a library handle not registered with the PAL.

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));
 

src/coreclr/pal/src/exception/seh-unwind.cpp

@@ -674,7 +674,7 @@ 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);
+        CONTEXT* activationContext = *(CONTEXT**)(CONTEXTGetFP(context) + g_inject_activation_context_locvar_offset);
         memcpy_s(context, sizeof(CONTEXT), activationContext, sizeof(CONTEXT));
 
         return TRUE;

src/coreclr/pal/src/exception/signal.cpp

@@ -847,6 +847,19 @@ static void InvokeActivationHandler(CONTEXT *pWinContext)
     g_activationFunction(pWinContext);
 }
 
+__attribute__((noinline))
+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.
+    // 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;
+}
+
 /*++
 Function :
     inject_activation_handler
@@ -892,11 +905,7 @@ static void inject_activation_handler(int code, siginfo_t *siginfo, void *contex
 
         if (g_safeActivationCheckFunction(CONTEXTGetPC(&winContext)))
         {
-            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;
-
+            HoldContextAndInvokeActivationHandler(&winContext);
             // Activation function may have modified the context, so update it.
             CONTEXTToNativeContext(&winContext, ucontext);
         }
@@ -1011,6 +1020,22 @@ void PAL_IgnoreProfileSignal(int signalNum)
 #endif
 }
 
+__attribute__((noinline))
+DISABLE_ASAN
+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.
+    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 :
@@ -1036,12 +1061,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))
     {
@@ -1094,14 +1117,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, ucontext);
+#else // !HAVE_MACH_EXCEPTIONS
     return false;
+#endif // !HAVE_MACH_EXCEPTIONS
 }
 
 /*++

src/coreclr/pal/src/include/pal/palinternal.h

@@ -205,6 +205,7 @@ function_name() to call the system's implementation
 // https://gcc.gnu.org/ml/libstdc++/2016-01/msg00025.html
 #define _GLIBCXX_INCLUDE_NEXT_C_HEADERS 1
 
+#include <minipal/utils.h>
 #ifdef __APPLE__
 
 #undef GetCurrentThread

src/coreclr/vm/excep.cpp

@@ -3182,7 +3182,7 @@ void UnwindFrameChain(Thread* pThread, LPVOID pvLimitSP)
     CONTRACTL_END;
 
     Frame* pFrame = pThread->m_pFrame;
-    if (pFrame < pvLimitSP)
+    if (pThread->IsStackPointerBefore(dac_cast<TADDR>(pFrame), dac_cast<TADDR>(pvLimitSP)))
     {
         GCX_COOP_THREAD_EXISTS(pThread);
 

src/coreclr/vm/exceptionhandling.cpp

@@ -339,7 +339,8 @@ StackWalkAction UpdateObjectRefInResumeContextCallback(CrawlFrame* pCF, LPVOID p
 
         if (pFrame->NeedsUpdateRegDisplay())
         {
-            CONSISTENCY_CHECK(pFrame >= pState->pHighestFrameWithRegisters);
+            CONSISTENCY_CHECK(pCF->GetThread()->IsStackPointerBefore(dac_cast<TADDR>(pState->pHighestFrameWithRegisters), dac_cast<TADDR>(pFrame)) ||
+                              pState->pHighestFrameWithRegisters == pFrame);
             pState->pHighestFrameWithRegisters = pFrame;
 
             // Is this an InlinedCallFrame?
@@ -395,7 +396,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<TADDR>(pFrame), uOriginalSP))
     {
         if (pFrame->NeedsUpdateRegDisplay())
             pHighestFrameWithRegisters = pFrame;
@@ -1310,12 +1311,14 @@ 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));
+                _ASSERTE(fFixedUp || (exceptionCode == STATUS_STACK_OVERFLOW));
 
 
-                CONSISTENCY_CHECK(pLimitFrame > dac_cast<PTR_VOID>(GetSP(pContextRecord)));
+                CONSISTENCY_CHECK(pThread->IsStackPointerBefore(dac_cast<TADDR>(GetSP(pContextRecord)), dac_cast<TADDR>(pLimitFrame)));
                 if (pICFSetAsLimitFrame != NULL)
                 {
                     _ASSERTE(pICFSetAsLimitFrame == pLimitFrame);
@@ -1910,7 +1913,7 @@ CLRUnwindStatus ExceptionTracker::ProcessOSExceptionNotification(
             cfThisFrame.SetCurGSCookie(Frame::SafeGetGSCookiePtr(pFrame));
         }
 
-        while (((UINT_PTR)pFrame) < uCallerSP)
+        while (pThread->IsStackPointerBefore(dac_cast<TADDR>(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.
@@ -1974,8 +1977,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;
 
@@ -3422,7 +3425,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<TADDR>(pThread->GetFrame()), dac_cast<TADDR>(pFrame))) { UnwindFrameChain(pThread, pFrame); }
 
 typedef DWORD_PTR (HandlerFn)(UINT_PTR uStackFrame, Object* pExceptionObj);
 
@@ -4116,15 +4119,15 @@ 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<TADDR>(pTracker->m_pLimitFrame), dac_cast<TADDR>(pThread->GetFrame())))
                 {
                     pTracker->ResetLimitFrame();
                 }
             }
         }
     }
 
-    _ASSERTE(pTracker->m_pLimitFrame >= pThread->GetFrame());
+    _ASSERTE(!pThread->IsStackPointerBefore(dac_cast<TADDR>(pTracker->m_pLimitFrame), dac_cast<TADDR>(pThread->GetFrame())));
 
     RETURN pTracker;
 }
@@ -6123,7 +6126,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<TADDR>(pf), dac_cast<TADDR>(MemoryStackFpForFrameChain)); pf = pf->PtrNextFrame())
     {
         PCODE retAddr = pf->GetReturnAddress();
         if (retAddr != (PCODE)NULL && ExecutionManager::IsManagedCode(retAddr))
@@ -6178,9 +6181,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,

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)) &&
+             dac_cast<PBYTE>(pThread->GetRealStackPointer(dac_cast<PTR_VOID>(this))) < dac_cast<PBYTE>(pThread->GetRealStackPointer(dac_cast<PTR_VOID>(m_Next))) + (2 * GetOsPageSize())) &&
              "Pushing a frame out of order ?");
 
     _ASSERTE(// If AssertOnFailFast is set, the test expects to do stack overrun
@@ -1004,7 +1004,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<PBYTE>(pThread->GetRealStackPointer(dac_cast<PTR_VOID>(this))) < dac_cast<PBYTE>(pThread->GetRealStackPointer(dac_cast<PTR_VOID>(m_Next))) + (2 * GetOsPageSize())) &&
              "Pushing a GCFrame out of order ?");
 
     pThread->SetGCFrame(this);