Prevent memory corruption when finalizing HIDAPI #130
Conversation
Replace atexit.register() with weakref.finalize(), since that should guarantee that there are no remaining references to the module (and, thus, HIDAPI) when it runs. Related: trezor#128 ("Fatal Python error: Segmentation fault on 0.11.0.post2")
hid_exit() is only safe to call once every hid_device has been closed. Fixes: trezor#128 ("Fatal Python error: Segmentation fault on 0.11.0.post2")
Calling hid_exit() can lead to memory bugs if care is not taken. This has been observed as segmentation faults with the LibUSB backend, where hid_exit() calls libusb_exit(), which in turn has a well documented contract: > Deinitialize libusb. Should be called after closing all open devices > and before your application terminates. While the Linux HIDRAW backend appears to be more tolerant, there may be other backends that behave like LibUSB, or even have additional prerequisites for calling hid_exit(). On its part, HIDAPI is not clear about the requirements of hid_exit(): > This function frees all of the static data associated with HIDAPI. It > should be called at the end of execution to avoid memory leaks. Because of these reasons, there is no alternative but to threat hid_exit() as a very unsafe function. As such, any wrapper we provide for it – in practice, hidapi_exit() – would also have to be made safe. Unfortunately, doing that is non-trivial, and possibly quite expensive. Instead, turn hidapi_exit() into a no-op for now. The HIDAPI library will still be cleanly finalized before the program terminates, through another – automatic – mechanism. Related: trezor#128 ("Fatal Python error: Segmentation fault on 0.11.0.post2")
|
NIce work! I think we can drop the Or do you know of someone doing so? |
|
@jonasmalacofilho It seems to work in my case. Both with docker and without. I get no warnings anymore when installed your PR manually. In my case - I did not do any changes on the code, just installed your branch on top of my virtualenv, so I am not explicitly calling close() in my case and it seems to work for me. |
|
I can also confirm it works! 👍 |
Only @Teslafly, as a workaround of sorts for #114 (failure to re-enumerate devices when using docker + libusb backend). Even so, I think maintaining memory safety is more important; that is, I don't think keeping So I agree that we can probably just drop it, and I'll shortly push a commit that does that. We can always bring the no-op version back if removing it breaks a concrete downstream project. By the way, @Teslafly, maybe you can (force) use the HIDRAW backend as a better fix? |
It was not supposed to be used explicitly in the first place (see comments on the PR this commit is a part of). The HIDAPI library will still be cleanly finalized before the program terminates, through another – automatic – mechanism. Additionally, the only currently known caller is using it as a workaround for another issue (trezor#114), and that use case would not be supported by the no-op compatibility shim anyway. Related: trezor#114 ("Cannot reenumarate devices in same python process") Related: trezor#128 ("Fatal Python error: Segmentation fault on 0.11.0.post2")
|
Thanks @jonasmalacofilho! Looks great so I merged it. I will publish a new release soon on PyPI. |
|
Thanks!
I'll working on a PR to change the default backend on Linux to HIDRAW... can you wait and take a look at it before making a new release? |
Ok, I will. |
| goes to zero. This is necessary to prevent issues later in the | ||
| execution, when finalizing the HIDAPI library itself. | ||
| """ | ||
| weakref.finalize(self, self.close) |
There was a problem hiding this comment.
@jonasmalacofilho I'm wondering how this works because the doc of weakref.finalize(obj, func, /, *args, **kwargs) say:
Note: It is important to ensure that func, args and kwargs do not own any references to obj, either directly or indirectly, since otherwise obj will never be garbage collected. In particular, func should not be a bound method of obj.
There was a problem hiding this comment.
In my testing both the device was closed and hid_exit() was called, but it might have been just luck.
I'll take another look...
There was a problem hiding this comment.
yep, it also worked in my testing... maybe the behavior is different for cdef classes
From the documentation: > It is important to ensure that func, args and kwargs do not own any references to obj, either directly or indirectly, since otherwise obj will never be garbage collected. In particular, func should not be a bound method of obj. The issue wasn't observed during testing, but it may be implementation-defined (as with __del__) and depend on specific combinations of interpreter and Cython versions. Instead of relying on unspecified behavior, only keep the hid_device pointer. This requires a wrapper because weakref.finalize() cannot be passed cdef functions or C pointers. Alternatives were attempted---__dealloc__, weakref.ref(self)---but they only worked in some cases (respectively: only for `del <device>` and except for `del <device>`). Reported-by: @bearsh Related: trezor#130
From the documentation: > It is important to ensure that func, args and kwargs do not own any references to obj, either directly or indirectly, since otherwise obj will never be garbage collected. In particular, func should not be a bound method of obj. The issue wasn't observed during testing, but it may be implementation-defined (as with __del__) and depend on specific combinations of interpreter and Cython versions. Instead of relying on unspecified behavior, only keep the hid_device pointer. This requires a wrapper because weakref.finalize() cannot be passed cdef functions or C pointers. Alternatives were attempted---__dealloc__, weakref.ref(self)---but they only worked in some cases (respectively: only for `del <device>` and except for `del <device>`). Reported-by: @bearsh Related: #130
Fixes: #128