From d43754c8d2a93b21bc9ec0177db8700865d00c8f Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 26 Jul 2021 15:18:38 -0700 Subject: [PATCH 1/2] Fix type loader not recognizing overriden 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 --- src/coreclr/vm/methodtable.cpp | 13 +++-- src/coreclr/vm/methodtable.h | 3 +- .../regressions/github44533.cs | 47 +++++++++++++++++++ .../regressions/github44533.csproj | 11 +++++ 4 files changed, 66 insertions(+), 8 deletions(-) create mode 100644 src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github44533.cs create mode 100644 src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github44533.csproj diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index 8bdc5f0c2a892b..1007f0b28c6e63 100644 --- 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 df712a378d4ece..816da7ee0f339b 100644 --- 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 index 00000000000000..df2642e1a9d1fc --- /dev/null +++ b/src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github44533.cs @@ -0,0 +1,47 @@ +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 index 00000000000000..a57478d7fa6d2e --- /dev/null +++ b/src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github44533.csproj @@ -0,0 +1,11 @@ + + + true + Exe + BuildAndRun + 1 + + + + + From 1443fa57055b6c31be8f805d7d64cc8edcfe6a27 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 27 Jul 2021 11:46:41 -0700 Subject: [PATCH 2/2] Add license header --- .../DefaultInterfaceMethods/regressions/github44533.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github44533.cs b/src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github44533.cs index df2642e1a9d1fc..84b29ec5f976cf 100644 --- a/src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github44533.cs +++ b/src/tests/Loader/classloader/DefaultInterfaceMethods/regressions/github44533.cs @@ -1,3 +1,6 @@ +// 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;