Fix catches around uplevel callbacks

This has been a latent issue that we've been working around for a
while now. The runtime shouldn't allow exceptions leaking out of
callbacks to the class library code to be dispatched across it.
The problem, though, is that the class library exceptions are in
one type space and the typed catch clauses are in another. I had
hoped to use filters to fix this, but Roslyn doesn't implement
them without the type check against System.Object (even if there's
no source reason for the type check). However, I realized that I
could simply fix the runtime exception dispatch to be more
permissible. It now permits catch ([runtime]System.Object) to
catch all exceptions, even those from foreign type spaces.

The workarounds have been in the class library-implemented
callbacks themselves where they have to put their own try/catch
clauses, but I always felt uneasy about not having a backstop in
the runtime to enforce this invariant.

I also ran across an unnecessary try/catch/rethrow that is from the olden days, so I removed it.

2 files changed, 57 insertions, 29 deletions

diff --git a/src/Runtime.Base/src/System/Runtime/EEType.cs b/src/Runtime.Base/src/System/Runtime/EEType.cs
index 61e60cb65..d33112dd2 100644
--- a/src/Runtime.Base/src/System/Runtime/EEType.cs
+++ b/src/Runtime.Base/src/System/Runtime/EEType.cs
@@ -601,6 +601,34 @@ namespace System.Runtime
         {
             return ((pThis->_usFlags | pOther->_usFlags) & (ushort)EETypeFlags.ComplexCastingMask) == (ushort)EETypeKind.CanonicalEEType;
         }
+
+        internal bool IsEquivalentTo(EEType* pOtherEEType)
+        {
+            fixed (EEType* pThis = &this)
+            {
+                if (pThis == pOtherEEType)
+                    return true;
+
+                EEType* pThisEEType = pThis;
+
+                if (pThisEEType->IsCloned)
+                    pThisEEType = pThisEEType->CanonicalEEType;
+
+                if (pOtherEEType->IsCloned)
+                    pOtherEEType = pOtherEEType->CanonicalEEType;
+
+                if (pThisEEType == pOtherEEType)
+                    return true;
+
+                if (pThisEEType->IsParameterizedType && pOtherEEType->IsParameterizedType)
+                {
+                    return pThisEEType->RelatedParameterType->IsEquivalentTo(pOtherEEType->RelatedParameterType) &&
+                        pThisEEType->ParameterizedTypeShape == pOtherEEType->ParameterizedTypeShape;
+                }
+            }
+
+            return false;
+        }
     }
 
     // CS0169: The private field '{blah}' is never used
diff --git a/src/Runtime.Base/src/System/Runtime/ExceptionHandling.cs b/src/Runtime.Base/src/System/Runtime/ExceptionHandling.cs
index 4957d6b30..175bdd424 100644
--- a/src/Runtime.Base/src/System/Runtime/ExceptionHandling.cs
+++ b/src/Runtime.Base/src/System/Runtime/ExceptionHandling.cs
@@ -180,11 +180,7 @@ namespace System.Runtime
             }
             catch
             {
-                // Unfortunately, this catch turns into "catch (System.Object)", which will not catch 
-                // exceptions thrown from the class library because their objects do not derive from our 
-                // System.Object. 
-                //
-                // @TODO: Use a filtered catch whose filter always returns 'true'.
+                // disallow all exceptions leaking out of callbacks
             }
 
             // The classlib's funciton should never return and should not throw. If it does, then we fail our way...
@@ -256,11 +252,7 @@ namespace System.Runtime
             }
             catch
             {
-                // Unfortunately, this catch turns into "catch (System.Object)", which will not catch 
-                // exceptions thrown from the class library because their objects do not derive from our 
-                // System.Object. 
-                //
-                // @TODO: Use a filtered catch whose filter always returns 'true'.
+                // disallow all exceptions leaking out of callbacks
             }
 
             // The classlib's funciton should never return and should not throw. If it does, then we fail our way...
@@ -289,11 +281,7 @@ namespace System.Runtime
                 }
                 catch
                 {
-                    // Unfortunately, this catch turns into "catch (System.Object)", which will not catch 
-                    // exceptions thrown from the class library because their objects do not derive from our 
-                    // System.Object. 
-                    //
-                    // @TODO: Use a filtered catch whose filter always returns 'true'.
+                    // disallow all exceptions leaking out of callbacks
                 }
             }
         }
@@ -318,11 +306,7 @@ namespace System.Runtime
                 }
                 catch
                 {
-                    // Unfortunately, this catch turns into "catch (System.Object)", which will not catch 
-                    // exceptions thrown from the class library because their objects do not derive from our 
-                    // System.Object. 
-                    //
-                    // @TODO: Use a filtered catch whose filter always returns 'true'.
+                    // disallow all exceptions leaking out of callbacks
                 }
 
                 // If the helper fails to yield an object, then we fail-fast.
@@ -446,14 +430,7 @@ namespace System.Runtime
                     return s_theOOMException;
 
                 case ExceptionIDs.Overflow:
-                    try
-                    {
-                        return new OverflowException();
-                    }
-                    catch (OutOfMemoryException)
-                    {
-                        throw;
-                    }
+                    return new OverflowException();
 
                 default:
                     Debug.Assert(false, "unexpected ExceptionID");
@@ -851,7 +828,7 @@ namespace System.Runtime
                 // most containing.
                 if (clauseKind == RhEHClauseKind.RH_EH_CLAUSE_TYPED)
                 {
-                    if (System.Runtime.TypeCast.IsInstanceOfClass(exception, ehClause._pTargetType) != null)
+                    if (ShouldTypedClauseCatchThisException(exception, (EEType*)ehClause._pTargetType))
                     {
                         pHandler = (pbMethodStartAddress + ehClause._handlerOffset);
                         tryRegionIdx = curIdx;
@@ -876,6 +853,29 @@ namespace System.Runtime
             return false;
         }
 
+        static EEType* s_pLowLevelObjectType;
+
+        private static bool ShouldTypedClauseCatchThisException(Exception exception, EEType* pClauseType)
+        {
+            if (TypeCast.IsInstanceOfClass(exception, pClauseType) != null)
+                return true;
+
+            if (s_pLowLevelObjectType == null)
+            {
+                s_pLowLevelObjectType = new System.Object().EEType;
+            }
+
+            // This allows the typical try { } catch { }--which expands to a typed catch of System.Object--to work on 
+            // all objects when the clause is in the low level runtime code.  This special case is needed because 
+            // objects from foreign type systems are sometimes throw back up at runtime code and this is the only way
+            // to catch them outside of having a filter with no type check in it, which isn't currently possible to 
+            // write in C#.  See https://github.com/dotnet/roslyn/issues/4388
+            if (pClauseType->IsEquivalentTo(s_pLowLevelObjectType))
+                return true;
+
+            return false;
+        }
+
         private static void InvokeSecondPass(ref ExInfo exInfo, uint idxStart)
         {
             InvokeSecondPass(ref exInfo, idxStart, MaxTryRegionIdx);