Fix Windows x86 exception handling issue (dotnet/coreclr#4830)

This change fixes an exception handling issue that happens on x86 on Windows
when exception is raised in System.RuntimeType.MakeGenericType.
The problem was caused by GCPROTECT_HOLDER macro in RuntimeTypeHandle::GetTypeByName
that causes popping of GCFrame (and zeroing its m_next field) that's in the middle of the thread's frames list during
the stack unwinding.
That breaks the list and when UnwindFrames happen later and tries to walk the stack, the StackFrameIterator::NextRaw
asserts when checking the validity of the list.
The fix is to move the keepAlive to the managed caller of the RuntimeTypeHandle::GetTypeByName QCall, which removes
the need for the GCPROTECT_HOLDER.
Since it was the only usage of that holder and of the underlying FrameWithCookieHolder class, I've removed those.
In addition to that, I've modified COMModule::GetType and AssemblyNative::GetType to use the same pattern, since
they could also suffer from the problem the GCPROTECT_HOLDER was attempting to fix.

Commit migrated from https://github.com/dotnet/coreclr/commit/3c7e477ac50e50b616a64c72efb81388c045e63f

diff --git a/src/coreclr/src/mscorlib/src/System/Reflection/Assembly.cs b/src/coreclr/src/mscorlib/src/System/Reflection/Assembly.cs
index b666566..4e5ca96 100644 (file)
--- a/src/coreclr/src/mscorlib/src/System/Reflection/Assembly.cs
+++ b/src/coreclr/src/mscorlib/src/System/Reflection/Assembly.cs
@@ -1367,7 +1367,8 @@ namespace System.Reflection
                                                         String name, 
                                                         bool throwOnError, 
                                                         bool ignoreCase,
-                                                        ObjectHandleOnStack type);
+                                                        ObjectHandleOnStack type,
+                                                        ObjectHandleOnStack keepAlive);
         
         [System.Security.SecuritySafeCritical]
         public override Type GetType(String name, bool throwOnError, bool ignoreCase) 
@@ -1377,7 +1378,10 @@ namespace System.Reflection
                 throw new ArgumentNullException("name");
 
             RuntimeType type = null;
-            GetType(GetNativeHandle(), name, throwOnError, ignoreCase, JitHelpers.GetObjectHandleOnStack(ref type));
+            Object keepAlive = null;
+            GetType(GetNativeHandle(), name, throwOnError, ignoreCase, JitHelpers.GetObjectHandleOnStack(ref type), JitHelpers.GetObjectHandleOnStack(ref keepAlive));
+            GC.KeepAlive(keepAlive);
+            
             return type;
         }
 

diff --git a/src/coreclr/src/mscorlib/src/System/Reflection/Module.cs b/src/coreclr/src/mscorlib/src/System/Reflection/Module.cs
index b8a1052..5420551 100644 (file)
--- a/src/coreclr/src/mscorlib/src/System/Reflection/Module.cs
+++ b/src/coreclr/src/mscorlib/src/System/Reflection/Module.cs
@@ -586,7 +586,7 @@ namespace System.Reflection
         [System.Security.SecurityCritical]  // auto-generated
         [DllImport(JitHelpers.QCall, CharSet = CharSet.Unicode)]
                [SuppressUnmanagedCodeSecurity]
-        private extern static void GetType(RuntimeModule module, String className, bool ignoreCase, bool throwOnError, ObjectHandleOnStack type);
+        private extern static void GetType(RuntimeModule module, String className, bool ignoreCase, bool throwOnError, ObjectHandleOnStack type, ObjectHandleOnStack keepAlive);
 
         [System.Security.SecurityCritical]
         [DllImport(JitHelpers.QCall)]
@@ -1056,7 +1056,9 @@ namespace System.Reflection
                 throw new ArgumentNullException("className");
 
             RuntimeType retType = null;
-            GetType(GetNativeHandle(), className, throwOnError, ignoreCase, JitHelpers.GetObjectHandleOnStack(ref retType));
+            Object keepAlive = null;
+            GetType(GetNativeHandle(), className, throwOnError, ignoreCase, JitHelpers.GetObjectHandleOnStack(ref retType), JitHelpers.GetObjectHandleOnStack(ref keepAlive));
+            GC.KeepAlive(keepAlive);
             return retType;
         }
 

diff --git a/src/coreclr/src/mscorlib/src/System/RuntimeHandles.cs b/src/coreclr/src/mscorlib/src/System/RuntimeHandles.cs
index b751985..c4e0a8d 100644 (file)
--- a/src/coreclr/src/mscorlib/src/System/RuntimeHandles.cs
+++ b/src/coreclr/src/mscorlib/src/System/RuntimeHandles.cs
@@ -537,7 +537,7 @@ namespace System
 #if FEATURE_HOSTED_BINDER
             IntPtr pPrivHostBinder,
 #endif
-            bool loadTypeFromPartialName, ObjectHandleOnStack type);
+            bool loadTypeFromPartialName, ObjectHandleOnStack type, ObjectHandleOnStack keepalive);
 
 #if FEATURE_HOSTED_BINDER
         // Wrapper function to reduce the need for ifdefs.
@@ -564,12 +564,14 @@ namespace System
 
             RuntimeType type = null;
 
+            Object keepAlive = null;
             GetTypeByName(name, throwOnError, ignoreCase, reflectionOnly,
                 JitHelpers.GetStackCrawlMarkHandle(ref stackMark),
 #if FEATURE_HOSTED_BINDER
                 pPrivHostBinder,
 #endif
-                loadTypeFromPartialName, JitHelpers.GetObjectHandleOnStack(ref type));
+                loadTypeFromPartialName, JitHelpers.GetObjectHandleOnStack(ref type), JitHelpers.GetObjectHandleOnStack(ref keepAlive));
+            GC.KeepAlive(keepAlive);
 
             return type;
         }

diff --git a/src/coreclr/src/vm/assemblynative.cpp b/src/coreclr/src/vm/assemblynative.cpp
index 96fd939..c3deaab 100644 (file)
--- a/src/coreclr/src/vm/assemblynative.cpp
+++ b/src/coreclr/src/vm/assemblynative.cpp
@@ -1170,7 +1170,7 @@ FCIMPL1(FC_BOOL_RET, AssemblyNative::IsReflectionOnly, AssemblyBaseObject *pAsse
 }
 FCIMPLEND
 
-void QCALLTYPE AssemblyNative::GetType(QCall::AssemblyHandle pAssembly, LPCWSTR wszName, BOOL bThrowOnError, BOOL bIgnoreCase, QCall::ObjectHandleOnStack retType)
+void QCALLTYPE AssemblyNative::GetType(QCall::AssemblyHandle pAssembly, LPCWSTR wszName, BOOL bThrowOnError, BOOL bIgnoreCase, QCall::ObjectHandleOnStack retType, QCall::ObjectHandleOnStack keepAlive)
 {
     CONTRACTL
     {
@@ -1186,27 +1186,17 @@ void QCALLTYPE AssemblyNative::GetType(QCall::AssemblyHandle pAssembly, LPCWSTR
     if (!wszName)
         COMPlusThrowArgumentNull(W("name"), W("ArgumentNull_String"));
 
-    GCX_COOP();
-
-    OBJECTREF keepAlive = NULL;
-    GCPROTECT_BEGIN(keepAlive);
-
-    {
-        GCX_PREEMP();
+    BOOL prohibitAsmQualifiedName = TRUE;
 
-        BOOL prohibitAsmQualifiedName = TRUE;
-
-        // Load the class from this assembly (fail if it is in a different one).
-        retTypeHandle = TypeName::GetTypeManaged(wszName, pAssembly, bThrowOnError, bIgnoreCase, pAssembly->IsIntrospectionOnly(), prohibitAsmQualifiedName, NULL, FALSE, &keepAlive);
-    }
+    // Load the class from this assembly (fail if it is in a different one).
+    retTypeHandle = TypeName::GetTypeManaged(wszName, pAssembly, bThrowOnError, bIgnoreCase, pAssembly->IsIntrospectionOnly(), prohibitAsmQualifiedName, NULL, FALSE, (OBJECTREF*)keepAlive.m_ppObject);
 
     if (!retTypeHandle.IsNull())
     {
-        retType.Set(retTypeHandle.GetManagedClassObject());
+         GCX_COOP();
+         retType.Set(retTypeHandle.GetManagedClassObject());
     }
 
-    GCPROTECT_END();
-
     END_QCALL;
 
     return;

diff --git a/src/coreclr/src/vm/assemblynative.hpp b/src/coreclr/src/vm/assemblynative.hpp
index 0c0df02..d191457 100644 (file)
--- a/src/coreclr/src/vm/assemblynative.hpp
+++ b/src/coreclr/src/vm/assemblynative.hpp
@@ -131,7 +131,7 @@ public:
                                              QCall::ObjectHandleOnStack retModule);
 
     static 
-    void QCALLTYPE GetType(QCall::AssemblyHandle pAssembly, LPCWSTR wszName, BOOL bThrowOnError, BOOL bIgnoreCase, QCall::ObjectHandleOnStack retType);
+    void QCALLTYPE GetType(QCall::AssemblyHandle pAssembly, LPCWSTR wszName, BOOL bThrowOnError, BOOL bIgnoreCase, QCall::ObjectHandleOnStack retType, QCall::ObjectHandleOnStack keepAlive);
     
     static 
     INT32 QCALLTYPE GetManifestResourceInfo(QCall::AssemblyHandle pAssembly, LPCWSTR wszName, QCall::ObjectHandleOnStack retAssembly, QCall::StringHandleOnStack retFileName, QCall::StackCrawlMarkHandle stackMark);

diff --git a/src/coreclr/src/vm/commodule.cpp b/src/coreclr/src/vm/commodule.cpp
index 7cab6b5..bb3e479 100644 (file)
--- a/src/coreclr/src/vm/commodule.cpp
+++ b/src/coreclr/src/vm/commodule.cpp
@@ -845,7 +845,7 @@ mdTypeSpec QCALLTYPE COMModule::GetTokenFromTypeSpec(QCall::ModuleHandle pModule
 // GetType
 // Given a class name, this method will look for that class
 //  with in the module. 
-void QCALLTYPE COMModule::GetType(QCall::ModuleHandle pModule, LPCWSTR wszName, BOOL bThrowOnError, BOOL bIgnoreCase, QCall::ObjectHandleOnStack retType)
+void QCALLTYPE COMModule::GetType(QCall::ModuleHandle pModule, LPCWSTR wszName, BOOL bThrowOnError, BOOL bIgnoreCase, QCall::ObjectHandleOnStack retType, QCall::ObjectHandleOnStack keepAlive)
 {
     CONTRACTL
     {
@@ -858,22 +858,13 @@ void QCALLTYPE COMModule::GetType(QCall::ModuleHandle pModule, LPCWSTR wszName,
 
     BEGIN_QCALL;
 
-    GCX_COOP();
-
     DomainAssembly *pAssembly = pModule->GetDomainAssembly();
     _ASSERTE(pAssembly);
 
-    OBJECTREF keepAlive = NULL;
-    GCPROTECT_BEGIN(keepAlive);
-
-    {
-        GCX_PREEMP();
-
-        BOOL prohibitAsmQualifiedName = TRUE;
+    BOOL prohibitAsmQualifiedName = TRUE;
 
-        // Load the class from this assembly (fail if it is in a different one).
-        retTypeHandle = TypeName::GetTypeManaged(wszName, pAssembly, bThrowOnError, bIgnoreCase, pAssembly->IsIntrospectionOnly(), prohibitAsmQualifiedName, NULL, FALSE, &keepAlive);
-    }
+    // Load the class from this assembly (fail if it is in a different one).
+    retTypeHandle = TypeName::GetTypeManaged(wszName, pAssembly, bThrowOnError, bIgnoreCase, pAssembly->IsIntrospectionOnly(), prohibitAsmQualifiedName, NULL, FALSE, (OBJECTREF*)keepAlive.m_ppObject);
 
     // Verify that it's in 'this' module
     // But, if it's in a different assembly than expected, that's okay, because
@@ -890,8 +881,7 @@ void QCALLTYPE COMModule::GetType(QCall::ModuleHandle pModule, LPCWSTR wszName,
         GCX_COOP();
         retType.Set(retTypeHandle.GetManagedClassObject());
     }
-    GCPROTECT_END();
-
+ 
     END_QCALL;
 
     return;

diff --git a/src/coreclr/src/vm/commodule.h b/src/coreclr/src/vm/commodule.h
index 5f52043..255b22d 100644 (file)
--- a/src/coreclr/src/vm/commodule.h
+++ b/src/coreclr/src/vm/commodule.h
@@ -91,7 +91,7 @@ public:
     // Given a class type, this method will look for that type
     //  with in the module.
     static
-    void QCALLTYPE GetType(QCall::ModuleHandle pModule, LPCWSTR wszName, BOOL bThrowOnError, BOOL bIgnoreCase, QCall::ObjectHandleOnStack retType);
+    void QCALLTYPE GetType(QCall::ModuleHandle pModule, LPCWSTR wszName, BOOL bThrowOnError, BOOL bIgnoreCase, QCall::ObjectHandleOnStack retType, QCall::ObjectHandleOnStack keepAlive);
 
     // Get class will return an array contain all of the classes
     //  that are defined within this Module.

diff --git a/src/coreclr/src/vm/excep.cpp b/src/coreclr/src/vm/excep.cpp
index 200e052..672f315 100644 (file)
--- a/src/coreclr/src/vm/excep.cpp
+++ b/src/coreclr/src/vm/excep.cpp
@@ -8678,10 +8678,6 @@ void UnwindAndContinueRethrowHelperInsideCatch(Frame* pEntryFrame, Exception* pE
     // policy here. Do we want to let such funcitons throw, etc.? Right now, we believe that there are no such
     // frames on the stack to be unwound, so the SetFrame is alright (see the first comment above.) At the very
     // least, we should add some way to assert that.
-    //
-    // ~FrameWithCookieHolder is also calling SetFrame() and if UnwindAndContinueRethrowHelperInsideCatch is ever changed
-    // to not call SetFrame then the change should be reflected in the FrameWithCookieHolder as well.
-    //
     pThread->SetFrame(pEntryFrame);
 
 #ifdef _DEBUG

diff --git a/src/coreclr/src/vm/frames.h b/src/coreclr/src/vm/frames.h
index 41e35bf..7b55c36 100644 (file)
--- a/src/coreclr/src/vm/frames.h
+++ b/src/coreclr/src/vm/frames.h
@@ -3679,70 +3679,6 @@ public:
     DEFINE_VTABLE_GETTER_AND_CTOR_AND_DTOR(SecurityContextFrame)
 };
 
-
-// This holder is defined for addressing a very specific issue:
-// Frames that use GCPROTECT_BEGIN/GCPROTECT_END can end up referencing corrupted object refs
-// when an exception is thrown until the point where the Frame is actually popped from the thread's Frame-chain.
-// Stack space allocated for OBJECTREFs in a try block may be reused in the catch block by other structures, 
-// corrupting our protected OBJECTREFs and the Frame containing them. While the Frame is still on the callstack
-// a GC may occur, detecting the corrupt OBJECTREF and taking down the process. The FrameWithCookieHolder 
-// forces the Frame to be popped out when exiting the current scope, therefore before the OBJECTREF is corrupted.
-//
-// This holder explicitly calls Thread::SetFrame, therefore potentially removing Frames from the thread's frame 
-// chain without properly calling their corresponding ExceptionUnwind() method. This is extremely dangerous to 
-// use unless it is backed by a call to UnwindAndContinueRethrowHelperInsideCatch() which does the same thing
-// (and has been vetted to be correct in doing so). Using this holder in any other circumstances may lead to bugs that 
-// are extremely difficult to track down.
-template <typename TYPE>
-class FrameWithCookieHolder 
-{
-    protected:
-               FrameWithCookie<TYPE>   m_frame;
-
-       public:
-               FORCEINLINE FrameWithCookieHolder()
-                       : m_frame()
-               {
-               }
-
-               //      GCFrame 
-               FORCEINLINE     FrameWithCookieHolder(OBJECTREF *pObjRefs, UINT numObjRefs, BOOL maybeInterior)
-                       : m_frame(pObjRefs, numObjRefs, maybeInterior)
-               {
-               }
-
-               FORCEINLINE ~FrameWithCookieHolder()
-               {
-#ifndef DACCESS_COMPILE
-
-                       Thread* pThread = GetThread();
-                       if (pThread)
-                       {
-                           GCX_COOP();
-                           pThread->SetFrame(&m_frame);
-                           m_frame.Pop();
-                       }
-
-#endif // #ifndef DACCESS_COMPILE
-               }
-
-};
-
-#ifndef DACCESS_COMPILE
-// Restrictions from FrameWithCookieHolder are also applying for GCPROTECT_HOLDER. 
-// Please read the FrameWithCookieHolder comments before using GCPROTECT_HOLDER.  
-#define        GCPROTECT_HOLDER(ObjRefStruct)                                                                  \
-                FrameWithCookieHolder<GCFrame> __gcframe((OBJECTREF*)&(ObjRefStruct),  \
-                        sizeof(ObjRefStruct)/sizeof(OBJECTREF),         \
-                        FALSE);                                          
-
-#else // #ifndef DACCESS_COMPILE
-
-#define        GCPROTECT_HOLDER(ObjRefStruct)
-
-#endif // #ifndef DACCESS_COMPILE
-
-
 //------------------------------------------------------------------------
 // These macros GC-protect OBJECTREF pointers on the EE's behalf.
 // In between these macros, the GC can move but not discard the protected

diff --git a/src/coreclr/src/vm/runtimehandles.cpp b/src/coreclr/src/vm/runtimehandles.cpp
index d0f36cb..601a8bc 100644 (file)
--- a/src/coreclr/src/vm/runtimehandles.cpp
+++ b/src/coreclr/src/vm/runtimehandles.cpp
@@ -1883,7 +1883,8 @@ void QCALLTYPE RuntimeTypeHandle::GetTypeByName(LPCWSTR pwzClassName, BOOL bThro
 #ifdef FEATURE_HOSTED_BINDER
                                                 ICLRPrivBinder * pPrivHostBinder,
 #endif
-                                                BOOL bLoadTypeFromPartialNameHack, QCall::ObjectHandleOnStack retType)
+                                                BOOL bLoadTypeFromPartialNameHack, QCall::ObjectHandleOnStack retType,
+                                                QCall::ObjectHandleOnStack keepAlive)
 {
     QCALL_CONTRACT;
     
@@ -1894,32 +1895,19 @@ void QCALLTYPE RuntimeTypeHandle::GetTypeByName(LPCWSTR pwzClassName, BOOL bThro
     if (!pwzClassName)
             COMPlusThrowArgumentNull(W("className"),W("ArgumentNull_String"));
 
-    GCX_COOP();
     {
-        OBJECTREF keepAlive = NULL;
-
-        // BEGIN_QCALL/END_QCALL define try/catch scopes for potential exceptions thrown when bThrowOnError is enabled.
-        // Originally, in case of an exception the GCFrame was removed from the Thread's Frame chain in the catch block, in UnwindAndContinueRethrowHelperInsideCatch.
-        // However, the catch block declared some local variables that overlapped the location of the now out of scope GCFrame and OBJECTREF, therefore corrupting
-        // those values. Having the GCX_COOP/GCX_PREEMP switching GC modes, allowed a situation where in case of an exception, the thread would wait for a GC to complete
-        // while still having the GCFrame in the Thread's Frame chain, but with a corrupt OBJECTREF due to stack location reuse in the catch block.
-        // The solution is to force the removal of GCFrame (and the Frames above) from the Thread's Frame chain before entering the catch block, at the time of 
-        // FrameWithCookieHolder's destruction.
-        GCPROTECT_HOLDER(keepAlive);
-
-        {
-            GCX_PREEMP();
-            typeHandle = TypeName::GetTypeManaged(pwzClassName, NULL, bThrowOnError, bIgnoreCase, bReflectionOnly, /*bProhibitAsmQualifiedName =*/ FALSE, pStackMark, bLoadTypeFromPartialNameHack, &keepAlive
+        typeHandle = TypeName::GetTypeManaged(pwzClassName, NULL, bThrowOnError, bIgnoreCase, bReflectionOnly, /*bProhibitAsmQualifiedName =*/ FALSE, pStackMark,
+                                              bLoadTypeFromPartialNameHack, (OBJECTREF*)keepAlive.m_ppObject
 #ifdef FEATURE_HOSTED_BINDER
-                                                  , pPrivHostBinder
+                                              , pPrivHostBinder
 #endif
-                );
-        }
+            );
+    }
 
-        if (!typeHandle.IsNull())
-        {
-            retType.Set(typeHandle.GetManagedClassObject());
-        }
+    if (!typeHandle.IsNull())
+    {
+        GCX_COOP();
+        retType.Set(typeHandle.GetManagedClassObject());
     }
 
     END_QCALL;

diff --git a/src/coreclr/src/vm/runtimehandles.h b/src/coreclr/src/vm/runtimehandles.h
index 73aa58b..ecb5c15 100644 (file)
--- a/src/coreclr/src/vm/runtimehandles.h
+++ b/src/coreclr/src/vm/runtimehandles.h
@@ -183,7 +183,8 @@ public:
 #ifdef FEATURE_HOSTED_BINDER
                                  ICLRPrivBinder * pPrivHostBinder,
 #endif
-                                 BOOL bLoadTypeFromPartialNameHack, QCall::ObjectHandleOnStack retType);
+                                 BOOL bLoadTypeFromPartialNameHack, QCall::ObjectHandleOnStack retType,
+                                 QCall::ObjectHandleOnStack keepAlive);
 
     static FCDECL1(AssemblyBaseObject*, GetAssembly, ReflectClassBaseObject *pType);
     static FCDECL1(ReflectClassBaseObject*, GetBaseType, ReflectClassBaseObject* pType);