gh-libusb/hidapi

Watch

Fork 0

linux: deliver hotplug ENUMERATE pass asynchronously on the event context #822

Open

Youw wants to merge 15 commits from hotplug-async-enumerate-linux into connection-callback

Youw commented

2026-07-13 17:39:29 -05:00

(Migrated from github.com)

Implements the hotplug contract documented in hidapi/hidapi.h (#790) for the linux/hidraw backend.

What changed

Asynchronous ENUMERATE pass. hid_hotplug_register_callback() no longer runs the HID_API_HOTPLUG_ENUMERATE initial pass synchronously on the calling thread. Instead it takes a deep-copied, registration-time snapshot of the matching entries of the internal device cache (only when HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED is requested), stores it on the callback record (replay), and the udev monitor thread delivers it as synthetic "arrived" events:

never on an application thread and never from within the register call itself;
before any live events for that callback — hid_internal_invoke_callbacks() flushes a callback's pending snapshot before handing it a live event, and the monitor thread also flushes pending snapshots at the top of its loop (the existing 5 ms poll timeout caps the latency, no extra wakeup fd needed);
exactly once per device connection: the snapshot is taken under the hotplug mutex, so a device arrival is reported either by the pass or as a live event, never both;
a non-zero callback return stops the remainder of the pass and deregisters the callback (undelivered snapshot entries are freed), same as for live events.

This also covers registrations made while the monitor thread is already running, including registrations made from within another hotplug callback.

device->next is now NULL for every invocation. The udev arrival path used to pass the chained multi-usage list to callbacks (the old TODO); each entry is now delivered on its own, keeping the internal cache intact for later "left" matching.

Error strings and out-parameter. Every failure path of register/deregister (NULL callback, empty/unknown events bits, unknown flags bits, allocation/udev/thread-start failures, unknown or stale deregister handle) now sets the global error string retrievable via hid_error(NULL), and register sets *callback_handle to 0 on failure before any error return. (when called from an application thread; per hidapi.h, a call made from within a hotplug callback reports only -1 and leaves hid_error(NULL) untouched)

Monitor-thread lifecycle fixes. Previously, when the last callback removed itself on the monitor thread (non-zero return or in-callback deregister), the thread exited on its own, was never joined, and freed the shared device list / udev monitor unlocked — racing a subsequent first registration that re-creates them. Now the exiting thread releases the monitoring context (device cache, udev monitor) itself under the mutex and publishes FINISHED; its pthread_t is retired onto a tracked list and joined by hid_exit() or the next register/deregister from an application thread.

Design decisions / deviations

hid_enumerate(0, 0) inside the first registration may set the "No HID devices found" global error on an empty system; register clears it, since an empty system is not a registration failure. A genuine enumeration failure does fail the registration.
The initial pass is all-or-nothing: if the snapshot cannot be copied in full, the registration is unwound and fails, rather than delivering a partial device list (each connection must be reported exactly once — never "neither").

Verification

gcc -fsyntax-only -Wall -Wextra -pedantic -Werror -Wformat-signedness and g++ -fsyntax-only -Wall -Wextra -Werror clean; full CMake builds (C and HIDAPI_BUILD_AS_CXX=ON with -Werror) pass.
The WSL2 kernel used for testing has no uhid/hidraw support (CONFIG_UHID not set), so the #815 virtual-device harness could not drive real udev events. Instead a throwaway whitebox harness (including linux/hid.c directly, injecting fake entries into the hotplug device cache and driving the internal arrival/removal handlers) exercised 119 checks: async delivery on the monitor thread (never the registering thread), VID/PID filtering of the snapshot, next == NULL on every invocation, exactly-once vs. later-connected devices and vs. a pre-queued add, non-zero return stopping the pass, self-deregistration and nested registration from within a callback, registration from within an in-flight dispatch (both event types, multi-usage devices), tombstoned/stale-handle deregister, handle exhaustion, last-callback self-removal followed by re-registration (thread reaping), hid_exit() with an undelivered snapshot, re-initialization after hid_exit(), concurrent first registrations, concurrent stale deregistrations, and all argument-validation error strings.
10/10 runs clean under ASan+UBSan+LSan (no leaks) and 10/10 clean under TSan.
Live udev arrival/removal delivery was not exercised against real hardware and still deserves a check on a real machine.

Addresses #792 and #793 for the linux/hidraw backend.

Review round 1

All findings from the first review round are addressed in linux: address the hotplug review findings:

Global error-string race. Register/deregister wrote last_global_error_str with no lock (on the validation paths, before the hotplug mutex exists, and on the post-unlock paths), and the monitor thread wrote it too (via create_device_info_for_device()); two concurrent, documented-thread-safe calls could double-free it. Every mutation now goes through a file-static mutex in the lowest-level writer (register_error_str()), which all the register_global_error*() / register_device_error*() helpers funnel into. Reading via hid_error() remains subject to the documented thread-safety rules.
First-initialization race. Two concurrent first registrations could both observe mutex_ready == 0 and both pthread_mutex_init() the hotplug mutex (undefined behaviour) while resetting the context fields under each other. A static bootstrap mutex now guards the machinery init/teardown block. (superseded: see Final state)
hid_init() race (found while making TSan clean). A registration implicitly initializes the library twice — directly, and again inside the initial enumeration — and setlocale() is not thread-safe against itself, so two concurrent registrations raced in hid_init(). hid_init() now serializes itself.
Liveness / publication. The monitor loop no longer reads any decision state unlocked: it trylocks every iteration and checks the callback list (and the pending initial passes) only under the acquired mutex, so a stale read cannot keep it alive or make it exit early. The thread announces its exit under the mutex, and hid_internal_hotplug_cleanup() unlocks before pthread_join() — nothing is ever joined while holding the mutex. This removed the previously "excused" races, and TSan is now fully clean.
Duplicate arrival. A device showing up between udev_monitor_enable_receiving() and the initial hid_enumerate() was in both the snapshot and the queued netlink event, producing a duplicate "arrived", a duplicate cache entry and later a duplicate "left". Arrivals whose path is already cached are now skipped.
Dispatch bounding and cache order. The device cache is updated before an event is dispatched, and every dispatch (both event types) is bounded to the callbacks registered when the event started. A callback registered from within a callback therefore observes the device exactly once, through its own snapshot, instead of also receiving the in-flight event — and, for a multi-usage device, all of its usage entries are added to (or removed from) the cache before any of them is dispatched, so such a callback can neither miss a device nor see a "left" with no matching "arrived".
All-or-nothing snapshot. A failed snapshot copy now frees the partial snapshot, unwinds the registration completely (leaving *callback_handle at 0), sets the error string and returns -1. A partial hid_internal_copy_device_info() result is likewise rejected instead of reaching a callback. A genuine hid_enumerate() failure now fails the registration, and is distinguished from an empty system.
libudev setup and fd 0. Every libudev call in the registration path is checked and fails the registration with an error string. 0 is a valid file descriptor: -1 is now the sentinel, and the checks test for < 0.
Handle exhaustion. next_handle++ at INT_MAX was signed-overflow undefined behaviour, and wrapping back to 1 could collide with a live handle. Registration now fails with an error instead of wrapping.
Tombstoned handle. Deregistering a handle that was already deregistered from within a callback (and is only awaiting its postponed removal) now reports "not found" (-1) instead of 0.
Early return in deregister. Deregistration now reaps a previously self-exited monitor thread before returning early on an empty callback list, instead of leaving the monitoring context behind until the next registration or hid_exit().
Robustness. The udev action and devnode are checked for NULL, and poll() replaces select() for the monitor file descriptor (no FD_SETSIZE undefined behaviour).

Final state (after review)

The narrative above traces the implementation; the merged design is: a udev-monitor thread delivering an asynchronous, registration-time replay snapshot; a process-lifetime recursive mutex; and a retired-thread-list lifecycle in which every monitor generation ever created is tracked and joined before hid_exit() returns (identity is a monotonic token, never a post-join pthread_t). On an unrecoverable udev-monitor failure the backend stops cleanly — no fabricated DEVICE_LEFT, cache and open handles untouched, new registrations refused. (An earlier monitor auto-recovery attempt was dropped as unfixable; recorded as a possible future enhancement in #828.) The round-1 bootstrap mutex was replaced by pthread_once() for one-time init plus an exiting flag checked under the process-lifetime recursive hotplug mutex (same design as the mac and Windows backends).

Verification. Every fix commit on this branch was adversarially delta-verified by an independent reviewer (Codex on a separate plan, or a fresh model) tasked to find what the fix itself broke — a loop that caught a defect in nearly every fix commit. A per-backend white-box harness injects fake device events into the real implementation and runs under ASan/UBSan/LSan and TSan (TSan proven live). The hotplug-integration branch — all four final backends merged with the test suite (#826) — is CI-green on every platform, with HotplugAPI_hidraw and the device-backed Hotplug_hidraw (uhid) executing per push.

Implements the device->next == NULL (#791, #792) and ENUMERATE return-value (#793) contract for hidraw. Part of hotplug support (#238); contract documented in #790. Follow-up: #828.

Assisted-by: claude-code:claude-opus-4-8
Assisted-by: claude-code:claude-opus-5
Assisted-by: claude-code:claude-fable-5
Assisted-by: claude-code:claude-fable-5-1
Assisted-by: codex-cli:gpt-6-astra

Implements the hotplug contract documented in `hidapi/hidapi.h` (#790) for the linux/hidraw backend. ## What changed **Asynchronous ENUMERATE pass.** `hid_hotplug_register_callback()` no longer runs the `HID_API_HOTPLUG_ENUMERATE` initial pass synchronously on the calling thread. Instead it takes a deep-copied, registration-time snapshot of the matching entries of the internal device cache (only when `HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED` is requested), stores it on the callback record (`replay`), and the udev monitor thread delivers it as synthetic "arrived" events: - never on an application thread and never from within the register call itself; - before any live events for that callback — `hid_internal_invoke_callbacks()` flushes a callback's pending snapshot before handing it a live event, and the monitor thread also flushes pending snapshots at the top of its loop (the existing 5 ms poll timeout caps the latency, no extra wakeup fd needed); - exactly once per device connection: the snapshot is taken under the hotplug mutex, so a device arrival is reported either by the pass or as a live event, never both; - a non-zero callback return stops the remainder of the pass and deregisters the callback (undelivered snapshot entries are freed), same as for live events. This also covers registrations made while the monitor thread is already running, including registrations made from within another hotplug callback. **`device->next` is now NULL for every invocation.** The udev arrival path used to pass the chained multi-usage list to callbacks (the old TODO); each entry is now delivered on its own, keeping the internal cache intact for later "left" matching. **Error strings and out-parameter.** Every failure path of register/deregister (NULL callback, empty/unknown events bits, unknown flags bits, allocation/udev/thread-start failures, unknown or stale deregister handle) now sets the global error string retrievable via `hid_error(NULL)`, and register sets `*callback_handle` to 0 on failure before any error return. (when called from an application thread; per hidapi.h, a call made from within a hotplug callback reports only -1 and leaves hid_error(NULL) untouched) **Monitor-thread lifecycle fixes.** Previously, when the last callback removed itself on the monitor thread (non-zero return or in-callback deregister), the thread exited on its own, was never joined, and freed the shared device list / udev monitor unlocked — racing a subsequent first registration that re-creates them. Now the exiting thread releases the monitoring context (device cache, udev monitor) itself under the mutex and publishes FINISHED; its pthread_t is retired onto a tracked list and joined by hid_exit() or the next register/deregister from an application thread. ## Design decisions / deviations - `hid_enumerate(0, 0)` inside the first registration may set the "No HID devices found" global error on an empty system; register clears it, since an empty system is not a registration failure. A *genuine* enumeration failure does fail the registration. - The initial pass is all-or-nothing: if the snapshot cannot be copied in full, the registration is unwound and fails, rather than delivering a partial device list (each connection must be reported exactly once — never "neither"). ## Verification - `gcc -fsyntax-only -Wall -Wextra -pedantic -Werror -Wformat-signedness` and `g++ -fsyntax-only -Wall -Wextra -Werror` clean; full CMake builds (C and `HIDAPI_BUILD_AS_CXX=ON` with `-Werror`) pass. - The WSL2 kernel used for testing has no uhid/hidraw support (`CONFIG_UHID` not set), so the #815 virtual-device harness could not drive real udev events. Instead a throwaway whitebox harness (including `linux/hid.c` directly, injecting fake entries into the hotplug device cache and driving the internal arrival/removal handlers) exercised 119 checks: async delivery on the monitor thread (never the registering thread), VID/PID filtering of the snapshot, `next == NULL` on every invocation, exactly-once vs. later-connected devices and vs. a pre-queued add, non-zero return stopping the pass, self-deregistration and nested registration from within a callback, registration from within an in-flight dispatch (both event types, multi-usage devices), tombstoned/stale-handle deregister, handle exhaustion, last-callback self-removal followed by re-registration (thread reaping), `hid_exit()` with an undelivered snapshot, re-initialization after `hid_exit()`, concurrent first registrations, concurrent stale deregistrations, and all argument-validation error strings. - 10/10 runs clean under ASan+UBSan+LSan (no leaks) and 10/10 clean under TSan. - Live udev arrival/removal delivery was not exercised against real hardware and still deserves a check on a real machine. Addresses #792 and #793 for the linux/hidraw backend. ## Review round 1 All findings from the first review round are addressed in `linux: address the hotplug review findings`: - **Global error-string race.** Register/deregister wrote `last_global_error_str` with no lock (on the validation paths, before the hotplug mutex exists, and on the post-unlock paths), and the monitor thread wrote it too (via `create_device_info_for_device()`); two concurrent, documented-thread-safe calls could double-free it. Every mutation now goes through a file-static mutex in the lowest-level writer (`register_error_str()`), which all the `register_global_error*()` / `register_device_error*()` helpers funnel into. Reading via `hid_error()` remains subject to the documented thread-safety rules. - **First-initialization race.** Two concurrent first registrations could both observe `mutex_ready == 0` and both `pthread_mutex_init()` the hotplug mutex (undefined behaviour) while resetting the context fields under each other. A static bootstrap mutex now guards the machinery init/teardown block. (superseded: see Final state) - **`hid_init()` race (found while making TSan clean).** A registration implicitly initializes the library twice — directly, and again inside the initial enumeration — and `setlocale()` is not thread-safe against itself, so two concurrent registrations raced in `hid_init()`. `hid_init()` now serializes itself. - **Liveness / publication.** The monitor loop no longer reads any decision state unlocked: it `trylock`s every iteration and checks the callback list (and the pending initial passes) only under the acquired mutex, so a stale read cannot keep it alive or make it exit early. The thread announces its exit under the mutex, and `hid_internal_hotplug_cleanup()` unlocks before `pthread_join()` — nothing is ever joined while holding the mutex. This removed the previously "excused" races, and TSan is now fully clean. - **Duplicate arrival.** A device showing up between `udev_monitor_enable_receiving()` and the initial `hid_enumerate()` was in both the snapshot and the queued netlink event, producing a duplicate "arrived", a duplicate cache entry and later a duplicate "left". Arrivals whose path is already cached are now skipped. - **Dispatch bounding and cache order.** The device cache is updated *before* an event is dispatched, and every dispatch (both event types) is bounded to the callbacks registered when the event started. A callback registered from within a callback therefore observes the device exactly once, through its own snapshot, instead of also receiving the in-flight event — and, for a multi-usage device, all of its usage entries are added to (or removed from) the cache before any of them is dispatched, so such a callback can neither miss a device nor see a "left" with no matching "arrived". - **All-or-nothing snapshot.** A failed snapshot copy now frees the partial snapshot, unwinds the registration completely (leaving `*callback_handle` at 0), sets the error string and returns -1. A partial `hid_internal_copy_device_info()` result is likewise rejected instead of reaching a callback. A genuine `hid_enumerate()` failure now fails the registration, and is distinguished from an empty system. - **libudev setup and fd 0.** Every libudev call in the registration path is checked and fails the registration with an error string. `0` is a valid file descriptor: `-1` is now the sentinel, and the checks test for `< 0`. - **Handle exhaustion.** `next_handle++` at `INT_MAX` was signed-overflow undefined behaviour, and wrapping back to 1 could collide with a live handle. Registration now fails with an error instead of wrapping. - **Tombstoned handle.** Deregistering a handle that was already deregistered from within a callback (and is only awaiting its postponed removal) now reports "not found" (-1) instead of 0. - **Early return in deregister.** Deregistration now reaps a previously self-exited monitor thread before returning early on an empty callback list, instead of leaving the monitoring context behind until the next registration or `hid_exit()`. - **Robustness.** The udev action and devnode are checked for NULL, and `poll()` replaces `select()` for the monitor file descriptor (no `FD_SETSIZE` undefined behaviour). --- ### Final state (after review) The narrative above traces the implementation; the merged design is: a udev-monitor thread delivering an asynchronous, registration-time replay snapshot; a process-lifetime recursive mutex; and a **retired-thread-list** lifecycle in which every monitor generation ever created is tracked and joined before `hid_exit()` returns (identity is a monotonic token, never a post-join `pthread_t`). On an unrecoverable udev-monitor failure the backend **stops cleanly** — no fabricated `DEVICE_LEFT`, cache and open handles untouched, new registrations refused. (An earlier monitor auto-recovery attempt was dropped as unfixable; recorded as a possible future enhancement in #828.) The round-1 bootstrap mutex was replaced by pthread_once() for one-time init plus an `exiting` flag checked under the process-lifetime recursive hotplug mutex (same design as the mac and Windows backends). **Verification.** Every fix commit on this branch was adversarially delta-verified by an independent reviewer (Codex on a separate plan, or a fresh model) tasked to find what the fix itself broke — a loop that caught a defect in nearly every fix commit. A per-backend white-box harness injects fake device events into the real implementation and runs under ASan/UBSan/LSan and TSan (TSan proven live). The `hotplug-integration` branch — all four final backends merged with the test suite (#826) — is CI-green on every platform, with HotplugAPI_hidraw and the device-backed Hotplug_hidraw (uhid) executing per push. Implements the `device->next == NULL` (#791, #792) and ENUMERATE return-value (#793) contract for hidraw. Part of hotplug support (#238); contract documented in #790. Follow-up: #828. Assisted-by: claude-code:claude-opus-4-8 Assisted-by: claude-code:claude-opus-5 Assisted-by: claude-code:claude-fable-5 Assisted-by: claude-code:claude-fable-5-1 Assisted-by: codex-cli:gpt-6-astra

Youw commented

2026-08-04 11:54:58 -05:00

(Migrated from github.com)

Full implementation review (fresh pass, head `72b00d7`)

Traced all hotplug paths against the contract in hidapi/hidapi.h: registration (first and subsequent), the replay-snapshot machinery, live dispatch, tombstoning/postponed removal, the monitor-thread loop, the retire/reap/join lifecycle, and hid_exit teardown. Each candidate finding was adversarially self-checked before being reported. Overall the implementation is in very good shape — the core contract (registration-time snapshot, exactly-once XOR delivery, replay-before-live ordering, nonzero-return deregistration, deregister-blocks-on-in-flight, handle monotonicity, monitor-death stop-cleanly, retired-thread-list join lifecycle) holds up under adversarial reading. 3 findings survived: 1 major, 2 minor. No blockers.

[major] `linux/hid.c:1412-1416` — `hid_exit()` from a TSD destructor on the finished monitor thread self-deadlocks and permanently bricks the hotplug API

The comment at 1412-1413 asserts "hid_exit() is never the monitor thread, so no retired entry is ever 'self' here" — but the file explicitly engineers for thread-specific-data destructors re-entering HIDAPI on the just-finished monitor thread (986-988, 2266-2271 support register from exactly that context), and hidapi.h's UB carve-out covers only hid_exit "from within the callback itself", not a TSD destructor.

Trace: monitor thread publishes FINISHED, its start routine returns, a user TSD destructor calls hid_exit() → hid_internal_hotplug_exit(): exiting is 0, mutex_in_use is 0, so it sets exiting = 1 and calls the reaper, which retires the generation onto retired and then skips it (pthread_equal(pthread_self(), ...), line 1205). Back at 1414, retired != NULL → pthread_cond_wait() — forever: the only broadcast source is a join completion (1230), but no other thread can ever reach the reaper, because register (2106), deregister (2416), and a second hid_exit (1356) all bail out early on exiting == 1, which is now stuck at 1. The destructor thread hangs, can never be joined, and every future hotplug call fails with "hid_exit() is in progress" for the life of the process.

Suggested fix: in the hid_internal_hotplug_exit() wait loop, before waiting, scan retired: if every remaining entry that is not being_joined is pthread_equal to self, break out instead of waiting (leaving the self entries for the next register/deregister/hid_exit to reap — the same documented bounded-leak degradation used for the in-callback case), so exiting resets to 0 and the API stays usable. Only wait on thread_cond when a non-self entry is actually being joined by another reaper. Correct the 1412-1413 comment.

[minor] `linux/hid.c:2080` — `quiet` detection misses the TSD-destructor path: register/deregister on the (finished) monitor thread write the global error string

quiet is derived solely from mutex_in_use (2080; 2406 in deregister). A registration from a TSD destructor running on the just-finished monitor thread — a path the file explicitly supports (2266-2271) — has mutex_in_use == 0, so quiet == 0: it calls hid_init() (2121, clears the global error) and on failure paths writes last_global_error_str (2256, 2303, 2323, plus register_global_error inside hid_internal_enumerate reached from 2243). That violates the invariant that internal threads never write the global error string — an application thread reading hid_error(NULL) (unlocked read, 2895-2897) cannot serialize against a write it doesn't know is happening. The comment at 2205-2210 is false for exactly this case.

Suggested fix: under the mutex, extend the check: quiet = mutex_in_use || on_monitor_thread(), where on_monitor_thread() compares pthread_self() via pthread_equal against the current generation's thread (when thread_state != NONE) and against every retired entry that is not being_joined (a being-joined thread has terminated and cannot be self; skipping it preserves the never-inspect-a-joined-pthread_t discipline). Fix the 2205-2210 comment.

[minor] `linux/hid.c:746,885-892` — unchecked `strdup`/`wcsdup` results defeat the all-or-nothing `failure` flag and can cache a NULL-path device

create_device_info_for_device() sets *failure only on the two calloc failures (736-739, 873-879), but cur_dev->path = dev_path ? strdup(dev_path) : NULL (746) and the strdup/wcsdup calls in the additional-usage loop (885-892) fail silently under OOM. Consequences: (a) the registration-time snapshot (2243), whose stated purpose is to fail rather than arm the callbacks against an incomplete device set, can succeed with silently degraded entries; (b) on the monitor-thread arrival path a path == NULL entry enters the cache, its ARRIVED is dispatched, but hid_internal_hotplug_process_removal (1838) and the duplicate check (1741) can never match it — an "arrived" with no possible "left", and broken same-node dedup, contradicting the exactly-once contract the flag was added to protect.

Suggested fix: apply the same partial-copy check already used in hid_internal_copy_device_info (1646-1652): after filling an entry, if any source string was non-NULL but its copy is NULL, set *failure (and on the monitor-thread arrival path drop the whole device chain instead of caching it).

Verified sound (checked, no issues): exactly-once delivery (monitor armed before initial enumeration, path-dedup, dispatch_bound handle-ordering for nested registrations); replay-before-live per-callback flush; all snapshot-free paths on deregister/tombstone/exit; thread lifecycle (pre-allocated retirement nodes, joins outside the mutex, self/double-join prevention, no lost wakeups); lock ordering (hotplug mutex → hid_init_mutex → error_str_mutex); EINTR/ENOBUFS/POLLERR handling with the 100-poll limit; monitor-death stop-cleanly semantics; udev-unavailable failure path.

Drafted with Claude Code.

## Full implementation review (fresh pass, head `72b00d7`) Traced all hotplug paths against the contract in `hidapi/hidapi.h`: registration (first and subsequent), the replay-snapshot machinery, live dispatch, tombstoning/postponed removal, the monitor-thread loop, the retire/reap/join lifecycle, and `hid_exit` teardown. Each candidate finding was adversarially self-checked before being reported. Overall the implementation is in very good shape — the core contract (registration-time snapshot, exactly-once XOR delivery, replay-before-live ordering, nonzero-return deregistration, deregister-blocks-on-in-flight, handle monotonicity, monitor-death stop-cleanly, retired-thread-list join lifecycle) holds up under adversarial reading. **3 findings survived: 1 major, 2 minor. No blockers.** ### [major] `linux/hid.c:1412-1416` — `hid_exit()` from a TSD destructor on the finished monitor thread self-deadlocks and permanently bricks the hotplug API The comment at 1412-1413 asserts *"hid_exit() is never the monitor thread, so no retired entry is ever 'self' here"* — but the file explicitly engineers for thread-specific-data destructors re-entering HIDAPI on the just-finished monitor thread (986-988, 2266-2271 support *register* from exactly that context), and `hidapi.h`'s UB carve-out covers only `hid_exit` "from within the callback itself", not a TSD destructor. Trace: monitor thread publishes FINISHED, its start routine returns, a user TSD destructor calls `hid_exit()` → `hid_internal_hotplug_exit()`: `exiting` is 0, `mutex_in_use` is 0, so it sets `exiting = 1` and calls the reaper, which retires the generation onto `retired` and then *skips it* (`pthread_equal(pthread_self(), ...)`, line 1205). Back at 1414, `retired != NULL` → `pthread_cond_wait()` — forever: the only broadcast source is a join completion (1230), but no other thread can ever reach the reaper, because `register` (2106), `deregister` (2416), and a second `hid_exit` (1356) all bail out early on `exiting == 1`, which is now stuck at 1. The destructor thread hangs, can never be joined, and every future hotplug call fails with "hid_exit() is in progress" for the life of the process. **Suggested fix:** in the `hid_internal_hotplug_exit()` wait loop, before waiting, scan `retired`: if every remaining entry that is not `being_joined` is `pthread_equal` to self, break out instead of waiting (leaving the self entries for the next register/deregister/`hid_exit` to reap — the same documented bounded-leak degradation used for the in-callback case), so `exiting` resets to 0 and the API stays usable. Only wait on `thread_cond` when a non-self entry is actually being joined by another reaper. Correct the 1412-1413 comment. ### [minor] `linux/hid.c:2080` — `quiet` detection misses the TSD-destructor path: register/deregister on the (finished) monitor thread write the global error string `quiet` is derived solely from `mutex_in_use` (2080; 2406 in deregister). A registration from a TSD destructor running on the just-finished monitor thread — a path the file explicitly supports (2266-2271) — has `mutex_in_use == 0`, so `quiet == 0`: it calls `hid_init()` (2121, clears the global error) and on failure paths writes `last_global_error_str` (2256, 2303, 2323, plus `register_global_error` inside `hid_internal_enumerate` reached from 2243). That violates the invariant that internal threads never write the global error string — an application thread reading `hid_error(NULL)` (unlocked read, 2895-2897) cannot serialize against a write it doesn't know is happening. The comment at 2205-2210 is false for exactly this case. **Suggested fix:** under the mutex, extend the check: `quiet = mutex_in_use || on_monitor_thread()`, where `on_monitor_thread()` compares `pthread_self()` via `pthread_equal` against the current generation's thread (when `thread_state != NONE`) and against every `retired` entry that is not `being_joined` (a being-joined thread has terminated and cannot be self; skipping it preserves the never-inspect-a-joined-`pthread_t` discipline). Fix the 2205-2210 comment. ### [minor] `linux/hid.c:746,885-892` — unchecked `strdup`/`wcsdup` results defeat the all-or-nothing `failure` flag and can cache a NULL-path device `create_device_info_for_device()` sets `*failure` only on the two `calloc` failures (736-739, 873-879), but `cur_dev->path = dev_path ? strdup(dev_path) : NULL` (746) and the strdup/wcsdup calls in the additional-usage loop (885-892) fail silently under OOM. Consequences: (a) the registration-time snapshot (2243), whose stated purpose is to *fail rather than arm the callbacks against an incomplete device set*, can succeed with silently degraded entries; (b) on the monitor-thread arrival path a `path == NULL` entry enters the cache, its ARRIVED is dispatched, but `hid_internal_hotplug_process_removal` (1838) and the duplicate check (1741) can never match it — an "arrived" with no possible "left", and broken same-node dedup, contradicting the exactly-once contract the flag was added to protect. **Suggested fix:** apply the same partial-copy check already used in `hid_internal_copy_device_info` (1646-1652): after filling an entry, if any source string was non-NULL but its copy is NULL, set `*failure` (and on the monitor-thread arrival path drop the whole device chain instead of caching it). --- Verified sound (checked, no issues): exactly-once delivery (monitor armed before initial enumeration, path-dedup, `dispatch_bound` handle-ordering for nested registrations); replay-before-live per-callback flush; all snapshot-free paths on deregister/tombstone/exit; thread lifecycle (pre-allocated retirement nodes, joins outside the mutex, self/double-join prevention, no lost wakeups); lock ordering (hotplug mutex → `hid_init_mutex` → `error_str_mutex`); EINTR/ENOBUFS/POLLERR handling with the 100-poll limit; monitor-death stop-cleanly semantics; udev-unavailable failure path. _Drafted with Claude Code._

Youw commented

2026-08-04 12:23:57 -05:00

(Migrated from github.com)

Re-review of fix commits `97760f9` + `18434e0`

Fresh delta reviews (independent reviewers, one per round) verified the fixes against all three findings and hunted for newly introduced defects.

Round 1 (97760f9): the unchecked-strdup finding was verified FIXED end-to-end (checks trip only on non-NULL-source → NULL-copy; utf8_to_wchar_t returns NULL only on allocation failure; the USB-with-parent attribute-absent exemption is scoped exactly right; the arrival path drops the whole chain on failure and process_arrival(NULL)/uncached-path removals are clean no-ops). The TSD-destructor deadlock and quiet-detection fixes were verified for the primary scenario, but the reviewer found a residual variant: a being_joined self entry was still misclassified as "others" — being_joined is set before pthread_join, and the join spans the target's TSD-destructor phase, so "being joined ⇒ terminated ⇒ cannot be self" was a false premise. A monitor thread whose entry was concurrently claimed by an app-thread reaper could still deadlock permanently in its destructor-phase hid_exit, and the same window defeated the quiet detection.

Round 2 (18434e0) replaces pthread_equal-based self-detection with a thread-local generation id, and the round-2 review confirms RESIDUAL CLOSED, 0 new defects:

Publication ordering holds: the registering thread holds the mutex continuously from pthread_create through id publication, and the new thread's first action is a blocking mutex lock, so its stash happens-after publication; a competing overwrite requires FINISHED, which the thread publishes strictly after its own stash — in every interleaving, including the post-create registration-failure unwind.
The exit scan classifies purely by id: the caller's own entry is recognized as self regardless of join state, falls through, resets exiting, and never frees anything (the only free is in the join claimant's post-join block — no double-free). App-thread hid_exit still reaps and joins every non-self entry, waiting only on claimed entries whose claimant is guaranteed to broadcast; the cross-generation two-destructor variant resolves via the exiting early-return.
hid_internal_on_monitor_thread() reduces to a thread-identity check with correct semantics for both quiet call sites; the one remaining pthread_equal (reap self-skip) is short-circuit-guarded by !being_joined, so a claimed pthread_t is never inspected. Types are uniform (unsigned long throughout); thread-locals are zero-initialized per C11, so recycled OS threads cannot inherit a stale id.

Both commits build green across the CI matrix. One pre-existing doc nit surfaced in passing (the cleanup comment about a slot staying FINISHED describes branches that can no longer occur with the pre-allocated retirement node) — cosmetic only, noted for a future pass.

PR #822 is review-clean from my side.

Drafted with Claude Code.

## Re-review of fix commits `97760f9` + `18434e0` Fresh delta reviews (independent reviewers, one per round) verified the fixes against all three findings and hunted for newly introduced defects. **Round 1 (`97760f9`):** the unchecked-strdup finding was verified FIXED end-to-end (checks trip only on non-NULL-source → NULL-copy; `utf8_to_wchar_t` returns NULL only on allocation failure; the USB-with-parent attribute-absent exemption is scoped exactly right; the arrival path drops the whole chain on failure and `process_arrival(NULL)`/uncached-path removals are clean no-ops). The TSD-destructor deadlock and quiet-detection fixes were verified for the primary scenario, but the reviewer found a **residual variant**: a `being_joined` *self* entry was still misclassified as "others" — `being_joined` is set *before* `pthread_join`, and the join spans the target's TSD-destructor phase, so "being joined ⇒ terminated ⇒ cannot be self" was a false premise. A monitor thread whose entry was concurrently claimed by an app-thread reaper could still deadlock permanently in its destructor-phase `hid_exit`, and the same window defeated the `quiet` detection. **Round 2 (`18434e0`)** replaces `pthread_equal`-based self-detection with a thread-local generation id, and the round-2 review confirms **RESIDUAL CLOSED, 0 new defects**: - Publication ordering holds: the registering thread holds the mutex continuously from `pthread_create` through id publication, and the new thread's first action is a blocking mutex lock, so its stash happens-after publication; a competing overwrite requires FINISHED, which the thread publishes strictly after its own stash — in every interleaving, including the post-create registration-failure unwind. - The exit scan classifies purely by id: the caller's own entry is recognized as self regardless of join state, falls through, resets `exiting`, and never frees anything (the only free is in the join claimant's post-join block — no double-free). App-thread `hid_exit` still reaps and joins every non-self entry, waiting only on claimed entries whose claimant is guaranteed to broadcast; the cross-generation two-destructor variant resolves via the `exiting` early-return. - `hid_internal_on_monitor_thread()` reduces to a thread-identity check with correct semantics for both `quiet` call sites; the one remaining `pthread_equal` (reap self-skip) is short-circuit-guarded by `!being_joined`, so a claimed `pthread_t` is never inspected. Types are uniform (`unsigned long` throughout); thread-locals are zero-initialized per C11, so recycled OS threads cannot inherit a stale id. Both commits build green across the CI matrix. One pre-existing doc nit surfaced in passing (the cleanup comment about a slot staying FINISHED describes branches that can no longer occur with the pre-allocated retirement node) — cosmetic only, noted for a future pass. PR #822 is review-clean from my side. _Drafted with Claude Code._

Youw commented

2026-09-08 15:49:53 -05:00

(Migrated from github.com)

Fresh review pass (2026-09-08)

Two independent reviews (Claude Fable 5.1 and gpt-6-astra), merged and deduplicated: 28 findings (1 major, 6 minor, 7 nit, 14 note).

Fixes landed in three commits:

179dbfb - lifecycle and error-handling findings (round 1)
bec001f - named timespec for the poll pacing (the C++ build rejected the compound-literal address)
b077b35 - join hardening (a failed join no longer spins hid_exit()) and only transport-level udev_monitor_receive_device() errors are treated as fatal

Final adversarial verification (Fable, on the final state): 14 fixed; 1 partial - linux-4, the reused-devnode window inside the arm-then-snapshot interval is documented (README and the process_arrival() comment) rather than fenced by syspath/seqnum, the fallback the brief allowed; 1 not fixed - linux-18, no TSan stress regression was added (note-level). The two notes the verifier raised (the hid_exit() spin after a failed join, per-message udev parse errors killing the monitor) are the ones fixed in b077b35.

CI: green on b077b35; the device-backed suites pass on the aggregate branch (#830).

Drafted with Claude Code.

## Fresh review pass (2026-09-08) Two independent reviews (Claude Fable 5.1 and gpt-6-astra), merged and deduplicated: **28 findings** (1 major, 6 minor, 7 nit, 14 note). Fixes landed in three commits: - 179dbfb - lifecycle and error-handling findings (round 1) - bec001f - named `timespec` for the poll pacing (the C++ build rejected the compound-literal address) - b077b35 - join hardening (a failed join no longer spins `hid_exit()`) and only transport-level `udev_monitor_receive_device()` errors are treated as fatal Final adversarial verification (Fable, on the final state): 14 fixed; 1 partial - linux-4, the reused-devnode window inside the arm-then-snapshot interval is documented (README and the `process_arrival()` comment) rather than fenced by syspath/seqnum, the fallback the brief allowed; 1 not fixed - linux-18, no TSan stress regression was added (note-level). The two notes the verifier raised (the `hid_exit()` spin after a failed join, per-message udev parse errors killing the monitor) are the ones fixed in b077b35. CI: green on b077b35; the device-backed suites pass on the aggregate branch (#830). _Drafted with Claude Code._

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin 'hotplug-async-enumerate-linux:hotplug-async-enumerate-linux'

git switch 'hotplug-async-enumerate-linux'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'connection-callback'

git merge --no-ff 'hotplug-async-enumerate-linux'

git switch 'hotplug-async-enumerate-linux'

git rebase 'connection-callback'

git switch 'connection-callback'

git merge --ff-only 'hotplug-async-enumerate-linux'

git switch 'hotplug-async-enumerate-linux'

git rebase 'connection-callback'

git switch 'connection-callback'

git merge --no-ff 'hotplug-async-enumerate-linux'

git switch 'connection-callback'

git merge --squash 'hotplug-async-enumerate-linux'