Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
May 11, 2023

Conversation

SomberNight
Copy link
Contributor

@SomberNight SomberNight commented May 10, 2023

fixes #142
fixes #148

note #148 (comment) :

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 #142 and #148, I can reproduce a SIGILL (without this patch).
With this patch, there is no SIGILL anymore, the sample scripts work as expected.

fixes trezor#142
fixes trezor#148

note trezor#148 (comment) :
> 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.
@SomberNight
Copy link
Contributor Author

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

@SomberNight
Copy link
Contributor Author

SomberNight commented May 10, 2023

important: this PR assumes that hidapi is imported from the main thread. -- as hid_exit will be called from the main thread by:

weakref.finalize(sys.modules[__name__], hid_exit)

and as in #148 (comment), the two calls should happen on the same thread.

EDIT: and in fact the SIGILL from #142 and #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
Copy link

mcuee commented May 11, 2023

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

@prusnak prusnak merged commit 04a31cb into trezor:master May 11, 2023
SomberNight added a commit to spesmilo/electrum that referenced this pull request Dec 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants