Fix exception handling in the prestub worker (#111937)

* Fix exception handling in 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.

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

* Regression test

* Fix ExternalMethodFixupWorker and VSD_ResolveWorker too

These two need the same treatment when they end up being called from
CallDescrWorkerInternal.

Add regression tests for those cases too. These two cases don't result
in a visible failure, so the regression tests just exercise the specific
code paths.

* Disable the regression test for NativeAOT / MonoAOT

* Reverting change in PrestubWorker_Preemptive

This change is not needed, as that function cannot be called via
CallDescrWorker.

* Few forgotten cleanups

* Remove the RequiresProcessIsolation

* Disable the regression test for WASM

* Attempt to properly disable the test for WASM

* Another attempt to prevent running the test on WASM

Author: janvorli

src/coreclr/vm/excep.cpp

@@ -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

src/coreclr/vm/exceptionhandling.cpp

@@ -932,7 +932,12 @@ 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))
         {
@@ -8520,7 +8525,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)
 {
@@ -8575,8 +8580,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;
             }

src/coreclr/vm/exceptmacros.h

@@ -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;              \
         }                                           \
@@ -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 {
@@ -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

src/coreclr/vm/prestub.cpp

@@ -2536,6 +2536,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.
@@ -2631,60 +2645,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;
@@ -3154,8 +3184,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;
@@ -3397,8 +3429,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
 

src/coreclr/vm/virtualcallstub.cpp

@@ -1262,6 +1262,8 @@ ResolveCacheElem* __fastcall VirtualCallStubManager::PromoteChainEntry(ResolveCa
 }
 #endif // CHAIN_LOOKUP
 
+bool IsCallDescrWorkerInternalReturnAddress(PCODE pCode);
+
 /* Resolve to a method and return its address or NULL if there is none.
    Our return value is the target address that control should continue to.  Our caller will
    enter the target address as if a direct call with the original stack frame had been made from
@@ -1309,14 +1311,16 @@ PCODE VSD_ResolveWorker(TransitionBlock * pTransitionBlock,
 
     PCODE target = (PCODE)NULL;
 
+    bool propagateExceptionToNativeCode = IsCallDescrWorkerInternalReturnAddress(pTransitionBlock->m_ReturnAddress);
+
     if (pObj == NULL) {
         pSDFrame->SetForNullReferenceException();
         pSDFrame->Push(CURRENT_THREAD);
-        INSTALL_MANAGED_EXCEPTION_DISPATCHER;
-        INSTALL_UNWIND_AND_CONTINUE_HANDLER;
+        INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX;
+        INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX;
         COMPlusThrow(kNullReferenceException);
-        UNINSTALL_UNWIND_AND_CONTINUE_HANDLER;
-        UNINSTALL_MANAGED_EXCEPTION_DISPATCHER;
+        UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_EX(propagateExceptionToNativeCode);
+        UNINSTALL_MANAGED_EXCEPTION_DISPATCHER_EX(propagateExceptionToNativeCode);
         _ASSERTE(!"Throw returned");
     }
 
@@ -1351,8 +1355,9 @@ PCODE VSD_ResolveWorker(TransitionBlock * pTransitionBlock,
 
     pSDFrame->SetRepresentativeSlot(pRepresentativeMT, representativeToken.GetSlotNumber());
     pSDFrame->Push(CURRENT_THREAD);
-    INSTALL_MANAGED_EXCEPTION_DISPATCHER;
-    INSTALL_UNWIND_AND_CONTINUE_HANDLER;
+
+    INSTALL_MANAGED_EXCEPTION_DISPATCHER_EX;
+    INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX;
 
     // For Virtual Delegates the m_siteAddr is a field of a managed object
     // Thus we have to report it as an interior pointer,
@@ -1388,8 +1393,8 @@ PCODE VSD_ResolveWorker(TransitionBlock * pTransitionBlock,
 
     GCPROTECT_END();
 
-    UNINSTALL_UNWIND_AND_CONTINUE_HANDLER;
-    UNINSTALL_MANAGED_EXCEPTION_DISPATCHER;
+    UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_EX(propagateExceptionToNativeCode);
+    UNINSTALL_MANAGED_EXCEPTION_DISPATCHER_EX(propagateExceptionToNativeCode);
     pSDFrame->Pop(CURRENT_THREAD);
 
     return target;

src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.cs

@@ -0,0 +1,14 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+
+namespace Dependency
+{
+    public class DependencyClass
+    {
+        public static void Test()
+        {
+        }
+    }
+}

src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.csproj

@@ -0,0 +1,9 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <CLRTestKind>BuildOnly</CLRTestKind>
+    <OutputType>Library</OutputType>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="dependencytodelete.cs" />
+  </ItemGroup>
+</Project>

src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.il

@@ -0,0 +1,17 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+.assembly extern legacy library mscorlib {}
+.assembly extern dependencytodelete {}
+.assembly 'tailcallinvoker' {}
+
+.class public sequential ansi sealed beforefieldinit TailCallInvoker
+       extends [mscorlib]System.Object
+{
+  .method public static void Test() cil managed noinlining
+  {
+    .maxstack  1
+    tail. call void [dependencytodelete]Dependency.DependencyClass::Test()
+    ret
+  } // end of method TailCallInvoker.Test
+} // end of class TailCallInvoker

src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.ilproj

@@ -0,0 +1,10 @@
+<Project Sdk="Microsoft.NET.Sdk.IL">
+  <PropertyGroup>
+    <CLRTestKind>BuildOnly</CLRTestKind>
+    <OutputType>Library</OutputType>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="tailcallinvoker.il" />
+    <ProjectReference Include="dependencytodelete.csproj" />
+  </ItemGroup>
+</Project>