Deadlock prevention patch for Connection-callback feature #676
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The hotplug thread in all 4 implementations goes through all callbacks on every iteration of it's loop to see if any were marked for removal. This consumes a ton of CPU power in that thread. I am going to add an additional flag to mark if the check is needed at all. Returning the draft status. In hindsight, it could've been a better idea to use bit fields for some of the flags in the context structure, unless this practice is frowned upon. Will be doing that in a moment. Additionally, added the cleanup procedure to the Register call anyway. Theoretically, there is nothing that would make removal of a callback by it's return value impossible if it gets cancelled inside a Register call. The only reason it would not work right now is because the documentation dictates so. |
|
All done. If needed, I can write a simple test code that does a few operations with the callbacks that would inevitably cause undefined behavior on the old implementations (including the original Windows implementation by @DJm00n ). However, a debugger would be needed to see what's going on on the inside. |
That would be great. I can carry out some tests with the test code. I was testing libusb Windows hotplug PR but the issue is a lack of good test utility. |
Here is the exploit piece (not the full testing application code, just the "important" bit). This should trigger the postponing mechanism in the PR, and cause undefined behavior in the original on Windows and deadlock on Linux, if at least 1 HID device is present. |
|
@mcuee As per your wish... I just finished rewriting the original hidtest utility in such a way that it is now interactive. I don't exactly understand what the fixed device test does and what kind of device is listed there, but it was in the original, so I kept it in as a separate mode, just in case.
|
|
Somehow I got segmentation fault after hitting q to exit the test. |
That is with a special firmware done by Alan Ott based on Microchip's USB Simple Custom HID Demo. But he did not share the firmware modifications. You can leave it as it is. Microchip Legacy MLA: |
|
Simplest way to reproduce the issue: press No problem if I just press |
Thank you for such extensive testing! |
|
Debug log for VS2019 Debug Build. Exception generated by VS2019 Debug Build.
|
|
Under Ubuntu Linux 20.04, somehow it does not capture keyboard input at all. I have to hit CTRL-C to exit the application. |
|
@mcuee found a typo that prevented the tests on Linux from receiving input. Found a couple of errors that caused a segfault with libusb backend and a deadlock with hidraw backend, both in the cleanup sequence. Fixed them, I think this should do it. There are ways to make the implementation better (the hotplug thread shutdown for HIDRAW may still cause issues - theoretically), but for now this is the best I can write. As for windows, I accidentally broke my debugger installation :( |
|
Now I get the same segfault results under macOS. Simple step to reproduce, press Run log |
|
Minor thing for the test, it is better to echo the option chosen by the user ( |
|
No crash under Linux, tested under Ubuntu 20.04. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR provides a solution to the possible issues that happen when a register or de-register call is made from inside a callback. The way it works is the same for all platforms and is described below.
Resolves #673
Themutex_readyvariable is renamed tomutex_state, it is no longer a boolean value, and the value of 2 in it now indicates that the mutex is "In Use" (locked by a function that calls a callback).mutex_readykept intact, added 2 more flags,mutex_in_useandcb_list_dirty.When the value of mutex_state is set to 2,When themitex_in_useflag is set, the Deregister call is no longer allowed to immediately remove any callbacks from the list. Instead, theeventsfield in the callback is set to 0, which makes it "invalid", as it will no longer match any eventscb_list_dirtyflag is set, the list of callbacks is checked for any callbacks marked for removal (eventsfield set to 0), and those are only removed after all the processing is finished.HID_API_HOTPLUG_ENUMERATEis specified,themutex_statevariable is set to 2 before it is called, while the old value is preserved and restored later.mutex_in_useflag is set to prevent callback removal in that new callback.For extra safety, no callbacks are removed from the list at the end of processing in the Register call, even if there are no valid callbacks left.Fixed.