dotnet / runtime

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

[API Proposal]: Provide API for freeing Variant memory #79402

Open lowleveldesign opened 1 year ago

lowleveldesign commented 1 year ago

Background and motivation

It is currently quite hard to reclaim memory allocated internally by the Marshal.GetNativeVariantForObject method. Let's take the following code as an example:

var variantPtr = Marshal.AllocHGlobal(VariantTypeSize); // VariantTypeSize = 24 (x64) or 16 (x86)

Marshal.GetNativeVariantForObject("test string", variantPtr);

// pass the variantPtr to native code and do the work

Marshal.FreeHGlobal(variantPtr);

We are leaking memory because GetNativeVariantForObject assigns the string to the AsBstr property of the internal Variant type. The AsBstr property internally allocates memory using Marshal.StringToBSTR. One solution could be to provide the Marshal.FreeNativeVariant method, which would reclaim the internal memory.

API Proposal

namespace System.Runtime.InteropServices;

public static class Marshal
{
    public static void FreeNativeVariant(nint addr);
}

API Usage

var variantPtr = Marshal.AllocHGlobal(VariantTypeSize); // VariantTypeSize = 24 (x64) or 16 (x86)

Marshal.GetNativeVariantForObject("test string", variantPtr);

// pass the variantPtr to native code and do the work

Marshal.FreeNativeVariant(variantPtr);
Marshal.FreeHGlobal(variantPtr);

Alternative Designs

Another approach that comes to my mind is to modify the BStrWrapper class so it will be responsible for allocating and freeing memory for the wrapped BSTR string. Then we would need to instruct the users to use the wrapper when calling GetNativeVariantForObject as using raw string leads to a leak. GetNativeVariantForObject already accepts BStrWrapper, so we won't need to modify the public API.

Risks

No response

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
### Background and motivation It is currently quite hard to reclaim memory allocated internally by the `Marshal.GetNativeVariantForObject` method. Let's take the following code as an example: ```csharp var variantPtr = Marshal.AllocHGlobal(VariantTypeSize); // VariantTypeSize = 24 (x64) or 16 (x86) Marshal.GetNativeVariantForObject("test string", variantPtr); // pass the variantPtr to native code and do the work Marshal.FreeHGlobal(variantPtr); ``` We are leaking memory because `GetNativeVariantForObject` assigns the string to the `AsBstr` property of the internal `Variant` type. The `AsBstr` property internally allocates memory using `Marshal.StringToBSTR`. One solution could be to provide the `Marshal.FreeNativeVariant` method, which would reclaim the internal memory. ### API Proposal ```csharp namespace System.Runtime.InteropServices; public static class Marshal { public static void FreeNativeVariant(nint addr); } ``` ### API Usage ```csharp var variantPtr = Marshal.AllocHGlobal(VariantTypeSize); // VariantTypeSize = 24 (x64) or 16 (x86) Marshal.GetNativeVariantForObject("test string", variantPtr); // pass the variantPtr to native code and do the work Marshal.FreeNativeVariant(variantPtr); Marshal.FreeHGlobal(variantPtr); ``` ### Alternative Designs Another approach that comes to my mind is to modify the `BStrWrapper` class so it will be responsible for allocating and freeing memory for the wrapped BSTR string. Then we would need to instruct the users to use the wrapper when calling `GetNativeVariantForObject` as using raw string leads to a leak. `GetNativeVariantForObject` already accepts `BStrWrapper`, so we won't need to modify the public API. ### Risks _No response_
Author: lowleveldesign
Assignees: -
Labels: `api-suggestion`, `area-System.Runtime.InteropServices`, `untriaged`
Milestone: -
AaronRobinsonMSFT commented 1 year ago

This is handled in Win32 via VariantClear(). When marshaling through the IL Stubs this is handled by the VM itself. It does seem like a missing low-level API. I'm not sure it is worth adding given how long it has been missing, but we can see how the community responds.