dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.42k stars 4.75k forks source link

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

Open jtschuster opened 1 year ago

jtschuster commented 1 year ago

The marshalling generators always use the length of the managed collection if it is only marshalled to unmanaged (and not unmarshalled to managed). However, during cleanup, they default to using 0 as the length when cleaning up an inner array of a 2D array. This causes the marshallers to not free any elements of the inner array on successful calls

Jagged arrays are technically an unsupported scenario, but this can cause a memory leak on successful calls to COM methods or LibraryImport methods. This issue is also present in .Net 7.

The fix for this would be to either require length parameters for parameters that are marshalled with marshallers that have a Free method (and use them -- we won't use them if they're provided now), or set length to __arrayMarshaller.GetManagedValuesSource().Length in these specific scenarios.

@jkoritzinsky @AaronRobinsonMSFT Should we try to get a fix in 8? I think it's probably okay since the behavior has been here since 7 with no issues, but what are your thoughts?

Source:

using System.Runtime.InteropServices;
using System.Runtime.InteropServices.Marshalling;

public static partial class PInvoke
{
    [LibraryImport("asdfasdf")]
    public static partial int Method(
        [MarshalUsing(typeof(MyClassMarshaller), ElementIndirectionDepth = 2)]
        [MarshalUsing(CountElementName = nameof(length))]
        [MarshalUsing(CountElementName = nameof(widths), ElementIndirectionDepth =1)]
        MyClass[][] arg,
        int length,
        int[] widths);

    public class MyClass
    {
    }

    [CustomMarshaller(typeof(MyClass), MarshalMode.Default, typeof(MyClassMarshaller))]
    public static class MyClassMarshaller
    {
        public static nint ConvertToUnmanaged(MyClass myClass) => throw null;
        public static MyClass ConverToManaged(nint nativeValue) => throw null;
        public static void Free(nint unmaanged) => throw null;

    }
}

Generated:

    public static partial int Method(global::PInvoke.MyClass[][] arg, int length, int[] widths)
    {
        ...
        finally {
            ...
            // CleanupCallerAllocated - Perform cleanup of caller allocated resources.
            {
                global::System.ReadOnlySpan<global::System.IntPtr> __arg_native__nativeSpan = __arg_native__marshaller.GetUnmanagedValuesDestination();
                for (int __i0 = 0; __i0 < __arg_native__lastIndexMarshalled; ++__i0)
                {
                    int __arg_native__nativeSpan____i0__numElements;
                    global::System.Runtime.CompilerServices.Unsafe.SkipInit(out __arg_native__nativeSpan____i0__numElements);
                    __arg_native__nativeSpan____i0__numElements = 0; // Issue
                    {
                        global::System.ReadOnlySpan<nint> __arg_native__nativeSpan____i0__nativeSpan = global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller<global::PInvoke.MyClass, nint>.GetUnmanagedValuesDestination((nint*)__arg_native__nativeSpan[__i0], __arg_native__nativeSpan____i0__numElements);
                        for (int __i1 = 0; __i1 < __arg_native__nativeSpan____i0__nativeSpan.Length; ++__i1)
                        {
                            // Will never execute
                            global::PInvoke.MyClassMarshaller.Free(__arg_native__nativeSpan____i0__nativeSpan[__i1]);
                        }
                    }

                    __arg_native__nativeSpan____i0__numElements = 0;
                    global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller<global::PInvoke.MyClass, nint>.Free((nint*)__arg_native__nativeSpan[__i0]);
                }
            }

            __arg_native__marshaller.Free();
        }
ghost commented 1 year ago

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

Issue Details
The marshalling generators always use the length of the managed collection if it is only marshalled to unmanaged (and not unmarshalled to managed). However, during cleanup, they default to using 0 as the length when cleaning up an inner array of a 2D array. This causes the marshallers to not free any elements of the inner array on successful calls Jagged arrays are technically an unsupported scenario, but this can cause a memory leak on successful calls to COM methods or LibraryImport methods. This issue is also present in .Net 7. The fix for this would be to either require length parameters for parameters that are marshalled with marshallers that have a `Free` method (and use them -- we won't use them if they're provided now), or set length to `__arrayMarshaller.GetManagedValuesSource().Length` in these specific scenarios. @jkoritzinsky @AaronRobinsonMSFT Should we try to get a fix in 8? I think it's probably okay since the behavior has been here since 7 with no issues, but what are your thoughts? Source: ```C# using System.Runtime.InteropServices; using System.Runtime.InteropServices.Marshalling; public static partial class PInvoke { [LibraryImport("asdfasdf")] public static partial int Method( [MarshalUsing(typeof(MyClassMarshaller), ElementIndirectionDepth = 2)] [MarshalUsing(CountElementName = nameof(length))] [MarshalUsing(CountElementName = nameof(widths), ElementIndirectionDepth =1)] MyClass[][] arg, int length, int[] widths); public class MyClass { } [CustomMarshaller(typeof(MyClass), MarshalMode.Default, typeof(MyClassMarshaller))] public static class MyClassMarshaller { public static nint ConvertToUnmanaged(MyClass myClass) => throw null; public static MyClass ConverToManaged(nint nativeValue) => throw null; public static void Free(nint unmaanged) => throw null; } } ``` Generated: ```C# public static partial int Method(global::PInvoke.MyClass[][] arg, int length, int[] widths) { ... finally { ... // CleanupCallerAllocated - Perform cleanup of caller allocated resources. { global::System.ReadOnlySpan __arg_native__nativeSpan = __arg_native__marshaller.GetUnmanagedValuesDestination(); for (int __i0 = 0; __i0 < __arg_native__lastIndexMarshalled; ++__i0) { int __arg_native__nativeSpan____i0__numElements; global::System.Runtime.CompilerServices.Unsafe.SkipInit(out __arg_native__nativeSpan____i0__numElements); __arg_native__nativeSpan____i0__numElements = 0; // Issue { global::System.ReadOnlySpan __arg_native__nativeSpan____i0__nativeSpan = global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller.GetUnmanagedValuesDestination((nint*)__arg_native__nativeSpan[__i0], __arg_native__nativeSpan____i0__numElements); for (int __i1 = 0; __i1 < __arg_native__nativeSpan____i0__nativeSpan.Length; ++__i1) { // Will never execute global::PInvoke.MyClassMarshaller.Free(__arg_native__nativeSpan____i0__nativeSpan[__i1]); } } __arg_native__nativeSpan____i0__numElements = 0; global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller.Free((nint*)__arg_native__nativeSpan[__i0]); } } __arg_native__marshaller.Free(); } ```
Author: jtschuster
Assignees: jtschuster
Labels: `area-System.Runtime.InteropServices`, `source-generator`
Milestone: -