Harden resolveVirtualMethod against invalid types (dotnet/coreclr#9989)

Split resolveVirtualMethod into a wrapper/helper to allow early
returns as failure reasons are discovered.

Before devirtualizing, ensure that the derived class claimed by the jit
is really a subclass of the class that introduces the virtual method.
Fail early if this is not the case.

Also add early out for interface call devirtualization. The jit currently
does not ask for this but asking for it should not trigger an assert.

Closes dotnet/coreclr#9945 (along with dotnet/coreclr#9959).

Commit migrated from https://github.com/dotnet/coreclr/commit/8902cdb044587ec713f25cb2a137b9532b98e7a2

diff --git a/src/coreclr/src/vm/jitinterface.cpp b/src/coreclr/src/vm/jitinterface.cpp
index b821575..45239c4 100644 (file)
--- a/src/coreclr/src/vm/jitinterface.cpp
+++ b/src/coreclr/src/vm/jitinterface.cpp
@@ -8701,73 +8701,99 @@ void CEEInfo::getMethodVTableOffset (CORINFO_METHOD_HANDLE methodHnd,
 }
 
 /*********************************************************************/
-CORINFO_METHOD_HANDLE CEEInfo::resolveVirtualMethod(CORINFO_METHOD_HANDLE methodHnd,
-                                                    CORINFO_CLASS_HANDLE derivedClass)
+static CORINFO_METHOD_HANDLE resolveVirtualMethodHelper(MethodDesc* callerMethod,
+                                                        CORINFO_METHOD_HANDLE baseMethod,
+                                                        CORINFO_CLASS_HANDLE derivedClass)
 {
-    CONTRACTL {
-        SO_TOLERANT;
-        NOTHROW;
-        GC_NOTRIGGER;
-        MODE_PREEMPTIVE;
-    } CONTRACTL_END;
-
-    CORINFO_METHOD_HANDLE result = nullptr;
+    STANDARD_VM_CONTRACT;
 
-    JIT_TO_EE_TRANSITION();
-
-    MethodDesc* method = GetMethod(methodHnd);
+    MethodDesc* pBaseMD = GetMethod(baseMethod);
+    MethodTable* pBaseMT = pBaseMD->GetMethodTable();
 
     // Method better be from a fully loaded class
-    _ASSERTE(method->IsRestored() && method->GetMethodTable()->IsFullyLoaded());
-
-    // Method better be virtual
-    _ASSERTE(method->IsVirtual());
+    _ASSERTE(pBaseMD->IsRestored() && pBaseMT->IsFullyLoaded());
 
     //@GENERICS: shouldn't be doing this for instantiated methods as they live elsewhere
-    _ASSERTE(!method->HasMethodInstantiation());
+    _ASSERTE(!pBaseMD->HasMethodInstantiation());
 
-    // Method's class better not be an interface
-    _ASSERTE(!method->GetMethodTable()->IsInterface());
+    // Interface call devirtualization is not yet supported.
+    if (pBaseMT->IsInterface())
+    {
+        return nullptr;
+    }
 
-    // Method better be in the vtable
-    WORD slot = method->GetSlot();
-    _ASSERTE(slot < method->GetMethodTable()->GetNumVirtuals());
+    // Method better be virtual
+    _ASSERTE(pBaseMD->IsVirtual());
 
-    // TODO: Derived class should have base class as parent...
     TypeHandle DerivedClsHnd(derivedClass);
+    MethodTable* pDerivedMT = DerivedClsHnd.GetMethodTable();
+    _ASSERTE(pDerivedMT->IsRestored() && pDerivedMT->IsFullyLoaded());
 
-    // If derived class is _Canon, we can't devirtualize.
-    if (DerivedClsHnd != TypeHandle(g_pCanonMethodTableClass))
+    // Can't devirtualize from __Canon.
+    if (DerivedClsHnd == TypeHandle(g_pCanonMethodTableClass))
     {
-        MethodTable* pMT = DerivedClsHnd.GetMethodTable();
+        return nullptr;
+    }
 
-        // MethodDescs returned to JIT at runtime are always fully loaded. Verify that it is the case.
-        _ASSERTE(pMT->IsRestored() && pMT->IsFullyLoaded());
+    // The derived class should be a subclass of the the base class.
+    MethodTable* pCheckMT = pDerivedMT;
 
-        MethodDesc* pDevirtMD = pMT->GetMethodDescForSlot(slot);
+    while (pCheckMT != nullptr)
+    {
+        if (pCheckMT->HasSameTypeDefAs(pBaseMT))
+        {
+            break;
+        }
 
-        _ASSERTE(pDevirtMD->IsRestored());
+        pCheckMT = pCheckMT->GetParentMethodTable();
+    }
+
+    if (pCheckMT == nullptr)
+    {
+        return nullptr;
+    }
+
+    // The base method should be in the base vtable
+    WORD slot = pBaseMD->GetSlot();
+    _ASSERTE(slot < pBaseMT->GetNumVirtuals());
+    _ASSERTE(pBaseMD == pBaseMT->GetMethodDescForSlot(slot));
 
-        // Allow devirtialization if jitting, or if prejitting and the
-        // method being jitted, the devirtualized class, and the
-        // devirtualized method are all in the same versioning bubble.
-        bool allowDevirt = true;
+    // Fetch the method that would be invoked if the class were
+    // exactly derived class. It is up to the jit to determine whether
+    // directly calling this method is correct.
+    MethodDesc* pDevirtMD = pDerivedMT->GetMethodDescForSlot(slot);
+    _ASSERTE(pDevirtMD->IsRestored());
 
 #ifdef FEATURE_READYTORUN_COMPILER
-        if (IsReadyToRunCompilation())
-        {
-            Assembly * pCallerAssembly = m_pMethodBeingCompiled->GetModule()->GetAssembly();
-            allowDevirt =
-                IsInSameVersionBubble(pCallerAssembly , pDevirtMD->GetModule()->GetAssembly())
-                && IsInSameVersionBubble(pCallerAssembly , pMT->GetAssembly());
-        }
-#endif
+    // Check if devirtualization is dependent upon cross-version
+    // bubble information and if so, disallow it.
+    if (IsReadyToRunCompilation())
+    {
+        Assembly* pCallerAssembly = callerMethod->GetModule()->GetAssembly();
+        bool allowDevirt =
+            IsInSameVersionBubble(pCallerAssembly , pDevirtMD->GetModule()->GetAssembly())
+            && IsInSameVersionBubble(pCallerAssembly , pDerivedMT->GetAssembly());
 
-        if (allowDevirt)
+        if (!allowDevirt)
         {
-            result = (CORINFO_METHOD_HANDLE) pDevirtMD;
+            return nullptr;
         }
     }
+#endif
+
+    return (CORINFO_METHOD_HANDLE) pDevirtMD;
+}
+
+CORINFO_METHOD_HANDLE CEEInfo::resolveVirtualMethod(CORINFO_METHOD_HANDLE methodHnd,
+                                                    CORINFO_CLASS_HANDLE derivedClass)
+{
+    STANDARD_VM_CONTRACT;
+
+    CORINFO_METHOD_HANDLE result = nullptr;
+
+    JIT_TO_EE_TRANSITION();
+
+    result = resolveVirtualMethodHelper(m_pMethodBeingCompiled, methodHnd, derivedClass);
 
     EE_TO_JIT_TRANSITION();
 

diff --git a/src/coreclr/tests/src/JIT/opt/Devirtualization/GitHub_9945_2.il b/src/coreclr/tests/src/JIT/opt/Devirtualization/GitHub_9945_2.il
new file mode 100644 (file)
index 0000000..6b76175
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/opt/Devirtualization/GitHub_9945_2.il
@@ -0,0 +1,53 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+.assembly extern mscorlib
+{
+}
+
+.assembly GitHub_9945_2
+{
+}
+
+.class public auto ansi beforefieldinit Base extends [mscorlib]System.Object
+{
+   // Need enough virtuals here to get F's slot past max virtual slot on object
+  .method public hidebysig newslot virtual instance void M0() cil managed { ret }
+  .method public hidebysig newslot virtual instance void M1() cil managed { ret }
+  .method public hidebysig newslot virtual instance void M2() cil managed { ret }
+  .method public hidebysig newslot virtual instance void M3() cil managed { ret }
+
+  .method public hidebysig newslot virtual instance void  F() cil managed
+  {
+    ldstr      "Base:F"
+    call       void [mscorlib]System.Console::WriteLine(string)
+    ret
+  }
+
+  .method public hidebysig specialname rtspecialname instance void  .ctor() cil managed
+  {
+    ldarg.0
+    call       instance void [mscorlib]System.Object::.ctor()
+    ret
+  }
+}
+
+.class public auto ansi beforefieldinit Test extends [mscorlib]System.Object
+{
+  .method public hidebysig static  int32  Main() cil managed
+  {
+    .locals (object)
+    .entrypoint
+    // Store ref in supertyped local to "forget" type
+    newobj     instance void Base::.ctor()
+    stloc.0
+    ldloc.0
+    // Now callvirt via the supertype
+    callvirt   instance void Base::F()
+    ldc.i4.s   100
+    ret
+  }
+}
+
+

diff --git a/src/coreclr/tests/src/JIT/opt/Devirtualization/GitHub_9945_2.ilproj b/src/coreclr/tests/src/JIT/opt/Devirtualization/GitHub_9945_2.ilproj
new file mode 100644 (file)
index 0000000..b1e2ecf
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/opt/Devirtualization/GitHub_9945_2.ilproj
@@ -0,0 +1,43 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <PropertyGroup>
+    <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
+    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
+    <AssemblyName>$(MSBuildProjectName)</AssemblyName>
+    <SchemaVersion>2.0</SchemaVersion>
+    <ProjectGuid>{95DFC527-4DC1-495E-97D7-E94EE1F7140D}</ProjectGuid>
+    <OutputType>Exe</OutputType>
+    <AppDesignerFolder>Properties</AppDesignerFolder>
+    <FileAlignment>512</FileAlignment>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <ReferencePath>$(ProgramFiles)\Common Files\microsoft shared\VSTT\11.0\UITestExtensionPackages</ReferencePath>
+    <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+    <NuGetPackageImportStamp>7a9bfb7d</NuGetPackageImportStamp>
+    <DefineConstants>$(DefineConstants);STATIC</DefineConstants>
+    <ReferenceLocalMscorlib>true</ReferenceLocalMscorlib>
+ </PropertyGroup>
+  <!-- Default configurations to help VS understand the configurations -->
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
+  </PropertyGroup>
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
+  </PropertyGroup>
+  <ItemGroup>
+    <CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
+      <Visible>False</Visible>
+    </CodeAnalysisDependentAssemblyPaths>
+  </ItemGroup>
+  <PropertyGroup>
+     <DebugType>None</DebugType>
+     <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="GitHub_9945_2.il" />
+  </ItemGroup>
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' ">
+  </PropertyGroup> 
+</Project>
\ No newline at end of file