Fix marshalling generators using 0 as cleanup length for ManagedToUnmanaged collections with NoCountInfo by Copilot · Pull Request #126853 · dotnet/runtime

Copilot · 2026-04-13T22:10:16Z

Description

When marshalling collections ManagedToUnmanaged with no explicit count info (e.g., inner arrays of jagged 2D arrays with element marshallers that have Free), the source generator emitted numElements = 0 during cleanup. This caused the cleanup loop to never execute, leaking unmanaged memory on every successful call.

The fix replaces the hardcoded 0 with GetManagedValuesSource().Length, which is the correct element count for ManagedToUnmanaged cleanup — the managed collection is the source of truth.

Before (generated cleanup code):

__arg0__numElements = 0; // Free is never called on any element

After:

__arg0__numElements = Marshaller.GetManagedValuesSource(__arg0_managed).Length;

For nested collections (jagged arrays), the inner element cleanup needs to reference the outer managed span via <managedSpan>[<index>]. The outer cleanup scope previously only declared the unmanaged span, so this required also declaring the managed span when direction is ManagedToUnmanaged.

Changes

ElementsMarshalling.cs:
- Expose GenerateNumElementsAssignmentFromManagedValuesSource() on the base class, wrapping the existing file static extension method so it's callable from StatelessMarshallingStrategy.cs.
- Update NonBlittableElementsMarshalling.GenerateElementCleanupStatement to also declare managedSpan in the outer cleanup scope when direction is ManagedToUnmanaged, so nested element cleanup can reference <managedSpan>[<index>].
StatelessMarshallingStrategy.cs — Replace numElements = 0 with the managed values source length in GenerateCleanupCallerAllocatedResourcesStatements.
Unit test snippets — Add ElementInWithFree marshaller and NonBlittableElementWithFreeByValue test cases for both stateless/stateful paths in LibraryImportGenerator and ComInterfaceGenerator unit tests.
Behavioral tests in LibraryImportGenerator.Tests/CollectionMarshallingFails.cs:
- Removed [ActiveIssue("#93423")] from MultidimensionalArray_CheckInnerArraysAreCleared — it now passes with expected free count of throwOn - throwOn % 10 (only fully-completed inner arrays are cleaned up).
- Added a new MultidimensionalArray_CheckInnerArraysAreCleared_ProperCleanup test marked [ActiveIssue("#93431")] covering the partial-inner-cleanup gap, which is a separate issue.
- Uncommented the BoolStructInMarshaller.Marshaller.AssertAllHaveBeenCleaned() checks in MultiDimensionalOutArray_EnsureAllCleaned, which now correctly verify all 100 inner elements are freed on both success and failure paths.

Testing

✅ All 704 LibraryImportGenerator unit tests pass
✅ All 854 ComInterfaceGenerator unit tests pass
✅ All 289 LibraryImportGenerator behavioral tests pass (including the previously [ActiveIssue]-marked tests now enabled)
✅ All 82 ComInterfaceGenerator behavioral tests pass

… arrays of jagged 2D arrays When the marshalling direction is ManagedToUnmanaged and there is no explicit count info (NoCountInfo), the cleanup code was setting numElements = 0, causing Free to never be called on inner array elements. This fix uses GetManagedValuesSource().Length instead to get the actual element count from the managed collection. Fixes #93423 Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/3665c6a3-655d-4215-a791-d3a838f80bd4 Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>

…esSource Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/3665c6a3-655d-4215-a791-d3a838f80bd4 Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>

Copilot

Pull request overview

Fixes a source-generator bug in Microsoft.Interop.SourceGeneration where cleanup for ManagedToUnmanaged linear collections with NoCountInfo incorrectly set numElements = 0, preventing per-element Free from running and leaking unmanaged resources.

Changes:

Update stateless linear-collection cleanup codegen to compute numElements from GetManagedValuesSource(...).Length for ManagedToUnmanaged + NoCountInfo.
Expose a reusable ElementsMarshalling.GenerateNumElementsAssignmentFromManagedValuesSource(...) helper to enable the above codegen change.
Add new compilation snippets covering an element marshaller that defines Free in both LibraryImportGenerator and ComInterfaceGenerator unit-test suites.

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/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/StatelessMarshallingStrategy.cs	Fixes cleanup codegen for ManagedToUnmanaged + `NoCountInfo` to use managed source length instead of `0`.
src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/ElementsMarshalling.cs	Adds an instance helper wrapping the existing file-local extension for assigning `numElements` from managed source length.
src/libraries/System.Runtime.InteropServices/tests/Common/CustomCollectionMarshallingCodeSnippets.cs	Adds `ElementInWithFree` and new snippet variants to exercise element marshallers with `Free`.
src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/Compiles.cs	Includes new snippet variants in compilation-only generator tests.
src/libraries/System.Runtime.InteropServices/tests/ComInterfaceGenerator.Unit.Tests/Compiles.cs	Includes new snippet variants in compilation-only generator tests for COM-related generators.

            yield return new[] { ID(), customCollectionMarshallingCodeSnippets.Stateless.NativeToManagedFinallyOnlyReturnValue<int>() };
            yield return new[] { ID(), customCollectionMarshallingCodeSnippets.Stateless.NestedMarshallerParametersAndModifiers<int>() };
            yield return new[] { ID(), customCollectionMarshallingCodeSnippets.Stateless.NonBlittableElementByValue };
+            yield return new[] { ID(), customCollectionMarshallingCodeSnippets.Stateless.NonBlittableElementWithFreeByValue };
            yield return new[] { ID(), customCollectionMarshallingCodeSnippets.Stateless.NonBlittableElementParametersAndModifiers };


            yield return new[] { ID(), customCollectionMarshallingCodeSnippetsManagedToUnmanaged.Stateless.NonBlittableElementByValue };
+            yield return new[] { ID(), customCollectionMarshallingCodeSnippetsManagedToUnmanaged.Stateless.NonBlittableElementWithFreeByValue };
            yield return new[] { ID(), customCollectionMarshallingCodeSnippetsManagedToUnmanaged.Stateless.NonBlittableElementNativeToManagedOnlyOutParameter };
            yield return new[] { ID(), customCollectionMarshallingCodeSnippetsManagedToUnmanaged.Stateless.NonBlittableElementNativeToManagedOnlyReturnValue };
            yield return new[] { ID(), customCollectionMarshallingCodeSnippetsManagedToUnmanaged.Stateful.NativeToManagedOnlyOutParameter<int>() };
            yield return new[] { ID(), customCollectionMarshallingCodeSnippetsManagedToUnmanaged.Stateful.NativeToManagedOnlyReturnValue<int>() };
            yield return new[] { ID(), customCollectionMarshallingCodeSnippetsManagedToUnmanaged.Stateful.NonBlittableElementByValue };
+            yield return new[] { ID(), customCollectionMarshallingCodeSnippetsManagedToUnmanaged.Stateful.NonBlittableElementWithFreeByValue };
            yield return new[] { ID(), customCollectionMarshallingCodeSnippetsManagedToUnmanaged.Stateful.NonBlittableElementNativeToManagedOnlyOutParameter };


github-actions · 2026-04-20T23:20:58Z

🤖 Copilot Code Review — PR #126853

Note

This review was generated by Copilot (Claude Opus 4.6), with additional analysis from Claude Sonnet 4.5. A GPT-5.3-codex sub-agent was launched but did not complete within the 10-minute timeout and is excluded from this synthesis.

Holistic Assessment

Motivation: Justified. The previous code set numElements = 0 in cleanup for the NoCountInfo + ManagedToUnmanaged path, which prevented element-level Free from ever being called. This is a real resource leak bug for jagged arrays with non-blittable elements whose marshallers have Free methods. The tracking issue (#93423) confirms this was a known gap.

Approach: Correct. Using GetManagedValuesSource(managed).Length is consistent with how the marshal stage determines element count. The new wrapper method on ElementsMarshalling delegates to the existing GetNumElementsAssignmentFromManagedValuesSource extension method, maintaining code reuse.

Summary: ✅ LGTM. The fix correctly replaces the broken numElements = 0 with the managed collection's actual length. Tests cover both stateless and stateful marshallers in both ComInterfaceGenerator and LibraryImportGenerator. One minor follow-up suggestion below.

Detailed Findings

✅ Core Fix — Correct cleanup element count

The change in StatelessMarshallingStrategy.cs (line 586) correctly replaces numElements = 0 with numElements = <GetManagedValuesSource>.Length. I verified:

The numElements variable controls the span size returned by GetUnmanagedValuesDestination, which in turn controls the cleanup loop bound in GenerateElementCleanupStatement
The same GetManagedValuesSource pattern is used during the marshal stage, making cleanup consistent with marshalling
The old TODO and tracking issue link (Marshalling generators use 0 as the length during cleanup for inner arrays of jagged 2D arrays that are [In] and ManagedToUnmanaged #93423) are properly removed since this PR fixes the issue

✅ Code Design — Well-placed wrapper method

GenerateNumElementsAssignmentFromManagedValuesSource on ElementsMarshalling (line 114) is a reasonable abstraction. It follows the pattern of other public methods on the class, has clear XML documentation, and delegates to the existing file-scoped extension method at line 122.

✅ Test Coverage — Adequate for a source generator fix

New NonBlittableElementWithFreeByValue snippets are added for both stateless and stateful marshallers, exercising exactly the code path that was broken (element marshaller with a Free method in a by-value ManagedToUnmanaged collection). The tests verify the generated code compiles correctly, which is the appropriate test level for source generator changes.

✅ `GenerateCleanupCalleeAllocatedResourcesStatements` — Not affected

I investigated whether the callee-allocated cleanup path (lines 610-641 in StatelessMarshallingStrategy.cs) needs the same fix. It does not: NoCountInfo only occurs for ManagedToUnmanaged direction (inner arrays of jagged arrays), and that direction uses CleanupCallerAllocated stage. The callee-allocated path handles UnmanagedToManaged scenarios which always have explicit count info. The existing GenerateNumElementsExpression call is correct for those cases.

💡 Follow-up — Consider a runtime integration test

The unit tests verify compilation but not runtime behavior. A follow-up test that tracks Free call counts (similar to the existing IntWrapperMarshallerWithFreeCounts pattern in CollectionTests.cs) for jagged arrays would provide stronger confidence that Free is actually invoked the correct number of times. This is non-blocking — the compilation tests are sufficient to verify the structural fix.

Generated by Code Review for issue #126853 · ◷

jtschuster · 2026-04-21T21:04:29Z

@copilot Can you add tests in the behavioral test projects to validate the marshalling works as expected?

The previous fix only changed the inner numElements assignment but did not declare the outer managedSpan in the cleanup scope, causing a compile error in generated code for nested collections. This commit: - Declares managedSpan in NonBlittableElementsMarshalling.GenerateElementCleanupStatement when direction is ManagedToUnmanaged, so inner cleanup can reference it. - Removes [ActiveIssue("#93423")] from MultidimensionalArray_CheckInnerArraysAreCleared and updates expected free count to throwOn - throwOn % 10 (only fully completed inner arrays are cleaned up; the remaining gap is tracked by #93431). - Splits a separate MultidimensionalArray_CheckInnerArraysAreCleared_ProperCleanup test marked with [ActiveIssue(#93431)] for the partial inner cleanup case. - Uncomments the BoolStructInMarshaller asserts in MultiDimensionalOutArray_EnsureAllCleaned which now correctly free all 100 inner elements on success and failure paths. Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/1d08ed99-2083-4c06-a7c1-15d6a4159e8e Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>

Copilot · 2026-04-21T21:38:21Z

@copilot Can you add tests in the behavioral test projects to validate the marshalling works as expected?

Added behavioral tests in 3ff90d5. Specifically:

Removed [ActiveIssue("#93423")] from MultidimensionalArray_CheckInnerArraysAreCleared — it now passes with the fix (with expected free count of throwOn - throwOn % 10, since only fully-completed inner arrays are cleaned up; the remaining partial-inner-cleanup gap is the separate issue Marshalling generators LastIndexMarshalled tracking doesn't free partially marshalled inner array elements if marshalling fails #93431, captured in a new MultidimensionalArray_CheckInnerArraysAreCleared_ProperCleanup test marked [ActiveIssue("#93431")]).
Uncommented the BoolStructInMarshaller.Marshaller.AssertAllHaveBeenCleaned() checks in MultiDimensionalOutArray_EnsureAllCleaned, which now correctly verify all 100 inner elements are freed on both success and failure paths.

The behavioral tests caught a real bug in the original code change — for nested collections the inner cleanup referenced __managedSpan[__i0] but __managedSpan was only declared in the marshal scope. Fixed by also declaring managedSpan in NonBlittableElementsMarshalling.GenerateElementCleanupStatement when direction is ManagedToUnmanaged.

Initial plan

a9ff969

Copilot AI assigned Copilot and jtschuster Apr 13, 2026

Copilot AI review requested due to automatic review settings April 13, 2026 22:10

github-project-automation Bot added this to AppModel Apr 13, 2026

Copilot AI review requested due to automatic review settings April 13, 2026 22:10

Copilot started work on behalf of jtschuster April 13, 2026 22:10 View session

Copilot AI linked an issue Apr 13, 2026 that may be closed by this pull request

Marshalling generators use 0 as the length during cleanup for inner arrays of jagged 2D arrays that are [In] and ManagedToUnmanaged #93423

Open

github-actions Bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 13, 2026

Copilot AI requested review from Copilot and removed request for Copilot April 13, 2026 22:42

Add XML documentation to GenerateNumElementsAssignmentFromManagedValu…

6b15de0

…esSource Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/3665c6a3-655d-4215-a791-d3a838f80bd4 Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>

Copilot AI requested review from Copilot and removed request for Copilot April 13, 2026 22:45

Copilot AI changed the title ~~[WIP] Fix marshalling cleanup for inner arrays of jagged 2D arrays~~ Fix marshalling generators using 0 as cleanup length for ManagedToUnmanaged collections with NoCountInfo Apr 13, 2026

Copilot finished work on behalf of jtschuster April 13, 2026 22:46

Copilot AI requested a review from jtschuster April 13, 2026 22:46

github-actions Bot added area-System.Runtime.InteropServices and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 13, 2026

This was referenced Apr 14, 2026

[browser] Wasm.Build.Tests timeout - Timed out after 10s waiting for 'WASM EXIT' #116697

Open

Android arm32 device not found (armeabi-v7a architecture unavailable) #125440

Open

Merge branch 'main' into copilot/fix-jagged-array-marshalling

bc25430

Copilot AI review requested due to automatic review settings April 20, 2026 22:58

Copilot started reviewing on behalf of jtschuster April 20, 2026 22:59 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

This was referenced Apr 21, 2026

XHarness package install failure on iOS due to devicectl NSPOSIXErrorDomain error 49 #123796

Open

Libraries .NET 4.8.1 build failing #127007

Closed

Copilot started work on behalf of jtschuster April 21, 2026 21:04 View session

Copilot finished work on behalf of jtschuster April 21, 2026 21:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix marshalling generators using 0 as cleanup length for ManagedToUnmanaged collections with NoCountInfo#126853

Fix marshalling generators using 0 as cleanup length for ManagedToUnmanaged collections with NoCountInfo#126853
Copilot wants to merge 5 commits intomainfrom
copilot/fix-jagged-array-marshalling

Copilot AI commented Apr 13, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

github-actions Bot commented Apr 20, 2026

Uh oh!

jtschuster commented Apr 21, 2026

Uh oh!

Copilot AI commented Apr 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Copilot AI commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

github-actions Bot commented Apr 20, 2026

🤖 Copilot Code Review — PR #126853

Holistic Assessment

Detailed Findings

✅ Core Fix — Correct cleanup element count

✅ Code Design — Well-placed wrapper method

✅ Test Coverage — Adequate for a source generator fix

✅ GenerateCleanupCalleeAllocatedResourcesStatements — Not affected

💡 Follow-up — Consider a runtime integration test

Uh oh!

jtschuster commented Apr 21, 2026

Uh oh!

Copilot AI commented Apr 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Apr 13, 2026 •

edited

Loading

✅ `GenerateCleanupCalleeAllocatedResourcesStatements` — Not affected