Consern regarding Hotplug callbacks implementation #673
Labels
enhancement
New feature or request
Comments
|
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 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
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:
currentpointer (as it will be pointing to an area that has just beenfree'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 incorrectlynextpointer 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)
The text was updated successfully, but these errors were encountered: