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

Consern regarding Hotplug callbacks implementation #673

Open
Youw opened this issue Apr 6, 2024 · 1 comment
Open

Consern regarding Hotplug callbacks implementation #673

Youw opened this issue Apr 6, 2024 · 1 comment
Labels
enhancement New feature or request

Comments

@Youw
Copy link
Member

Youw commented Apr 6, 2024

          A small concern here: calling any Register or Unregister functions from within a callback may (and probably will) cause a deadlock. On Windows, Critical Sections seem to keep an internal counter that allows the same thread to enter on multiple times, while in pthread, unless PTHREAD_MUTEX_RECURSIVE is specified, it will cause a deadlock.

Additionally, just unlocking the mutex before calling the callback (or making it recursive) will allow the user to remove elements in such a way that it disturbs the execution flow, which can already happen on Windows.

Bad scenarios:

  • Removing the element right before the one currently being processed will invalidate the current pointer (as it will be pointing to an area that has just been free'd), and even if use-after-free situation doesn't break things, returning a value from the callback that deletes the callback will process the list incorrectly
  • Removing the current element will lead to exactly the same use-after-free scenario
  • Removing all elements may cause even more severe side-effects, as removing the last element triggers a cleanup sequence
  • If the next pointer is stored in advance (which it isn't in current implementation, but should be warned against in the future), adding a new element while the last one is being processed will only cause the new one to not be processed while removing an element right after the current one will cause unpredictable behavior.

There is a way to circumvent this, by adding a flag to each callback that indicates that it should be removed later when it is considered safe, which is set by the Unregister function if the mutex is already locked at the moment of the call. It seems that, as long as we know for a fact that the mutex is locked by the same thread, it should be safe to read and navigate the list of callbacks and even set flags to it, and the list will not be changed (the mutex is still locked and will not be unloced until we leave the callback and the callback processing part). As for the Register function, I see no problem with additional callbacks being registered from a callback. However, this approach still requires to somehow know for a fact that the mutex is locked by the same thread we are currently in.

Another possible resolution would be to warn against the use of Register and Unregister functions inside a callback, however, there are scenarios where the user code could make use of that (a widely used example would be the scenario where the unplug callback is only registered after a device was connected).

Originally posted by @k1-801 in #238 (comment)

@k1-801
Copy link

k1-801 commented Apr 8, 2024

Put a bit more thought into how that would work, the scenario where the current callback is removed might actually not cause any damage, if the pointer to the current element is not cahced and we only use a *current to get there, but it would still make the execution skip the next callback and immediately proceed to the one after the next one.

Anyway, I added a PR with my fix for this, but I'm asking for comments if anyone sees any issues with the proposed algorithm.

Youw pushed a commit that referenced this issue Sep 6, 2024
Fix 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 

1) The mutex is made recursive, so it can be locked by the same thread multiple times
2) `mutex_ready` kept intact, added 2 more flags, `mutex_in_use` and `cb_list_dirty`.
3) When the `mitex_in_use` flag is set, the Deregister call is no longer allowed to immediately remove any callbacks from the list. Instead, the `events` field in the callback is set to 0, which makes it "invalid", as it will no longer match any events, and the `cb_list_dirty` flag is set to 1 to indicate that the list needs to be checked for invalid events later.
4) When a callback returns a non-zero value, indicating that the callback is to be disarmed and removed from the list, it is marked in the same manner until the processing finishes (unless the callback was called directly by the Register call, in which case it's return value is ignored on purpose)
5) After all the callbacks are processed, if `cb_list_dirty` flag is set, the list of callbacks is checked for any callbacks marked for removal (`events` field set to 0), and those are only removed after all the processing is finished.
6) The Register call is allowed to register callbacks, as it causes no issues so long as the mutex it locks is recursive
7) Since the Register call can also call the new callback if `HID_API_HOTPLUG_ENUMERATE` is specified, `mutex_in_use` flag is set to prevent callback removal in that new callback.
8) The return value of any callbacks called for pre-existing devices is still ignored as per documentation and does not mark them invalid.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants