Fix Windows x86 exception handling issue (#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.

diff --git a/src/mscorlib/src/System/Reflection/Assembly.cs b/src/mscorlib/src/System/Reflection/Assembly.cs
index b6665660661f682672d96e3aadc06a4182034707..4e5ca96fe341bee8fb1c56a0c87c0534c671f2b5 100644 (file)
--- a/src/mscorlib/src/System/Reflection/Assembly.cs
+++ b/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/mscorlib/src/System/Reflection/Module.cs b/src/mscorlib/src/System/Reflection/Module.cs
index b8a10527a320edd8de4f3c92fdfae997ab56764e..54205517b602e33d8cf8e37768ce4acbd42481bc 100644 (file)
--- a/src/mscorlib/src/System/Reflection/Module.cs
+++ b/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/mscorlib/src/System/RuntimeHandles.cs b/src/mscorlib/src/System/RuntimeHandles.cs
index b751985323260aa9008b82fdb892cb496e93deb3..c4e0a8d203c798c82fe1bd396b160d46629c5102 100644 (file)
--- a/src/mscorlib/src/System/RuntimeHandles.cs
+++ b/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/vm/assemblynative.cpp b/src/vm/assemblynative.cpp
index 96fd9391263158ea94638c1b1f8cf24c535ad86a..c3deaaba087ff8d9a5a4f8e83bbfee501b16c766 100644 (file)
--- a/src/vm/assemblynative.cpp
+++ b/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/vm/assemblynative.hpp b/src/vm/assemblynative.hpp
index 0c0df020c2ea9b6c4da915b5052890fa6a7856af..d191457c9f5d8f6618a042d4a35ce46c80080622 100644 (file)
--- a/src/vm/assemblynative.hpp
+++ b/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/vm/commodule.cpp b/src/vm/commodule.cpp
index 7cab6b54f263afdd45ae6431751d70a152f755e3..bb3e47967c4a27c022e960715ee64983a87c8be7 100644 (file)
--- a/src/vm/commodule.cpp
+++ b/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/vm/commodule.h b/src/vm/commodule.h
index 5f520436323c3389f72c4a5a434ad88ee360c952..255b22dfbfe1f4cc8b40bb5d79753ad2b3b427cd 100644 (file)
--- a/src/vm/commodule.h
+++ b/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/vm/excep.cpp b/src/vm/excep.cpp
index 200e052aa8a41137198ccf8ae5ec08b5cbe3788d..672f315fcddb1fda7c872f2fd7f37c733ffd6871 100644 (file)
--- a/src/vm/excep.cpp
+++ b/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/vm/frames.h b/src/vm/frames.h
index 41e35bf0f4d324c49cac4f4c4a194146af32c1c1..7b55c3688481f413dd89accddb6456a9e366b84c 100644 (file)
--- a/src/vm/frames.h
+++ b/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/vm/runtimehandles.cpp b/src/vm/runtimehandles.cpp
index d0f36cb95c4b0f81caaeabfd3e5bedc6d5a7e8f7..601a8bc8f96af675c7e8fe2fc662d26a30108593 100644 (file)
--- a/src/vm/runtimehandles.cpp
+++ b/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/vm/runtimehandles.h b/src/vm/runtimehandles.h
index 73aa58bd87ce30008c7f26e9df8d50aa8703a861..ecb5c154c12a8883021a3ba398ad82d607151032 100644 (file)
--- a/src/vm/runtimehandles.h
+++ b/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);