supercollider / hidapi

A Simple library for communicating with USB and Bluetooth HID devices on Linux, Mac, and Windows.
http://www.signal11.us/oss/hidapi/
Other
13 stars 14 forks source link

Fix memory leak in hid_parse_report_descriptor #16

Closed Teemperor closed 1 year ago

Teemperor commented 2 years ago

This is allocated at the top of the function but never free'd.

dyfer commented 1 year ago

@Teemperor we received a report of an issue with HID in SC 3.13: https://github.com/supercollider/supercollider/issues/6016 Do you have any insight on whether this change could've caused it?

beatboxchad commented 6 months ago

https://github.com/supercollider/supercollider/issues/6016#issuecomment-1965485813

^ my strace log shows some process trying to seek right after memory is freed, it might be cute to discover this is the culprit and put that memory leak fix somewhere after whatever code is doing that.

bgola commented 4 months ago

@dyfer I found this issue with HID today while testing some NTMI things with @adcxyz on Linux. This commit should definitely be reverted.

The code here is freeing the device_collection but that is still referenced by the device_desc which is passed by reference to the hid_parse_report_descriptor() function. See:

https://github.com/supercollider/hidapi/blob/main/hidapi_parser/hidapi_parser.c#L276

this device_desc is initialized when a new HID device is opened by calling hid_open_device() or hid_open_device_path() and it's members should not be freed until the HID device is closed by calling hid_close_device(). the device_collection is indeed freed by hid_close_device() , see:

https://github.com/supercollider/hidapi/blob/main/hidapi_parser/hidapi_parser.c#L1231

The bug only happens on Linux because Mac and Windows use hid_parse_element_info() instead of hid_parse_report_descriptor() (the one that is called on Linux), and if you check hid_parse_element_info() it is also creating a device_collection but not freeing it:

https://github.com/supercollider/hidapi/blob/main/hidapi_parser/hidapi_parser.c#L1569

In the sclang side everything seems fine, devices are created and closed using the correct functions, so device_collection. will be freed accordingly.

https://github.com/supercollider/supercollider/blob/develop/lang/LangPrimSource/SC_HID_api.cpp#L152