search

trezor / cython-hidapi

:snake: Python wrapper for the HIDAPI
Other
287 stars 110 forks source link

Call `hid_init()` at import time, to avoid macOS crash #150

Closed SomberNight closed 1 year ago

SomberNight commented 1 year ago

fixes https://github.com/trezor/cython-hidapi/issues/142 fixes https://github.com/trezor/cython-hidapi/issues/148

note https://github.com/trezor/cython-hidapi/issues/148#issuecomment-1542302679 :

HIDAPI on macOS has an additional issue regarding thread-safety. I did encountered it in my project(s) but didn't have enough time to gather info/find a root cause. Looks like on macOS hid_init/hid_exit needs to be called in the same thread, which is an additional restriction compared to other platforms.

I have tested on macOS 12.5 (amd64), where using the reproducers in the opening posts of https://github.com/trezor/cython-hidapi/issues/142 and https://github.com/trezor/cython-hidapi/issues/148, I can reproduce a SIGILL (without this patch). With this patch, there is no SIGILL anymore, the sample scripts work as expected.

SomberNight commented 1 year ago

note: I've only tested on macOS so far.

SomberNight commented 1 year ago

important: this PR assumes that hidapi is imported from the main thread. -- as hid_exit will be called from the main thread by: https://github.com/trezor/cython-hidapi/blob/8e92053d0d7f5a5dd9bdfeb6a78191ef717cdccf/hid.pyx#L418 and as in https://github.com/trezor/cython-hidapi/issues/148#issuecomment-1542302679, the two calls should happen on the same thread.

EDIT: and in fact the SIGILL from https://github.com/trezor/cython-hidapi/issues/142 and https://github.com/trezor/cython-hidapi/issues/148 can still be triggered even with this PR, if import hid is not run on the main thread.

EDIT2: to make this more explicit, one approach we could take is use the warnings module to emit a warning if import hid runs on a thread other than the main thread. I can have a look at that if it seems desired.

mcuee commented 1 year ago

I can confirm with this PR, Issue #148 reproducer no longer crashes under macOS Ventura 13.3.1 with my Mac Mini M1.

Same for Issue 142 reproducer -- it no longer crashes under macOS Ventura 13.3.1 with my Mac Mini M1