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.

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