Fix a bug in implementation of ICastable when CoreCLR throws exceptio…

…n on IL isinst if IsInstanceOfInterface() sets the exception out param. Correct behavior: If false is returned when IsInstanceOfInterface() called as part of a castclass then the usual InvalidCastException will be thrown unless an alternate exception is assigned to the castError output parameter. This parameter is ignored on successful casts or during the evaluation of an isinst (which returns null rather than throwing on error).
DrewScoggins · May 30, 2015 · 3b7181a · 3b7181a
1 parent 718a024
commit 3b7181a
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 23 deletions.
diff --git a/src/vm/interpreter.cpp b/src/vm/interpreter.cpp
@@ -6172,10 +6172,9 @@ void Interpreter::CastClass()
     Object * pObj = OpStackGet<Object*>(idx);
     if (pObj != NULL)
     {
-        if (!ObjIsInstanceOf(pObj, TypeHandle(cls)))
+        if (!ObjIsInstanceOf(pObj, TypeHandle(cls), TRUE))
         {
-            OBJECTREF oref = ObjectToOBJECTREF(OpStackGet<Object*>(idx));
-            COMPlusThrowInvalidCastException(&oref, TypeHandle(cls));
+            UNREACHABLE(); //ObjIsInstanceOf will throw if cast can't be done
         }
     }
 
@@ -8822,8 +8821,10 @@ void Interpreter::UnboxAny()
     if ((boxTypeAttribs & CORINFO_FLG_VALUECLASS) == 0)
     {
         Object* obj = OpStackGet<Object*>(tos);
-        if (obj != NULL && !ObjIsInstanceOf(obj, TypeHandle(boxTypeClsHnd)))
-            COMPlusThrowInvalidCastException(&ObjectToOBJECTREF(obj), TypeHandle(boxTypeClsHnd));
+        if (obj != NULL && !ObjIsInstanceOf(obj, TypeHandle(boxTypeClsHnd), TRUE))
+        {
+            UNREACHABLE(); //ObjIsInstanceOf will throw if cast can't be done
+        }
     }
     else
     {

diff --git a/src/vm/jithelpers.cpp b/src/vm/jithelpers.cpp
@@ -2395,7 +2395,7 @@ TypeHandle::CastResult STDCALL ObjIsInstanceOfNoGC(Object *pObject, TypeHandle t
     return pMT->CanCastToClassOrInterfaceNoGC(toTypeHnd.AsMethodTable());
 }
 
-BOOL ObjIsInstanceOf(Object *pObject, TypeHandle toTypeHnd)
+BOOL ObjIsInstanceOf(Object *pObject, TypeHandle toTypeHnd, BOOL throwCastException)
 {
     CONTRACTL {
         THROWS;
@@ -2464,15 +2464,20 @@ BOOL ObjIsInstanceOf(Object *pObject, TypeHandle toTypeHnd)
         CALL_MANAGED_METHOD(fCast, BOOL, args);
         INDEBUG(managedType = NULL); // managedType isn't protected during the call
 
-        if (exception != NULL)
+        if (!fCast && throwCastException && exception != NULL)
         {
             RealCOMPlusThrow(exception);
         }
         GCPROTECT_END(); //exception
     }
 #endif // FEATURE_ICASTABLE
 
-    GCPROTECT_END();
+    if (!fCast && throwCastException) 
+    {
+        COMPlusThrowInvalidCastException(&obj, toTypeHnd);
+    }    
+
+    GCPROTECT_END(); // obj
 
     return(fCast);
 }
@@ -2805,8 +2810,10 @@ NOINLINE HCIMPL2(Object *, JITutil_ChkCastAny, CORINFO_CLASS_HANDLE type, Object
     TypeHandle clsHnd(type);
 
     HELPER_METHOD_FRAME_BEGIN_RET_1(oref);
-    if (!ObjIsInstanceOf(OBJECTREFToObject(oref), clsHnd))
-        COMPlusThrowInvalidCastException(&oref, clsHnd);
+    if (!ObjIsInstanceOf(OBJECTREFToObject(oref), clsHnd, TRUE))
+    {
+        UNREACHABLE(); //ObjIsInstanceOf will throw if cast can't be done
+    }
     HELPER_METHOD_FRAME_END();
 
     return OBJECTREFToObject(oref);

diff --git a/src/vm/jitinterface.h b/src/vm/jitinterface.h
@@ -1616,7 +1616,7 @@ FCDECL0(VOID, JIT_PollGC);
 EXTERN_C FCDECL0(VOID, JIT_PollGC_Nop);
 #endif
 
-BOOL ObjIsInstanceOf(Object *pObject, TypeHandle toTypeHnd);
+BOOL ObjIsInstanceOf(Object *pObject, TypeHandle toTypeHnd, BOOL throwCastException = FALSE);
 EXTERN_C TypeHandle::CastResult STDCALL ObjIsInstanceOfNoGC(Object *pObject, TypeHandle toTypeHnd);
 
 #ifdef _WIN64

diff --git a/src/vm/prestub.cpp b/src/vm/prestub.cpp
@@ -2617,21 +2617,17 @@ extern "C" SIZE_T STDCALL DynamicHelperWorker(TransitionBlock * pTransitionBlock
         {
         case ENCODE_ISINSTANCEOF_HELPER:
         case ENCODE_CHKCAST_HELPER:
-            if (*(Object **)pArgument == NULL || ObjIsInstanceOf(*(Object **)pArgument, th))
             {
-                result = (SIZE_T)(*(Object **)pArgument);
-            }
-            else
-            {
-                if (kind == ENCODE_CHKCAST_HELPER)
+                BOOL throwInvalidCast = (kind == ENCODE_CHKCAST_HELPER);
+                if (*(Object **)pArgument == NULL || ObjIsInstanceOf(*(Object **)pArgument, th, throwInvalidCast))
                 {
-                    OBJECTREF obj = ObjectToOBJECTREF(*(Object **)pArgument);
-                    GCPROTECT_BEGIN(obj);
-                    COMPlusThrowInvalidCastException(&obj, th);
-                    GCPROTECT_END();
+                    result = (SIZE_T)(*(Object **)pArgument);
+                }
+                else
+                {
+                    _ASSERTE (!throwInvalidCast);
+                    result = NULL;
                 }
-
-                result = NULL;
             }
             break;
         case ENCODE_STATIC_BASE_NONGC_HELPER:

diff --git a/tests/src/Interop/ICastable/Castable.cs b/tests/src/Interop/ICastable/Castable.cs
@@ -183,6 +183,11 @@ public static int Main()
             } 
             catch (CastableException) {}
 
+            Assert(!(nullCastable is IRetThis), "null castable shouldn't be allowed to be casted to anything");
+
+            var shouldBeNull = nullCastable as IRetThis;
+            Assert(shouldBeNull == null, "shouldBeNull should be assigned null");
+
             object badCastable = new BadCastable();
             try
             {