Skip to content

Fix COM interface method calling wrong method for base/derived classes#128913

Open
elinor-fung wants to merge 4 commits into
dotnet:mainfrom
elinor-fung:com-virt-wrong-method
Open

Fix COM interface method calling wrong method for base/derived classes#128913
elinor-fung wants to merge 4 commits into
dotnet:mainfrom
elinor-fung:com-virt-wrong-method

Conversation

@elinor-fung
Copy link
Copy Markdown
Member

@elinor-fung elinor-fung commented Jun 2, 2026

When a COM interface is implemented by a class with a virtual method and a derived class overrides it, they share a ComMethodTable. With #126002, when a ComMethodTable lay out happens, we resolve each interface method into its target method and create a corresponding UMEntryThunkData. This means that if a derived class is used first, its override gets baked into the layout, so the base class will call the wrong method - and vice versa if the base class is used first.

This change prevents sharing of a ComMethodTable between a base/derived class if the actual targets of any interface method differ.

Found while investigating #127512.

cc @dotnet/appmodel @AaronRobinsonMSFT

Example setup:

[ComVisible(true)]
public interface IFoo
{
    void DoWork();
}
 
[ComVisible(true), ComDefaultInterface(typeof(IFoo))]
public class Base : IFoo
{
    public virtual void DoWork() { }
}
 
[ComVisible(true), ComDefaultInterface(typeof(IFoo))]
public class Derived : Base
{
    public override void DoWork() { }
}

If Derived is created first, calling Base.DoWork calls Derived.DoWork.

elinor-fung and others added 2 commits June 2, 2026 10:43
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 dotnet#127512

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 dotnet#127512

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adjusts CoreCLR COM callable wrapper (CCW) vtable sharing behavior so that a derived type cannot reuse a parent’s ComMethodTable when any interface method would resolve to a different target MethodDesc, and adds a regression test covering base-first vs derived-first COM access order.

Changes:

  • Add a new COM interop regression test that calls interface methods via the COM vtable and asserts the correct base/derived override is invoked independent of access order.
  • Replace the prior “same slots as parent” check with a stricter shareability check that also compares resolved dispatch targets (MethodDesc*) between parent and child.
  • Remove the now-unused MethodTable::ImplementsInterfaceWithSameSlotsAsParent helper.
Show a summary per file
File Description
src/coreclr/vm/comcallablewrapper.cpp Adds CanShareComMethodTableWithParent and uses it to gate parent ComMethodTable reuse based on resolved targets.
src/coreclr/vm/methodtable.h Removes declaration of ImplementsInterfaceWithSameSlotsAsParent.
src/coreclr/vm/methodtable.cpp Removes implementation of ImplementsInterfaceWithSameSlotsAsParent.
src/tests/Interop/COM/VirtualMethodOverride/VirtualMethodOverrideTest.csproj Adds a new COM interop test project for the regression scenario.
src/tests/Interop/COM/VirtualMethodOverride/VirtualMethodOverrideTest.cs Adds regression coverage for correct dispatch to base vs derived overrides when COM access order differs.

Copilot's findings

  • Files reviewed: 5/5 changed files
  • Comments generated: 2

Comment thread src/coreclr/vm/comcallablewrapper.cpp
- 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>
Comment thread src/coreclr/vm/comcallablewrapper.cpp Outdated
Comment thread src/coreclr/vm/comcallablewrapper.cpp Outdated
Comment thread src/coreclr/vm/comcallablewrapper.cpp Outdated
Comment thread src/coreclr/vm/comcallablewrapper.cpp Outdated
// table can only be shared if the targets are identical.
for (unsigned i = 0; i < pItfMT->GetNumVirtuals(); i++)
{
MethodDesc *pItfMD = pItfMT->GetMethodDescForSlot_NoThrow(i);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does it being NULL make this an issue?

Comment thread src/coreclr/vm/comcallablewrapper.cpp Outdated
Comment thread src/coreclr/vm/comcallablewrapper.cpp Outdated
Comment thread src/coreclr/vm/comcallablewrapper.cpp Outdated
Comment thread src/coreclr/vm/comcallablewrapper.cpp Outdated
Co-authored-by: Aaron R Robinson <arobins@microsoft.com>
Copilot AI review requested due to automatic review settings June 3, 2026 20:36
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 5/5 changed files
  • Comments generated: 0 new

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants