Fix COM reentrancy contract violations in ExtensionPoints-related paths

[Copilot]

Note

This PR description was updated with GitHub Copilot assistance.

This PR fixes the Interop/COM/ExtensionPoints failure by correcting CoreCLR COM interop contract handling for paths that can legitimately re-enter managed code during COM allocation hooks, and it includes a small cleanup of the related COM visibility helpers.

Changes

• Keep OleVariant::ClearLPSTRArray and OleVariant::ClearLPWSTRArray as NOTHROW, but mark the CoTaskMemFree cleanup paths with a permanent ThrowsViolation for runtime reentrancy, since CoTaskMemFree can invoke a managed IMallocSpy implementation.
• Wrap the ComPreStubWorker setup around the managed exception dispatcher and unwind handler in a BEGIN_CONTRACT_VIOLATION(ThrowsViolation) / END_CONTRACT_VIOLATION block.
• Update the outer static contract on ComPreStubWorker to STATIC_CONTRACT_NOTHROW so it matches the intended HRESULT-returning behavior outside the explicit violation scope.
• Remove the unused fForIDispatch parameter from CheckParentComVisibility.
• Delete the redundant CheckParentComVisibilityNoThrow helper and use HasInvisibleParent() directly at the remaining visibility check.

Why this addresses the failure

The failing test exercises COM extension points that can callback into managed code during allocation cleanup and stub initialization. Those paths were operating under overly strict contracts, which made valid reentrant behavior show up as contract violations. This change narrows the violation handling to the specific reentrant regions while keeping the surrounding HRESULT-returning contracts intact. The visibility-helper refactors are supporting cleanups with no intended behavioral change.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR adjusts CoreCLR COM interop contract handling to accommodate managed re-entrancy (e.g., managed IMallocSpy) during marshaling cleanup and related COM-call paths.

Changes:

• Update OleVariant::ClearLPSTRArray / ClearLPWSTRArray cleanup paths to tolerate CoTaskMemFree re-entrancy via a contract-violation annotation.
• Simplify CheckParentComVisibility API by removing an unused BOOL fForIDispatch parameter and updating call sites.
• Add a ThrowsViolation contract-violation scope in ComPreStubWorker around the managed exception dispatcher / unwind handler region.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/coreclr/vm/stubhelpers.cpp	Update call site to new CheckParentComVisibility() signature.
src/coreclr/vm/stdinterfaces.cpp	Update IDispatch-related call sites to new CheckParentComVisibility() signature.
src/coreclr/vm/olevariant.cpp	Annotate LPSTR/LPWSTR array cleanup to allow managed re-entrancy during CoTaskMemFree.
src/coreclr/vm/comcallablewrapper.h	Remove unused fForIDispatch parameter from CheckParentComVisibility* APIs.
src/coreclr/vm/comcallablewrapper.cpp	Add contract-violation scope in ComPreStubWorker; update visibility checks to new signature.

Comments suppressed due to low confidence (1)

src/coreclr/vm/comcallablewrapper.cpp:324

• BEGIN_CONTRACT_VIOLATION(ThrowsViolation) introduces a scope that must always reach END_CONTRACT_VIOLATION. Under FEATURE_INTERPRETER, the early return pInterpreterTarget; exits the function without calling END_CONTRACT_VIOLATION, leaving the violation state active on the thread (and skipping the explicit leave call). Restructure so all returns after BEGIN_CONTRACT_VIOLATION leave the scope (e.g., move the interpreter fast-path before BEGIN_CONTRACT_VIOLATION, or ensure it executes END_CONTRACT_VIOLATION before returning).

    BEGIN_CONTRACT_VIOLATION(ThrowsViolation);

    INSTALL_MANAGED_EXCEPTION_DISPATCHER;
    INSTALL_UNWIND_AND_CONTINUE_HANDLER;

#ifdef FEATURE_INTERPRETER
    PCODE pInterpreterTarget = pEntryThunk->GetInterpreterTarget();
    if (pInterpreterTarget != (PCODE)0)
    {
        t_MostRecentUMEntryThunkData = pEntryThunk;
        return pInterpreterTarget;
    }

[AaronRobinsonMSFT]

@MichalStrehovsky This PR should fix the contract violations you found.

[MichalStrehovsky]

@MichalStrehovsky This PR should fix the contract violations you found.

Sounds good. We'll only be able to test this once #126517 merges since right now the test doesn't run at all.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/coreclr/vm/comcallablewrapper.cpp:324

• ComPreStubWorker now enters BEGIN_CONTRACT_VIOLATION(ThrowsViolation) and installs the managed exception dispatcher / unwind handler, but the FEATURE_INTERPRETER fast path returns early (return pInterpreterTarget;) before UNINSTALL_* and END_CONTRACT_VIOLATION. ContractViolationHolder does not auto-leave on scope exit, so this leaves the violation mask (and the try/catch dispatcher scope) unbalanced for the rest of the thread’s execution. Consider moving the interpreter-target early-return before the BEGIN_CONTRACT_VIOLATION / INSTALL_* sequence, or refactor to a single-exit cleanup path that always uninstalls and ends the contract violation before returning.

    BEGIN_CONTRACT_VIOLATION(ThrowsViolation);

    INSTALL_MANAGED_EXCEPTION_DISPATCHER;
    INSTALL_UNWIND_AND_CONTINUE_HANDLER;

#ifdef FEATURE_INTERPRETER
    PCODE pInterpreterTarget = pEntryThunk->GetInterpreterTarget();
    if (pInterpreterTarget != (PCODE)0)
    {
        t_MostRecentUMEntryThunkData = pEntryThunk;
        return pInterpreterTarget;
    }

[github-actions]

Note

This review was generated by Copilot.

🤖 Copilot Code Review — PR #126571

Holistic Assessment

Motivation: This PR addresses contract annotation correctness in COM interop code. It fixes three distinct issues: (1) ClearLPWSTRArray and ClearLPSTRArray call CoTaskMemFree which can trigger managed IMallocSpy callbacks — violating their NOTHROW contract, (2) ComPreStubWorker was annotated THROWS despite the INSTALL_UNWIND_AND_CONTINUE_HANDLER macros catching all exceptions before they escape, and (3) the fForIDispatch parameter on CheckParentComVisibility/CheckParentComVisibilityNoThrow was never read, making it dead code. All three are clearly correct from inspecting the code.

Approach: The approach is appropriate for each change: PERMANENT_CONTRACT_VIOLATION(ThrowsViolation, ReasonRuntimeReentrancy) for the IMallocSpy case follows existing precedent (e.g., eetoprofinterfaceimpl.cpp uses this for profiler callouts, olecontexthelpers.cpp documents the same IMallocSpy concern). BEGIN_CONTRACT_VIOLATION / END_CONTRACT_VIOLATION for ComPreStubWorker matches the pattern used in interoputil.cpp. The fForIDispatch parameter removal and CheckParentComVisibilityNoThrow deletion are straightforward dead-code cleanup.

Summary: ✅ LGTM. All changes are correct, well-motivated, and consistent with established codebase patterns. The dead fForIDispatch parameter was confirmed unused by inspecting the old implementation — CheckParentComVisibilityNoThrow only called HasInvisibleParent() without referencing the parameter. All callers have been updated consistently across comcallablewrapper.cpp, stdinterfaces.cpp, and stubhelpers.cpp.

---

Detailed Findings

✅ Contract Correctness — ClearLPWSTRArray / ClearLPSTRArray IMallocSpy annotation

The PERMANENT_CONTRACT_VIOLATION(ThrowsViolation, ReasonRuntimeReentrancy) annotation is correctly placed after CONTRACTL_END and before the CoTaskMemFree loop. CoTaskMemFree can trigger managed IMallocSpy callbacks (the same concern documented in olecontexthelpers.cpp:17). The ReasonRuntimeReentrancy reason code is the correct choice, matching how the codebase documents this class of reentrancy (e.g., SafeQueryInterface cited in the enum definition). Both ClearLPWSTRArray and ClearLPSTRArray are correctly annotated.

I verified that ClearBSTRArray (which uses SysFreeString, not CoTaskMemFree) and ClearInterfaceArray (which uses SafeReleasePreemp) are not affected by IMallocSpy and correctly do not need this annotation.

✅ Contract Correctness — ComPreStubWorker NOTHROW contract

The outer function contract change from STATIC_CONTRACT_THROWS to STATIC_CONTRACT_NOTHROW is semantically correct. The INSTALL_UNWIND_AND_CONTINUE_HANDLER macro sets up a PAL_CPP_TRY/PAL_CPP_CATCH block that catches all Exception-derived and std::bad_alloc exceptions. The BEGIN_CONTRACT_VIOLATION(ThrowsViolation) properly annotates the inner region where exceptions may be thrown before being caught by the handler. The comment explaining why the violation is needed is helpful.

✅ Dead Code Removal — CheckParentComVisibilityNoThrow and fForIDispatch

The removed CheckParentComVisibilityNoThrow method was a trivial wrapper that just returned !HasInvisibleParent(). Its THROWS contract annotation was also misleading (named "NoThrow" but contracted as THROWS). The fForIDispatch parameter was accepted by both CheckParentComVisibility and CheckParentComVisibilityNoThrow but never read in either implementation. All six call sites (3 in comcallablewrapper.cpp, 2 in stdinterfaces.cpp, 1 in stubhelpers.cpp) have been updated. The forwarding methods in ComMethodTable struct have also been updated and the NoThrow variant removed.

💡 Observation — #ifdef FEATURE_INTERPRETER early return inside contract violation scope

In ComPreStubWorker, the return pInterpreterTarget; at line 326 exits from inside the BEGIN_CONTRACT_VIOLATION block without reaching END_CONTRACT_VIOLATION. Since ContractViolationHolder (unlike AutoCleanupContractViolationHolder) does not call Leave() in its destructor, the ThrowsViolation mask would leak on the thread in checked/debug builds when this path is taken. However, this early return was pre-existing (before this PR) and already bypassed UNINSTALL_UNWIND_AND_CONTINUE_HANDLER — the contract violation is just one more thing added to the existing set of leaked cleanup on that path. This is not a blocking issue, but worth noting for a potential future cleanup of the interpreter path.

Generated by Code Review for issue #126571 · ◷