Fix type loader not recognizing overridden method (#56337)

* Fix type loader not recognizing overridden method
- Result of a bad change when removing support for full stub dispatch in .NET 4.0 timeframe (circa 2008)
- Caused issue when the following set of conditions were all true
  - The type implements an interface
  - The interface has more virtual methods on it than the number of virtual methods on the base type of the type.
  - The base type implements the interface partially (and the partial implementation has a slot number greater than the number of virtual methods on the base type + its base types)
  - The type does not re-implement the interface methods implemented by the base type.
- The comment referred to situations where stub dispatch was used to resolve non-virtual calls which is a very long time removed feature and is not applicable to today's codebase.
- Not reachable with versions of C# that shipped before the default interfaces feature, but with that feature became easily reachable. Has been a bug since .NET 4 for handwritten IL.

Fixes #44533

diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp
index 8bdc5f0c2a892ba3932d6033d6764017c47f07b4..1007f0b28c6e631d5eaa20984535fc9786d6d083 100644 (file)
--- a/src/coreclr/vm/methodtable.cpp
+++ b/src/coreclr/vm/methodtable.cpp
@@ -7950,7 +7950,8 @@ MethodTable::MethodData::ProcessMap(
     UINT32                    cTypeIDs,
     MethodTable *             pMT,
     UINT32                    iCurrentChainDepth,
-    MethodDataEntry *         rgWorkingData)
+    MethodDataEntry *         rgWorkingData,
+    size_t                    cWorkingData)
 {
     LIMITED_METHOD_CONTRACT;
 
@@ -7961,10 +7962,8 @@ MethodTable::MethodData::ProcessMap(
             if (it.Entry()->GetTypeID() == rgTypeIDs[nTypeIDIndex])
             {
                 UINT32 curSlot = it.Entry()->GetSlotNumber();
-                // If we're processing an interface, or it's for a virtual, or it's for a non-virtual
-                // for the most derived type, we want to process the entry. In other words, we
-                // want to ignore non-virtuals for parent classes.
-                if ((curSlot < pMT->GetNumVirtuals()) || (iCurrentChainDepth == 0))
+                // This if check is defensive, and should never fail
+                if (curSlot < cWorkingData)
                 {
                     MethodDataEntry * pCurEntry = &rgWorkingData[curSlot];
                     if (!pCurEntry->IsDeclInit() && !pCurEntry->IsImplInit())
@@ -8331,7 +8330,7 @@ MethodTable::MethodDataInterfaceImpl::PopulateNextLevel()
 
     if (m_cDeclTypeIDs != 0)
     {   // We got the TypeIDs from TypeLoader, use them
-        ProcessMap(m_rgDeclTypeIDs, m_cDeclTypeIDs, pMTCur, iChainDepth, GetEntryData());
+        ProcessMap(m_rgDeclTypeIDs, m_cDeclTypeIDs, pMTCur, iChainDepth, GetEntryData(), GetNumVirtuals());
     }
     else
     {   // We should decode all interface duplicates of code:m_pDecl
@@ -8348,7 +8347,7 @@ MethodTable::MethodDataInterfaceImpl::PopulateNextLevel()
                 INDEBUG(dbg_fInterfaceFound = TRUE);
                 DispatchMapTypeID declTypeID = DispatchMapTypeID::InterfaceClassID(it.GetIndex());
 
-                ProcessMap(&declTypeID, 1, pMTCur, iChainDepth, GetEntryData());
+                ProcessMap(&declTypeID, 1, pMTCur, iChainDepth, GetEntryData(), GetNumVirtuals());
             }
         }
         // The interface code:m_Decl should be found at least once in the interface map of code:m_pImpl,

diff --git a/src/coreclr/vm/methodtable.h b/src/coreclr/vm/methodtable.h
index df712a378d4ecee95249e777d91ac79abafac073..816da7ee0f339b0dfef7e39d26fe1702f9a9ebae 100644 (file)
--- a/src/coreclr/vm/methodtable.h
+++ b/src/coreclr/vm/methodtable.h
@@ -3242,7 +3242,8 @@ public:
             UINT32                    cTypeIDs,
             MethodTable *             pMT,
             UINT32                    cCurrentChainDepth,
-            MethodDataEntry *         rgWorkingData);
+            MethodDataEntry *         rgWorkingData,
+            size_t                    cWorkingData);
     };  // class MethodData
 
     typedef ::Holder < MethodData *, MethodData::HolderAcquire, MethodData::HolderRelease > MethodDataHolder;

diff --git a/src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github44533.cs b/src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github44533.cs
new file mode 100644 (file)
index 0000000..84b29ec
--- /dev/null
+++ b/src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github44533.cs
@@ -0,0 +1,50 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Linq;
+
+namespace BugInReflection
+{
+    class Program
+    {
+        static int Main(string[] args)
+        {
+            // This tests the ability to load a type when
+            // 1. The type implements an interface
+            // 2. The interface has more virtual methods on it than the number of virtual methods
+            //    on the base type of the type.
+            // 3. The base type implements the interface partially (and the partial implementation
+            //    has a slot number greater than the number of virtual methods on the base type + its base types)
+            // 4. The type does not re-implement the interface methods implemented by the base type.
+            //
+            // This is permitted in IL, but is a situation which can only be reached with .il code in versions of
+            // .NET prior to .NET 5.
+            //
+            // In .NET 5, this became straightforward to hit with default interface methods.
+            //
+            // To workaround the bug in .NET 5, simply make the Post class have enough virtual methods to match
+            // the number of virtual methods on the ITitle interface.
+            new BlogPost();
+            return 100;
+        }
+    }
+
+    public interface ITitle
+    {
+        // commenting out one or more of these NotMapped properties fixes the problem
+        public string Temp1 => "abcd";
+        public string Temp2 => "abcd";
+        public string Temp3 => "abcd";
+        public string Temp4 => "abcd";
+        public string Temp5 => "abcd";
+
+        public string Title { get; set; } // commenting out this property also fixes the problem
+    }
+
+    public abstract class Post : ITitle // making this non-abstract also fixes the problem
+    {
+        public string Title { get; set; }
+    }
+    public class BlogPost : Post { }
+}

diff --git a/src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github44533.csproj b/src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github44533.csproj
new file mode 100644 (file)
index 0000000..a57478d
--- /dev/null
+++ b/src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github44533.csproj
@@ -0,0 +1,11 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <OutputType>Exe</OutputType>
+    <CLRTestKind>BuildAndRun</CLRTestKind>
+    <CLRTestPriority>1</CLRTestPriority>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>