MARSHALLED_UNICODE_STRING[] array marshalling leaks memory

[jkotas]

The marshalling code generated for

runtime/src/libraries/Common/src/Interop/Windows/Advapi32/Interop.LsaLookupNames2.cs

Lines 13 to 21 in 0c631ab

	[LibraryImport(Interop.Libraries.Advapi32, EntryPoint = "LsaLookupNames2", SetLastError = true, StringMarshalling = StringMarshalling.Utf16)]
	internal static partial uint LsaLookupNames2(
	SafeLsaPolicyHandle handle,
	int flags,
	int count,
	MARSHALLED_UNICODE_STRING[] names,
	out SafeLsaMemoryHandle referencedDomains,
	out SafeLsaMemoryHandle sids
	);

looks like this (the code snippet only shows marshalling names argument, the remaining arguments omitted for brevity):

// <auto-generated/>
unsafe partial class Test
{
 [System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Interop.LibraryImportGenerator", "7.0.6.47809")]
 [System.Runtime.CompilerServices.SkipLocalsInitAttribute]
 internal static partial uint LsaLookupNames2(global::Test.MARSHALLED_UNICODE_STRING[] names)
 {
 int __lastError;
 global::Test.MARSHALLED_UNICODE_STRING.Marshaller.Native* __names_native = default;
 uint __retVal;
 // Setup - Perform required setup.
 global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller<global::Test.MARSHALLED_UNICODE_STRING, global::Test.MARSHALLED_UNICODE_STRING.Marshaller.Native>.ManagedToUnmanagedIn __names_native__marshaller = new();
 try
 {
 // Marshal - Convert managed data to native data.
 global::Test.MARSHALLED_UNICODE_STRING.Marshaller.Native* __names_native__stackptr = stackalloc global::Test.MARSHALLED_UNICODE_STRING.Marshaller.Native[global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller<global::Test.MARSHALLED_UNICODE_STRING, global::Test.MARSHALLED_UNICODE_STRING.Marshaller.Native>.ManagedToUnmanagedIn.BufferSize];
 __names_native__marshaller.FromManaged(names, new System.Span<global::Test.MARSHALLED_UNICODE_STRING.Marshaller.Native>(__names_native__stackptr, global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller<global::Test.MARSHALLED_UNICODE_STRING, global::Test.MARSHALLED_UNICODE_STRING.Marshaller.Native>.ManagedToUnmanagedIn.BufferSize));
 {
 System.ReadOnlySpan<global::Test.MARSHALLED_UNICODE_STRING> __names_native__managedSpan = __names_native__marshaller.GetManagedValuesSource();
 System.Span<global::Test.MARSHALLED_UNICODE_STRING.Marshaller.Native> __names_native__nativeSpan = __names_native__marshaller.GetUnmanagedValuesDestination();
 for (int __i0 = 0; __i0 < __names_native__managedSpan.Length; ++__i0)
 {
 __names_native__nativeSpan[__i0] = global::Test.MARSHALLED_UNICODE_STRING.Marshaller.ConvertToUnmanaged(__names_native__managedSpan[__i0]);
 }
 }

 // Pin - Pin data in preparation for calling the P/Invoke.
 fixed (void* __names_native__unused = __names_native__marshaller)
 {
 // PinnedMarshal - Convert managed data to native data that requires the managed data to be pinned.
 __names_native = __names_native__marshaller.ToUnmanaged();
 System.Runtime.InteropServices.Marshal.SetLastSystemError(0);
 __retVal = __PInvoke(__names_native);
 __lastError = System.Runtime.InteropServices.Marshal.GetLastSystemError();
 }
 }
 finally
 {
 // Cleanup - Perform required cleanup.
 __names_native__marshaller.Free();
 }

 System.Runtime.InteropServices.Marshal.SetLastPInvokeError(__lastError);
 return __retVal;
 // Local P/Invoke
 [System.Runtime.InteropServices.DllImportAttribute("Advapi32", EntryPoint = "LsaLookupNames2", ExactSpelling = true)]
 static extern unsafe uint __PInvoke(global::Test.MARSHALLED_UNICODE_STRING.Marshaller.Native* names);
 }
}

Notice that there is no cleanup of the unmanaged array after the PInvoke. Are the unmanaged string copies leaking?

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

Issue Details

---

The marshalling code generated for

runtime/src/libraries/Common/src/Interop/Windows/Advapi32/Interop.LsaLookupNames2.cs

Lines 13 to 21 in 0c631ab

	[LibraryImport(Interop.Libraries.Advapi32, EntryPoint = "LsaLookupNames2", SetLastError = true, StringMarshalling = StringMarshalling.Utf16)]
	internal static partial uint LsaLookupNames2(
	SafeLsaPolicyHandle handle,
	int flags,
	int count,
	MARSHALLED_UNICODE_STRING[] names,
	out SafeLsaMemoryHandle referencedDomains,
	out SafeLsaMemoryHandle sids
	);

looks like this (the code snippet only shows marshalling names argument, the remaining arguments omitted for brevity):

// <auto-generated/>
unsafe partial class Test
{
 [System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Interop.LibraryImportGenerator", "7.0.6.47809")]
 [System.Runtime.CompilerServices.SkipLocalsInitAttribute]
 internal static partial uint LsaLookupNames2(global::Test.MARSHALLED_UNICODE_STRING[] names)
 {
 int __lastError;
 global::Test.MARSHALLED_UNICODE_STRING.Marshaller.Native* __names_native = default;
 uint __retVal;
 // Setup - Perform required setup.
 global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller<global::Test.MARSHALLED_UNICODE_STRING, global::Test.MARSHALLED_UNICODE_STRING.Marshaller.Native>.ManagedToUnmanagedIn __names_native__marshaller = new();
 try
 {
 // Marshal - Convert managed data to native data.
 global::Test.MARSHALLED_UNICODE_STRING.Marshaller.Native* __names_native__stackptr = stackalloc global::Test.MARSHALLED_UNICODE_STRING.Marshaller.Native[global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller<global::Test.MARSHALLED_UNICODE_STRING, global::Test.MARSHALLED_UNICODE_STRING.Marshaller.Native>.ManagedToUnmanagedIn.BufferSize];
 __names_native__marshaller.FromManaged(names, new System.Span<global::Test.MARSHALLED_UNICODE_STRING.Marshaller.Native>(__names_native__stackptr, global::System.Runtime.InteropServices.Marshalling.ArrayMarshaller<global::Test.MARSHALLED_UNICODE_STRING, global::Test.MARSHALLED_UNICODE_STRING.Marshaller.Native>.ManagedToUnmanagedIn.BufferSize));
 {
 System.ReadOnlySpan<global::Test.MARSHALLED_UNICODE_STRING> __names_native__managedSpan = __names_native__marshaller.GetManagedValuesSource();
 System.Span<global::Test.MARSHALLED_UNICODE_STRING.Marshaller.Native> __names_native__nativeSpan = __names_native__marshaller.GetUnmanagedValuesDestination();
 for (int __i0 = 0; __i0 < __names_native__managedSpan.Length; ++__i0)
 {
 __names_native__nativeSpan[__i0] = global::Test.MARSHALLED_UNICODE_STRING.Marshaller.ConvertToUnmanaged(__names_native__managedSpan[__i0]);
 }
 }

 // Pin - Pin data in preparation for calling the P/Invoke.
 fixed (void* __names_native__unused = __names_native__marshaller)
 {
 // PinnedMarshal - Convert managed data to native data that requires the managed data to be pinned.
 __names_native = __names_native__marshaller.ToUnmanaged();
 System.Runtime.InteropServices.Marshal.SetLastSystemError(0);
 __retVal = __PInvoke(__names_native);
 __lastError = System.Runtime.InteropServices.Marshal.GetLastSystemError();
 }
 }
 finally
 {
 // Cleanup - Perform required cleanup.
 __names_native__marshaller.Free();
 }

 System.Runtime.InteropServices.Marshal.SetLastPInvokeError(__lastError);
 return __retVal;
 // Local P/Invoke
 [System.Runtime.InteropServices.DllImportAttribute("Advapi32", EntryPoint = "LsaLookupNames2", ExactSpelling = true)]
 static extern unsafe uint __PInvoke(global::Test.MARSHALLED_UNICODE_STRING.Marshaller.Native* names);
 }
}

Notice that there is no cleanup of the unmanaged array after the PInvoke. Are the unmanaged string copies leaking?

Author:	jkotas
Assignees:	-
Labels:	area-System.Runtime.InteropServices
Milestone:	-

[jkotas]

Found by review of ccd67b0#diff-0004b77bd28bade5f8de26f55217352ec12d8af3c7de3d2c344f8e4ff8c38cebR51 prompted by #76538

[jkotas]

Other places that marshal array using non-trivial marshallers seem to have the same problem.

[AaronRobinsonMSFT]

Yes. This looks like we are missing the element clean-up. @jkoritzinsky Can you take a look at why the container is cleaned up but we don't do the same for the elements?

runtime/src/libraries/Common/src/Interop/Windows/Advapi32/Interop.LsaLookupNames2.cs

Lines 23 to 55 in c075692

	[NativeMarshalling(typeof(Marshaller))]
	internal struct MARSHALLED_UNICODE_STRING
	{
	internal ushort Length;
	internal ushort MaximumLength;
	internal string Buffer;
	[CustomMarshaller(typeof(MARSHALLED_UNICODE_STRING), MarshalMode.ManagedToUnmanagedIn, typeof(Marshaller))]
	[CustomMarshaller(typeof(MARSHALLED_UNICODE_STRING), MarshalMode.ElementIn, typeof(Marshaller))]
	public static class Marshaller
	{
	public static Native ConvertToUnmanaged(MARSHALLED_UNICODE_STRING managed)
	{
	Native n;
	n.Length = managed.Length;
	n.MaximumLength = managed.MaximumLength;
	n.Buffer = Marshal.StringToCoTaskMemUni(managed.Buffer);
	return n;
	}
	public static void Free(Native native)
	{
	Marshal.FreeCoTaskMem(native.Buffer);
	}
	public struct Native
	{
	internal ushort Length;
	internal ushort MaximumLength;
	internal IntPtr Buffer;
	}
	}
	}

[AaronRobinsonMSFT]

Other places that marshal array using non-trivial marshallers seem to have the same problem.

Agreed. Perhaps the ElementIn mode is assuming there is nothing to clean up for element instances?

[elinor-fung]

I expect we missed doing it in our marshalling strategy for collections with non-blittable elements.

runtime/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/StatelessMarshallingStrategy.cs

Line 490 in 4ca326f

internal sealed class StatelessLinearCollectionNonBlittableElementsMarshalling : NonBlittableElementsMarshalling, ICustomTypeMarshallingStrategy