Fix native memory leak in AsAnyMarshaler.ConvertLayoutToNative on exception

[Copilot]

Description

AsAnyMarshaler.ConvertLayoutToNative leaks the Marshal.AllocCoTaskMem-allocated buffer when LayoutTypeConvertToUnmanaged throws — the exception unwinds past the return pNativeHome with no cleanup.

Fix

Wrap the conversion call in a try/catch that frees on failure, matching the defensive pattern already used in sibling code in the same file:

IntPtr pNativeHome = Marshal.AllocCoTaskMem(allocSize);
try
{
    if (IsIn(dwFlags))
        StubHelpers.LayoutTypeConvertToUnmanaged(pManagedHome, (byte*)pNativeHome, ref cleanupWorkList);
}
catch
{
    Marshal.FreeCoTaskMem(pNativeHome);
    throw;
}

Changes

• src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs: Added try/catch around LayoutTypeConvertToUnmanaged call in ConvertLayoutToNative to free native memory on exception.

Testing

Risk

Low — strictly adds a cleanup path that was previously missing; no behavioral change on the happy path.

[Copilot]

Pull request overview

Fixes an exception-path leak in CoreCLR’s AsAnyMarshaler.ConvertLayoutToNative (in System.Private.CoreLib) by ensuring unmanaged memory allocated via Marshal.AllocCoTaskMem is freed if LayoutTypeConvertToUnmanaged throws during layout marshaling.

Changes:

• Wrap LayoutTypeConvertToUnmanaged in try/catch within ConvertLayoutToNative.
• Free the CoTaskMem buffer on exception before rethrowing.

[Copilot]

Pull request overview

Fixes an exception-path native memory leak in AsAnyMarshaler.ConvertLayoutToNative within CoreLib interop marshaling by ensuring native allocations (and any accumulated cleanup work) are released when LayoutTypeConvertToUnmanaged throws.

Changes:

• Wraps the LayoutTypeConvertToUnmanaged call in ConvertLayoutToNative with a try/catch.
• On exception, destroys the cleanupWorkList and frees the Marshal.AllocCoTaskMem buffer before rethrowing.

[dotnet-policy-service]

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

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "e439821dede5d567aa78b379fa8e9bdc2764d0b9",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "cead7e78dc1bb6bd8f3c1f2e918df5e2c97156d6",
  "last_reviewed_commit": "e439821dede5d567aa78b379fa8e9bdc2764d0b9",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "cead7e78dc1bb6bd8f3c1f2e918df5e2c97156d6",
  "last_recorded_worker_run_id": "29766969872",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "7e6b02951dede8088a2b29a2780b358bb04d2d56",
      "review_id": 4730179668
    },
    {
      "commit": "e439821dede5d567aa78b379fa8e9bdc2764d0b9",
      "review_id": 4737835721
    }
  ]
}

[github-actions]

Holistic Review

Motivation: AsAnyMarshaler.ConvertLayoutToNative allocates a native buffer with Marshal.AllocCoTaskMem and then calls StubHelpers.LayoutTypeConvertToUnmanaged. If that conversion throws, the stack unwinds past return pNativeHome and the buffer is never freed, leaking native memory. Because ClearNative/ConvertToManaged only run for a marshaler whose ConvertToNative returned successfully, nothing else reclaims the buffer on this path. The motivation is sound and the leak is real.

Approach: The change wraps the IsIn-guarded LayoutTypeConvertToUnmanaged call in a try/catch. On failure it calls StubHelpers.DestroyCleanupList(ref cleanupWorkList), frees the CoTaskMem buffer, and rethrows. This mirrors the defensive cleanup already performed in ClearNative and matches the sibling error-handling pattern in this file. The approach is minimal and targeted, with no change to the happy path.

Summary: The fix correctly plugs the leak and, beyond the PR description, also releases any cleanupWorkList entries (e.g. SafeHandle references) accumulated during the partial conversion, which is the right thing to do. DestroyCleanupList sets cleanupWorkList to null and pNativeHome is a local, so there is no double-free risk with the normal ClearNative path (which never runs when ConvertToNative throws). One residual, non-blocking observation: fields that LayoutTypeConvertToUnmanaged had already marshaled into the native buffer before throwing are not unwound via DestroyStructure/LayoutTypeConvertToManaged (unlike the success-time ClearNative, which uses layoutType). That is inherent to a partial-conversion failure and consistent with existing behavior, so it is acceptable here. Verdict: LGTM.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 74 AIC · ⌖ 10.3 AIC · ⊞ 10K

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

[github-actions]

Holistic Review

Motivation: Unchanged from the prior review. AsAnyMarshaler's layout marshaling allocates a native buffer with Marshal.AllocCoTaskMem and then calls StubHelpers.LayoutTypeConvertToUnmanaged. If that conversion throws, the stack unwinds past the return and the buffer (plus any accumulated cleanup-work-list entries) is never reclaimed, since ClearNative/ConvertToManaged only run for a marshaler whose ConvertToNative returned successfully. The leak is real and worth fixing.

Approach: Unchanged in intent — wrap the IsIn-guarded conversion in a try/catch that, on failure, destroys the cleanup work list, frees the CoTaskMem buffer, and rethrows. Since the previous review, the PR was rebased onto a base where AsAnyMarshaler was refactored: the old flat ConvertLayoutToNative(pManagedHome, ...) method is now LayoutImplementation.ConvertToNative(object managed, ...) with a pNative local and an _cleanupWorkList field. The merge-conflict resolution updated the cleanup-list field reference to _cleanupWorkList but did not rename the argument/buffer references, so the reapplied patch still uses the removed identifiers pManagedHome and pNativeHome.

Summary: The incremental change since commit 7e6b029 is the rebase and its conflict resolution. That resolution is incorrect: the try-block calls LayoutTypeConvertToUnmanaged(pManagedHome, (byte*)pNativeHome, ...) and the catch frees pNativeHome, but neither identifier exists in the new LayoutImplementation.ConvertToNative scope (the parameter is managed, the buffer local is pNative). This is a hard CS0103 compile break for System.Private.CoreLib — see the inline finding for the exact lines and corrected code. The underlying fix remains correct once the identifiers are updated to managed/pNative. Verdict: the fix is sound in principle but currently does not compile and must be corrected before merge.

Detailed Findings

See the inline comment on src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs (lines 978–987): the rebased patch references the removed pManagedHome/pNativeHome identifiers instead of the new managed/pNative, causing a compile failure.

Assessment History

• review 4730179668 reviewed commit 7e6b029 with an LGTM verdict. Current verdict: not mergeable as-is (compile break). Assessment changed: the motivation and the fundamental approach are unchanged, but the rebase's merge-conflict resolution left stale identifiers (pManagedHome/pNativeHome) in the refactored LayoutImplementation.ConvertToNative, turning a previously-compiling, correct fix into non-compiling code.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 56.2 AIC · ⌖ 10.9 AIC · ⊞ 10K

[Copilot]

Pull request overview

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

[Copilot]

Copilot was unable to review this pull request because there is no eligible user to bill. To allow Copilot reviews on bot-authored pull requests, enable direct organization billing in your organization's Copilot settings.