Should PHIDP_PREPARSED_DATA be a SafeHandle?

[SSL-Julian]

Hi,
I'm working on migrating to use CsWin32 instead of our own native interop definitions to ensure the interop is correct but I'm not sure how to proceed with Hid Preparsed data.

CsWin32 has created this definition for HidD_GetPreparsedData:
internal static unsafe winmdroot.Foundation.BOOLEAN HidD_GetPreparsedData(SafeHandle HidDeviceObject, out winmdroot.Devices.HumanInterfaceDevice.PHIDP_PREPARSED_DATA PreparsedData)

The PHIDP_PREPARSED_DATA is a struct that only contains a nint property, and is only passed to other HidD_* functions, but is allocated by the HidD_GetPreparsedData and must be freed with HidD_FreePreparsedData when no longer required.

Recently we discovered a memory leak in our software where the preparsed data was not being freed in some cases and changed our interop definitions to use a custom PreParsedDataSafeHandle, but I'm not sure how to proceed with CsWin32 when the definition it returns a struct containing a single nint. Should I bundle the PHIDP_PREPARSED_DATA in a custom SafeHandle, or should CsWin32 be providing a definition that returns a SafeHandle?

many thanks.

[Joel Town Road (hippi345)]

CsWin32 only generates a SafeHandle when win32metadata says what frees the type (the CloseApi field, e.g. HKEY -> RegCloseKey). PHIDP_PREPARSED_DATA has no CloseApi, so you get a plain struct. The real fix is a one-line win32metadata PR adding HidD_FreePreparsedData there.

Until then wrapping it yourself is fine:

sealed class PreparsedDataSafeHandle : SafeHandleZeroOrMinusOneIsInvalid
{
    public PreparsedDataSafeHandle(PHIDP_PREPARSED_DATA data) : base(ownsHandle: true) => SetHandle(data.Value);
    public PHIDP_PREPARSED_DATA Data => new(handle);
    protected override bool ReleaseHandle() => PInvoke.HidD_FreePreparsedData(new PHIDP_PREPARSED_DATA(handle));
}

Wrap it immediately after HidD_GetPreparsedData returns, with nothing that can throw in between.

[Jevan Saks (jevansaks)]

HidD_GetPreparsedData should give you an owned SafeHandle, and we're fixing that in win32metadata and CsWin32.

• Add RAIIFree annotations for HidD_GetPreparsedData and related producers win32metadata#2376 adds the missing annotation, tracked in HidD_GetPreparsedData and related producers are missing RAIIFree annotations win32metadata#2375. It marks the output parameter of HidD_GetPreparsedData with RAIIFree("HidD_FreePreparsedData") and sets 0 as the invalid value of PHIDP_PREPARSED_DATA. It also annotates the producers of 14 other handle types that were missing the same annotation.
• Generate SafeHandle owners for cleanup functions that return BOOLEAN #1859 lets CsWin32 generate the owner when the cleanup function returns BOOLEAN, as HidD_FreePreparsedData does.

Once both are released, the generated overload has an out HidD_FreePreparsedDataSafeHandle parameter where it has out PHIDP_PREPARSED_DATA today. The generated code gives the pointer to the owner as soon as the native call returns, so there's no window where it is unowned.

The HidP_* functions still take PHIDP_PREPARSED_DATA. When the owner is a local, pass DangerousValue inside its using block:

BOOLEAN ok = PInvoke.HidD_GetPreparsedData(device, out HidD_FreePreparsedDataSafeHandle preparsed);
using (preparsed)
{
    if (ok)
    {
        NTSTATUS status = PInvoke.HidP_GetCaps(preparsed.DangerousValue, out HIDP_CAPS caps);
    }
}

DangerousValue doesn't add a reference. It's safe here because nothing else can dispose preparsed before the using block ends. Don't store the value or free it yourself.

If the owner outlives one method, for example in a field that another thread can dispose, take a lease for each use. Lease() adds a reference, so if the owner is disposed while the lease is active, the free waits until the lease ends:

using var lease = this.preparsed.Lease();
NTSTATUS status = PInvoke.HidP_GetCaps(lease.Value, out HIDP_CAPS caps);

We ran both patterns with CsWin32 from #1859 and a winmd built from microsoft/win32metadata#2376. They read the capabilities of 32 HID collections on a test machine. Over 20,000 get-and-dispose cycles, private bytes grew by under 1 MB. The same loop without a free grew them by over 9 MB.

The annotation goes on the HidD_GetPreparsedData output parameter because HidD_FreePreparsedData frees only what HidD_GetPreparsedData allocates. Other APIs put preparsed data in buffers the caller allocates: GetRawInputDeviceInfo with RIDI_PREPARSEDDATA, and IOCTL_HID_GET_COLLECTION_DESCRIPTOR for drivers. A close function on the PHIDP_PREPARSED_DATA type would say that every value is freed with HidD_FreePreparsedData, which is wrong for those buffers. As of #1844, CsWin32 generates owned SafeHandles from annotations on the function that produces the value.

Until both changes are released, the wrapper posted above is a good way to handle this. When you switch, you can delete the wrapper, change the out type, and pass DangerousValue or lease.Value to your HidP_* calls.