From 8026a5d9e1c02c104858b500022fb8bb8df4789d Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Mon, 1 Jun 2026 13:58:41 -0700 Subject: [PATCH 1/5] Fix COM-to-CLR wrong method dispatch with shared ComMethodTable On main (post-PR#126002), the COM-to-CLR dispatch uses UMEntryThunkData with a baked-in m_pManagedTarget resolved at layout time. When a ComMethodTable is shared between a base and derived class, and the derived class triggers layout first, the target is bound to the derived override. Base class objects then incorrectly dispatch to the derived method. Replace ImplementsInterfaceWithSameSlotsAsParent (which only checked for interface re-implementations in the dispatch map) with a COM-specific CanShareComMethodTableWithParent that also compares the resolved dispatch targets for each interface method. Since ImplementsInterfaceWithSameSlotsAsParent had no other callers, remove it entirely. Fixes https://github.com/dotnet/runtime/issues/127512 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/vm/comcallablewrapper.cpp | 75 +++++++++++++++++++++++---- src/coreclr/vm/methodtable.cpp | 33 ------------ src/coreclr/vm/methodtable.h | 5 -- 3 files changed, 65 insertions(+), 48 deletions(-) diff --git a/src/coreclr/vm/comcallablewrapper.cpp b/src/coreclr/vm/comcallablewrapper.cpp index 3bd5be1351ff10..9771f6ea920c34 100644 --- a/src/coreclr/vm/comcallablewrapper.cpp +++ b/src/coreclr/vm/comcallablewrapper.cpp @@ -4206,6 +4206,67 @@ ComMethodTable* ComCallWrapperTemplate::CreateComMethodTableForBasic(MethodTable RETURN pComMT; } +//-------------------------------------------------------------------------- +// Returns TRUE if the parent's ComMethodTable for pItfMT can be reused for +// pClassMT. This requires that no class between pClassMT and pParentMT has +// re-implemented pItfMT in its dispatch map, and that the interface methods +// resolve to the same MethodDescs on both pClassMT and pParentMT. +//-------------------------------------------------------------------------- +static BOOL CanShareComMethodTableWithParent(MethodTable *pClassMT, MethodTable *pParentMT, MethodTable *pItfMT) +{ + CONTRACTL + { + THROWS; + GC_TRIGGERS; + MODE_ANY; + PRECONDITION(pClassMT != NULL && !pClassMT->IsInterface()); + PRECONDITION(pParentMT != NULL && !pParentMT->IsInterface()); + PRECONDITION(pItfMT != NULL && pItfMT->IsInterface()); + } + CONTRACTL_END; + + // Check for explicit interface re-implementations in the dispatch map. + MethodTable *pMT = pClassMT; + do + { + DispatchMap::EncodedMapIterator mapIt(pMT); + for (; mapIt.IsValid(); mapIt.Next()) + { + DispatchMapEntry *pEntry = mapIt.Entry(); + if (pMT->DispatchMapTypeMatchesMethodTable(pEntry->GetTypeID(), pItfMT)) + { + return FALSE; + } + } + + pMT = pMT->GetParentMethodTable(); + _ASSERTE(pMT != NULL); + } + while (pMT != pParentMT); + + // Check that interface methods resolve to the same MethodDescs on both + // this class and pParentMT. With the baked-in dispatch target model, the + // ComMethodTable stores the resolved MethodDesc at layout time, so the + // table can only be shared if the targets are identical. + for (unsigned i = 0; i < pItfMT->GetNumVirtuals(); i++) + { + MethodDesc *pItfMD = pItfMT->GetMethodDescForSlot_NoThrow(i); + if (pItfMD == NULL || pItfMD->IsAsyncMethod()) + continue; + + DispatchSlot childSlot(pClassMT->FindDispatchSlotForInterfaceMD(pItfMD, FALSE /* throwOnConflict */)); + DispatchSlot parentSlot(pParentMT->FindDispatchSlotForInterfaceMD(pItfMD, FALSE /* throwOnConflict */)); + + if (childSlot.IsNull() || parentSlot.IsNull()) + return FALSE; + + if (childSlot.GetMethodDesc() != parentSlot.GetMethodDesc()) + return FALSE; + } + + return TRUE; +} + //-------------------------------------------------------------------------- // Creates a ComMethodTable for an interface and stores it in the m_rgpIPtr array. //-------------------------------------------------------------------------- @@ -4222,22 +4283,16 @@ ComMethodTable *ComCallWrapperTemplate::InitializeForInterface(MethodTable *pPar ComMethodTable *pItfComMT = NULL; if (m_pParent != NULL) { - pItfComMT = m_pParent->GetComMTForItf(pItfMT); - if (pItfComMT != NULL) + // Check if we can reuse the parent's ComMethodTable for this interface. + ComMethodTable *pParentComMT = m_pParent->GetComMTForItf(pItfMT); + if (pParentComMT != NULL && CanShareComMethodTableWithParent(m_thClass.GetMethodTable(), pParentMT, pItfMT)) { - // if the parent COM MT is not a trivial aggregate, simple MethodTable slot check is enough - if (!m_thClass.GetMethodTable()->ImplementsInterfaceWithSameSlotsAsParent(pItfMT, pParentMT)) - { - // the interface is implemented by parent but this class reimplemented - // its method(s) so we will need to build a new COM vtable for it - pItfComMT = NULL; - } + pItfComMT = pParentComMT; } } if (pItfComMT == NULL) { - // we couldn't use parent's vtable so we create a new one pItfComMT = CreateComMethodTableForInterface(pItfMT); } diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index 50e628b85b0cec..935d282119e605 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -5977,39 +5977,6 @@ UINT32 MethodTable::LookupTypeID() return AppDomain::GetCurrentDomain()->LookupTypeID(pMT); } -//========================================================================================== -BOOL MethodTable::ImplementsInterfaceWithSameSlotsAsParent(MethodTable *pItfMT, MethodTable *pParentMT) -{ - CONTRACTL - { - THROWS; - GC_TRIGGERS; - PRECONDITION(!IsInterface() && !pParentMT->IsInterface()); - PRECONDITION(pItfMT->IsInterface()); - } CONTRACTL_END; - - MethodTable *pMT = this; - do - { - DispatchMap::EncodedMapIterator it(pMT); - for (; it.IsValid(); it.Next()) - { - DispatchMapEntry *pCurEntry = it.Entry(); - if (DispatchMapTypeMatchesMethodTable(pCurEntry->GetTypeID(), pItfMT)) - { - // this class and its parents up to pParentMT must have no mappings for the interface - return FALSE; - } - } - - pMT = pMT->GetParentMethodTable(); - _ASSERTE(pMT != NULL); - } - while (pMT != pParentMT); - - return TRUE; -} - #endif // !DACCESS_COMPILE //========================================================================================== diff --git a/src/coreclr/vm/methodtable.h b/src/coreclr/vm/methodtable.h index 3a7c7e0cf2b2f7..b72bfe7a7f0fda 100644 --- a/src/coreclr/vm/methodtable.h +++ b/src/coreclr/vm/methodtable.h @@ -2572,11 +2572,6 @@ class MethodTable MethodTable *LookupDispatchMapType(DispatchMapTypeID typeID); bool DispatchMapTypeMatchesMethodTable(DispatchMapTypeID typeID, MethodTable* pMT); - // Determines whether all methods in the given interface have their final implementing - // slot in a parent class. I.e. if this returns TRUE, it is trivial (no VSD lookup) to - // dispatch pItfMT methods on this class if one knows how to dispatch them on pParentMT. - BOOL ImplementsInterfaceWithSameSlotsAsParent(MethodTable *pItfMT, MethodTable *pParentMT); - // Try to resolve a given static virtual method override on this type. Return nullptr // when not found. MethodDesc *TryResolveVirtualStaticMethodOnThisType(MethodTable* pInterfaceType, MethodDesc* pInterfaceMD, ResolveVirtualStaticMethodFlags resolveVirtualStaticMethodFlags, ClassLoadLevel level); From d4eceb1b7b413becbd0b1abda6ec43d01b5e97ce Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Mon, 1 Jun 2026 13:58:44 -0700 Subject: [PATCH 2/5] Add regression test for COM virtual method override dispatch Test that COM-to-CLR dispatch correctly resolves virtual method overrides regardless of whether the base or derived class is accessed via COM first. Uses separate type sets to independently validate both orderings within the same process. Regression test for https://github.com/dotnet/runtime/issues/127512 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../VirtualMethodOverrideTest.cs | 120 ++++++++++++++++++ .../VirtualMethodOverrideTest.csproj | 10 ++ 2 files changed, 130 insertions(+) create mode 100644 src/tests/Interop/COM/VirtualMethodOverride/VirtualMethodOverrideTest.cs create mode 100644 src/tests/Interop/COM/VirtualMethodOverride/VirtualMethodOverrideTest.csproj diff --git a/src/tests/Interop/COM/VirtualMethodOverride/VirtualMethodOverrideTest.cs b/src/tests/Interop/COM/VirtualMethodOverride/VirtualMethodOverrideTest.cs new file mode 100644 index 00000000000000..fb15cf17ac7463 --- /dev/null +++ b/src/tests/Interop/COM/VirtualMethodOverride/VirtualMethodOverrideTest.cs @@ -0,0 +1,120 @@ +// 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.Runtime.InteropServices; +using Xunit; + +[ComVisible(true)] +[Guid("A1111111-0000-0000-0000-000000000001")] +public interface IFoo +{ + void DoWork(); +} + +[ComVisible(true)] +[Guid("A1111111-0000-0000-0000-000000000002")] +[ComDefaultInterface(typeof(IFoo))] +public class Foo : IFoo +{ + public virtual void DoWork() => VirtualMethodOverrideTest.LastCalledType = nameof(Foo); +} + +[ComVisible(true)] +[Guid("A1111111-0000-0000-0000-000000000003")] +[ComDefaultInterface(typeof(IFoo))] +public class FooDerived : Foo +{ + public override void DoWork() => VirtualMethodOverrideTest.LastCalledType = nameof(FooDerived); +} + +[ComVisible(true)] +[Guid("B2222222-0000-0000-0000-000000000001")] +public interface IBar +{ + void DoWork(); +} + +[ComVisible(true)] +[Guid("B2222222-0000-0000-0000-000000000002")] +[ComDefaultInterface(typeof(IBar))] +public class Bar : IBar +{ + public virtual void DoWork() => VirtualMethodOverrideTest.LastCalledType = nameof(Bar); +} + +[ComVisible(true)] +[Guid("B2222222-0000-0000-0000-000000000003")] +[ComDefaultInterface(typeof(IBar))] +public class BarDerived : Bar +{ + public override void DoWork() => VirtualMethodOverrideTest.LastCalledType = nameof(BarDerived); +} + +/// +/// Tests that COM-to-CLR dispatch correctly resolves virtual method overrides +/// regardless of whether the base or derived class is accessed via COM first. +/// +public class VirtualMethodOverrideTest +{ + internal static string? LastCalledType; + + [UnmanagedFunctionPointer(CallingConvention.StdCall)] + delegate int DoWorkDelegate(IntPtr pThis); + + private static int CallDoWork(IntPtr pInterface, int slot) + { + IntPtr vtbl = Marshal.ReadIntPtr(pInterface); + IntPtr fnPtr = Marshal.ReadIntPtr(vtbl, slot * IntPtr.Size); + Assert.NotEqual(IntPtr.Zero, fnPtr); + + var fn = Marshal.GetDelegateForFunctionPointer(fnPtr); + return fn(pInterface); + } + + [Fact] + public static void DerivedFirst() + { + int doWorkSlot = Marshal.GetStartComSlot(typeof(IFoo)); + IntPtr pDerived = Marshal.GetComInterfaceForObject(new FooDerived(), typeof(IFoo)); + IntPtr pBase = Marshal.GetComInterfaceForObject(new Foo(), typeof(IFoo)); + try + { + LastCalledType = null; + Assert.True(CallDoWork(pDerived, doWorkSlot) >= 0); + Assert.Equal(nameof(FooDerived), LastCalledType); + + LastCalledType = null; + Assert.True(CallDoWork(pBase, doWorkSlot) >= 0); + Assert.Equal(nameof(Foo), LastCalledType); + } + finally + { + Marshal.Release(pDerived); + Marshal.Release(pBase); + } + } + + [Fact] + public static void BaseFirst() + { + int doWorkSlot = Marshal.GetStartComSlot(typeof(IBar)); + IntPtr pBase = Marshal.GetComInterfaceForObject(new Bar(), typeof(IBar)); + IntPtr pDerived = Marshal.GetComInterfaceForObject(new BarDerived(), typeof(IBar)); + try + { + LastCalledType = null; + Assert.True(CallDoWork(pBase, doWorkSlot) >= 0); + Assert.Equal(nameof(Bar), LastCalledType); + + LastCalledType = null; + Assert.True(CallDoWork(pDerived, doWorkSlot) >= 0); + Assert.Equal(nameof(BarDerived), LastCalledType); + } + finally + { + Marshal.Release(pBase); + Marshal.Release(pDerived); + } + } +} diff --git a/src/tests/Interop/COM/VirtualMethodOverride/VirtualMethodOverrideTest.csproj b/src/tests/Interop/COM/VirtualMethodOverride/VirtualMethodOverrideTest.csproj new file mode 100644 index 00000000000000..412688de2f640c --- /dev/null +++ b/src/tests/Interop/COM/VirtualMethodOverride/VirtualMethodOverrideTest.csproj @@ -0,0 +1,10 @@ + + + true + true + true + + + + + From 93f08daf99cd1760689104fb6a029b91d8174581 Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Tue, 2 Jun 2026 12:00:57 -0700 Subject: [PATCH 3/5] Address PR feedback - Treat null MethodDesc from GetMethodDescForSlot_NoThrow as 'cannot share' instead of skipping the slot. - Move COM interface creation inside try/finally to avoid leaking the first pointer if the second creation throws. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/vm/comcallablewrapper.cpp | 5 +++- .../VirtualMethodOverrideTest.cs | 25 +++++++++++++------ 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/src/coreclr/vm/comcallablewrapper.cpp b/src/coreclr/vm/comcallablewrapper.cpp index 9771f6ea920c34..0d7f3cca4822ea 100644 --- a/src/coreclr/vm/comcallablewrapper.cpp +++ b/src/coreclr/vm/comcallablewrapper.cpp @@ -4251,7 +4251,10 @@ static BOOL CanShareComMethodTableWithParent(MethodTable *pClassMT, MethodTable for (unsigned i = 0; i < pItfMT->GetNumVirtuals(); i++) { MethodDesc *pItfMD = pItfMT->GetMethodDescForSlot_NoThrow(i); - if (pItfMD == NULL || pItfMD->IsAsyncMethod()) + if (pItfMD == NULL) + return FALSE; + + if (pItfMD->IsAsyncMethod()) continue; DispatchSlot childSlot(pClassMT->FindDispatchSlotForInterfaceMD(pItfMD, FALSE /* throwOnConflict */)); diff --git a/src/tests/Interop/COM/VirtualMethodOverride/VirtualMethodOverrideTest.cs b/src/tests/Interop/COM/VirtualMethodOverride/VirtualMethodOverrideTest.cs index fb15cf17ac7463..67b643361e7a05 100644 --- a/src/tests/Interop/COM/VirtualMethodOverride/VirtualMethodOverrideTest.cs +++ b/src/tests/Interop/COM/VirtualMethodOverride/VirtualMethodOverrideTest.cs @@ -76,10 +76,13 @@ private static int CallDoWork(IntPtr pInterface, int slot) public static void DerivedFirst() { int doWorkSlot = Marshal.GetStartComSlot(typeof(IFoo)); - IntPtr pDerived = Marshal.GetComInterfaceForObject(new FooDerived(), typeof(IFoo)); - IntPtr pBase = Marshal.GetComInterfaceForObject(new Foo(), typeof(IFoo)); + IntPtr pDerived = IntPtr.Zero; + IntPtr pBase = IntPtr.Zero; try { + pDerived = Marshal.GetComInterfaceForObject(new FooDerived(), typeof(IFoo)); + pBase = Marshal.GetComInterfaceForObject(new Foo(), typeof(IFoo)); + LastCalledType = null; Assert.True(CallDoWork(pDerived, doWorkSlot) >= 0); Assert.Equal(nameof(FooDerived), LastCalledType); @@ -90,8 +93,11 @@ public static void DerivedFirst() } finally { - Marshal.Release(pDerived); - Marshal.Release(pBase); + if (pDerived != IntPtr.Zero) + Marshal.Release(pDerived); + + if (pBase != IntPtr.Zero) + Marshal.Release(pBase); } } @@ -99,10 +105,13 @@ public static void DerivedFirst() public static void BaseFirst() { int doWorkSlot = Marshal.GetStartComSlot(typeof(IBar)); - IntPtr pBase = Marshal.GetComInterfaceForObject(new Bar(), typeof(IBar)); - IntPtr pDerived = Marshal.GetComInterfaceForObject(new BarDerived(), typeof(IBar)); + IntPtr pBase = IntPtr.Zero; + IntPtr pDerived = IntPtr.Zero; try { + pBase = Marshal.GetComInterfaceForObject(new Bar(), typeof(IBar)); + pDerived = Marshal.GetComInterfaceForObject(new BarDerived(), typeof(IBar)); + LastCalledType = null; Assert.True(CallDoWork(pBase, doWorkSlot) >= 0); Assert.Equal(nameof(Bar), LastCalledType); @@ -113,8 +122,8 @@ public static void BaseFirst() } finally { - Marshal.Release(pBase); - Marshal.Release(pDerived); + if (pBase != IntPtr.Zero) Marshal.Release(pBase); + if (pDerived != IntPtr.Zero) Marshal.Release(pDerived); } } } From 3e7686c821e0a3bbb1ed1cb1c41cff052b06a456 Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Wed, 3 Jun 2026 13:36:09 -0700 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: Aaron R Robinson --- src/coreclr/vm/comcallablewrapper.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/coreclr/vm/comcallablewrapper.cpp b/src/coreclr/vm/comcallablewrapper.cpp index 0d7f3cca4822ea..217fb83b986388 100644 --- a/src/coreclr/vm/comcallablewrapper.cpp +++ b/src/coreclr/vm/comcallablewrapper.cpp @@ -4212,7 +4212,7 @@ ComMethodTable* ComCallWrapperTemplate::CreateComMethodTableForBasic(MethodTable // re-implemented pItfMT in its dispatch map, and that the interface methods // resolve to the same MethodDescs on both pClassMT and pParentMT. //-------------------------------------------------------------------------- -static BOOL CanShareComMethodTableWithParent(MethodTable *pClassMT, MethodTable *pParentMT, MethodTable *pItfMT) +static bool CanShareComMethodTableWithParent(MethodTable* pClassMT, MethodTable* pParentMT, MethodTable* pItfMT) { CONTRACTL { @@ -4226,7 +4226,7 @@ static BOOL CanShareComMethodTableWithParent(MethodTable *pClassMT, MethodTable CONTRACTL_END; // Check for explicit interface re-implementations in the dispatch map. - MethodTable *pMT = pClassMT; + MethodTable* pMT = pClassMT; do { DispatchMap::EncodedMapIterator mapIt(pMT); @@ -4235,7 +4235,7 @@ static BOOL CanShareComMethodTableWithParent(MethodTable *pClassMT, MethodTable DispatchMapEntry *pEntry = mapIt.Entry(); if (pMT->DispatchMapTypeMatchesMethodTable(pEntry->GetTypeID(), pItfMT)) { - return FALSE; + return false; } } @@ -4252,7 +4252,7 @@ static BOOL CanShareComMethodTableWithParent(MethodTable *pClassMT, MethodTable { MethodDesc *pItfMD = pItfMT->GetMethodDescForSlot_NoThrow(i); if (pItfMD == NULL) - return FALSE; + return false; if (pItfMD->IsAsyncMethod()) continue; @@ -4261,13 +4261,13 @@ static BOOL CanShareComMethodTableWithParent(MethodTable *pClassMT, MethodTable DispatchSlot parentSlot(pParentMT->FindDispatchSlotForInterfaceMD(pItfMD, FALSE /* throwOnConflict */)); if (childSlot.IsNull() || parentSlot.IsNull()) - return FALSE; + return false; if (childSlot.GetMethodDesc() != parentSlot.GetMethodDesc()) - return FALSE; + return false; } - return TRUE; + return true; } //-------------------------------------------------------------------------- @@ -4287,7 +4287,7 @@ ComMethodTable *ComCallWrapperTemplate::InitializeForInterface(MethodTable *pPar if (m_pParent != NULL) { // Check if we can reuse the parent's ComMethodTable for this interface. - ComMethodTable *pParentComMT = m_pParent->GetComMTForItf(pItfMT); + ComMethodTable* pParentComMT = m_pParent->GetComMTForItf(pItfMT); if (pParentComMT != NULL && CanShareComMethodTableWithParent(m_thClass.GetMethodTable(), pParentMT, pItfMT)) { pItfComMT = pParentComMT; From 94b7e10d1a71d701203aa45225598f1d07b7e61c Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Wed, 3 Jun 2026 14:05:46 -0700 Subject: [PATCH 5/5] Switch to assert --- src/coreclr/vm/comcallablewrapper.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/vm/comcallablewrapper.cpp b/src/coreclr/vm/comcallablewrapper.cpp index 217fb83b986388..a04559c8ea9030 100644 --- a/src/coreclr/vm/comcallablewrapper.cpp +++ b/src/coreclr/vm/comcallablewrapper.cpp @@ -4251,8 +4251,7 @@ static bool CanShareComMethodTableWithParent(MethodTable* pClassMT, MethodTable* for (unsigned i = 0; i < pItfMT->GetNumVirtuals(); i++) { MethodDesc *pItfMD = pItfMT->GetMethodDescForSlot_NoThrow(i); - if (pItfMD == NULL) - return false; + _ASSERTE(pItfMD != NULL); if (pItfMD->IsAsyncMethod()) continue;