From 04513facbf7d42cdceae5da6e2619440dfeac2ba Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 6 Mar 2017 19:31:24 -0800 Subject: [PATCH] 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 --- src/coreclr/src/vm/jitinterface.cpp | 116 +++++++++++++-------- .../src/JIT/opt/Devirtualization/GitHub_9945_2.il | 53 ++++++++++ .../JIT/opt/Devirtualization/GitHub_9945_2.ilproj | 43 ++++++++ 3 files changed, 167 insertions(+), 45 deletions(-) create mode 100644 src/coreclr/tests/src/JIT/opt/Devirtualization/GitHub_9945_2.il create mode 100644 src/coreclr/tests/src/JIT/opt/Devirtualization/GitHub_9945_2.ilproj diff --git a/src/coreclr/src/vm/jitinterface.cpp b/src/coreclr/src/vm/jitinterface.cpp index b8215754..45239c4 100644 --- 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 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 index 0000000..b1e2ecf --- /dev/null +++ b/src/coreclr/tests/src/JIT/opt/Devirtualization/GitHub_9945_2.ilproj @@ -0,0 +1,43 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {95DFC527-4DC1-495E-97D7-E94EE1F7140D} + Exe + Properties + 512 + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + $(ProgramFiles)\Common Files\microsoft shared\VSTT\11.0\UITestExtensionPackages + ..\..\ + 7a9bfb7d + $(DefineConstants);STATIC + true + + + + + + + + + False + + + + None + True + + + + + + + + + + + \ No newline at end of file -- 2.7.4