search

EstebanBorai / network-interface

Retrieve system's Network Interfaces on Linux, macOS and Windows on a standardized manner
https://crates.io/crates/network-interface
Apache License 2.0
62 stars 27 forks source link

fix(windows): memory leaks #53

Closed njeisecke closed 7 months ago

njeisecke commented 7 months ago

Hi Esteban!

This fixes two leaks in the Windows implementation.

Thank you!

njeisecke commented 7 months ago

Ok, this should be the more idiomatic fix. Sorry for the noise.

scanban commented 7 months ago

about allocation retry count, in my use case it's possible that new interfaces appears in 5-10 per second. So one retry might not be enough tbh.

njeisecke commented 7 months ago

Mmh, the documentation https://docs.microsoft.com/en-us/windows/win32/api/iphlpapi/nf-iphlpapi-getadaptersaddresses#return-value is slightly vague. What does "multiple times" mean? Maybe the counter should actually be kept. In my environment, the initial call fails (probably due the various VPN virtual adapters on that machine), the second one works.

EstebanBorai commented 7 months ago

Hi guys! Currently I dont have a Windows Environment I could test against.

Changes seems okay so far, have you had any issues with this version?

EstebanBorai commented 7 months ago

It would be nice te have any memory usage audition tool integrated in CI! Im curious on the fact that both PR went up almost at same time 😅.

If its possible to share how you guys found these leaks, and if theres a way to automate such auditing, I am happy to integrate as part of existing Windows workflows!

scanban commented 7 months ago

Mmh, the documentation https://docs.microsoft.com/en-us/windows/win32/api/iphlpapi/nf-iphlpapi-getadaptersaddresses#return-value is slightly vague. What does "multiple times" mean? Maybe the counter should actually be kept. In my environment, the initial call fails (probably due the various VPN virtual adapters on that machine), the second one works.

In my use case, it's possible that new interface 'll be added after return from the 1st call to the GetAdaptersAddresses and second call to it. Ofc it's quite rare, but possible. I have multiple devices connected via USB and using RNDIS network interfaces and they are being powered up all at once. So I am getting multiple new interfaces in system in a very short time frame.

njeisecke commented 7 months ago

In my use case, it's possible that new interface 'll be added after return from the 1st call to the GetAdaptersAddresses and second call to it. Ofc it's quite rare, but possible. I have multiple devices connected via USB and using RNDIS network interfaces and they are being powered up all at once. So I am getting multiple new interfaces in system in a very short time frame.

There is no delay between the calls so it would be a rare incident for a third call to return a diifferent list than the second.

To me this sound more like you should provide some mechanism to query the adapter list in some interval und your control.

scanban commented 7 months ago

I am not sure, but I guess that windows are locking its device database during new device instantiation. Google guys think so too: https://chromium.googlesource.com/chromium/src/+/HEAD/net/base/network_interfaces_win.cc#210

njeisecke commented 7 months ago

This now also contains the @scanban changes relevant to the leak fix.