MelbourneDeveloper / Device.Net

A C# cross platform connected device framework
MIT License
620 stars 119 forks source link

Unmanaged memory leak in WindowsHidApiService.GetHidCapabilities #219

Open VerusMaya opened 3 years ago

VerusMaya commented 3 years ago

Describe the issue I observed an ongoing unmanaged memory leak when regularly polling for connected devices. I reproduced it with a minimal CLI .exe and narrowed it down to WindowsHidApiService.GetHidCapabilities. To resolve the leak, change the HidD_FreePreparsedData pointerToPreparsedData parameter to not be ref.

The HidD_GetPreparsedData documentation shows a *PreparsedData parameter. This matches with the out parameter option used. https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/hidsdi/nf-hidsdi-hidd_getpreparseddata

The HidD_FreePreparsedData documentation shows a PreparsedData parameter with no dereference. I believe it should therefore be called without ref. https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/hidsdi/nf-hidsdi-hidd_freepreparseddata

Relevant call stack - WindowsHidApiService.GetHidCapabilities WindowsHidApiService.GetDeviceDefinition WindowsHidDeviceFactoryExtensions.GetDeviceDefinition WindowsDeviceEnumerator.GetConnectedDeviceDefinitionsAsync

Screenshots Memory profiler running for one minute. Notice the bottom-right section for unmanaged memory breakdown by module.

hid memory leak - 1m without fix

hid memory leak - 1m fixed with fix

Your Code The memory leak can be reproduced easily.

static async Task Main(string[] args) {
    IDeviceFactory factory = new FilterDeviceDefinition(0x1234, 0x5678).CreateWindowsHidDeviceFactory();
    while (true) {
        _ = await factory.GetConnectedDeviceDefinitionsAsync();
        await Task.Delay(TimeSpan.FromMilliseconds(100));
    }
}

The leak can be resolved with two changes. https://github.com/MelbourneDeveloper/Device.Net/blob/417330ef418a7bacce36dae29eb6f67fc7620723/src/Hid.Net/Windows/WindowsHidApiService.cs#L61

private static extern bool HidD_FreePreparsedData(IntPtr pointerToPreparsedData);

https://github.com/MelbourneDeveloper/Device.Net/blob/417330ef418a7bacce36dae29eb6f67fc7620723/src/Hid.Net/Windows/WindowsHidApiService.cs#L117

isSuccess = HidD_FreePreparsedData(pointerToPreParsedData);

Log / Stack Trace No relevant logging or stack trace.

Info

PR coming soon.

VerusMaya commented 2 years ago

Following up since it has been a bit.

Have you had a chance to look into this?

It would be great to get an updated nuget package with the memory leak resolved.

Thank you!

Resonanz commented 2 years ago

?

HossamElwahsh commented 1 year ago

Great work finding this. 💯 forked and fixed. until this gets merged into main branch.