From d9c9926b114a03cd91f579d09e9f42505db22814 Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Tue, 14 Jul 2026 01:39:58 +0300 Subject: [PATCH 01/10] mac: deliver the hotplug ENUMERATE pass asynchronously on the event thread Implement the hotplug contract documented in hidapi.h for the darwin backend: the HID_API_HOTPLUG_ENUMERATE initial pass is now a registration-time snapshot replayed on the hotplug event thread via a second run loop source, always before any live events for that callback. Also fix the event thread joining itself when a callback deregisters the last callback from within a device-removal event (the join is deferred to the next registration or hid_exit), make hid_exit() tear down the hotplug state without an explicit hid_init(), initialize the library implicitly on registration, reject invalid handles in deregister, keep device->next NULL for every callback invocation, and set the global error string on all register/deregister failure paths. Fixes #794 Assisted-by: claude-code:claude-fable-5 --- mac/hid.c | 358 +++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 312 insertions(+), 46 deletions(-) diff --git a/mac/hid.c b/mac/hid.c index 70951da72..29a0488d9 100644 --- a/mac/hid.c +++ b/mac/hid.c @@ -438,7 +438,8 @@ static wchar_t *dup_wcs(const wchar_t *s) { size_t len = wcslen(s); wchar_t *ret = (wchar_t*) malloc((len+1)*sizeof(wchar_t)); - wcscpy(ret, s); + if (ret) + wcscpy(ret, s); return ret; } @@ -476,6 +477,11 @@ struct hid_hotplug_callback { void *user_data; hid_hotplug_callback_fn callback; + /* Snapshot of the matching devices connected at registration time, + to be delivered ("replayed") as synthetic HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED + events on the event thread (HID_API_HOTPLUG_ENUMERATE); NULL once delivered */ + struct hid_device_info *replay; + /* Pointer to the next notification */ struct hid_hotplug_callback *next; }; @@ -495,6 +501,7 @@ static struct hid_hotplug_context { pthread_t thread; CFRunLoopRef run_loop; CFRunLoopSourceRef source; + CFRunLoopSourceRef replay_source; /* Delivers the initial HID_API_HOTPLUG_ENUMERATE pass of new registrations */ CFStringRef run_loop_mode; pthread_barrier_t startup_barrier; /* Ensures correct startup sequence */ int thread_state; /* 0 = starting (events ignored), 1 = running (events processed), 2 = shutting down */ @@ -508,6 +515,7 @@ static struct hid_hotplug_context { unsigned char mutex_ready; unsigned char mutex_in_use; unsigned char cb_list_dirty; + unsigned char thread_needs_join; /* Event thread was started and has not been joined yet */ /* Linked list of the hotplug callbacks */ struct hid_hotplug_callback *hotplug_cbs; @@ -531,6 +539,7 @@ static void hid_internal_hotplug_remove_postponed(void) struct hid_hotplug_callback *callback = *current; if (!callback->events) { *current = (*current)->next; + hid_free_enumeration(callback->replay); free(callback); continue; } @@ -541,6 +550,30 @@ static void hid_internal_hotplug_remove_postponed(void) hid_hotplug_context.cb_list_dirty = 0; } +/* Joins the event thread and releases what only the joiner may release. + Must be called with the mutex held (when the mutex exists at all) + and never from the event thread itself. */ +static void hid_internal_hotplug_join_thread(void) +{ + pthread_join(hid_hotplug_context.thread, NULL); + hid_hotplug_context.thread_needs_join = 0; + + pthread_barrier_destroy(&hid_hotplug_context.startup_barrier); + + /* The run loop sources are created by the event thread, but the thread + exits without releasing them: the references must stay valid so that + the run loop can still be woken up while the thread is winding down. */ + if (hid_hotplug_context.source) { + CFRelease(hid_hotplug_context.source); + hid_hotplug_context.source = NULL; + } + if (hid_hotplug_context.replay_source) { + CFRelease(hid_hotplug_context.replay_source); + hid_hotplug_context.replay_source = NULL; + } + hid_hotplug_context.run_loop = NULL; +} + static void hid_internal_hotplug_cleanup(void) { if (!hid_hotplug_context.mutex_ready || hid_hotplug_context.mutex_in_use) { @@ -558,15 +591,29 @@ static void hid_internal_hotplug_cleanup(void) hid_free_enumeration(hid_hotplug_context.devs); hid_hotplug_context.devs = NULL; - /* Cause hotplug_thread() to stop. */ - hid_hotplug_context.thread_state = 2; + if (!hid_hotplug_context.thread_needs_join) { + /* The event thread is not running */ + return; + } - /* Wake up the run thread's event loop so that the thread can exit. */ - CFRunLoopSourceSignal(hid_hotplug_context.source); - CFRunLoopWakeUp(hid_hotplug_context.run_loop); + if (hid_hotplug_context.thread_state != 2) { + /* Cause hotplug_thread() to stop. */ + hid_hotplug_context.thread_state = 2; - /* Wait for read_thread() to end. */ - pthread_join(hid_hotplug_context.thread, NULL); + /* Wake up the run thread's event loop so that the thread can exit. */ + CFRunLoopSourceSignal(hid_hotplug_context.source); + CFRunLoopWakeUp(hid_hotplug_context.run_loop); + } + + if (pthread_equal(pthread_self(), hid_hotplug_context.thread)) { + /* A callback deregistered the last callback from the event thread itself: + the thread cannot join itself (issue #794). It exits on its own; the join + is deferred until the next registration or hid_exit(). */ + return; + } + + /* Wait for hotplug_thread() to end. */ + hid_internal_hotplug_join_thread(); } static void hid_internal_hotplug_init(void) @@ -600,6 +647,7 @@ static void hid_internal_hotplug_exit(void) /* Remove all callbacks from the list */ while (*current) { struct hid_hotplug_callback* next = (*current)->next; + hid_free_enumeration((*current)->replay); free(*current); *current = next; } @@ -607,6 +655,11 @@ static void hid_internal_hotplug_exit(void) pthread_mutex_unlock(&hid_hotplug_context.mutex); hid_hotplug_context.mutex_ready = 0; pthread_mutex_destroy(&hid_hotplug_context.mutex); + + if (hid_hotplug_context.run_loop_mode) { + CFRelease(hid_hotplug_context.run_loop_mode); + hid_hotplug_context.run_loop_mode = NULL; + } } /* Initialize the IOHIDManager if necessary. This is the public function, and @@ -628,12 +681,16 @@ int HID_API_EXPORT hid_init(void) int HID_API_EXPORT hid_exit(void) { + /* The hotplug thread and the callbacks are stopped/freed unconditionally: + hid_hotplug_register_callback() may have initialized the library implicitly + without ever creating hid_mgr */ + hid_internal_hotplug_exit(); + if (hid_mgr) { /* Close the HID manager. */ IOHIDManagerClose(hid_mgr, kIOHIDOptionsTypeNone); CFRelease(hid_mgr); hid_mgr = NULL; - hid_internal_hotplug_exit(); } /* Free global error message */ @@ -967,6 +1024,67 @@ void HID_API_EXPORT hid_free_enumeration(struct hid_device_info *devs) } } +/* Makes a deep copy of a single hid_device_info entry (the next pointer of + the copy is always NULL). Returns NULL on allocation failure. */ +static struct hid_device_info *hid_internal_copy_device_info(const struct hid_device_info *src) +{ + struct hid_device_info *dst = (struct hid_device_info*) calloc(1, sizeof(struct hid_device_info)); + if (dst == NULL) { + return NULL; + } + + dst->path = src->path ? strdup(src->path) : NULL; + dst->vendor_id = src->vendor_id; + dst->product_id = src->product_id; + dst->serial_number = src->serial_number ? dup_wcs(src->serial_number) : NULL; + dst->release_number = src->release_number; + dst->manufacturer_string = src->manufacturer_string ? dup_wcs(src->manufacturer_string) : NULL; + dst->product_string = src->product_string ? dup_wcs(src->product_string) : NULL; + dst->usage_page = src->usage_page; + dst->usage = src->usage; + dst->interface_number = src->interface_number; + dst->bus_type = src->bus_type; + dst->next = NULL; + + /* Treat a failed string copy as a failed allocation */ + if ((src->path && !dst->path) + || (src->serial_number && !dst->serial_number) + || (src->manufacturer_string && !dst->manufacturer_string) + || (src->product_string && !dst->product_string)) { + hid_free_enumeration(dst); + return NULL; + } + + return dst; +} + +/* Delivers the pending synthetic HID_API_HOTPLUG_ENUMERATE events (the initial + pass) of a single callback. Called on the event thread, with the mutex held + and mutex_in_use set. */ +static void hid_internal_hotplug_replay_one(struct hid_hotplug_callback *callback) +{ + while (callback->replay != NULL) { + struct hid_device_info *info = callback->replay; + callback->replay = info->next; + info->next = NULL; + + /* Skip the delivery if the callback got deregistered meanwhile */ + if (callback->events & HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED) { + int result = (*callback->callback)(callback->handle, info, HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED, callback->user_data); + if (result) { + /* The callback asked to be deregistered: mark it for removal + and drop the rest of its initial pass */ + callback->events = 0; + hid_hotplug_context.cb_list_dirty = 1; + hid_free_enumeration(callback->replay); + callback->replay = NULL; + } + } + + hid_free_enumeration(info); + } +} + static void hid_internal_invoke_callbacks(struct hid_device_info *info, hid_hotplug_event event) { pthread_mutex_lock(&hid_hotplug_context.mutex); @@ -975,6 +1093,12 @@ static void hid_internal_invoke_callbacks(struct hid_device_info *info, hid_hotp struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; while (*current) { struct hid_hotplug_callback *callback = *current; + /* The initial HID_API_HOTPLUG_ENUMERATE pass (if still pending) is always + delivered before any live events for the callback: the replay source + might not have fired yet - flush it first */ + if (callback->replay != NULL) { + hid_internal_hotplug_replay_one(callback); + } if ((callback->events & event) && hid_internal_match_device_id(info->vendor_id, info->product_id, callback->vendor_id, callback->product_id)) { int result = callback->callback(callback->handle, info, event, callback->user_data); @@ -1012,11 +1136,14 @@ static void hid_internal_hotplug_connect_callback(void *context, IOReturn result /* Lock the mutex to avoid race conditions */ pthread_mutex_lock(&hid_hotplug_context.mutex); - /* Invoke all callbacks */ + /* Invoke all callbacks (device->next must be NULL for every delivery) */ while (info_cur) { + struct hid_device_info *info_next = info_cur->next; + info_cur->next = NULL; hid_internal_invoke_callbacks(info_cur, HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED); - info_cur = info_cur->next; + info_cur->next = info_next; + info_cur = info_next; } } @@ -1036,6 +1163,8 @@ static void hid_internal_hotplug_connect_callback(void *context, IOReturn result if (hid_hotplug_context.thread_state > 0) { + /* Clean up if the last callback was removed during the events */ + hid_internal_hotplug_cleanup(); pthread_mutex_unlock(&hid_hotplug_context.mutex); } } @@ -1092,31 +1221,90 @@ static void hotplug_stop_callback(void* context) CFRunLoopStop(hid_hotplug_context.run_loop); } +static void hotplug_replay_callback(void* context) +{ + (void) context; + + pthread_mutex_lock(&hid_hotplug_context.mutex); + + unsigned char old_state = hid_hotplug_context.mutex_in_use; + hid_hotplug_context.mutex_in_use = 1; + + for (struct hid_hotplug_callback *callback = hid_hotplug_context.hotplug_cbs; callback != NULL; callback = callback->next) { + if (callback->replay != NULL) { + hid_internal_hotplug_replay_one(callback); + } + } + + hid_hotplug_context.mutex_in_use = old_state; + + /* An initial-pass callback may have deregistered the last callback: clean up if so */ + hid_internal_hotplug_cleanup(); + + pthread_mutex_unlock(&hid_hotplug_context.mutex); +} + static void* hotplug_thread(void* user_data) { (void) user_data; + int startup_ok = 0; + hid_hotplug_context.thread_state = 0; - hid_hotplug_context.manager = IOHIDManagerCreate(kCFAllocatorDefault, kIOHIDOptionsTypeNone); + + /* Store a reference to this runloop if we ever need to wake it up - e.g. if we have no callbacks left or hid_exit was called */ + hid_hotplug_context.run_loop = CFRunLoopGetCurrent(); if (!hid_hotplug_context.run_loop_mode) { const char *str = "HIDAPI_hotplug"; hid_hotplug_context.run_loop_mode = CFStringCreateWithCString(NULL, str, kCFStringEncodingASCII); } - /* Ensure the manager runs in this thread */ - IOHIDManagerScheduleWithRunLoop(hid_hotplug_context.manager, CFRunLoopGetCurrent(), hid_hotplug_context.run_loop_mode); - /* Store a reference to this runloop if we ever need to stop it - e.g. if we have no callbacks left or hid_exit was called */ - hid_hotplug_context.run_loop = CFRunLoopGetCurrent(); - - /* Create the RunLoopSource which is used to signal the - event loop to stop when hid_internal_hotplug_cleanup() is called. */ - CFRunLoopSourceContext ctx; - memset(&ctx, 0, sizeof(ctx)); - ctx.version = 0; - ctx.perform = &hotplug_stop_callback; - hid_hotplug_context.source = CFRunLoopSourceCreate(kCFAllocatorDefault, 0/*order*/, &ctx); - CFRunLoopAddSource(hid_hotplug_context.run_loop, hid_hotplug_context.source, hid_hotplug_context.run_loop_mode); + if (hid_hotplug_context.run_loop_mode) { + hid_hotplug_context.manager = IOHIDManagerCreate(kCFAllocatorDefault, kIOHIDOptionsTypeNone); + } + + if (hid_hotplug_context.manager) { + CFRunLoopSourceContext ctx; + + /* Ensure the manager runs in this thread */ + IOHIDManagerScheduleWithRunLoop(hid_hotplug_context.manager, hid_hotplug_context.run_loop, hid_hotplug_context.run_loop_mode); + + /* Create the RunLoopSource which is used to signal the + event loop to stop when hid_internal_hotplug_cleanup() is called. */ + memset(&ctx, 0, sizeof(ctx)); + ctx.version = 0; + ctx.perform = &hotplug_stop_callback; + hid_hotplug_context.source = CFRunLoopSourceCreate(kCFAllocatorDefault, 0/*order*/, &ctx); + + /* Create the RunLoopSource used to deliver the initial + HID_API_HOTPLUG_ENUMERATE pass of new registrations on this thread. */ + memset(&ctx, 0, sizeof(ctx)); + ctx.version = 0; + ctx.perform = &hotplug_replay_callback; + hid_hotplug_context.replay_source = CFRunLoopSourceCreate(kCFAllocatorDefault, 0/*order*/, &ctx); + + if (hid_hotplug_context.source && hid_hotplug_context.replay_source) { + CFRunLoopAddSource(hid_hotplug_context.run_loop, hid_hotplug_context.source, hid_hotplug_context.run_loop_mode); + CFRunLoopAddSource(hid_hotplug_context.run_loop, hid_hotplug_context.replay_source, hid_hotplug_context.run_loop_mode); + startup_ok = 1; + } + } + + if (!startup_ok) { + /* Report the failure to hid_hotplug_register_callback() waiting at the barrier; + the sources (if any got created) are released by the thread that joins us */ + hid_hotplug_context.thread_state = 2; + pthread_barrier_wait(&hid_hotplug_context.startup_barrier); + + if (hid_hotplug_context.manager) { + IOHIDManagerUnscheduleFromRunLoop(hid_hotplug_context.manager, hid_hotplug_context.run_loop, hid_hotplug_context.run_loop_mode); + CFRelease(hid_hotplug_context.manager); + hid_hotplug_context.manager = NULL; + } + + return NULL; + } /* Set the manager to receive events for ALL HID devices */ IOHIDManagerSetDeviceMatching(hid_hotplug_context.manager, NULL); @@ -1166,23 +1354,36 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven { struct hid_hotplug_callback* hotplug_cb; + /* No events are ever delivered for a failed registration */ + if (callback_handle != NULL) { + *callback_handle = 0; + } + /* Check params */ if (events == 0 || (events & ~(HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED | HID_API_HOTPLUG_EVENT_DEVICE_LEFT)) || (flags & ~(HID_API_HOTPLUG_ENUMERATE)) || callback == NULL) { + register_global_error("hid_hotplug_register_callback: invalid arguments"); return -1; } - hotplug_cb = (struct hid_hotplug_callback*)calloc(1, sizeof(struct hid_hotplug_callback)); + /* The registration initializes the library implicitly (as if by hid_init()) */ + if (hid_init() != 0) { + /* register_global_error: global error is already set by hid_init */ + return -1; + } + hotplug_cb = (struct hid_hotplug_callback*)calloc(1, sizeof(struct hid_hotplug_callback)); if (hotplug_cb == NULL) { + register_global_error("hid_hotplug_register_callback: failed to allocate a callback"); return -1; } /* Fill out the record */ hotplug_cb->next = NULL; + hotplug_cb->replay = NULL; hotplug_cb->vendor_id = vendor_id; hotplug_cb->product_id = product_id; hotplug_cb->events = events; @@ -1203,11 +1404,6 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven hid_hotplug_context.next_handle = 1; } - /* Return allocated handle */ - if (callback_handle != NULL) { - *callback_handle = hotplug_cb->handle; - } - /* Append a new callback to the end */ if (hid_hotplug_context.hotplug_cbs != NULL) { struct hid_hotplug_callback *last = hid_hotplug_context.hotplug_cbs; @@ -1217,36 +1413,97 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven last->next = hotplug_cb; } else { + /* A previous event thread may still await its deferred join (it stops + on its own when a callback deregisters the last callback from the + event thread itself): join it before starting a new one */ + if (hid_hotplug_context.thread_needs_join) { + hid_internal_hotplug_join_thread(); + } + pthread_barrier_init(&hid_hotplug_context.startup_barrier, NULL, 2); - pthread_create(&hid_hotplug_context.thread, NULL, hotplug_thread, NULL); + + if (pthread_create(&hid_hotplug_context.thread, NULL, hotplug_thread, NULL) != 0) { + pthread_barrier_destroy(&hid_hotplug_context.startup_barrier); + pthread_mutex_unlock(&hid_hotplug_context.mutex); + free(hotplug_cb); + register_global_error("hid_hotplug_register_callback: failed to create the hotplug events thread"); + return -1; + } + + hid_hotplug_context.thread_needs_join = 1; /* Wait for the thread to finish setting up - without it the callback may be registered too early*/ pthread_barrier_wait(&hid_hotplug_context.startup_barrier); + if (hid_hotplug_context.thread_state != 1) { + /* The thread failed to set up the device monitoring and is exiting */ + hid_internal_hotplug_join_thread(); + pthread_mutex_unlock(&hid_hotplug_context.mutex); + free(hotplug_cb); + register_global_error("hid_hotplug_register_callback: failed to start the device monitoring"); + return -1; + } + /* Don't forget to actually register the callback */ hid_hotplug_context.hotplug_cbs = hotplug_cb; } - /* Mark the mutex as IN USE, to prevent callback removal from inside a callback */ - unsigned char old_state = hid_hotplug_context.mutex_in_use; - hid_hotplug_context.mutex_in_use = 1; - if ((flags & HID_API_HOTPLUG_ENUMERATE) && (events & HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED)) { - struct hid_device_info* device = hid_hotplug_context.devs; - /* Notify about already connected devices, if asked so */ - while (device != NULL) { - if (hid_internal_match_device_id(device->vendor_id, device->product_id, hotplug_cb->vendor_id, hotplug_cb->product_id)) { - (*hotplug_cb->callback)(hotplug_cb->handle, device, HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED, hotplug_cb->user_data); + int snapshot_ok = 1; + struct hid_device_info *dev_info = hid_hotplug_context.devs; + struct hid_device_info **replay_tail = &hotplug_cb->replay; + + /* Take a snapshot of the already connected matching devices: it is + delivered ("replayed") as synthetic arrival events on the event + thread, never from within this call */ + for (; dev_info != NULL; dev_info = dev_info->next) { + struct hid_device_info *dev_info_copy; + if (!hid_internal_match_device_id(dev_info->vendor_id, dev_info->product_id, hotplug_cb->vendor_id, hotplug_cb->product_id)) { + continue; + } + dev_info_copy = hid_internal_copy_device_info(dev_info); + if (dev_info_copy == NULL) { + snapshot_ok = 0; + break; } + *replay_tail = dev_info_copy; + replay_tail = &dev_info_copy->next; + } - device = device->next; + if (!snapshot_ok) { + /* Fail the registration rather than deliver a partial initial pass. + The mutex has been held since before the callback became visible, + so it has not been invoked yet: it is safe to detach and free it. */ + struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; + while (*current != NULL && *current != hotplug_cb) { + current = &(*current)->next; + } + if (*current != NULL) { + *current = hotplug_cb->next; + } + hid_free_enumeration(hotplug_cb->replay); + free(hotplug_cb); + + /* Stop the event thread if no other callbacks are left */ + hid_internal_hotplug_cleanup(); + pthread_mutex_unlock(&hid_hotplug_context.mutex); + register_global_error("hid_hotplug_register_callback: failed to take a snapshot of the connected devices"); + return -1; } - } - hid_hotplug_context.mutex_in_use = old_state; + if (hotplug_cb->replay != NULL && hid_hotplug_context.thread_state == 1) { + /* Ask the event thread to deliver the initial pass */ + CFRunLoopSourceSignal(hid_hotplug_context.replay_source); + CFRunLoopWakeUp(hid_hotplug_context.run_loop); + } + } - hid_internal_hotplug_cleanup(); + /* Return the allocated handle: written before any events can be delivered, + as the events are only ever delivered under this mutex */ + if (callback_handle != NULL) { + *callback_handle = hotplug_cb->handle; + } pthread_mutex_unlock(&hid_hotplug_context.mutex); @@ -1255,7 +1512,8 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_callback_handle callback_handle) { - if (!hid_hotplug_context.mutex_ready) { + if (callback_handle <= 0 || !hid_hotplug_context.mutex_ready) { + register_global_error("hid_hotplug_deregister_callback: not a registered callback handle"); return -1; } @@ -1263,6 +1521,7 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_call if (hid_hotplug_context.hotplug_cbs == NULL) { pthread_mutex_unlock(&hid_hotplug_context.mutex); + register_global_error("hid_hotplug_deregister_callback: no callbacks are registered"); return -1; } @@ -1271,6 +1530,9 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_call /* Remove this notification */ for (struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; *current != NULL; current = &(*current)->next) { if ((*current)->handle == callback_handle) { + /* Free the undelivered initial pass: once deregistered, the callback must never fire */ + hid_free_enumeration((*current)->replay); + (*current)->replay = NULL; /* Check if we were already in a locked state, as we are NOT allowed to remove any callbacks if we are */ if (hid_hotplug_context.mutex_in_use) { (*current)->events = 0; @@ -1285,6 +1547,10 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_call } } + if (result != 0) { + register_global_error("hid_hotplug_deregister_callback: unknown callback handle"); + } + hid_internal_hotplug_cleanup(); pthread_mutex_unlock(&hid_hotplug_context.mutex); From 1506abe0b2a59829abb43368478ec374eb0c7561 Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Tue, 14 Jul 2026 02:21:47 +0300 Subject: [PATCH 02/10] mac: address round-1 review findings for the hotplug backend Never join the event thread with the hotplug mutex held: the join moved out of hid_internal_hotplug_cleanup() into a dedicated collector that runs from the public entry points with the mutex released, serializing concurrent joiners (fixes the register+deregister deadlock with a pending ENUMERATE replay). Serialize the one-time setup (implicit hid_init and hotplug mutex creation) and the hid_exit teardown with a static bootstrap mutex, and serialize all mutations of the global error string with a static mutex. Drain the initial device-matching burst in the private run loop mode the manager is actually scheduled on, before releasing the startup barrier - so the device cache is populated for the first registrant's snapshot and pre-connected devices never surface as live arrival events. Freeze each dispatch at the tail callback registered at dispatch start, and append arriving cache entries before invoking callbacks: a callback registered from within a callback never receives the in-flight event and its snapshot covers arrivals exactly once. Also: check IOHIDManagerOpen result and fail the registration; check pthread_barrier_init and fix the shim's error checks (pthread errors are positive); reject deregistering an already-deregistered handle; fail registration on callback handle exhaustion instead of wrapping; publish the unsolicited run-loop exit under the mutex; use save/restore discipline for mutex_in_use consistently. Assisted-by: claude-code:claude-fable-5 --- mac/hid.c | 364 ++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 242 insertions(+), 122 deletions(-) diff --git a/mac/hid.c b/mac/hid.c index 29a0488d9..b3b3827e9 100644 --- a/mac/hid.c +++ b/mac/hid.c @@ -31,9 +31,11 @@ #include #include #include +#include #include #include #include +#include #include #include #include @@ -66,10 +68,10 @@ static int pthread_barrier_init(pthread_barrier_t *barrier, const pthread_barrie return -1; } - if (pthread_mutex_init(&barrier->mutex, 0) < 0) { + if (pthread_mutex_init(&barrier->mutex, 0) != 0) { return -1; } - if (pthread_cond_init(&barrier->cond, 0) < 0) { + if (pthread_cond_init(&barrier->cond, 0) != 0) { pthread_mutex_destroy(&barrier->mutex); return -1; } @@ -261,6 +263,11 @@ static void register_error_str_vformat(wchar_t **error_str, const char *format, register_error_str(error_str, msg); } +/* Serializes the mutations of the global error string: the hotplug API is + thread-safe and its failure paths (and the implicit hid_init()) may write + the global error from multiple threads concurrently. */ +static pthread_mutex_t global_error_mutex = PTHREAD_MUTEX_INITIALIZER; + /* Set the last global error to be reported by hid_error(NULL). * The given error message will be copied (and decoded according to the * currently locale, so do not pass in string constants). @@ -268,7 +275,9 @@ static void register_error_str_vformat(wchar_t **error_str, const char *format, * Use register_global_error(NULL) to indicate "no error". */ static void register_global_error(const char *msg) { + pthread_mutex_lock(&global_error_mutex); register_error_str(&last_global_error_str, msg); + pthread_mutex_unlock(&global_error_mutex); } /* Similar to register_global_error, but allows passing a format string into this function. */ @@ -276,7 +285,9 @@ static void register_global_error_format(const char *format, ...) { va_list args; va_start(args, format); + pthread_mutex_lock(&global_error_mutex); register_error_str_vformat(&last_global_error_str, format, args); + pthread_mutex_unlock(&global_error_mutex); va_end(args); } @@ -516,6 +527,7 @@ static struct hid_hotplug_context { unsigned char mutex_in_use; unsigned char cb_list_dirty; unsigned char thread_needs_join; /* Event thread was started and has not been joined yet */ + unsigned char join_in_progress; /* A thread is currently joining the event thread (with the mutex released) */ /* Linked list of the hotplug callbacks */ struct hid_hotplug_callback *hotplug_cbs; @@ -524,6 +536,11 @@ static struct hid_hotplug_context { struct hid_device_info *devs; } hid_hotplug_context; /* zero-initialized (static storage); next_handle set on first init */ +/* Serializes the one-time hotplug setup (the implicit hid_init() and the + creation of the hotplug mutex) against concurrent first registrations, + and the teardown in hid_exit() against them. */ +static pthread_mutex_t hid_hotplug_startup_mutex = PTHREAD_MUTEX_INITIALIZER; + static void hid_internal_hotplug_remove_postponed(void) { /* Unregister the callbacks whose removal was postponed */ @@ -550,28 +567,57 @@ static void hid_internal_hotplug_remove_postponed(void) hid_hotplug_context.cb_list_dirty = 0; } -/* Joins the event thread and releases what only the joiner may release. - Must be called with the mutex held (when the mutex exists at all) - and never from the event thread itself. */ -static void hid_internal_hotplug_join_thread(void) +/* Collects (joins) the event thread once it has been told to stop, and + releases what only the joiner may release. Serializes concurrent joiners + and waits out a join running on another thread. + Must be called with the hotplug mutex NOT held by the calling thread, + except from the event thread itself, where it is a guaranteed no-op. */ +static void hid_internal_hotplug_collect_thread(void) { - pthread_join(hid_hotplug_context.thread, NULL); - hid_hotplug_context.thread_needs_join = 0; + pthread_mutex_lock(&hid_hotplug_context.mutex); - pthread_barrier_destroy(&hid_hotplug_context.startup_barrier); + while (hid_hotplug_context.thread_needs_join + && hid_hotplug_context.hotplug_cbs == NULL + && hid_hotplug_context.thread_state == 2 + && !pthread_equal(pthread_self(), hid_hotplug_context.thread)) { + if (hid_hotplug_context.join_in_progress) { + /* Another thread is already joining: wait for it to finish */ + pthread_mutex_unlock(&hid_hotplug_context.mutex); + sched_yield(); + pthread_mutex_lock(&hid_hotplug_context.mutex); + continue; + } - /* The run loop sources are created by the event thread, but the thread - exits without releasing them: the references must stay valid so that - the run loop can still be woken up while the thread is winding down. */ - if (hid_hotplug_context.source) { - CFRelease(hid_hotplug_context.source); - hid_hotplug_context.source = NULL; - } - if (hid_hotplug_context.replay_source) { - CFRelease(hid_hotplug_context.replay_source); - hid_hotplug_context.replay_source = NULL; + hid_hotplug_context.join_in_progress = 1; + pthread_mutex_unlock(&hid_hotplug_context.mutex); + + /* Join with the mutex released: the exiting thread may still need the + mutex to finish an in-flight callback dispatch (issue #794 and the + matching cross-thread deadlock). No new event thread can be started + while thread_needs_join is set, so the thread handle is stable. */ + pthread_join(hid_hotplug_context.thread, NULL); + + pthread_mutex_lock(&hid_hotplug_context.mutex); + hid_hotplug_context.join_in_progress = 0; + hid_hotplug_context.thread_needs_join = 0; + + pthread_barrier_destroy(&hid_hotplug_context.startup_barrier); + + /* The run loop sources are created by the event thread, but the thread + exits without releasing them: the references must stay valid so that + the run loop can still be woken up while the thread is winding down. */ + if (hid_hotplug_context.source) { + CFRelease(hid_hotplug_context.source); + hid_hotplug_context.source = NULL; + } + if (hid_hotplug_context.replay_source) { + CFRelease(hid_hotplug_context.replay_source); + hid_hotplug_context.replay_source = NULL; + } + hid_hotplug_context.run_loop = NULL; } - hid_hotplug_context.run_loop = NULL; + + pthread_mutex_unlock(&hid_hotplug_context.mutex); } static void hid_internal_hotplug_cleanup(void) @@ -605,15 +651,11 @@ static void hid_internal_hotplug_cleanup(void) CFRunLoopWakeUp(hid_hotplug_context.run_loop); } - if (pthread_equal(pthread_self(), hid_hotplug_context.thread)) { - /* A callback deregistered the last callback from the event thread itself: - the thread cannot join itself (issue #794). It exits on its own; the join - is deferred until the next registration or hid_exit(). */ - return; - } - - /* Wait for hotplug_thread() to end. */ - hid_internal_hotplug_join_thread(); + /* The join is never performed here: this function runs with the mutex + held, and the exiting thread may still need the mutex to finish an + in-flight dispatch (it may even be the current thread - issue #794). + The stopped thread is collected by hid_internal_hotplug_collect_thread() + from the public entry points, with the mutex released. */ } static void hid_internal_hotplug_init(void) @@ -637,7 +679,10 @@ static void hid_internal_hotplug_init(void) static void hid_internal_hotplug_exit(void) { + pthread_mutex_lock(&hid_hotplug_startup_mutex); + if (!hid_hotplug_context.mutex_ready) { + pthread_mutex_unlock(&hid_hotplug_startup_mutex); return; } @@ -653,6 +698,10 @@ static void hid_internal_hotplug_exit(void) } hid_internal_hotplug_cleanup(); pthread_mutex_unlock(&hid_hotplug_context.mutex); + + /* Join the stopped event thread, with the hotplug mutex released */ + hid_internal_hotplug_collect_thread(); + hid_hotplug_context.mutex_ready = 0; pthread_mutex_destroy(&hid_hotplug_context.mutex); @@ -660,6 +709,8 @@ static void hid_internal_hotplug_exit(void) CFRelease(hid_hotplug_context.run_loop_mode); hid_hotplug_context.run_loop_mode = NULL; } + + pthread_mutex_unlock(&hid_hotplug_startup_mutex); } /* Initialize the IOHIDManager if necessary. This is the public function, and @@ -712,6 +763,18 @@ static void process_pending_events(void) } while (res != kCFRunLoopRunFinished && res != kCFRunLoopRunTimedOut); } +/* Runs the hotplug event thread's private run loop mode until no more events + are pending. Called on the event thread only, during its startup: the + hotplug IOHIDManager is scheduled in that private mode, so this is where + the device-matching events of the already connected devices are delivered. */ +static void hid_internal_hotplug_process_pending_events(void) +{ + SInt32 res; + do { + res = CFRunLoopRunInMode(hid_hotplug_context.run_loop_mode, 0.001, FALSE); + } while (res != kCFRunLoopRunFinished && res != kCFRunLoopRunTimedOut); +} + static int read_usb_interface_from_hid_service_parent(io_service_t hid_service) { int32_t result = -1; @@ -1088,8 +1151,20 @@ static void hid_internal_hotplug_replay_one(struct hid_hotplug_callback *callbac static void hid_internal_invoke_callbacks(struct hid_device_info *info, hid_hotplug_event event) { pthread_mutex_lock(&hid_hotplug_context.mutex); + + unsigned char old_state = hid_hotplug_context.mutex_in_use; hid_hotplug_context.mutex_in_use = 1; + /* Freeze the dispatch at the last callback registered at this moment: + a callback registered from within a callback must not receive the + in-flight event - its HID_API_HOTPLUG_ENUMERATE snapshot (taken at + registration) and the subsequent events cover it with no losses or + duplicates. The list is append-only while mutex_in_use is set. */ + struct hid_hotplug_callback *stop_after = hid_hotplug_context.hotplug_cbs; + while (stop_after != NULL && stop_after->next != NULL) { + stop_after = stop_after->next; + } + struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; while (*current) { struct hid_hotplug_callback *callback = *current; @@ -1102,18 +1177,20 @@ static void hid_internal_invoke_callbacks(struct hid_device_info *info, hid_hotp if ((callback->events & event) && hid_internal_match_device_id(info->vendor_id, info->product_id, callback->vendor_id, callback->product_id)) { int result = callback->callback(callback->handle, info, event, callback->user_data); - /* If the result is non-zero, we mark the callback for removal and proceed */ + /* If the result is non-zero, we mark the callback for removal */ /* Do not use the deregister call as it locks the mutex, and we are currently in a lock */ if (result) { - (*current)->events = 0; + callback->events = 0; hid_hotplug_context.cb_list_dirty = 1; - continue; } } + if (callback == stop_after) { + break; + } current = &callback->next; } - - hid_hotplug_context.mutex_in_use = 0; + + hid_hotplug_context.mutex_in_use = old_state; hid_internal_hotplug_remove_postponed(); pthread_mutex_unlock(&hid_hotplug_context.mutex); } @@ -1128,15 +1205,36 @@ static void hid_internal_hotplug_connect_callback(void *context, IOReturn result if (!info) { return; } - struct hid_device_info* info_cur = info; - + /* NOTE: we don't call any callbacks and we don't lock the mutex during initialization: the mutex is held by the main thread, but it's waiting by a barrier */ if (hid_hotplug_context.thread_state > 0) { /* Lock the mutex to avoid race conditions */ pthread_mutex_lock(&hid_hotplug_context.mutex); - - /* Invoke all callbacks (device->next must be NULL for every delivery) */ + } + + /* Append all we got to the end of the device list BEFORE invoking any + callbacks: a callback registering with HID_API_HOTPLUG_ENUMERATE from + within a callback must find the arriving entries in its snapshot, + as it does not receive the in-flight events */ + if (hid_hotplug_context.devs != NULL) { + struct hid_device_info* last = hid_hotplug_context.devs; + while (last->next != NULL) { + last = last->next; + } + last->next = info; + } + else { + hid_hotplug_context.devs = info; + } + + if (hid_hotplug_context.thread_state > 0) + { + /* Invoke the callbacks for each entry; device->next must be NULL for + every delivery, so each entry is temporarily severed, truncating the + device cache at that entry for the duration of its dispatch (a + snapshot taken during the dispatch then ends at the delivered entry) */ + struct hid_device_info *info_cur = info; while (info_cur) { struct hid_device_info *info_next = info_cur->next; @@ -1145,24 +1243,7 @@ static void hid_internal_hotplug_connect_callback(void *context, IOReturn result info_cur->next = info_next; info_cur = info_next; } - } - /* Append all we got to the end of the device list */ - if (info) { - if (hid_hotplug_context.devs != NULL) { - struct hid_device_info* last = hid_hotplug_context.devs; - while (last->next != NULL) { - last = last->next; - } - last->next = info; - } - else { - hid_hotplug_context.devs = info; - } - } - - if (hid_hotplug_context.thread_state > 0) - { /* Clean up if the last callback was removed during the events */ hid_internal_hotplug_cleanup(); pthread_mutex_unlock(&hid_hotplug_context.mutex); @@ -1287,7 +1368,24 @@ static void* hotplug_thread(void* user_data) if (hid_hotplug_context.source && hid_hotplug_context.replay_source) { CFRunLoopAddSource(hid_hotplug_context.run_loop, hid_hotplug_context.source, hid_hotplug_context.run_loop_mode); CFRunLoopAddSource(hid_hotplug_context.run_loop, hid_hotplug_context.replay_source, hid_hotplug_context.run_loop_mode); - startup_ok = 1; + + /* Set the manager to receive events for ALL HID devices */ + IOHIDManagerSetDeviceMatching(hid_hotplug_context.manager, NULL); + + /* Install callbacks */ + IOHIDManagerRegisterDeviceMatchingCallback(hid_hotplug_context.manager, + hid_internal_hotplug_connect_callback, + NULL); + + IOHIDManagerRegisterDeviceRemovalCallback(hid_hotplug_context.manager, + hid_internal_hotplug_disconnect_callback, + NULL); + + /* Opening the manager enqueues the device-matching events + for all the devices already connected */ + if (IOHIDManagerOpen(hid_hotplug_context.manager, kIOHIDOptionsTypeNone) == kIOReturnSuccess) { + startup_ok = 1; + } } } @@ -1306,24 +1404,11 @@ static void* hotplug_thread(void* user_data) return NULL; } - /* Set the manager to receive events for ALL HID devices */ - IOHIDManagerSetDeviceMatching(hid_hotplug_context.manager, NULL); - - /* Install callbacks */ - IOHIDManagerRegisterDeviceMatchingCallback(hid_hotplug_context.manager, - hid_internal_hotplug_connect_callback, - NULL); - - IOHIDManagerRegisterDeviceRemovalCallback(hid_hotplug_context.manager, - hid_internal_hotplug_disconnect_callback, - NULL); - - /* After monitoring is all set up, enumerate all devices */ - /* Opening the manager should result in the internal callback being called for all connected devices */ - IOHIDManagerOpen(hid_hotplug_context.manager, kIOHIDOptionsTypeNone); - - /* TODO: We need to flush all events from the runloop to ensure the already connected devices don't send any unwanted events */ - process_pending_events(); + /* Drain the device-matching events of the already connected devices: the + manager delivers them in the private run loop mode it is scheduled on, + and with thread_state still 0 they only populate the device cache and + invoke no callbacks - so they can never surface as live arrival events */ + hid_internal_hotplug_process_pending_events(); /* Now that all events are flushed, we are ready to notify the main thread that we are ready */ hid_hotplug_context.thread_state = 1; @@ -1334,7 +1419,12 @@ static void* hotplug_thread(void* user_data) /* If runloop stopped for whatever reason, exit the thread */ if (code != kCFRunLoopRunTimedOut && code != kCFRunLoopRunHandledSource) { + /* Publish the shutdown under the mutex, so that a concurrent + registration cannot observe a running thread (thread_state 1) + and signal-and-wake a run loop that is winding down */ + pthread_mutex_lock(&hid_hotplug_context.mutex); hid_hotplug_context.thread_state = 2; + pthread_mutex_unlock(&hid_hotplug_context.mutex); break; } } @@ -1368,12 +1458,23 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven return -1; } + /* Serialize the one-time setup against concurrent first registrations: + both the implicit hid_init() and the hotplug mutex creation must + happen exactly once */ + pthread_mutex_lock(&hid_hotplug_startup_mutex); + /* The registration initializes the library implicitly (as if by hid_init()) */ - if (hid_init() != 0) { + if (!hid_mgr && hid_init() != 0) { + pthread_mutex_unlock(&hid_hotplug_startup_mutex); /* register_global_error: global error is already set by hid_init */ return -1; } + /* Ensure we are ready to actually use the mutex */ + hid_internal_hotplug_init(); + + pthread_mutex_unlock(&hid_hotplug_startup_mutex); + hotplug_cb = (struct hid_hotplug_callback*)calloc(1, sizeof(struct hid_hotplug_callback)); if (hotplug_cb == NULL) { @@ -1390,20 +1491,32 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven hotplug_cb->user_data = user_data; hotplug_cb->callback = callback; - /* Ensure we are ready to actually use the mutex */ - hid_internal_hotplug_init(); - /* Lock the mutex to avoid race conditions */ pthread_mutex_lock(&hid_hotplug_context.mutex); - hotplug_cb->handle = hid_hotplug_context.next_handle++; + /* If a stopped event thread has not been collected (joined) yet, collect + it before the machinery can be restarted; the join must not happen with + the mutex held, so drop the mutex for the collection and re-check. + Never entered on the event thread itself: the list cannot be empty + while a callback dispatch is in flight. */ + while (hid_hotplug_context.hotplug_cbs == NULL && hid_hotplug_context.thread_needs_join) { + /* Make sure the stop was actually requested (idempotent) */ + hid_internal_hotplug_cleanup(); + pthread_mutex_unlock(&hid_hotplug_context.mutex); + hid_internal_hotplug_collect_thread(); + pthread_mutex_lock(&hid_hotplug_context.mutex); + } - /* handle the unlikely case of handle overflow */ - if (hid_hotplug_context.next_handle < 0) - { - hid_hotplug_context.next_handle = 1; + /* Handles are not recycled even on overflow: recycling could collide with a live handle */ + if (hid_hotplug_context.next_handle == INT_MAX) { + register_global_error("hid_hotplug_register_callback: out of callback handles"); + pthread_mutex_unlock(&hid_hotplug_context.mutex); + free(hotplug_cb); + return -1; } + hotplug_cb->handle = hid_hotplug_context.next_handle++; + /* Append a new callback to the end */ if (hid_hotplug_context.hotplug_cbs != NULL) { struct hid_hotplug_callback *last = hid_hotplug_context.hotplug_cbs; @@ -1413,20 +1526,18 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven last->next = hotplug_cb; } else { - /* A previous event thread may still await its deferred join (it stops - on its own when a callback deregisters the last callback from the - event thread itself): join it before starting a new one */ - if (hid_hotplug_context.thread_needs_join) { - hid_internal_hotplug_join_thread(); + if (pthread_barrier_init(&hid_hotplug_context.startup_barrier, NULL, 2) != 0) { + register_global_error("hid_hotplug_register_callback: failed to create the startup barrier"); + pthread_mutex_unlock(&hid_hotplug_context.mutex); + free(hotplug_cb); + return -1; } - pthread_barrier_init(&hid_hotplug_context.startup_barrier, NULL, 2); - if (pthread_create(&hid_hotplug_context.thread, NULL, hotplug_thread, NULL) != 0) { + register_global_error("hid_hotplug_register_callback: failed to create the hotplug events thread"); pthread_barrier_destroy(&hid_hotplug_context.startup_barrier); pthread_mutex_unlock(&hid_hotplug_context.mutex); free(hotplug_cb); - register_global_error("hid_hotplug_register_callback: failed to create the hotplug events thread"); return -1; } @@ -1437,11 +1548,12 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven pthread_barrier_wait(&hid_hotplug_context.startup_barrier); if (hid_hotplug_context.thread_state != 1) { - /* The thread failed to set up the device monitoring and is exiting */ - hid_internal_hotplug_join_thread(); + /* The thread failed to set up the device monitoring and is exiting: + it must be collected (joined) with the mutex released */ + register_global_error("hid_hotplug_register_callback: failed to start the device monitoring"); pthread_mutex_unlock(&hid_hotplug_context.mutex); + hid_internal_hotplug_collect_thread(); free(hotplug_cb); - register_global_error("hid_hotplug_register_callback: failed to start the device monitoring"); return -1; } @@ -1485,10 +1597,13 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven hid_free_enumeration(hotplug_cb->replay); free(hotplug_cb); - /* Stop the event thread if no other callbacks are left */ + register_global_error("hid_hotplug_register_callback: failed to take a snapshot of the connected devices"); + + /* Stop the event thread if no other callbacks are left, + and collect it with the mutex released */ hid_internal_hotplug_cleanup(); pthread_mutex_unlock(&hid_hotplug_context.mutex); - register_global_error("hid_hotplug_register_callback: failed to take a snapshot of the connected devices"); + hid_internal_hotplug_collect_thread(); return -1; } @@ -1517,44 +1632,49 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_call return -1; } + int result = -1; + pthread_mutex_lock(&hid_hotplug_context.mutex); if (hid_hotplug_context.hotplug_cbs == NULL) { - pthread_mutex_unlock(&hid_hotplug_context.mutex); register_global_error("hid_hotplug_deregister_callback: no callbacks are registered"); - return -1; } - - int result = -1; - - /* Remove this notification */ - for (struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; *current != NULL; current = &(*current)->next) { - if ((*current)->handle == callback_handle) { - /* Free the undelivered initial pass: once deregistered, the callback must never fire */ - hid_free_enumeration((*current)->replay); - (*current)->replay = NULL; - /* Check if we were already in a locked state, as we are NOT allowed to remove any callbacks if we are */ - if (hid_hotplug_context.mutex_in_use) { - (*current)->events = 0; - hid_hotplug_context.cb_list_dirty = 1; - } else { - struct hid_hotplug_callback *next = (*current)->next; - free(*current); - *current = next; + else { + /* Remove this notification: the entries already marked for removal are + skipped, so that a handle cannot be deregistered a second time */ + for (struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; *current != NULL; current = &(*current)->next) { + if ((*current)->handle == callback_handle && (*current)->events != 0) { + /* Free the undelivered initial pass: once deregistered, the callback must never fire */ + hid_free_enumeration((*current)->replay); + (*current)->replay = NULL; + /* Check if we were already in a locked state, as we are NOT allowed to remove any callbacks if we are */ + if (hid_hotplug_context.mutex_in_use) { + (*current)->events = 0; + hid_hotplug_context.cb_list_dirty = 1; + } else { + struct hid_hotplug_callback *next = (*current)->next; + free(*current); + *current = next; + } + result = 0; + break; } - result = 0; - break; } - } - if (result != 0) { - register_global_error("hid_hotplug_deregister_callback: unknown callback handle"); - } + if (result != 0) { + register_global_error("hid_hotplug_deregister_callback: unknown callback handle"); + } - hid_internal_hotplug_cleanup(); + hid_internal_hotplug_cleanup(); + } pthread_mutex_unlock(&hid_hotplug_context.mutex); + /* If this deregistration stopped the event thread, join it with the mutex + released (a no-op from within a callback: the join is then performed by + the next registration or by hid_exit()) */ + hid_internal_hotplug_collect_thread(); + return result; } From 93086d9cb446c289baa69e4fd9f91fbc020206db Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Tue, 14 Jul 2026 20:55:55 +0300 Subject: [PATCH 03/10] mac: fix the deadlocks and races found in the round-2 hotplug review The round-1 bootstrap mutex was ordered outside the hotplug mutex, which deadlocks against a registration made from inside a callback: replace it with pthread_once() and guard the hid_exit() teardown from inside the hotplug mutex with an `exiting` flag. The hotplug mutex is never destroyed any more, so no thread can lock it while hid_exit() frees it. Make the initial HID_API_HOTPLUG_ENUMERATE snapshot a real completion boundary (a synchronous IOHIDManagerCopyDevices(), with the matching burst deduplicated by io_service_t) instead of a 1 ms run loop pump, hoist the removal dispatch under the startup guard (a removal during the startup window could lock the mutex held by the registrant parked at the barrier), replace the join spin with a condition variable, and keep all of the event thread's lifecycle state under the mutex. Assisted-by: claude-code:claude-opus-4-8 --- mac/hid.c | 784 +++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 625 insertions(+), 159 deletions(-) diff --git a/mac/hid.c b/mac/hid.c index b3b3827e9..1741614c9 100644 --- a/mac/hid.c +++ b/mac/hid.c @@ -35,7 +35,6 @@ #include #include #include -#include #include #include #include @@ -504,6 +503,48 @@ struct hid_device_info_ex io_service_t service; }; +/* --- Hotplug locking: the one global lock order --- + + Two locks are involved in the hotplug machinery: + + (1) hid_hotplug_context.mutex - recursive; guards ALL of the hotplug + context: the callback list, the device cache and every lifecycle flag + (thread_state, thread_needs_join, join_in_progress, exiting, ...) as + well as the CoreFoundation references of the event thread. It is held + for the whole duration of every callback invocation, and it is + re-entrant so that a callback may call hid_hotplug_register_callback() + or hid_hotplug_deregister_callback() from the event thread itself - + which the API documentation guarantees cannot deadlock. + + (2) global_error_mutex - a leaf lock, held only while the global error + string is replaced. Nothing is ever acquired while it is held. + + The startup barrier's internal lock (inside pthread_barrier_wait()) is a leaf + as well. + + GLOBAL LOCK ORDER: + hid_hotplug_context.mutex -> { global_error_mutex, startup_barrier } + + The hotplug mutex is always the OUTERMOST lock; no code holding a leaf lock + ever tries to acquire it, so no cycle can exist. In particular there is + deliberately NO bootstrap/startup mutex: a lock ordered *outside* the hotplug + mutex is fundamentally incompatible with registering from inside a callback + (which is entered with the hotplug mutex already held), so the one-time + initialization uses pthread_once(), and the hid_exit() teardown is guarded + from *inside* the hotplug mutex by the `exiting` flag. + + pthread_join() is only ever called with the hotplug mutex released, and + pthread_cond_wait() only with exactly one recursion level held (see + hid_internal_hotplug_collect_thread()). + + The only exception to "all context state is accessed under the mutex" is the + event thread's startup phase - everything it does before reaching the startup + barrier: the registering thread that started it holds the mutex and is parked + at that barrier, so the event thread has exclusive access to the context and + MUST NOT take the mutex there (that would deadlock against the parked + registrant). The barrier is the release/acquire edge that publishes what the + thread has set up. */ + static struct hid_hotplug_context { /* MacOS specific notification handles */ IOHIDManagerRef manager; @@ -515,31 +556,49 @@ static struct hid_hotplug_context { CFRunLoopSourceRef replay_source; /* Delivers the initial HID_API_HOTPLUG_ENUMERATE pass of new registrations */ CFStringRef run_loop_mode; pthread_barrier_t startup_barrier; /* Ensures correct startup sequence */ - int thread_state; /* 0 = starting (events ignored), 1 = running (events processed), 2 = shutting down */ - + + /* Lifecycle of the event thread: 0 = starting, 1 = running, 2 = stopping or + stopped. Only ever read and written under the mutex - the event thread + itself never writes it before the startup barrier (the registering thread + publishes the startup result, see startup_ok) */ + int thread_state; + /* HIDAPI unique callback handle counter */ hid_hotplug_callback_handle next_handle; pthread_mutex_t mutex; + pthread_cond_t join_done; /* Broadcast once the stopped event thread has been collected */ /* Boolean flags */ - unsigned char mutex_ready; + unsigned char mutex_ready; /* The mutex and the condition variable are usable (written once, under pthread_once) */ unsigned char mutex_in_use; unsigned char cb_list_dirty; - unsigned char thread_needs_join; /* Event thread was started and has not been joined yet */ + unsigned char thread_needs_join; /* Event thread was started and has not been collected yet */ unsigned char join_in_progress; /* A thread is currently joining the event thread (with the mutex released) */ + unsigned char exiting; /* hid_exit() is tearing the hotplug machinery down */ + + /* Set while the event thread has not passed its startup barrier yet. + Read and written ONLY on the event thread (the registering thread sets it + before pthread_create(), which is a synchronization point), so it needs no + lock - and it must not: during that phase the mutex is held by the parked + registrant */ + unsigned char startup_phase; + + /* Written by the event thread before the startup barrier, read by the + registering thread after it (the barrier is the synchronization edge) */ + unsigned char startup_ok; /* Linked list of the hotplug callbacks */ struct hid_hotplug_callback *hotplug_cbs; /* Linked list of the device infos (mandatory when the device is disconnected) */ struct hid_device_info *devs; -} hid_hotplug_context; /* zero-initialized (static storage); next_handle set on first init */ +} hid_hotplug_context; /* zero-initialized (static storage) */ -/* Serializes the one-time hotplug setup (the implicit hid_init() and the - creation of the hotplug mutex) against concurrent first registrations, - and the teardown in hid_exit() against them. */ -static pthread_mutex_t hid_hotplug_startup_mutex = PTHREAD_MUTEX_INITIALIZER; +/* The hotplug mutex and condition variable are created exactly once and are + never destroyed: they live for the lifetime of the process, so that no thread + can ever lock a mutex that hid_exit() destroyed underneath it */ +static pthread_once_t hid_hotplug_init_once = PTHREAD_ONCE_INIT; static void hid_internal_hotplug_remove_postponed(void) { @@ -567,11 +626,40 @@ static void hid_internal_hotplug_remove_postponed(void) hid_hotplug_context.cb_list_dirty = 0; } -/* Collects (joins) the event thread once it has been told to stop, and - releases what only the joiner may release. Serializes concurrent joiners - and waits out a join running on another thread. - Must be called with the hotplug mutex NOT held by the calling thread, - except from the event thread itself, where it is a guaranteed no-op. */ +/* Releases everything that only the collector of the event thread may release. + Called with the hotplug mutex held, either by the thread that has just joined + the event thread, or by the event thread itself when nobody is joining it (it + then detaches itself - see hid_internal_hotplug_thread_epilogue()). + Both participants have left the startup barrier by then: the registering + thread holds the mutex across the barrier and releases it only afterwards, so + acquiring the mutex proves it is out. */ +static void hid_internal_hotplug_release_thread(void) +{ + hid_hotplug_context.thread_needs_join = 0; + + pthread_barrier_destroy(&hid_hotplug_context.startup_barrier); + + /* The run loop sources are created by the event thread, but the thread does + not release them while it winds down: the references must stay valid so + that the run loop can still be woken up until the thread is collected. */ + if (hid_hotplug_context.source) { + CFRelease(hid_hotplug_context.source); + hid_hotplug_context.source = NULL; + } + if (hid_hotplug_context.replay_source) { + CFRelease(hid_hotplug_context.replay_source); + hid_hotplug_context.replay_source = NULL; + } + hid_hotplug_context.run_loop = NULL; +} + +/* Collects (joins) the event thread once it has been told to stop, and releases + what only the collector may release. Serializes concurrent joiners and waits + out a join running on another thread. + Must be called with the hotplug mutex NOT held by the calling thread, except + from the event thread itself, where it is a guaranteed no-op (the + pthread_equal() check below) - that is what keeps pthread_cond_wait() from + ever being reached with the recursive mutex locked more than once. */ static void hid_internal_hotplug_collect_thread(void) { pthread_mutex_lock(&hid_hotplug_context.mutex); @@ -581,10 +669,11 @@ static void hid_internal_hotplug_collect_thread(void) && hid_hotplug_context.thread_state == 2 && !pthread_equal(pthread_self(), hid_hotplug_context.thread)) { if (hid_hotplug_context.join_in_progress) { - /* Another thread is already joining: wait for it to finish */ - pthread_mutex_unlock(&hid_hotplug_context.mutex); - sched_yield(); - pthread_mutex_lock(&hid_hotplug_context.mutex); + /* Another thread is already joining: wait for it to finish. + A condition variable (and not a spin) is essential: the joiner is + blocked in pthread_join() waiting for the event thread, which may + still need this very mutex to finish an in-flight dispatch. */ + pthread_cond_wait(&hid_hotplug_context.join_done, &hid_hotplug_context.mutex); continue; } @@ -599,27 +688,16 @@ static void hid_internal_hotplug_collect_thread(void) pthread_mutex_lock(&hid_hotplug_context.mutex); hid_hotplug_context.join_in_progress = 0; - hid_hotplug_context.thread_needs_join = 0; + hid_internal_hotplug_release_thread(); - pthread_barrier_destroy(&hid_hotplug_context.startup_barrier); - - /* The run loop sources are created by the event thread, but the thread - exits without releasing them: the references must stay valid so that - the run loop can still be woken up while the thread is winding down. */ - if (hid_hotplug_context.source) { - CFRelease(hid_hotplug_context.source); - hid_hotplug_context.source = NULL; - } - if (hid_hotplug_context.replay_source) { - CFRelease(hid_hotplug_context.replay_source); - hid_hotplug_context.replay_source = NULL; - } - hid_hotplug_context.run_loop = NULL; + /* Wake the threads waiting for this join to complete */ + pthread_cond_broadcast(&hid_hotplug_context.join_done); } pthread_mutex_unlock(&hid_hotplug_context.mutex); } +/* Must be called with the hotplug mutex held */ static void hid_internal_hotplug_cleanup(void) { if (!hid_hotplug_context.mutex_ready || hid_hotplug_context.mutex_in_use) { @@ -646,9 +724,14 @@ static void hid_internal_hotplug_cleanup(void) /* Cause hotplug_thread() to stop. */ hid_hotplug_context.thread_state = 2; - /* Wake up the run thread's event loop so that the thread can exit. */ - CFRunLoopSourceSignal(hid_hotplug_context.source); - CFRunLoopWakeUp(hid_hotplug_context.run_loop); + /* Wake up the run thread's event loop so that the thread can exit. + Both references are still alive: they are only released once the + thread has been collected, which cannot happen while this thread + holds the mutex. */ + if (hid_hotplug_context.source != NULL && hid_hotplug_context.run_loop != NULL) { + CFRunLoopSourceSignal(hid_hotplug_context.source); + CFRunLoopWakeUp(hid_hotplug_context.run_loop); + } } /* The join is never performed here: this function runs with the mutex @@ -658,38 +741,74 @@ static void hid_internal_hotplug_cleanup(void) from the public entry points, with the mutex released. */ } -static void hid_internal_hotplug_init(void) +/* The one-time hotplug initialization, run by pthread_once(). On failure + mutex_ready is left at 0 and the hotplug API stays unavailable. */ +static void hid_internal_hotplug_init_once(void) { - if (!hid_hotplug_context.mutex_ready) { - /* Initialize the mutex as recursive */ - pthread_mutexattr_t attr; - pthread_mutexattr_init(&attr); - pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); - pthread_mutex_init(&hid_hotplug_context.mutex, &attr); + pthread_mutexattr_t attr; + + if (pthread_mutexattr_init(&attr) != 0) { + return; + } + + /* The mutex must be recursive: a callback runs with it held and is allowed + to call hid_hotplug_register_callback()/hid_hotplug_deregister_callback() */ + if (pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE) != 0) { pthread_mutexattr_destroy(&attr); + return; + } + + if (pthread_mutex_init(&hid_hotplug_context.mutex, &attr) != 0) { + pthread_mutexattr_destroy(&attr); + return; + } - /* Set state to Ready */ - hid_hotplug_context.mutex_ready = 1; - hid_hotplug_context.mutex_in_use = 0; - hid_hotplug_context.cb_list_dirty = 0; - if (hid_hotplug_context.next_handle < FIRST_HOTPLUG_CALLBACK_HANDLE) - hid_hotplug_context.next_handle = FIRST_HOTPLUG_CALLBACK_HANDLE; + pthread_mutexattr_destroy(&attr); + + if (pthread_cond_init(&hid_hotplug_context.join_done, NULL) != 0) { + pthread_mutex_destroy(&hid_hotplug_context.mutex); + return; } + + hid_hotplug_context.next_handle = FIRST_HOTPLUG_CALLBACK_HANDLE; + + /* Publish the mutex as usable, last */ + hid_hotplug_context.mutex_ready = 1; +} + +/* Ensures the hotplug mutex is created. Returns 0 when the hotplug machinery is + usable, -1 when it could not be initialized (the caller must then fail with a + retrievable error - locking an uninitialized mutex is undefined behavior). + pthread_once() provides both the one-time guarantee and the memory + synchronization for the read of mutex_ready below. */ +static int hid_internal_hotplug_init(void) +{ + pthread_once(&hid_hotplug_init_once, hid_internal_hotplug_init_once); + + return hid_hotplug_context.mutex_ready ? 0 : -1; } +/* Tears the hotplug machinery down (from hid_exit()). Leaves `exiting` set, so + that a concurrent hid_hotplug_register_callback()/hid_hotplug_deregister_callback() + fails instead of racing the rest of hid_exit(); hid_internal_hotplug_exit_done() + clears it once hid_exit() is finished. */ static void hid_internal_hotplug_exit(void) { - pthread_mutex_lock(&hid_hotplug_startup_mutex); + struct hid_hotplug_callback **current; - if (!hid_hotplug_context.mutex_ready) { - pthread_mutex_unlock(&hid_hotplug_startup_mutex); + if (hid_internal_hotplug_init() != 0) { + /* The hotplug mutex could not be created: nothing can ever have been + registered, and there is nothing to tear down */ return; } pthread_mutex_lock(&hid_hotplug_context.mutex); - struct hid_hotplug_callback** current = &hid_hotplug_context.hotplug_cbs; + + /* Close the hotplug API for the duration of the teardown */ + hid_hotplug_context.exiting = 1; /* Remove all callbacks from the list */ + current = &hid_hotplug_context.hotplug_cbs; while (*current) { struct hid_hotplug_callback* next = (*current)->next; hid_free_enumeration((*current)->replay); @@ -702,15 +821,30 @@ static void hid_internal_hotplug_exit(void) /* Join the stopped event thread, with the hotplug mutex released */ hid_internal_hotplug_collect_thread(); - hid_hotplug_context.mutex_ready = 0; - pthread_mutex_destroy(&hid_hotplug_context.mutex); + /* The hotplug mutex is deliberately NOT destroyed: another thread may be + about to lock it (it only has to observe `exiting` afterwards), and + destroying a mutex under it would be undefined behavior. It costs nothing + to keep it for the lifetime of the process. */ +} +/* Re-opens the hotplug API after hid_exit() has finished. */ +static void hid_internal_hotplug_exit_done(void) +{ + if (hid_internal_hotplug_init() != 0) { + return; + } + + pthread_mutex_lock(&hid_hotplug_context.mutex); + + /* The event thread has been collected by now, so it no longer uses the mode */ if (hid_hotplug_context.run_loop_mode) { CFRelease(hid_hotplug_context.run_loop_mode); hid_hotplug_context.run_loop_mode = NULL; } - pthread_mutex_unlock(&hid_hotplug_startup_mutex); + hid_hotplug_context.exiting = 0; + + pthread_mutex_unlock(&hid_hotplug_context.mutex); } /* Initialize the IOHIDManager if necessary. This is the public function, and @@ -734,7 +868,10 @@ int HID_API_EXPORT hid_exit(void) { /* The hotplug thread and the callbacks are stopped/freed unconditionally: hid_hotplug_register_callback() may have initialized the library implicitly - without ever creating hid_mgr */ + without ever creating hid_mgr. + This leaves the hotplug API closed (`exiting`), so that a concurrent + registration cannot re-enter hid_init() while hid_mgr is being destroyed + below */ hid_internal_hotplug_exit(); if (hid_mgr) { @@ -747,6 +884,9 @@ int HID_API_EXPORT hid_exit(void) /* Free global error message */ register_global_error(NULL); + /* Re-open the hotplug API: the library may be initialized again */ + hid_internal_hotplug_exit_done(); + return 0; } @@ -763,18 +903,6 @@ static void process_pending_events(void) } while (res != kCFRunLoopRunFinished && res != kCFRunLoopRunTimedOut); } -/* Runs the hotplug event thread's private run loop mode until no more events - are pending. Called on the event thread only, during its startup: the - hotplug IOHIDManager is scheduled in that private mode, so this is where - the device-matching events of the already connected devices are delivered. */ -static void hid_internal_hotplug_process_pending_events(void) -{ - SInt32 res; - do { - res = CFRunLoopRunInMode(hid_hotplug_context.run_loop_mode, 0.001, FALSE); - } while (res != kCFRunLoopRunFinished && res != kCFRunLoopRunTimedOut); -} - static int read_usb_interface_from_hid_service_parent(io_service_t hid_service) { int32_t result = -1; @@ -1088,7 +1216,16 @@ void HID_API_EXPORT hid_free_enumeration(struct hid_device_info *devs) } /* Makes a deep copy of a single hid_device_info entry (the next pointer of - the copy is always NULL). Returns NULL on allocation failure. */ + the copy is always NULL). Returns NULL on allocation failure. + Every field is copied by hand: this function must be updated whenever + struct hid_device_info gains a new field. + Note the allocation asymmetry with the hotplug device cache: the entries of + the cache are allocated as struct hid_device_info_ex (they carry the + io_service_t used to recognize a device on removal), while a copy made here + is a plain struct hid_device_info. A copy must therefore never be passed to + match_ref_to_info() or added to the device cache - it is only ever handed to + a hotplug callback, and freed with hid_free_enumeration() like any other + hid_device_info. */ static struct hid_device_info *hid_internal_copy_device_info(const struct hid_device_info *src) { struct hid_device_info *dst = (struct hid_device_info*) calloc(1, sizeof(struct hid_device_info)); @@ -1148,8 +1285,18 @@ static void hid_internal_hotplug_replay_one(struct hid_hotplug_callback *callbac } } +/* Dispatches one event to every matching callback. Called on the event thread + only, and only once it has passed its startup barrier. */ static void hid_internal_invoke_callbacks(struct hid_device_info *info, hid_hotplug_event event) { + /* Defensive: during the startup phase the callback list is provably empty + (the first registration inserts its callback only after the barrier) and + the hotplug mutex is held by the registering thread parked at that + barrier - locking it here would deadlock it and this thread forever */ + if (hid_hotplug_context.startup_phase) { + return; + } + pthread_mutex_lock(&hid_hotplug_context.mutex); unsigned char old_state = hid_hotplug_context.mutex_in_use; @@ -1195,22 +1342,81 @@ static void hid_internal_invoke_callbacks(struct hid_device_info *info, hid_hotp pthread_mutex_unlock(&hid_hotplug_context.mutex); } +/* Matches an IOHIDDeviceRef against an entry of the hotplug device cache. + The entries of the cache are allocated as struct hid_device_info_ex and carry + the io_service_t of the device: the path cannot be regenerated once the device + is gone. Never pass an entry that did not come from the cache (see + hid_internal_copy_device_info()). */ +static int match_ref_to_info(IOHIDDeviceRef device, struct hid_device_info *info) +{ + if (!device || !info) { + return 0; + } + + struct hid_device_info_ex* ex = (struct hid_device_info_ex*)info; + io_service_t service = IOHIDDeviceGetService(device); + + return (service == ex->service); +} + +/* Returns non-zero when the device is already in the hotplug device cache. + Called on the event thread, with the mutex held (or during its startup phase, + where the thread has exclusive access to the context). */ +static int hid_internal_hotplug_is_known_device(IOHIDDeviceRef device) +{ + struct hid_device_info *info; + + for (info = hid_hotplug_context.devs; info != NULL; info = info->next) { + if (match_ref_to_info(device, info)) { + return 1; + } + } + + return 0; +} + static void hid_internal_hotplug_connect_callback(void *context, IOReturn result, void *sender, IOHIDDeviceRef device) { + struct hid_device_info *info; + + /* The event thread does not lock the mutex and does not dispatch anything + before it has passed the startup barrier (the whole initial enumeration is + such a window): the mutex is held by the registering thread parked at that + barrier - locking it here would deadlock - and the callback list is + provably empty then, so there is nothing to dispatch to. The device still + goes into the cache: that is what the initial HID_API_HOTPLUG_ENUMERATE + snapshot is taken from. */ + const int startup = hid_hotplug_context.startup_phase; + (void) context; (void) result; (void) sender; - struct hid_device_info* info = create_device_info(device); - if (!info) { + if (!startup) { + /* Lock the mutex to avoid race conditions */ + pthread_mutex_lock(&hid_hotplug_context.mutex); + } + + /* Once the run loop runs, the IOHIDManager re-reports every device that was + already connected when it was opened. Those devices are all in the cache - + it is completed synchronously during the thread's startup, before any + callback can be registered - so they are NOT new arrivals and must never be + dispatched as live events. This is what makes the snapshot boundary + deterministic instead of dependent on how long the initial matching burst + takes to be delivered. */ + if (hid_internal_hotplug_is_known_device(device)) { + if (!startup) { + pthread_mutex_unlock(&hid_hotplug_context.mutex); + } return; } - /* NOTE: we don't call any callbacks and we don't lock the mutex during initialization: the mutex is held by the main thread, but it's waiting by a barrier */ - if (hid_hotplug_context.thread_state > 0) - { - /* Lock the mutex to avoid race conditions */ - pthread_mutex_lock(&hid_hotplug_context.mutex); + info = create_device_info(device); + if (!info) { + if (!startup) { + pthread_mutex_unlock(&hid_hotplug_context.mutex); + } + return; } /* Append all we got to the end of the device list BEFORE invoking any @@ -1228,8 +1434,7 @@ static void hid_internal_hotplug_connect_callback(void *context, IOReturn result hid_hotplug_context.devs = info; } - if (hid_hotplug_context.thread_state > 0) - { + if (!startup) { /* Invoke the callbacks for each entry; device->next must be NULL for every delivery, so each entry is temporarily severed, truncating the device cache at that entry for the duration of its dispatch (a @@ -1250,37 +1455,37 @@ static void hid_internal_hotplug_connect_callback(void *context, IOReturn result } } -static int match_ref_to_info(IOHIDDeviceRef device, struct hid_device_info *info) -{ - if (!device || !info) { - return 0; - } - - struct hid_device_info_ex* ex = (struct hid_device_info_ex*)info; - io_service_t service = IOHIDDeviceGetService(device); - - return (service == ex->service); -} - static void hid_internal_hotplug_disconnect_callback(void *context, IOReturn result, void *sender, IOHIDDeviceRef device) { + struct hid_device_info **current; + + /* Same guard as in the connect callback - and it covers the dispatch, not + just the lock: a device removed while the event thread is still starting + up (the whole initial enumeration is such a window) must not lock the + mutex held by the registrant parked at the startup barrier, nor dispatch + anything. There is no callback to notify at that point either: dropping + the device from the cache is all that is needed, and the registration's + HID_API_HOTPLUG_ENUMERATE snapshot - copied from the cache only after this + thread reaches the barrier - then simply does not contain it. */ + const int startup = hid_hotplug_context.startup_phase; + (void) context; (void) result; (void) sender; - /* NOTE: we don't call any callbacks and we don't lock the mutex during initialization: the mutex is held by the main thread, but it's waiting by a barrier*/ - if (hid_hotplug_context.thread_state > 0) - { + if (!startup) { pthread_mutex_lock(&hid_hotplug_context.mutex); } - for (struct hid_device_info **current = &hid_hotplug_context.devs; *current;) { + for (current = &hid_hotplug_context.devs; *current;) { struct hid_device_info* info = *current; - if (match_ref_to_info(device, *current)) { + if (match_ref_to_info(device, info)) { /* If the IOHIDDeviceRef device that's left matches this HID device, we detach it from the list */ - *current = (*current)->next; + *current = info->next; info->next = NULL; - hid_internal_invoke_callbacks(info, HID_API_HOTPLUG_EVENT_DEVICE_LEFT); + if (!startup) { + hid_internal_invoke_callbacks(info, HID_API_HOTPLUG_EVENT_DEVICE_LEFT); + } /* Free every removed device */ hid_free_enumeration(info); } else { @@ -1288,8 +1493,7 @@ static void hid_internal_hotplug_disconnect_callback(void *context, IOReturn res } } - if (hid_hotplug_context.thread_state > 0) - { + if (!startup) { /* Clean up if the last callback was removed */ hid_internal_hotplug_cleanup(); pthread_mutex_unlock(&hid_hotplug_context.mutex); @@ -1306,6 +1510,14 @@ static void hotplug_replay_callback(void* context) { (void) context; + /* Defensive, same as in hid_internal_invoke_callbacks(): the replay source is + only ever signalled by hid_hotplug_register_callback() under the mutex and + after the startup barrier, so it cannot be performed by the startup drain - + where taking the mutex would deadlock against the parked registrant */ + if (hid_hotplug_context.startup_phase) { + return; + } + pthread_mutex_lock(&hid_hotplug_context.mutex); unsigned char old_state = hid_hotplug_context.mutex_in_use; @@ -1325,13 +1537,159 @@ static void hotplug_replay_callback(void* context) pthread_mutex_unlock(&hid_hotplug_context.mutex); } +/* Lets the hotplug IOHIDManager process the device-matching events it queued for + the already connected devices, exactly like the process_pending_events() call + hid_enumerate() makes before IOHIDManagerCopyDevices(). + + This is NOT the snapshot boundary - hid_internal_hotplug_build_device_cache() + below is - and nothing depends on it draining the burst completely: it can + only ADD devices to the cache, never move one from the initial snapshot to the + live events. It runs on the event thread during its startup phase, so the + connect/disconnect callbacks it triggers only maintain the cache and dispatch + nothing (no callback is registered yet, and the mutex must not be taken - see + the locking note at the top of the hotplug code). */ +static void hid_internal_hotplug_drain_pending_events(void) +{ + SInt32 res; + do { + res = CFRunLoopRunInMode(hid_hotplug_context.run_loop_mode, 0.001, FALSE); + } while (res != kCFRunLoopRunFinished && res != kCFRunLoopRunTimedOut && res != kCFRunLoopRunStopped); +} + +/* Completes the initial device cache from the devices the hotplug IOHIDManager + matches right now. Runs on the event thread during its startup phase, i.e. + before any callback can be registered and without the mutex. + + This is the deterministic boundary between "was already connected" and + "arrived live": IOHIDManagerCopyDevices() answers synchronously - which is + exactly what hid_enumerate() relies on - so, unlike a timed pump of the run + loop, the completeness of the snapshot does not depend on how long the initial + matching burst takes. Every device connected at this point ends up in the + cache; when the run loop later delivers the matching events for those same + devices, they are recognized as already known and dropped (see + hid_internal_hotplug_connect_callback()), so they can never surface as live + arrivals. + + Devices already added to the cache (by the drain above) are kept: the two + sources are merged by io_service_t. + + Returns 0 on success, -1 on failure. */ +static int hid_internal_hotplug_build_device_cache(void) +{ + CFSetRef device_set; + CFIndex num_devices; + CFIndex i; + IOHIDDeviceRef *device_array; + struct hid_device_info *tail = hid_hotplug_context.devs; + + while (tail != NULL && tail->next != NULL) { + tail = tail->next; + } + + device_set = IOHIDManagerCopyDevices(hid_hotplug_context.manager); + if (device_set == NULL) { + /* No device is currently matched: an empty cache is a valid snapshot */ + return 0; + } + + num_devices = CFSetGetCount(device_set); + if (num_devices <= 0) { + CFRelease(device_set); + return 0; + } + + device_array = (IOHIDDeviceRef*) calloc((size_t) num_devices, sizeof(IOHIDDeviceRef)); + if (device_array == NULL) { + CFRelease(device_set); + return -1; + } + CFSetGetValues(device_set, (const void **) device_array); + + for (i = 0; i < num_devices; i++) { + struct hid_device_info *info; + + if (device_array[i] == NULL) { + continue; + } + + /* Already in the cache (the drain got to it first) */ + if (hid_internal_hotplug_is_known_device(device_array[i])) { + continue; + } + + info = create_device_info(device_array[i]); + if (info == NULL) { + /* Out of memory: fail the startup rather than commit a snapshot + that is missing a connected device (it would later be reported as + a live arrival, which is exactly what the snapshot must prevent) */ + free(device_array); + CFRelease(device_set); + return -1; + } + + if (tail != NULL) { + tail->next = info; + } + else { + hid_hotplug_context.devs = info; + } + + /* A device contributes one entry per usage pair */ + while (info->next != NULL) { + info = info->next; + } + tail = info; + } + + free(device_array); + CFRelease(device_set); + + return 0; +} + +/* Runs at the very end of the event thread. If no other thread is joining it, + the thread detaches itself and releases its own resources here: otherwise a + callback that deregisters the last callback from within a callback (including + by returning non-zero) would leave an unjoined thread, two run loop sources + and the run loop behind until the next register/deregister/hid_exit() - which + may never come. */ +static void hid_internal_hotplug_thread_epilogue(void) +{ + pthread_mutex_lock(&hid_hotplug_context.mutex); + + if (hid_hotplug_context.thread_needs_join && !hid_hotplug_context.join_in_progress) { + /* Nobody is inside pthread_join() on this thread, and nobody can enter + it any more: the decision is taken under the mutex on both sides (see + hid_internal_hotplug_collect_thread()), so there is no double join and + no join of a detached thread. */ + pthread_detach(pthread_self()); + hid_internal_hotplug_release_thread(); + pthread_cond_broadcast(&hid_hotplug_context.join_done); + } + + /* Past this point the thread must not touch the context any more: as soon as + the mutex is released, a new event thread may be started */ + pthread_mutex_unlock(&hid_hotplug_context.mutex); +} + static void* hotplug_thread(void* user_data) { + int manager_opened = 0; + (void) user_data; - int startup_ok = 0; + /* Startup phase: the registering thread holds the hotplug mutex and is + parked at the startup barrier, so this thread has exclusive access to the + context - and it MUST NOT take the mutex until the barrier has been passed + (see the locking note at the top of the hotplug code). No hotplug callback + can be dispatched here either: none is registered yet (the first one is + inserted only after the barrier). */ - hid_hotplug_context.thread_state = 0; + /* The device cache is empty at this point: the event thread is only ever + started with no callbacks registered, which is exactly when the previous + cache was freed by hid_internal_hotplug_cleanup() */ + hid_free_enumeration(hid_hotplug_context.devs); + hid_hotplug_context.devs = NULL; /* Store a reference to this runloop if we ever need to wake it up - e.g. if we have no callbacks left or hid_exit was called */ hid_hotplug_context.run_loop = CFRunLoopGetCurrent(); @@ -1372,7 +1730,10 @@ static void* hotplug_thread(void* user_data) /* Set the manager to receive events for ALL HID devices */ IOHIDManagerSetDeviceMatching(hid_hotplug_context.manager, NULL); - /* Install callbacks */ + /* Install callbacks. They only ever fire from this thread's run loop + (the manager is scheduled in a private run loop mode of this + thread), i.e. from the startup drain below or from the event loop + once the startup barrier is passed. */ IOHIDManagerRegisterDeviceMatchingCallback(hid_hotplug_context.manager, hid_internal_hotplug_connect_callback, NULL); @@ -1381,61 +1742,89 @@ static void* hotplug_thread(void* user_data) hid_internal_hotplug_disconnect_callback, NULL); - /* Opening the manager enqueues the device-matching events - for all the devices already connected */ + /* Opening the manager enqueues the device-matching events for all + the devices that are already connected */ if (IOHIDManagerOpen(hid_hotplug_context.manager, kIOHIDOptionsTypeNone) == kIOReturnSuccess) { - startup_ok = 1; + manager_opened = 1; + + /* Give the manager a chance to process what it just enqueued + (best effort; not a fence - see the function comment) ... */ + hid_internal_hotplug_drain_pending_events(); + + /* ... and then take the authoritative snapshot of the connected + devices synchronously: THIS - and not a timed pump of the run + loop - is the boundary between the initial + HID_API_HOTPLUG_ENUMERATE pass and the live events */ + if (hid_internal_hotplug_build_device_cache() == 0) { + hid_hotplug_context.startup_ok = 1; + } } } } - if (!startup_ok) { - /* Report the failure to hid_hotplug_register_callback() waiting at the barrier; - the sources (if any got created) are released by the thread that joins us */ - hid_hotplug_context.thread_state = 2; - pthread_barrier_wait(&hid_hotplug_context.startup_barrier); + /* Hand the startup result over to hid_hotplug_register_callback(), which is + waiting at the barrier and publishes it (thread_state) under the mutex. + The barrier also publishes everything this thread has set up so far. */ + pthread_barrier_wait(&hid_hotplug_context.startup_barrier); - if (hid_hotplug_context.manager) { - IOHIDManagerUnscheduleFromRunLoop(hid_hotplug_context.manager, hid_hotplug_context.run_loop, hid_hotplug_context.run_loop_mode); - CFRelease(hid_hotplug_context.manager); - hid_hotplug_context.manager = NULL; - } + /* The context is shared again from here on: the mutex is required */ + hid_hotplug_context.startup_phase = 0; - return NULL; - } + if (hid_hotplug_context.startup_ok) { + for (;;) { + SInt32 code; + int stop; - /* Drain the device-matching events of the already connected devices: the - manager delivers them in the private run loop mode it is scheduled on, - and with thread_state still 0 they only populate the device cache and - invoke no callbacks - so they can never surface as live arrival events */ - hid_internal_hotplug_process_pending_events(); + /* All of the lifecycle state is read under the mutex */ + pthread_mutex_lock(&hid_hotplug_context.mutex); + stop = (hid_hotplug_context.thread_state == 2); + pthread_mutex_unlock(&hid_hotplug_context.mutex); - /* Now that all events are flushed, we are ready to notify the main thread that we are ready */ - hid_hotplug_context.thread_state = 1; - pthread_barrier_wait(&hid_hotplug_context.startup_barrier); + if (stop) { + break; + } - while (hid_hotplug_context.thread_state != 2) { - int code = CFRunLoopRunInMode(hid_hotplug_context.run_loop_mode, 1000/*sec*/, FALSE); - /* If runloop stopped for whatever reason, exit the thread */ - if (code != kCFRunLoopRunTimedOut && - code != kCFRunLoopRunHandledSource) { - /* Publish the shutdown under the mutex, so that a concurrent - registration cannot observe a running thread (thread_state 1) - and signal-and-wake a run loop that is winding down */ + code = CFRunLoopRunInMode(hid_hotplug_context.run_loop_mode, 1000/*sec*/, FALSE); + + if (code == kCFRunLoopRunTimedOut || code == kCFRunLoopRunHandledSource) { + continue; + } + + /* The run loop is gone: either the stop source stopped it + (thread_state is already 2), or it exited on its own. Publish the + shutdown under the mutex, so that a concurrent registration cannot + observe a running thread (thread_state 1) and signal-and-wake a run + loop that is winding down; a registration that finds the thread + stopped while callbacks are still registered fails instead of + attaching to a dead thread. */ pthread_mutex_lock(&hid_hotplug_context.mutex); hid_hotplug_context.thread_state = 2; pthread_mutex_unlock(&hid_hotplug_context.mutex); break; } } + /* else: the startup failed - hid_hotplug_register_callback() fails the + registration and collects this thread (or lets it detach itself below); + the run loop sources (if any got created) and the startup barrier are + released by whoever collects it */ - /* Kill the manager */ - IOHIDManagerClose(hid_hotplug_context.manager, kIOHIDOptionsTypeNone); + /* Kill the manager. No mutex is needed (and none may be held across + IOHIDManagerClose()): nothing else ever touches the manager, and no other + thread may start a new event thread or release the run loop mode before + this thread has been collected - which cannot happen before the epilogue + below, i.e. after the last use of the run loop and of its mode here. */ + if (hid_hotplug_context.manager) { + if (manager_opened) { + IOHIDManagerClose(hid_hotplug_context.manager, kIOHIDOptionsTypeNone); + } + + IOHIDManagerUnscheduleFromRunLoop(hid_hotplug_context.manager, hid_hotplug_context.run_loop, hid_hotplug_context.run_loop_mode); - IOHIDManagerUnscheduleFromRunLoop(hid_hotplug_context.manager, hid_hotplug_context.run_loop, hid_hotplug_context.run_loop_mode); + CFRelease(hid_hotplug_context.manager); + hid_hotplug_context.manager = NULL; + } - CFRelease(hid_hotplug_context.manager); - hid_hotplug_context.manager = NULL; + hid_internal_hotplug_thread_epilogue(); return NULL; } @@ -1458,27 +1847,49 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven return -1; } - /* Serialize the one-time setup against concurrent first registrations: - both the implicit hid_init() and the hotplug mutex creation must - happen exactly once */ - pthread_mutex_lock(&hid_hotplug_startup_mutex); + /* Create the hotplug mutex, exactly once. There is deliberately no + bootstrap lock around this: any lock ordered outside the hotplug mutex + would deadlock against a registration made from within a callback (which + already holds the hotplug mutex) - see the locking note above. */ + if (hid_internal_hotplug_init() != 0) { + register_global_error("hid_hotplug_register_callback: failed to initialize the hotplug mutex"); + return -1; + } + + /* Lock the mutex to avoid race conditions */ + pthread_mutex_lock(&hid_hotplug_context.mutex); - /* The registration initializes the library implicitly (as if by hid_init()) */ + if (hid_hotplug_context.exiting) { + /* hid_exit() is tearing the machinery down: it invalidates every + callback handle, so there is nothing to register into */ + register_global_error("hid_hotplug_register_callback: hid_exit() is in progress"); + pthread_mutex_unlock(&hid_hotplug_context.mutex); + return -1; + } + + /* The registration initializes the library implicitly (as if by hid_init()). + Done under the hotplug mutex, together with the `exiting` check above, so + that it cannot race hid_exit() destroying hid_mgr (hid_exit() keeps + `exiting` set across the whole of its teardown). + NOTE: hid_init() schedules the global IOHIDManager on the run loop of the + CURRENT thread. When the implicit initialization happens here, that is the + registering thread rather than the thread that later calls hid_enumerate() + or hid_open() - those pump their own run loop, so they no longer service + the manager's run loop source. They do not depend on it (the manager + answers IOHIDManagerCopyDevices() synchronously), but an application that + wants the classic behavior should call hid_init() explicitly, from the + thread it uses HIDAPI on, before registering a hotplug callback. */ if (!hid_mgr && hid_init() != 0) { - pthread_mutex_unlock(&hid_hotplug_startup_mutex); /* register_global_error: global error is already set by hid_init */ + pthread_mutex_unlock(&hid_hotplug_context.mutex); return -1; } - /* Ensure we are ready to actually use the mutex */ - hid_internal_hotplug_init(); - - pthread_mutex_unlock(&hid_hotplug_startup_mutex); - hotplug_cb = (struct hid_hotplug_callback*)calloc(1, sizeof(struct hid_hotplug_callback)); if (hotplug_cb == NULL) { register_global_error("hid_hotplug_register_callback: failed to allocate a callback"); + pthread_mutex_unlock(&hid_hotplug_context.mutex); return -1; } @@ -1491,9 +1902,6 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven hotplug_cb->user_data = user_data; hotplug_cb->callback = callback; - /* Lock the mutex to avoid race conditions */ - pthread_mutex_lock(&hid_hotplug_context.mutex); - /* If a stopped event thread has not been collected (joined) yet, collect it before the machinery can be restarted; the join must not happen with the mutex held, so drop the mutex for the collection and re-check. @@ -1505,6 +1913,28 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven pthread_mutex_unlock(&hid_hotplug_context.mutex); hid_internal_hotplug_collect_thread(); pthread_mutex_lock(&hid_hotplug_context.mutex); + + /* hid_exit() may have started while the mutex was released */ + if (hid_hotplug_context.exiting) { + register_global_error("hid_hotplug_register_callback: hid_exit() is in progress"); + pthread_mutex_unlock(&hid_hotplug_context.mutex); + free(hotplug_cb); + return -1; + } + } + + /* A stopped event thread while callbacks are still registered means the + thread stopped on its own (an unsolicited run loop exit): a solicited stop + is only ever requested once the callback list is empty. The machinery is + dead - it can deliver neither the initial pass nor any live event - so the + registration must fail rather than silently attach to it. + (With no callbacks left, the loop above has already collected the thread + and a fresh one is started below.) */ + if (hid_hotplug_context.hotplug_cbs != NULL && hid_hotplug_context.thread_state == 2) { + register_global_error("hid_hotplug_register_callback: the hotplug event thread has stopped"); + pthread_mutex_unlock(&hid_hotplug_context.mutex); + free(hotplug_cb); + return -1; } /* Handles are not recycled even on overflow: recycling could collide with a live handle */ @@ -1533,8 +1963,17 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven return -1; } + /* Set up the state the event thread starts from. The thread must not + touch the mutex before the startup barrier - this thread holds it and + parks at that barrier - so it never writes thread_state itself: it + reports its result in startup_ok, which is published here instead. */ + hid_hotplug_context.thread_state = 0; + hid_hotplug_context.startup_ok = 0; + hid_hotplug_context.startup_phase = 1; + if (pthread_create(&hid_hotplug_context.thread, NULL, hotplug_thread, NULL) != 0) { register_global_error("hid_hotplug_register_callback: failed to create the hotplug events thread"); + hid_hotplug_context.startup_phase = 0; pthread_barrier_destroy(&hid_hotplug_context.startup_barrier); pthread_mutex_unlock(&hid_hotplug_context.mutex); free(hotplug_cb); @@ -1547,10 +1986,19 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven pthread_barrier_wait(&hid_hotplug_context.startup_barrier); - if (hid_hotplug_context.thread_state != 1) { + /* Publish the thread's startup result */ + hid_hotplug_context.thread_state = hid_hotplug_context.startup_ok ? 1 : 2; + + if (!hid_hotplug_context.startup_ok) { /* The thread failed to set up the device monitoring and is exiting: it must be collected (joined) with the mutex released */ register_global_error("hid_hotplug_register_callback: failed to start the device monitoring"); + + /* Free whatever the thread may have cached before it failed + (the callback list is empty, so this also stops nothing and + re-signals nothing: thread_state is already 2) */ + hid_internal_hotplug_cleanup(); + pthread_mutex_unlock(&hid_hotplug_context.mutex); hid_internal_hotplug_collect_thread(); free(hotplug_cb); @@ -1620,6 +2068,9 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven *callback_handle = hotplug_cb->handle; } + /* Clear the stale global error on success, like hid_init()/hid_enumerate() do */ + register_global_error(NULL); + pthread_mutex_unlock(&hid_hotplug_context.mutex); return 0; @@ -1627,15 +2078,29 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_callback_handle callback_handle) { - if (callback_handle <= 0 || !hid_hotplug_context.mutex_ready) { + int result = -1; + + if (callback_handle <= 0) { register_global_error("hid_hotplug_deregister_callback: not a registered callback handle"); return -1; } - int result = -1; + /* The mutex is created here as well: this may be the first hotplug call */ + if (hid_internal_hotplug_init() != 0) { + register_global_error("hid_hotplug_deregister_callback: failed to initialize the hotplug mutex"); + return -1; + } pthread_mutex_lock(&hid_hotplug_context.mutex); + if (hid_hotplug_context.exiting) { + /* hid_exit() is tearing the machinery down and invalidates every handle: + deregistering is a no-op */ + register_global_error("hid_hotplug_deregister_callback: hid_exit() is in progress"); + pthread_mutex_unlock(&hid_hotplug_context.mutex); + return -1; + } + if (hid_hotplug_context.hotplug_cbs == NULL) { register_global_error("hid_hotplug_deregister_callback: no callbacks are registered"); } @@ -1671,8 +2136,9 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_call pthread_mutex_unlock(&hid_hotplug_context.mutex); /* If this deregistration stopped the event thread, join it with the mutex - released (a no-op from within a callback: the join is then performed by - the next registration or by hid_exit()) */ + released. A no-op when called from within a callback (the event thread + cannot join itself): the thread then detaches and releases itself in its + epilogue - see hid_internal_hotplug_thread_epilogue() */ hid_internal_hotplug_collect_thread(); return result; From 98d23542e3c2166d6691135d392facb65ba88a57 Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Wed, 15 Jul 2026 02:57:38 +0300 Subject: [PATCH 04/10] mac: do not treat MACH_PORT_NULL as a device identity in hotplug matching Two devices that both lack an IOService (service == MACH_PORT_NULL) compared equal, so the arrival dedupe would suppress the second one and a removal could evict the wrong cache entry. Require a non-null service before matching. Assisted-by: claude-code:claude-opus-4-8 --- mac/hid.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/mac/hid.c b/mac/hid.c index 1741614c9..8a07696a5 100644 --- a/mac/hid.c +++ b/mac/hid.c @@ -1356,7 +1356,11 @@ static int match_ref_to_info(IOHIDDeviceRef device, struct hid_device_info *info struct hid_device_info_ex* ex = (struct hid_device_info_ex*)info; io_service_t service = IOHIDDeviceGetService(device); - return (service == ex->service); + /* MACH_PORT_NULL is not a valid identity: two devices that both lack a + service must not be treated as the same device (that would make the + arrival dedupe suppress the second one, and a removal evict the wrong + cache entry). */ + return (service != MACH_PORT_NULL && service == ex->service); } /* Returns non-zero when the device is already in the hotplug device cache. From cc60780ef2ce55c00931a02f53e1a30227926fe3 Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Wed, 15 Jul 2026 03:22:42 +0300 Subject: [PATCH 05/10] mac: suppress global-error writes on the hotplug event thread Honor the cross-backend contract (documented in hidapi.h and already enforced by the libusb and linux backends) that HIDAPI calls made from within a hotplug callback do not update the global error string: the callback runs on HIDAPI's internal event thread, so such a write races an application's hid_error(NULL) read - a use-after-free of last_global_error_str, the same class as the original hotplug blocker. The event thread now publishes its pthread id (guarded by the leaf global_error_mutex) as its first action and clears it in its epilogue. register_global_error()[_format]() skip the write when invoked on that thread, covering both the failure paths and the success-path clear of a callback that re-enters hid_hotplug_(de)register_callback(). Writes from application threads are unaffected, and per-device errors are never suppressed. Assisted-by: claude-code:claude-opus-4-8 --- mac/hid.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 66 insertions(+), 2 deletions(-) diff --git a/mac/hid.c b/mac/hid.c index 8a07696a5..09e45b3e2 100644 --- a/mac/hid.c +++ b/mac/hid.c @@ -262,6 +262,12 @@ static void register_error_str_vformat(wchar_t **error_str, const char *format, register_error_str(error_str, msg); } +/* True when the calling thread is HIDAPI's internal hotplug event thread; used + to suppress writes to the global error string made from that thread (see the + definition after the hotplug context for the full rationale). Must be called + with global_error_mutex held. */ +static int hid_internal_on_event_thread(void); + /* Serializes the mutations of the global error string: the hotplug API is thread-safe and its failure paths (and the implicit hid_init()) may write the global error from multiple threads concurrently. */ @@ -275,7 +281,14 @@ static pthread_mutex_t global_error_mutex = PTHREAD_MUTEX_INITIALIZER; static void register_global_error(const char *msg) { pthread_mutex_lock(&global_error_mutex); - register_error_str(&last_global_error_str, msg); + /* Honor the cross-backend contract (see hidapi.h): a global-error write + attempted on the internal hotplug event thread - e.g. from a + hid_hotplug_(de)register_callback() call re-entered from within a user + callback - must not touch the global error string. Per-device errors go + through register_error_str() with a different target and are unaffected; + only this process-global string is suppressed. */ + if (!hid_internal_on_event_thread()) + register_error_str(&last_global_error_str, msg); pthread_mutex_unlock(&global_error_mutex); } @@ -285,7 +298,9 @@ static void register_global_error_format(const char *format, ...) va_list args; va_start(args, format); pthread_mutex_lock(&global_error_mutex); - register_error_str_vformat(&last_global_error_str, format, args); + /* See register_global_error(): suppressed on the internal event thread. */ + if (!hid_internal_on_event_thread()) + register_error_str_vformat(&last_global_error_str, format, args); pthread_mutex_unlock(&global_error_mutex); va_end(args); } @@ -588,6 +603,15 @@ static struct hid_hotplug_context { registering thread after it (the barrier is the synchronization edge) */ unsigned char startup_ok; + /* Identity of the running hotplug event thread. Published by that thread as + its first action and cleared in its epilogue, so it is valid exactly while + an event thread exists. Guarded by global_error_mutex (a leaf mutex), NOT + the hotplug mutex: the global-error writer consults it while holding + global_error_mutex and must never take the hotplug mutex it may already + hold. Read only via hid_internal_on_event_thread(). */ + pthread_t event_thread_id; + unsigned char event_thread_id_valid; + /* Linked list of the hotplug callbacks */ struct hid_hotplug_callback *hotplug_cbs; @@ -600,6 +624,24 @@ static struct hid_hotplug_context { can ever lock a mutex that hid_exit() destroyed underneath it */ static pthread_once_t hid_hotplug_init_once = PTHREAD_ONCE_INIT; +/* HIDAPI's public API contract (see hidapi.h) is that HIDAPI calls made from + within a hotplug callback do not update the global error string: the callback + runs on this internal event thread, and an application cannot serialize a + hid_error(NULL) read against a write from that thread - that would be a + use-after-free of last_global_error_str. This mirrors the libusb and linux + backends, which likewise suppress such writes. A callback may re-enter the + public hid_hotplug_register_callback()/hid_hotplug_deregister_callback(), + whose success and failure paths both write the global error; those writes are + suppressed via this check in register_global_error()[_format](). + Returns non-zero when the caller is the hotplug event thread. Must be called + with global_error_mutex held (the event_thread_id* fields are guarded by it), + which the global-error writer already holds. */ +static int hid_internal_on_event_thread(void) +{ + return hid_hotplug_context.event_thread_id_valid + && pthread_equal(pthread_self(), hid_hotplug_context.event_thread_id); +} + static void hid_internal_hotplug_remove_postponed(void) { /* Unregister the callbacks whose removal was postponed */ @@ -1661,6 +1703,16 @@ static void hid_internal_hotplug_thread_epilogue(void) { pthread_mutex_lock(&hid_hotplug_context.mutex); + /* The event thread is exiting: stop suppressing global-error writes for its + pthread id. Cleared under the hotplug mutex - before the thread is detached + or collected, and thus before any replacement event thread can be started + and publish its own id - so a later thread's id can never be clobbered. + Ordering is hotplug mutex -> global_error_mutex, the same order the + global-error writer uses when it is called under the hotplug mutex. */ + pthread_mutex_lock(&global_error_mutex); + hid_hotplug_context.event_thread_id_valid = 0; + pthread_mutex_unlock(&global_error_mutex); + if (hid_hotplug_context.thread_needs_join && !hid_hotplug_context.join_in_progress) { /* Nobody is inside pthread_join() on this thread, and nobody can enter it any more: the decision is taken under the mutex on both sides (see @@ -1682,6 +1734,18 @@ static void* hotplug_thread(void* user_data) (void) user_data; + /* Publish this thread's identity as the very first action, before anything + here can attempt a global-error write, so that any such write on this + internal event thread - notably from a user callback that re-enters + hid_hotplug_(de)register_callback() - is suppressed (see + hid_internal_on_event_thread()). Uses global_error_mutex only: the hotplug + mutex must not be taken during the startup phase (the registrant holds it, + parked at the startup barrier). */ + pthread_mutex_lock(&global_error_mutex); + hid_hotplug_context.event_thread_id = pthread_self(); + hid_hotplug_context.event_thread_id_valid = 1; + pthread_mutex_unlock(&global_error_mutex); + /* Startup phase: the registering thread holds the hotplug mutex and is parked at the startup barrier, so this thread has exclusive access to the context - and it MUST NOT take the mutex until the barrier has been passed From 72169eb9878bda47ff81e6275638521ef20ae33a Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Tue, 4 Aug 2026 20:00:41 +0300 Subject: [PATCH 06/10] mac: ignore hotplug devices with no backing io_service_t Such a device has no usable identity for the cache, so it could be reported twice and never evicted; skip it at both cache-insertion points instead. Also document that the "reported exactly once" guarantee is best effort when the live-arrival path runs out of memory, unlike the registration-time paths which can still fail loudly. Assisted-by: claude-code:claude-opus-5 --- mac/hid.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/mac/hid.c b/mac/hid.c index 09e45b3e2..9540192d7 100644 --- a/mac/hid.c +++ b/mac/hid.c @@ -1438,6 +1438,14 @@ static void hid_internal_hotplug_connect_callback(void *context, IOReturn result (void) result; (void) sender; + /* A device without a backing io_service_t carries no usable identity (see + match_ref_to_info()): once cached it would match neither the arrival + dedupe nor its own removal, so it would be reported more than once and + never evicted. Keep it consistently invisible instead. */ + if (!device || IOHIDDeviceGetService(device) == MACH_PORT_NULL) { + return; + } + if (!startup) { /* Lock the mutex to avoid race conditions */ pthread_mutex_lock(&hid_hotplug_context.mutex); @@ -1459,6 +1467,17 @@ static void hid_internal_hotplug_connect_callback(void *context, IOReturn result info = create_device_info(device); if (!info) { + /* Out of memory on the live-arrival path: the device ends up neither in + the cache nor in an event, so "reported exactly once" is best effort + here. The registration-time paths are hardened against this - + hid_internal_hotplug_build_device_cache() fails the startup and + hid_hotplug_register_callback() fails the registration rather than + commit a partial initial pass - because both still have a caller to + report the failure to. An IOKit callback has none, and the + IOHIDManager does not re-report the device, so there is nothing left + to fail or retry against. (During the startup phase the device is + still picked up by hid_internal_hotplug_build_device_cache(), which + does fail loudly if it cannot allocate either.) */ if (!startup) { pthread_mutex_unlock(&hid_hotplug_context.mutex); } @@ -1658,6 +1677,12 @@ static int hid_internal_hotplug_build_device_cache(void) continue; } + /* Same identity requirement as the live-arrival path: an entry with no + backing io_service_t could never be deduped against, nor evicted */ + if (IOHIDDeviceGetService(device_array[i]) == MACH_PORT_NULL) { + continue; + } + /* Already in the cache (the drain got to it first) */ if (hid_internal_hotplug_is_known_device(device_array[i])) { continue; From 5894384d6c534e42e066e7c7b20a90faef2fbe9f Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Tue, 8 Sep 2026 22:36:46 +0300 Subject: [PATCH 07/10] Fix macOS hotplug startup and thread collection mac-1: Monitor devices without opening the hotplug manager. mac-2: Require complete usage chains for hotplug snapshots. mac-3: Correct matching and startup-drain comments. mac-4: Document implicit initialization thread ownership. mac-5: Correct unconditional hotplug teardown rationale. mac-6: Clarify hotplug lock and startup ownership exceptions. mac-7: Explain inline deferred callback removal. mac-8: Add bounded macOS hotplug lifecycle tests to ASan CI. mac-9: Remove the inaccurate sibling global-error comparison. mac-11: Document unsolicited run-loop stop publication limits. mac-12: Keep self-removed threads joinable through destruction. mac-13: Simplify the startup pump and main-loop result check. mac-14: Remove the obsolete self-detach completion broadcast. mac-15: Document failed-registration tail rollback. mac-16: Clarify ownership of run-loop reference release. mac-17: Leave global errors unchanged on registration success. mac-18: Label the replay state check as defensive. mac-19: Label startup cache clearing as defensive. mac-20: Fix startup-barrier comment spacing. mac-21: Label locked mutex-readiness guards as defensive. mac-23: Reject incomplete device-info string allocations. mac-24: Document service-port cache identity lifetime. mac-25: Document persistent effects of live-arrival OOM. mac-30: Report specific monitoring startup failure reasons. Assisted-by: codex-cli:gpt-6-astra --- .github/workflows/builds.yml | 6 +- mac/hid.c | 340 +++++++++++++++++------------------ src/tests/CMakeLists.txt | 16 +- src/tests/test_hotplug_mac.c | 289 +++++++++++++++++++++++++++++ 4 files changed, 470 insertions(+), 181 deletions(-) create mode 100644 src/tests/test_hotplug_mac.c diff --git a/.github/workflows/builds.yml b/.github/workflows/builds.yml index 4c059aca6..df132f898 100644 --- a/.github/workflows/builds.yml +++ b/.github/workflows/builds.yml @@ -56,14 +56,16 @@ jobs: - name: Build CMake Framework working-directory: build/framework run: make install - - name: Run virtual-device tests (IOHIDUserDevice self-skips on hosted CI) + - name: Run device-I/O and hotplug lifecycle tests working-directory: build/shared run: | # The macOS virtual device needs the com.apple.developer.hid.virtual.device # entitlement and interactive user consent, neither available on a hosted # runner, so DeviceIO_darwin self-skips (CTest code 77). This still # verifies the provider builds and the test runs/links. - ASAN_OPTIONS=detect_leaks=0 ctest --output-on-failure + # Verbose output also reports individual hotplug cases skipped when + # no real device is available; device-independent lifecycle checks run. + ASAN_OPTIONS=detect_leaks=0 ctest --verbose --output-on-failure - name: Check artifacts uses: andstor/file-existence-action@v2 with: diff --git a/mac/hid.c b/mac/hid.c index 9540192d7..a02ff5f76 100644 --- a/mac/hid.c +++ b/mac/hid.c @@ -264,8 +264,8 @@ static void register_error_str_vformat(wchar_t **error_str, const char *format, /* True when the calling thread is HIDAPI's internal hotplug event thread; used to suppress writes to the global error string made from that thread (see the - definition after the hotplug context for the full rationale). Must be called - with global_error_mutex held. */ + definition after the hotplug context for the full rationale). The thread-local + read needs no lock. */ static int hid_internal_on_event_thread(void); /* Serializes the mutations of the global error string: the hotplug API is @@ -522,17 +522,18 @@ struct hid_device_info_ex Two locks are involved in the hotplug machinery: - (1) hid_hotplug_context.mutex - recursive; guards ALL of the hotplug - context: the callback list, the device cache and every lifecycle flag - (thread_state, thread_needs_join, join_in_progress, exiting, ...) as + (1) hid_hotplug_context.mutex - recursive; guards the shared hotplug + context, with the exceptions below: the callback list, the device cache + and lifecycle flags (thread_state, thread_needs_join, join_in_progress, + exiting, ...) as well as the CoreFoundation references of the event thread. It is held for the whole duration of every callback invocation, and it is re-entrant so that a callback may call hid_hotplug_register_callback() or hid_hotplug_deregister_callback() from the event thread itself - which the API documentation guarantees cannot deadlock. - (2) global_error_mutex - a leaf lock, held only while the global error - string is replaced. Nothing is ever acquired while it is held. + (2) global_error_mutex - a leaf lock; guards the global error string. + Nothing is ever acquired while it is held. The startup barrier's internal lock (inside pthread_barrier_wait()) is a leaf as well. @@ -552,13 +553,14 @@ struct hid_device_info_ex pthread_cond_wait() only with exactly one recursion level held (see hid_internal_hotplug_collect_thread()). - The only exception to "all context state is accessed under the mutex" is the - event thread's startup phase - everything it does before reaching the startup - barrier: the registering thread that started it holds the mutex and is parked - at that barrier, so the event thread has exclusive access to the context and + The event-thread marker is thread-local. startup_phase is owned by the event + thread, and startup_ok/startup_error are published through the startup barrier. + Before that barrier, the registering thread holds the mutex and is parked + there, so the event thread has exclusive access to the context and MUST NOT take the mutex there (that would deadlock against the parked registrant). The barrier is the release/acquire edge that publishes what the - thread has set up. */ + thread has set up. Its CoreFoundation objects also remain event-thread-owned + until the epilogue; see the manager release at the end of hotplug_thread(). */ static struct hid_hotplug_context { /* MacOS specific notification handles */ @@ -588,7 +590,7 @@ static struct hid_hotplug_context { unsigned char mutex_ready; /* The mutex and the condition variable are usable (written once, under pthread_once) */ unsigned char mutex_in_use; unsigned char cb_list_dirty; - unsigned char thread_needs_join; /* Event thread was started and has not been collected yet */ + unsigned char thread_needs_join; /* Event thread was started and has not been joined yet, even after its epilogue */ unsigned char join_in_progress; /* A thread is currently joining the event thread (with the mutex released) */ unsigned char exiting; /* hid_exit() is tearing the hotplug machinery down */ @@ -602,15 +604,7 @@ static struct hid_hotplug_context { /* Written by the event thread before the startup barrier, read by the registering thread after it (the barrier is the synchronization edge) */ unsigned char startup_ok; - - /* Identity of the running hotplug event thread. Published by that thread as - its first action and cleared in its epilogue, so it is valid exactly while - an event thread exists. Guarded by global_error_mutex (a leaf mutex), NOT - the hotplug mutex: the global-error writer consults it while holding - global_error_mutex and must never take the hotplug mutex it may already - hold. Read only via hid_internal_on_event_thread(). */ - pthread_t event_thread_id; - unsigned char event_thread_id_valid; + const char *startup_error; /* Static reason string, published with startup_ok */ /* Linked list of the hotplug callbacks */ struct hid_hotplug_callback *hotplug_cbs; @@ -624,22 +618,23 @@ static struct hid_hotplug_context { can ever lock a mutex that hid_exit() destroyed underneath it */ static pthread_once_t hid_hotplug_init_once = PTHREAD_ONCE_INIT; +/* Compiler TLS keeps this marker through pthread-specific destructors without + comparing a pthread_t whose lifetime may have ended in a concurrent join. */ +static __thread unsigned char hid_hotplug_event_thread; + /* HIDAPI's public API contract (see hidapi.h) is that HIDAPI calls made from within a hotplug callback do not update the global error string: the callback runs on this internal event thread, and an application cannot serialize a hid_error(NULL) read against a write from that thread - that would be a - use-after-free of last_global_error_str. This mirrors the libusb and linux - backends, which likewise suppress such writes. A callback may re-enter the - public hid_hotplug_register_callback()/hid_hotplug_deregister_callback(), - whose success and failure paths both write the global error; those writes are - suppressed via this check in register_global_error()[_format](). - Returns non-zero when the caller is the hotplug event thread. Must be called - with global_error_mutex held (the event_thread_id* fields are guarded by it), - which the global-error writer already holds. */ + use-after-free of last_global_error_str. A callback may re-enter the public + hid_hotplug_register_callback()/hid_hotplug_deregister_callback(), whose + failure paths report global errors; those writes are suppressed via this + check in register_global_error()[_format](). + Returns non-zero when the caller is the hotplug event thread, including during + its thread-specific destructors. The thread-local read needs no lock. */ static int hid_internal_on_event_thread(void) { - return hid_hotplug_context.event_thread_id_valid - && pthread_equal(pthread_self(), hid_hotplug_context.event_thread_id); + return hid_hotplug_event_thread; } static void hid_internal_hotplug_remove_postponed(void) @@ -647,6 +642,7 @@ static void hid_internal_hotplug_remove_postponed(void) /* Unregister the callbacks whose removal was postponed */ /* This function is always called inside a locked mutex */ /* However, any actions are only allowed if the mutex is NOT in use and if the DIRTY flag is set */ + /* mutex_ready is defensive: callers must hold the initialized hotplug mutex. */ if (!hid_hotplug_context.mutex_ready || hid_hotplug_context.mutex_in_use || !hid_hotplug_context.cb_list_dirty) { return; } @@ -668,22 +664,19 @@ static void hid_internal_hotplug_remove_postponed(void) hid_hotplug_context.cb_list_dirty = 0; } -/* Releases everything that only the collector of the event thread may release. - Called with the hotplug mutex held, either by the thread that has just joined - the event thread, or by the event thread itself when nobody is joining it (it - then detaches itself - see hid_internal_hotplug_thread_epilogue()). +/* Releases the startup barrier and run loop references in the event thread's + epilogue, with the hotplug mutex held. Join ownership remains with the + collector until pthread_join() completes, including thread-specific destructors. Both participants have left the startup barrier by then: the registering thread holds the mutex across the barrier and releases it only afterwards, so acquiring the mutex proves it is out. */ static void hid_internal_hotplug_release_thread(void) { - hid_hotplug_context.thread_needs_join = 0; - pthread_barrier_destroy(&hid_hotplug_context.startup_barrier); - /* The run loop sources are created by the event thread, but the thread does - not release them while it winds down: the references must stay valid so - that the run loop can still be woken up until the thread is collected. */ + /* Only the epilogue releases these references, after their last event-thread + use. Cleanup reads them under this same mutex; NULL records their release, + not completion of the still-joinable thread. */ if (hid_hotplug_context.source) { CFRelease(hid_hotplug_context.source); hid_hotplug_context.source = NULL; @@ -695,12 +688,12 @@ static void hid_internal_hotplug_release_thread(void) hid_hotplug_context.run_loop = NULL; } -/* Collects (joins) the event thread once it has been told to stop, and releases - what only the collector may release. Serializes concurrent joiners and waits +/* Collects (joins) the event thread once it has been told to stop, and clears + its join ownership. Serializes concurrent joiners and waits out a join running on another thread. Must be called with the hotplug mutex NOT held by the calling thread, except from the event thread itself, where it is a guaranteed no-op (the - pthread_equal() check below) - that is what keeps pthread_cond_wait() from + hid_internal_on_event_thread() check below) - that keeps pthread_cond_wait() from ever being reached with the recursive mutex locked more than once. */ static void hid_internal_hotplug_collect_thread(void) { @@ -709,7 +702,7 @@ static void hid_internal_hotplug_collect_thread(void) while (hid_hotplug_context.thread_needs_join && hid_hotplug_context.hotplug_cbs == NULL && hid_hotplug_context.thread_state == 2 - && !pthread_equal(pthread_self(), hid_hotplug_context.thread)) { + && !hid_internal_on_event_thread()) { if (hid_hotplug_context.join_in_progress) { /* Another thread is already joining: wait for it to finish. A condition variable (and not a spin) is essential: the joiner is @@ -730,7 +723,7 @@ static void hid_internal_hotplug_collect_thread(void) pthread_mutex_lock(&hid_hotplug_context.mutex); hid_hotplug_context.join_in_progress = 0; - hid_internal_hotplug_release_thread(); + hid_hotplug_context.thread_needs_join = 0; /* Wake the threads waiting for this join to complete */ pthread_cond_broadcast(&hid_hotplug_context.join_done); @@ -742,6 +735,7 @@ static void hid_internal_hotplug_collect_thread(void) /* Must be called with the hotplug mutex held */ static void hid_internal_hotplug_cleanup(void) { + /* mutex_ready is defensive: callers must hold the initialized hotplug mutex. */ if (!hid_hotplug_context.mutex_ready || hid_hotplug_context.mutex_in_use) { return; } @@ -767,9 +761,8 @@ static void hid_internal_hotplug_cleanup(void) hid_hotplug_context.thread_state = 2; /* Wake up the run thread's event loop so that the thread can exit. - Both references are still alive: they are only released once the - thread has been collected, which cannot happen while this thread - holds the mutex. */ + Both references are still alive: the epilogue releases them under + this mutex, after thread_state has reached 2. */ if (hid_hotplug_context.source != NULL && hid_hotplug_context.run_loop != NULL) { CFRunLoopSourceSignal(hid_hotplug_context.source); CFRunLoopWakeUp(hid_hotplug_context.run_loop); @@ -878,7 +871,8 @@ static void hid_internal_hotplug_exit_done(void) pthread_mutex_lock(&hid_hotplug_context.mutex); - /* The event thread has been collected by now, so it no longer uses the mode */ + /* The event thread has been joined, including its thread-specific + destructors; releasing its resources in the epilogue alone is not enough. */ if (hid_hotplug_context.run_loop_mode) { CFRelease(hid_hotplug_context.run_loop_mode); hid_hotplug_context.run_loop_mode = NULL; @@ -908,12 +902,10 @@ int HID_API_EXPORT hid_init(void) int HID_API_EXPORT hid_exit(void) { - /* The hotplug thread and the callbacks are stopped/freed unconditionally: - hid_hotplug_register_callback() may have initialized the library implicitly - without ever creating hid_mgr. - This leaves the hotplug API closed (`exiting`), so that a concurrent - registration cannot re-enter hid_init() while hid_mgr is being destroyed - below */ + /* Hotplug synchronization state exists independently of hid_mgr; teardown + is safe even when nothing was initialized. Set `exiting` under the hotplug + mutex before destroying hid_mgr, so registration cannot re-enter hid_init() + during the teardown below. */ hid_internal_hotplug_exit(); if (hid_mgr) { @@ -1036,9 +1028,11 @@ static struct hid_device_info *create_device_info_with_usage(IOHIDDeviceRef dev, 9+1+20+1=31 bytes buffer, but allocate 32 for simple alignment */ const size_t path_len = 32; cur_dev->path = (char *) calloc(1, path_len); - if (cur_dev->path != NULL) { - snprintf(cur_dev->path, path_len, "DevSrvsID:%llu", entry_id); + if (cur_dev->path == NULL) { + hid_free_enumeration(cur_dev); + return NULL; } + snprintf(cur_dev->path, path_len, "DevSrvsID:%llu", entry_id); } if (cur_dev->path == NULL) { @@ -1056,6 +1050,12 @@ static struct hid_device_info *create_device_info_with_usage(IOHIDDeviceRef dev, get_product_string(dev, buf, BufLen); cur_dev->product_string = dup_wcs(buf); + if (!cur_dev->path || !cur_dev->serial_number + || !cur_dev->manufacturer_string || !cur_dev->product_string) { + hid_free_enumeration(cur_dev); + return NULL; + } + /* VID/PID */ cur_dev->vendor_id = dev_vid; cur_dev->product_id = dev_pid; @@ -1102,7 +1102,8 @@ static struct hid_device_info *create_device_info_with_usage(IOHIDDeviceRef dev, return cur_dev; } -static struct hid_device_info *create_device_info(IOHIDDeviceRef device) +/* Hotplug requires every usage entry; ordinary enumeration is best effort. */ +static struct hid_device_info *create_device_info(IOHIDDeviceRef device, int strict) { const int32_t primary_usage_page = get_int_property(device, CFSTR(kIOHIDPrimaryUsagePageKey)); const int32_t primary_usage = get_int_property(device, CFSTR(kIOHIDPrimaryUsageKey)); @@ -1139,6 +1140,10 @@ static struct hid_device_info *create_device_info(IOHIDDeviceRef device) continue; /* Already added. */ next = create_device_info_with_usage(device, usage_page, usage); + if (next == NULL && strict) { + hid_free_enumeration(root); + return NULL; + } cur->next = next; if (next != NULL) { cur = next; @@ -1208,7 +1213,7 @@ struct hid_device_info HID_API_EXPORT *hid_enumerate(unsigned short vendor_id, continue; } - struct hid_device_info *tmp = create_device_info(dev); + struct hid_device_info *tmp = create_device_info(dev, 0); if (tmp == NULL) { continue; } @@ -1348,7 +1353,10 @@ static void hid_internal_invoke_callbacks(struct hid_device_info *info, hid_hotp a callback registered from within a callback must not receive the in-flight event - its HID_API_HOTPLUG_ENUMERATE snapshot (taken at registration) and the subsequent events cover it with no losses or - duplicates. The list is append-only while mutex_in_use is set. */ + duplicates. While mutex_in_use is set, the list grows only at the tail, + except that a failed registration can unlink its own new tail before any + traversal reaches it (see the snapshot rollback in + hid_hotplug_register_callback()). */ struct hid_hotplug_callback *stop_after = hid_hotplug_context.hotplug_cbs; while (stop_after != NULL && stop_after->next != NULL) { stop_after = stop_after->next; @@ -1366,8 +1374,8 @@ static void hid_internal_invoke_callbacks(struct hid_device_info *info, hid_hotp if ((callback->events & event) && hid_internal_match_device_id(info->vendor_id, info->product_id, callback->vendor_id, callback->product_id)) { int result = callback->callback(callback->handle, info, event, callback->user_data); - /* If the result is non-zero, we mark the callback for removal */ - /* Do not use the deregister call as it locks the mutex, and we are currently in a lock */ + /* Traversal is active, so mark for postponed removal inline. Public + deregistration would do the same, plus redundant lookup and bookkeeping. */ if (result) { callback->events = 0; hid_hotplug_context.cb_list_dirty = 1; @@ -1388,7 +1396,10 @@ static void hid_internal_invoke_callbacks(struct hid_device_info *info, hid_hotp The entries of the cache are allocated as struct hid_device_info_ex and carry the io_service_t of the device: the path cannot be regenerated once the device is gone. Never pass an entry that did not come from the cache (see - hid_internal_copy_device_info()). */ + hid_internal_copy_device_info()). Comparison uses task-local io_service_t + port names, not registry-entry IDs. The manager-owned IOHIDDevice must retain + its service right until removal or teardown evicts every corresponding cache + entry, preventing port-name reuse while entries are live. */ static int match_ref_to_info(IOHIDDeviceRef device, struct hid_device_info *info) { if (!device || !info) { @@ -1451,8 +1462,8 @@ static void hid_internal_hotplug_connect_callback(void *context, IOReturn result pthread_mutex_lock(&hid_hotplug_context.mutex); } - /* Once the run loop runs, the IOHIDManager re-reports every device that was - already connected when it was opened. Those devices are all in the cache - + /* Once the run loop runs, the IOHIDManager reports devices that were already + connected when matching was set up. Those devices are all in the cache - it is completed synchronously during the thread's startup, before any callback can be registered - so they are NOT new arrivals and must never be dispatched as live events. This is what makes the snapshot boundary @@ -1465,7 +1476,7 @@ static void hid_internal_hotplug_connect_callback(void *context, IOReturn result return; } - info = create_device_info(device); + info = create_device_info(device, 1); if (!info) { /* Out of memory on the live-arrival path: the device ends up neither in the cache nor in an event, so "reported exactly once" is best effort @@ -1478,6 +1489,9 @@ static void hid_internal_hotplug_connect_callback(void *context, IOReturn result to fail or retry against. (During the startup phase the device is still picked up by hid_internal_hotplug_build_device_cache(), which does fail loudly if it cannot allocate either.) */ + /* An uncached live device is also absent from later ENUMERATE snapshots + and produces no LEFT callback until a successful arrival caches it, + normally after replug or monitor restart. */ if (!startup) { pthread_mutex_unlock(&hid_hotplug_context.mutex); } @@ -1602,23 +1616,20 @@ static void hotplug_replay_callback(void* context) pthread_mutex_unlock(&hid_hotplug_context.mutex); } -/* Lets the hotplug IOHIDManager process the device-matching events it queued for - the already connected devices, exactly like the process_pending_events() call +/* Gives the hotplug IOHIDManager a short best-effort pump of queued matching + events for connected devices, like the process_pending_events() call hid_enumerate() makes before IOHIDManagerCopyDevices(). This is NOT the snapshot boundary - hid_internal_hotplug_build_device_cache() - below is - and nothing depends on it draining the burst completely: it can - only ADD devices to the cache, never move one from the initial snapshot to the - live events. It runs on the event thread during its startup phase, so the - connect/disconnect callbacks it triggers only maintain the cache and dispatch + below is - and nothing depends on it draining the burst completely. It may + add cache entries and remove devices disconnected meanwhile. It runs on the + event thread during startup, so the connect/disconnect callbacks it triggers + only maintain the cache and dispatch nothing (no callback is registered yet, and the mutex must not be taken - see the locking note at the top of the hotplug code). */ static void hid_internal_hotplug_drain_pending_events(void) { - SInt32 res; - do { - res = CFRunLoopRunInMode(hid_hotplug_context.run_loop_mode, 0.001, FALSE); - } while (res != kCFRunLoopRunFinished && res != kCFRunLoopRunTimedOut && res != kCFRunLoopRunStopped); + CFRunLoopRunInMode(hid_hotplug_context.run_loop_mode, 0.001, FALSE); } /* Completes the initial device cache from the devices the hotplug IOHIDManager @@ -1626,9 +1637,10 @@ static void hid_internal_hotplug_drain_pending_events(void) before any callback can be registered and without the mutex. This is the deterministic boundary between "was already connected" and - "arrived live": IOHIDManagerCopyDevices() answers synchronously - which is - exactly what hid_enumerate() relies on - so, unlike a timed pump of the run - loop, the completeness of the snapshot does not depend on how long the initial + "arrived live": IOHIDManagerSetDeviceMatching() populates the manager's device + set, which IOHIDManagerCopyDevices() copies synchronously - as hid_enumerate() + relies on - so, unlike a timed pump of the run loop, the completeness of the + snapshot does not depend on how long the initial matching burst takes. Every device connected at this point ends up in the cache; when the run loop later delivers the matching events for those same devices, they are recognized as already known and dropped (see @@ -1688,7 +1700,7 @@ static int hid_internal_hotplug_build_device_cache(void) continue; } - info = create_device_info(device_array[i]); + info = create_device_info(device_array[i], 1); if (info == NULL) { /* Out of memory: fail the startup rather than commit a snapshot that is missing a connected device (it would later be reported as @@ -1718,58 +1730,30 @@ static int hid_internal_hotplug_build_device_cache(void) return 0; } -/* Runs at the very end of the event thread. If no other thread is joining it, - the thread detaches itself and releases its own resources here: otherwise a - callback that deregisters the last callback from within a callback (including - by returning non-zero) would leave an unjoined thread, two run loop sources - and the run loop behind until the next register/deregister/hid_exit() - which - may never come. */ +/* Releases the event thread's resources after their last use. Self-removal + leaves the thread joinable: the next application-thread collector or hid_exit() + must wait for actual termination before the thread record can be reused. */ static void hid_internal_hotplug_thread_epilogue(void) { pthread_mutex_lock(&hid_hotplug_context.mutex); - /* The event thread is exiting: stop suppressing global-error writes for its - pthread id. Cleared under the hotplug mutex - before the thread is detached - or collected, and thus before any replacement event thread can be started - and publish its own id - so a later thread's id can never be clobbered. - Ordering is hotplug mutex -> global_error_mutex, the same order the - global-error writer uses when it is called under the hotplug mutex. */ - pthread_mutex_lock(&global_error_mutex); - hid_hotplug_context.event_thread_id_valid = 0; - pthread_mutex_unlock(&global_error_mutex); - - if (hid_hotplug_context.thread_needs_join && !hid_hotplug_context.join_in_progress) { - /* Nobody is inside pthread_join() on this thread, and nobody can enter - it any more: the decision is taken under the mutex on both sides (see - hid_internal_hotplug_collect_thread()), so there is no double join and - no join of a detached thread. */ - pthread_detach(pthread_self()); - hid_internal_hotplug_release_thread(); - pthread_cond_broadcast(&hid_hotplug_context.join_done); - } + hid_internal_hotplug_release_thread(); - /* Past this point the thread must not touch the context any more: as soon as - the mutex is released, a new event thread may be started */ + /* The collector still owns the thread record until pthread_join() returns. */ pthread_mutex_unlock(&hid_hotplug_context.mutex); } static void* hotplug_thread(void* user_data) { - int manager_opened = 0; - (void) user_data; - /* Publish this thread's identity as the very first action, before anything + /* Mark this event thread as the very first action, before anything here can attempt a global-error write, so that any such write on this internal event thread - notably from a user callback that re-enters hid_hotplug_(de)register_callback() - is suppressed (see - hid_internal_on_event_thread()). Uses global_error_mutex only: the hotplug - mutex must not be taken during the startup phase (the registrant holds it, - parked at the startup barrier). */ - pthread_mutex_lock(&global_error_mutex); - hid_hotplug_context.event_thread_id = pthread_self(); - hid_hotplug_context.event_thread_id_valid = 1; - pthread_mutex_unlock(&global_error_mutex); + hid_internal_on_event_thread()). The marker remains set through pthread + destructors and is private to this thread; no lock is needed. */ + hid_hotplug_event_thread = 1; /* Startup phase: the registering thread holds the hotplug mutex and is parked at the startup barrier, so this thread has exclusive access to the @@ -1778,9 +1762,8 @@ static void* hotplug_thread(void* user_data) can be dispatched here either: none is registered yet (the first one is inserted only after the barrier). */ - /* The device cache is empty at this point: the event thread is only ever - started with no callbacks registered, which is exactly when the previous - cache was freed by hid_internal_hotplug_cleanup() */ + /* The cache should already be empty after the previous generation's + hid_internal_hotplug_cleanup(); clear it defensively before startup. */ hid_free_enumeration(hid_hotplug_context.devs); hid_hotplug_context.devs = NULL; @@ -1794,6 +1777,12 @@ static void* hotplug_thread(void* user_data) if (hid_hotplug_context.run_loop_mode) { hid_hotplug_context.manager = IOHIDManagerCreate(kCFAllocatorDefault, kIOHIDOptionsTypeNone); + if (!hid_hotplug_context.manager) { + hid_hotplug_context.startup_error = "failed to create the HID manager"; + } + } + else { + hid_hotplug_context.startup_error = "failed to create the run loop mode"; } if (hid_hotplug_context.manager) { @@ -1835,24 +1824,25 @@ static void* hotplug_thread(void* user_data) hid_internal_hotplug_disconnect_callback, NULL); - /* Opening the manager enqueues the device-matching events for all - the devices that are already connected */ - if (IOHIDManagerOpen(hid_hotplug_context.manager, kIOHIDOptionsTypeNone) == kIOReturnSuccess) { - manager_opened = 1; - - /* Give the manager a chance to process what it just enqueued - (best effort; not a fence - see the function comment) ... */ - hid_internal_hotplug_drain_pending_events(); - - /* ... and then take the authoritative snapshot of the connected - devices synchronously: THIS - and not a timed pump of the run - loop - is the boundary between the initial - HID_API_HOTPLUG_ENUMERATE pass and the live events */ - if (hid_internal_hotplug_build_device_cache() == 0) { - hid_hotplug_context.startup_ok = 1; - } + /* Give the manager a chance to process the matching events + (best effort; not a fence - see the function comment) ... */ + hid_internal_hotplug_drain_pending_events(); + + /* ... and then take the authoritative snapshot of the connected + devices synchronously: THIS - and not a timed pump of the run + loop - is the boundary between the initial + HID_API_HOTPLUG_ENUMERATE pass and the live events */ + if (hid_internal_hotplug_build_device_cache() == 0) { + hid_hotplug_context.startup_ok = 1; + } + else { + hid_hotplug_context.startup_error = "failed to allocate the device cache"; } } + else { + hid_hotplug_context.startup_error = !hid_hotplug_context.source + ? "failed to create the stop source" : "failed to create the replay source"; + } } /* Hand the startup result over to hid_hotplug_register_callback(), which is @@ -1879,17 +1869,15 @@ static void* hotplug_thread(void* user_data) code = CFRunLoopRunInMode(hid_hotplug_context.run_loop_mode, 1000/*sec*/, FALSE); - if (code == kCFRunLoopRunTimedOut || code == kCFRunLoopRunHandledSource) { + if (code == kCFRunLoopRunTimedOut) { continue; } - /* The run loop is gone: either the stop source stopped it - (thread_state is already 2), or it exited on its own. Publish the - shutdown under the mutex, so that a concurrent registration cannot - observe a running thread (thread_state 1) and signal-and-wake a run - loop that is winding down; a registration that finds the thread - stopped while callbacks are still registered fails instead of - attaching to a dead thread. */ + /* Publish the stopped state under the mutex. Registration is rejected + after publication while callbacks remain. An unsolicited stop can + leave existing and just-accepted callbacks without delivery, + including pending replay: a registration may still have observed + state 1 between the run loop's return and this publication. */ pthread_mutex_lock(&hid_hotplug_context.mutex); hid_hotplug_context.thread_state = 2; pthread_mutex_unlock(&hid_hotplug_context.mutex); @@ -1897,20 +1885,13 @@ static void* hotplug_thread(void* user_data) } } /* else: the startup failed - hid_hotplug_register_callback() fails the - registration and collects this thread (or lets it detach itself below); - the run loop sources (if any got created) and the startup barrier are - released by whoever collects it */ - - /* Kill the manager. No mutex is needed (and none may be held across - IOHIDManagerClose()): nothing else ever touches the manager, and no other - thread may start a new event thread or release the run loop mode before - this thread has been collected - which cannot happen before the epilogue - below, i.e. after the last use of the run loop and of its mode here. */ - if (hid_hotplug_context.manager) { - if (manager_opened) { - IOHIDManagerClose(hid_hotplug_context.manager, kIOHIDOptionsTypeNone); - } + registration and joins this thread after its epilogue releases the run + loop sources (if any got created) and the startup barrier */ + /* Release the manager. No mutex is needed: nothing else ever touches it, + and no other thread may start a new event thread or release the run loop + mode before this thread has been joined, after its last use of them here. */ + if (hid_hotplug_context.manager) { IOHIDManagerUnscheduleFromRunLoop(hid_hotplug_context.manager, hid_hotplug_context.run_loop, hid_hotplug_context.run_loop_mode); CFRelease(hid_hotplug_context.manager); @@ -1965,13 +1946,11 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven that it cannot race hid_exit() destroying hid_mgr (hid_exit() keeps `exiting` set across the whole of its teardown). NOTE: hid_init() schedules the global IOHIDManager on the run loop of the - CURRENT thread. When the implicit initialization happens here, that is the - registering thread rather than the thread that later calls hid_enumerate() - or hid_open() - those pump their own run loop, so they no longer service - the manager's run loop source. They do not depend on it (the manager - answers IOHIDManagerCopyDevices() synchronously), but an application that - wants the classic behavior should call hid_init() explicitly, from the - thread it uses HIDAPI on, before registering a hotplug callback. */ + CURRENT thread. Implicit initialization makes the registering thread the + hid_init() owner: on macOS it must remain alive until all devices are closed + and hid_exit() has run, and hid_exit() must run on that same thread. + Applications registering from a transient worker must call hid_init() + first from their intended long-lived owner thread. */ if (!hid_mgr && hid_init() != 0) { /* register_global_error: global error is already set by hid_init */ pthread_mutex_unlock(&hid_hotplug_context.mutex); @@ -1998,9 +1977,17 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven /* If a stopped event thread has not been collected (joined) yet, collect it before the machinery can be restarted; the join must not happen with the mutex held, so drop the mutex for the collection and re-check. - Never entered on the event thread itself: the list cannot be empty - while a callback dispatch is in flight. */ + The list cannot be empty while a callback dispatch is in flight, but + thread-specific destructors may re-enter after the epilogue. */ while (hid_hotplug_context.hotplug_cbs == NULL && hid_hotplug_context.thread_needs_join) { + if (hid_internal_on_event_thread()) { + /* A destructor cannot join itself or start a replacement generation. */ + register_global_error("hid_hotplug_register_callback: the hotplug event thread is stopping"); + pthread_mutex_unlock(&hid_hotplug_context.mutex); + free(hotplug_cb); + return -1; + } + /* Make sure the stop was actually requested (idempotent) */ hid_internal_hotplug_cleanup(); pthread_mutex_unlock(&hid_hotplug_context.mutex); @@ -2062,6 +2049,7 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven reports its result in startup_ok, which is published here instead. */ hid_hotplug_context.thread_state = 0; hid_hotplug_context.startup_ok = 0; + hid_hotplug_context.startup_error = NULL; hid_hotplug_context.startup_phase = 1; if (pthread_create(&hid_hotplug_context.thread, NULL, hotplug_thread, NULL) != 0) { @@ -2075,8 +2063,7 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven hid_hotplug_context.thread_needs_join = 1; - /* Wait for the thread to finish setting up - without it the callback may be registered too early*/ - + /* Wait for the thread to finish setting up - without it the callback may be registered too early */ pthread_barrier_wait(&hid_hotplug_context.startup_barrier); /* Publish the thread's startup result */ @@ -2085,7 +2072,7 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven if (!hid_hotplug_context.startup_ok) { /* The thread failed to set up the device monitoring and is exiting: it must be collected (joined) with the mutex released */ - register_global_error("hid_hotplug_register_callback: failed to start the device monitoring"); + register_global_error_format("hid_hotplug_register_callback: %s", hid_hotplug_context.startup_error); /* Free whatever the thread may have cached before it failed (the callback list is empty, so this also stops nothing and @@ -2149,7 +2136,9 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven } if (hotplug_cb->replay != NULL && hid_hotplug_context.thread_state == 1) { - /* Ask the event thread to deliver the initial pass */ + /* Ask the event thread to deliver the initial pass. The state check + is defensive: startup or the earlier running-state check established + state 1 while this mutex has remained held. */ CFRunLoopSourceSignal(hid_hotplug_context.replay_source); CFRunLoopWakeUp(hid_hotplug_context.run_loop); } @@ -2161,9 +2150,6 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven *callback_handle = hotplug_cb->handle; } - /* Clear the stale global error on success, like hid_init()/hid_enumerate() do */ - register_global_error(NULL); - pthread_mutex_unlock(&hid_hotplug_context.mutex); return 0; @@ -2230,8 +2216,8 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_call /* If this deregistration stopped the event thread, join it with the mutex released. A no-op when called from within a callback (the event thread - cannot join itself): the thread then detaches and releases itself in its - epilogue - see hid_internal_hotplug_thread_epilogue() */ + cannot join itself): its epilogue releases the resources, and the next + application-thread collector joins it before reusing the thread record. */ hid_internal_hotplug_collect_thread(); return result; @@ -2922,7 +2908,7 @@ HID_API_EXPORT struct hid_device_info *HID_API_CALL hid_get_device_info(hid_devi register_device_error(dev, NULL); } else { - dev->device_info = create_device_info(dev->device_handle); + dev->device_info = create_device_info(dev->device_handle, 0); if (!dev->device_info) { register_device_error(dev, "Failed to create hid_device_info"); } diff --git a/src/tests/CMakeLists.txt b/src/tests/CMakeLists.txt index ff17785c6..8a90cc620 100644 --- a/src/tests/CMakeLists.txt +++ b/src/tests/CMakeLists.txt @@ -1,6 +1,6 @@ -# Backend-generic HIDAPI (unit-)tests, run against a virtual HID device. +# Backend-generic device-I/O tests and macOS hotplug lifecycle tests. # -# The tests are written against the public HIDAPI API and the backend-agnostic +# The device-I/O tests use the public HIDAPI API and the backend-agnostic # test_virtual_device interface (test_virtual_device.h), so the same test runs # against any backend for which a virtual-device provider exists. Each provider # implements the pre-recorded "scenario" protocol: the test triggers a scenario @@ -85,4 +85,16 @@ if(APPLE AND TARGET hidapi_darwin) hidapi_add_vdev_test(DeviceIO_darwin test_virtual_device_mac.c hidapi_darwin) target_link_libraries(DeviceIO_darwin PRIVATE "-framework IOKit" "-framework CoreFoundation") + + add_executable(HotplugLifecycle_darwin test_hotplug_mac.c) + set_target_properties(HotplugLifecycle_darwin PROPERTIES + C_STANDARD 11 + C_STANDARD_REQUIRED TRUE + ) + target_link_libraries(HotplugLifecycle_darwin PRIVATE hidapi_darwin Threads::Threads) + if(HIDAPI_ENABLE_ASAN) + target_link_options(HotplugLifecycle_darwin PRIVATE -fsanitize=address) + endif() + add_test(NAME HotplugLifecycle_darwin COMMAND HotplugLifecycle_darwin) + set_tests_properties(HotplugLifecycle_darwin PROPERTIES TIMEOUT 60) endif() diff --git a/src/tests/test_hotplug_mac.c b/src/tests/test_hotplug_mac.c new file mode 100644 index 000000000..e517d0ab9 --- /dev/null +++ b/src/tests/test_hotplug_mac.c @@ -0,0 +1,289 @@ +/******************************************************* + HIDAPI - Multi-Platform library for + communication with HID devices. + + libusb/hidapi Team + + Copyright 2026. + + macOS hotplug lifecycle regression tests. + + The contents of this file may be used by anyone for any + reason without any conditions and may be used as a + starting point for your own applications which use HIDAPI. +********************************************************/ + +#include +#include +#include +#include +#include +#include + +#include + +#define WORKERS 4 +#define ITERATIONS 8 +#define CHECK(cond) do { \ + if (!(cond)) { \ + fprintf(stderr, "CHECK failed: %s (line %d)\n", #cond, __LINE__); \ + exit(EXIT_FAILURE); \ + } \ +} while (0) + +static struct timespec deadline_ms(int ms) +{ + struct timespec deadline; + CHECK(clock_gettime(CLOCK_REALTIME, &deadline) == 0); + deadline.tv_sec += ms / 1000; + deadline.tv_nsec += (long)(ms % 1000) * 1000000L; + if (deadline.tv_nsec >= 1000000000L) { + deadline.tv_sec++; + deadline.tv_nsec -= 1000000000L; + } + return deadline; +} + +/* All condition waits and API calls are bounded, including pthread_join: + alarm() terminates the standalone test if a library call deadlocks. */ +static void wait_flag(pthread_cond_t *cond, pthread_mutex_t *mutex, const int *flag) +{ + struct timespec deadline = deadline_ms(5000); + while (!*flag) + CHECK(pthread_cond_timedwait(cond, mutex, &deadline) == 0); +} + +static int HID_API_CALL keep_callback(hid_hotplug_callback_handle handle, + struct hid_device_info *device, hid_hotplug_event event, void *user_data) +{ + (void)handle; + (void)device; + (void)event; + (void)user_data; + return 0; +} + +static hid_hotplug_callback_handle register_quiet(void) +{ + hid_hotplug_callback_handle handle = 0; + CHECK(hid_hotplug_register_callback(0, 0, HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED, + 0, keep_callback, NULL, &handle) == 0); + CHECK(handle > 0); + return handle; +} + +struct worker_gate { + pthread_mutex_t mutex; + pthread_cond_t cond; + int arrived; + int generation; +}; + +static void wait_workers(struct worker_gate *gate) +{ + struct timespec deadline = deadline_ms(5000); + int generation; + CHECK(pthread_mutex_lock(&gate->mutex) == 0); + generation = gate->generation; + if (++gate->arrived == WORKERS) { + gate->arrived = 0; + gate->generation++; + CHECK(pthread_cond_broadcast(&gate->cond) == 0); + } else { + while (generation == gate->generation) + CHECK(pthread_cond_timedwait(&gate->cond, &gate->mutex, &deadline) == 0); + } + CHECK(pthread_mutex_unlock(&gate->mutex) == 0); +} + +static void *registration_worker(void *arg) +{ + struct worker_gate *gate = (struct worker_gate *)arg; + int i; + for (i = 0; i < ITERATIONS; i++) { + hid_hotplug_callback_handle handle; + wait_workers(gate); + handle = register_quiet(); + wait_workers(gate); + CHECK(hid_hotplug_deregister_callback(handle) == 0); + } + return NULL; +} + +static void test_concurrent_registration(void) +{ + struct worker_gate gate = { PTHREAD_MUTEX_INITIALIZER, PTHREAD_COND_INITIALIZER, 0, 0 }; + pthread_t workers[WORKERS]; + int i; + puts("Concurrent register/deregister and competing collectors"); + for (i = 0; i < WORKERS; i++) + CHECK(pthread_create(&workers[i], NULL, registration_worker, &gate) == 0); + for (i = 0; i < WORKERS; i++) + CHECK(pthread_join(workers[i], NULL) == 0); + CHECK(pthread_cond_destroy(&gate.cond) == 0); + CHECK(pthread_mutex_destroy(&gate.mutex) == 0); +} + +enum callback_action { REMOVE_BY_RETURN, REMOVE_EXPLICITLY, WAIT_FOR_RELEASE, REMOVE_THEN_EXIT }; + +static pthread_key_t callback_key; + +struct callback_state { + pthread_mutex_t mutex; + pthread_cond_t cond; + enum callback_action action; + int entered; + int release; + int completed; + int deregister_started; + int deregister_done; + int destructor_completed; + hid_hotplug_callback_handle handle; +}; + +static void event_thread_destructor(void *arg) +{ + struct callback_state *state = (struct callback_state *)arg; + struct timespec delay = { 0, 100000000L }; + hid_hotplug_callback_handle handle = -1; + /* Resource release alone must not let a collector skip this destructor. */ + while (nanosleep(&delay, &delay) != 0) + CHECK(errno == EINTR); + CHECK(hid_hotplug_register_callback(0, 0, HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED, + 0, keep_callback, NULL, &handle) == -1); + CHECK(handle == 0); + CHECK(pthread_mutex_lock(&state->mutex) == 0); + state->destructor_completed = 1; + CHECK(pthread_mutex_unlock(&state->mutex) == 0); +} + +static int HID_API_CALL lifecycle_callback(hid_hotplug_callback_handle handle, + struct hid_device_info *device, hid_hotplug_event event, void *user_data) +{ + struct callback_state *state = (struct callback_state *)user_data; + (void)device; + CHECK(event == HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED); + if (state->action != WAIT_FOR_RELEASE) + CHECK(pthread_setspecific(callback_key, state) == 0); + if (state->action == REMOVE_EXPLICITLY) + CHECK(hid_hotplug_deregister_callback(handle) == 0); + CHECK(pthread_mutex_lock(&state->mutex) == 0); + if (state->action != WAIT_FOR_RELEASE) + CHECK(!state->entered); + state->entered = 1; + CHECK(pthread_cond_broadcast(&state->cond) == 0); + if (state->action == WAIT_FOR_RELEASE) + wait_flag(&state->cond, &state->mutex, &state->release); + state->completed = 1; + CHECK(pthread_mutex_unlock(&state->mutex) == 0); + return state->action == REMOVE_BY_RETURN || state->action == REMOVE_THEN_EXIT; +} + +static void *deregister_worker(void *arg) +{ + struct callback_state *state = (struct callback_state *)arg; + CHECK(pthread_mutex_lock(&state->mutex) == 0); + state->deregister_started = 1; + CHECK(pthread_cond_broadcast(&state->cond) == 0); + CHECK(pthread_mutex_unlock(&state->mutex) == 0); + CHECK(hid_hotplug_deregister_callback(state->handle) == 0); + CHECK(pthread_mutex_lock(&state->mutex) == 0); + CHECK(state->release && state->completed); + state->deregister_done = 1; + CHECK(pthread_cond_broadcast(&state->cond) == 0); + CHECK(pthread_mutex_unlock(&state->mutex) == 0); + return NULL; +} + +static void test_device_callback(enum callback_action action, const char *name) +{ + struct callback_state state = { PTHREAD_MUTEX_INITIALIZER, PTHREAD_COND_INITIALIZER, + action, 0, 0, 0, 0, 0, 0, 0 }; + struct hid_device_info *devices = hid_enumerate(0, 0); + unsigned short vendor_id, product_id; + + /* Enumeration and init/exit stay on the owner thread, outside worker calls. */ + if (!devices) { + printf("SKIP: %s (no HID device available)\n", name); + } else { + vendor_id = devices->vendor_id; + product_id = devices->product_id; + hid_free_enumeration(devices); + printf("%s (VID %04hx, PID %04hx)\n", name, vendor_id, product_id); + CHECK(hid_hotplug_register_callback(vendor_id, product_id, + HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED, HID_API_HOTPLUG_ENUMERATE, + lifecycle_callback, &state, &state.handle) == 0); + CHECK(pthread_mutex_lock(&state.mutex) == 0); + wait_flag(&state.cond, &state.mutex, &state.entered); + CHECK(pthread_mutex_unlock(&state.mutex) == 0); + + if (action == WAIT_FOR_RELEASE) { + pthread_t worker; + struct timespec deadline; + int result = 0; + CHECK(pthread_create(&worker, NULL, deregister_worker, &state) == 0); + CHECK(pthread_mutex_lock(&state.mutex) == 0); + wait_flag(&state.cond, &state.mutex, &state.deregister_started); + /* Give the external call time to block while the callback is held. */ + deadline = deadline_ms(100); + while (!state.deregister_done && result == 0) + result = pthread_cond_timedwait(&state.cond, &state.mutex, &deadline); + CHECK(result == ETIMEDOUT && !state.deregister_done); + state.release = 1; + CHECK(pthread_cond_broadcast(&state.cond) == 0); + wait_flag(&state.cond, &state.mutex, &state.deregister_done); + CHECK(pthread_mutex_unlock(&state.mutex) == 0); + CHECK(pthread_join(worker, NULL) == 0); + } else { + /* The API mutex waits for the self-removing callback to return; + collection must also wait for its thread-specific destructor. */ + hid_hotplug_callback_handle restarted = 0; + if (action == REMOVE_THEN_EXIT) + CHECK(hid_exit() == 0); + else + restarted = register_quiet(); + CHECK(pthread_mutex_lock(&state.mutex) == 0); + CHECK(state.destructor_completed); + CHECK(pthread_mutex_unlock(&state.mutex) == 0); + if (action == REMOVE_THEN_EXIT) { + CHECK(hid_init() == 0); + } else { + CHECK(hid_hotplug_deregister_callback(restarted) == 0); + CHECK(hid_hotplug_deregister_callback(state.handle) == -1); + } + } + } + CHECK(pthread_cond_destroy(&state.cond) == 0); + CHECK(pthread_mutex_destroy(&state.mutex) == 0); +} + +int main(void) +{ + int i; + setvbuf(stdout, NULL, _IONBF, 0); + alarm(45); + /* macOS requires the initializing thread to remain alive through hid_exit. */ + CHECK(hid_init() == 0); + test_concurrent_registration(); + puts("Immediate restart after last deregistration"); + for (i = 0; i < ITERATIONS; i++) { + hid_hotplug_callback_handle handle = register_quiet(); + CHECK(hid_hotplug_deregister_callback(handle) == 0); + } + CHECK(pthread_key_create(&callback_key, event_thread_destructor) == 0); + test_device_callback(REMOVE_BY_RETURN, "Last callback removal by return value and restart"); + test_device_callback(REMOVE_EXPLICITLY, "Last callback explicit deregistration and restart"); + test_device_callback(REMOVE_THEN_EXIT, "Last callback removal followed by owner-thread hid_exit"); + test_device_callback(WAIT_FOR_RELEASE, "External deregistration waits for an in-flight callback"); + CHECK(pthread_key_delete(callback_key) == 0); + CHECK(hid_exit() == 0); + puts("Repeated owner-thread hid_init/register/hid_exit"); + for (i = 0; i < ITERATIONS; i++) { + CHECK(hid_init() == 0); + (void)register_quiet(); + CHECK(hid_exit() == 0); + } + alarm(0); + puts("Hotplug lifecycle tests passed"); + return EXIT_SUCCESS; +} From 1c846637fbb338b272fc8dc6ce5a9c020e708bb2 Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Tue, 8 Sep 2026 22:55:16 +0300 Subject: [PATCH 08/10] Fix macOS hotplug destructor thread identification mac-r2-1: Retain event identity and join ownership through teardown. Assisted-by: codex-cli:gpt-6-astra --- mac/hid.c | 103 +++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 75 insertions(+), 28 deletions(-) diff --git a/mac/hid.c b/mac/hid.c index a02ff5f76..7dcb963da 100644 --- a/mac/hid.c +++ b/mac/hid.c @@ -264,8 +264,8 @@ static void register_error_str_vformat(wchar_t **error_str, const char *format, /* True when the calling thread is HIDAPI's internal hotplug event thread; used to suppress writes to the global error string made from that thread (see the - definition after the hotplug context for the full rationale). The thread-local - read needs no lock. */ + definition after the hotplug context for the full rationale). May take the + hotplug mutex, so call before taking global_error_mutex. */ static int hid_internal_on_event_thread(void); /* Serializes the mutations of the global error string: the hotplug API is @@ -280,15 +280,16 @@ static pthread_mutex_t global_error_mutex = PTHREAD_MUTEX_INITIALIZER; * Use register_global_error(NULL) to indicate "no error". */ static void register_global_error(const char *msg) { - pthread_mutex_lock(&global_error_mutex); /* Honor the cross-backend contract (see hidapi.h): a global-error write attempted on the internal hotplug event thread - e.g. from a hid_hotplug_(de)register_callback() call re-entered from within a user callback - must not touch the global error string. Per-device errors go through register_error_str() with a different target and are unaffected; only this process-global string is suppressed. */ - if (!hid_internal_on_event_thread()) - register_error_str(&last_global_error_str, msg); + if (hid_internal_on_event_thread()) + return; + pthread_mutex_lock(&global_error_mutex); + register_error_str(&last_global_error_str, msg); pthread_mutex_unlock(&global_error_mutex); } @@ -296,11 +297,12 @@ static void register_global_error(const char *msg) static void register_global_error_format(const char *format, ...) { va_list args; + /* See register_global_error(): suppressed on the internal event thread. */ + if (hid_internal_on_event_thread()) + return; va_start(args, format); pthread_mutex_lock(&global_error_mutex); - /* See register_global_error(): suppressed on the internal event thread. */ - if (!hid_internal_on_event_thread()) - register_error_str_vformat(&last_global_error_str, format, args); + register_error_str_vformat(&last_global_error_str, format, args); pthread_mutex_unlock(&global_error_mutex); va_end(args); } @@ -553,8 +555,9 @@ struct hid_device_info_ex pthread_cond_wait() only with exactly one recursion level held (see hid_internal_hotplug_collect_thread()). - The event-thread marker is thread-local. startup_phase is owned by the event - thread, and startup_ok/startup_error are published through the startup barrier. + The thread-local event-thread marker is only a fast-path hint. startup_phase + is owned by the event thread; startup_ok/startup_error/thread_id are published + through the startup barrier. Before that barrier, the registering thread holds the mutex and is parked there, so the event thread has exclusive access to the context and MUST NOT take the mutex there (that would deadlock against the parked @@ -568,6 +571,7 @@ static struct hid_hotplug_context { /* Thread and RunLoop for the manager to work in */ pthread_t thread; + uint64_t thread_id; /* OS thread ID; disambiguates pthread_t reuse during join */ CFRunLoopRef run_loop; CFRunLoopSourceRef source; CFRunLoopSourceRef replay_source; /* Delivers the initial HID_API_HOTPLUG_ENUMERATE pass of new registrations */ @@ -618,10 +622,12 @@ static struct hid_hotplug_context { can ever lock a mutex that hid_exit() destroyed underneath it */ static pthread_once_t hid_hotplug_init_once = PTHREAD_ONCE_INIT; -/* Compiler TLS keeps this marker through pthread-specific destructors without - comparing a pthread_t whose lifetime may have ended in a concurrent join. */ +/* Avoids locking during startup. Darwin may clear this TLS before application + thread-specific destructors, so a zero marker requires checking the record. */ static __thread unsigned char hid_hotplug_event_thread; +static int hid_internal_hotplug_init(void); + /* HIDAPI's public API contract (see hidapi.h) is that HIDAPI calls made from within a hotplug callback do not update the global error string: the callback runs on this internal event thread, and an application cannot serialize a @@ -631,10 +637,31 @@ static __thread unsigned char hid_hotplug_event_thread; failure paths report global errors; those writes are suppressed via this check in register_global_error()[_format](). Returns non-zero when the caller is the hotplug event thread, including during - its thread-specific destructors. The thread-local read needs no lock. */ + its thread-specific destructors. The fallback takes the hotplug mutex; the + thread record is retained until a successful join is published under it. */ static int hid_internal_on_event_thread(void) { - return hid_hotplug_event_thread; + int on_event_thread = 0; + uint64_t thread_id; + + if (hid_hotplug_event_thread) + return 1; + if (hid_internal_hotplug_init() != 0) + return 0; + + pthread_mutex_lock(&hid_hotplug_context.mutex); + if (hid_hotplug_context.thread_needs_join) { + /* pthread_join() can free the pthread_t before the collector reacquires + this mutex. During a join, compare only on the still-living event + thread, identified by its OS ID, never on a thread reusing its handle. */ + if (!hid_hotplug_context.join_in_progress + || (pthread_threadid_np(NULL, &thread_id) == 0 && thread_id == hid_hotplug_context.thread_id)) { + on_event_thread = pthread_equal(pthread_self(), hid_hotplug_context.thread); + } + } + pthread_mutex_unlock(&hid_hotplug_context.mutex); + + return on_event_thread; } static void hid_internal_hotplug_remove_postponed(void) @@ -666,7 +693,7 @@ static void hid_internal_hotplug_remove_postponed(void) /* Releases the startup barrier and run loop references in the event thread's epilogue, with the hotplug mutex held. Join ownership remains with the - collector until pthread_join() completes, including thread-specific destructors. + collector until pthread_join() succeeds, including thread-specific destructors. Both participants have left the startup barrier by then: the registering thread holds the mutex across the barrier and releases it only afterwards, so acquiring the mutex proves it is out. */ @@ -694,9 +721,12 @@ static void hid_internal_hotplug_release_thread(void) Must be called with the hotplug mutex NOT held by the calling thread, except from the event thread itself, where it is a guaranteed no-op (the hid_internal_on_event_thread() check below) - that keeps pthread_cond_wait() from - ever being reached with the recursive mutex locked more than once. */ -static void hid_internal_hotplug_collect_thread(void) + ever being reached with the recursive mutex locked more than once. + Returns the pthread_join() error, if any, without releasing the thread record. */ +static int hid_internal_hotplug_collect_thread(void) { + int result = 0; + pthread_mutex_lock(&hid_hotplug_context.mutex); while (hid_hotplug_context.thread_needs_join @@ -719,17 +749,21 @@ static void hid_internal_hotplug_collect_thread(void) mutex to finish an in-flight callback dispatch (issue #794 and the matching cross-thread deadlock). No new event thread can be started while thread_needs_join is set, so the thread handle is stable. */ - pthread_join(hid_hotplug_context.thread, NULL); + result = pthread_join(hid_hotplug_context.thread, NULL); pthread_mutex_lock(&hid_hotplug_context.mutex); hid_hotplug_context.join_in_progress = 0; - hid_hotplug_context.thread_needs_join = 0; + if (result == 0) + hid_hotplug_context.thread_needs_join = 0; /* Wake the threads waiting for this join to complete */ pthread_cond_broadcast(&hid_hotplug_context.join_done); + if (result != 0) + break; } pthread_mutex_unlock(&hid_hotplug_context.mutex); + return result; } /* Must be called with the hotplug mutex held */ @@ -827,14 +861,14 @@ static int hid_internal_hotplug_init(void) that a concurrent hid_hotplug_register_callback()/hid_hotplug_deregister_callback() fails instead of racing the rest of hid_exit(); hid_internal_hotplug_exit_done() clears it once hid_exit() is finished. */ -static void hid_internal_hotplug_exit(void) +static int hid_internal_hotplug_exit(void) { struct hid_hotplug_callback **current; if (hid_internal_hotplug_init() != 0) { /* The hotplug mutex could not be created: nothing can ever have been registered, and there is nothing to tear down */ - return; + return 0; } pthread_mutex_lock(&hid_hotplug_context.mutex); @@ -854,12 +888,14 @@ static void hid_internal_hotplug_exit(void) pthread_mutex_unlock(&hid_hotplug_context.mutex); /* Join the stopped event thread, with the hotplug mutex released */ - hid_internal_hotplug_collect_thread(); + if (hid_internal_hotplug_collect_thread() != 0) + return -1; /* The hotplug mutex is deliberately NOT destroyed: another thread may be about to lock it (it only has to observe `exiting` afterwards), and destroying a mutex under it would be undefined behavior. It costs nothing to keep it for the lifetime of the process. */ + return 0; } /* Re-opens the hotplug API after hid_exit() has finished. */ @@ -906,7 +942,10 @@ int HID_API_EXPORT hid_exit(void) is safe even when nothing was initialized. Set `exiting` under the hotplug mutex before destroying hid_mgr, so registration cannot re-enter hid_init() during the teardown below. */ - hid_internal_hotplug_exit(); + if (hid_internal_hotplug_exit() != 0) { + register_global_error("hid_exit: failed to join the hotplug events thread"); + return -1; + } if (hid_mgr) { /* Close the HID manager. */ @@ -1739,7 +1778,7 @@ static void hid_internal_hotplug_thread_epilogue(void) hid_internal_hotplug_release_thread(); - /* The collector still owns the thread record until pthread_join() returns. */ + /* The collector still owns the thread record until pthread_join() succeeds. */ pthread_mutex_unlock(&hid_hotplug_context.mutex); } @@ -1751,9 +1790,10 @@ static void* hotplug_thread(void* user_data) here can attempt a global-error write, so that any such write on this internal event thread - notably from a user callback that re-enters hid_hotplug_(de)register_callback() - is suppressed (see - hid_internal_on_event_thread()). The marker remains set through pthread - destructors and is private to this thread; no lock is needed. */ + hid_internal_on_event_thread()). The marker avoids locking before the + startup barrier; destructor re-entry can fall back to the thread record. */ hid_hotplug_event_thread = 1; + pthread_threadid_np(NULL, &hid_hotplug_context.thread_id); /* Startup phase: the registering thread holds the hotplug mutex and is parked at the startup barrier, so this thread has exclusive access to the @@ -1991,7 +2031,11 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven /* Make sure the stop was actually requested (idempotent) */ hid_internal_hotplug_cleanup(); pthread_mutex_unlock(&hid_hotplug_context.mutex); - hid_internal_hotplug_collect_thread(); + if (hid_internal_hotplug_collect_thread() != 0) { + register_global_error("hid_hotplug_register_callback: failed to join the hotplug events thread"); + free(hotplug_cb); + return -1; + } pthread_mutex_lock(&hid_hotplug_context.mutex); /* hid_exit() may have started while the mutex was released */ @@ -2218,7 +2262,10 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_call released. A no-op when called from within a callback (the event thread cannot join itself): its epilogue releases the resources, and the next application-thread collector joins it before reusing the thread record. */ - hid_internal_hotplug_collect_thread(); + if (hid_internal_hotplug_collect_thread() != 0) { + register_global_error("hid_hotplug_deregister_callback: failed to join the hotplug events thread"); + return -1; + } return result; } From a35f00a6287f3c51bbb7a6a2ac11def32f36cf65 Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Tue, 8 Sep 2026 23:12:15 +0300 Subject: [PATCH 09/10] Avoid locking for macOS hotplug thread identification mac-r3-1: Publish the OS thread ID atomically and test owner-lock calls. Assisted-by: codex-cli:gpt-6-astra --- mac/hid.c | 59 ++++++++++++++++-------------------- src/tests/test_hotplug_mac.c | 39 +++++++++++++++++++++--- 2 files changed, 61 insertions(+), 37 deletions(-) diff --git a/mac/hid.c b/mac/hid.c index 7dcb963da..03a387597 100644 --- a/mac/hid.c +++ b/mac/hid.c @@ -264,8 +264,7 @@ static void register_error_str_vformat(wchar_t **error_str, const char *format, /* True when the calling thread is HIDAPI's internal hotplug event thread; used to suppress writes to the global error string made from that thread (see the - definition after the hotplug context for the full rationale). May take the - hotplug mutex, so call before taking global_error_mutex. */ + definition after the hotplug context for the full rationale). Takes no lock. */ static int hid_internal_on_event_thread(void); /* Serializes the mutations of the global error string: the hotplug API is @@ -555,9 +554,10 @@ struct hid_device_info_ex pthread_cond_wait() only with exactly one recursion level held (see hid_internal_hotplug_collect_thread()). - The thread-local event-thread marker is only a fast-path hint. startup_phase - is owned by the event thread; startup_ok/startup_error/thread_id are published - through the startup barrier. + The thread-local event-thread marker is only a fast-path hint. The OS thread + ID is published atomically, so identity checks never need the hotplug mutex. + startup_phase is owned by the event thread; startup_ok/startup_error are + published through the startup barrier. Before that barrier, the registering thread holds the mutex and is parked there, so the event thread has exclusive access to the context and MUST NOT take the mutex there (that would deadlock against the parked @@ -571,7 +571,7 @@ static struct hid_hotplug_context { /* Thread and RunLoop for the manager to work in */ pthread_t thread; - uint64_t thread_id; /* OS thread ID; disambiguates pthread_t reuse during join */ + uint64_t thread_id; /* Atomic OS thread ID; retained until a successful join */ CFRunLoopRef run_loop; CFRunLoopSourceRef source; CFRunLoopSourceRef replay_source; /* Delivers the initial HID_API_HOTPLUG_ENUMERATE pass of new registrations */ @@ -622,12 +622,10 @@ static struct hid_hotplug_context { can ever lock a mutex that hid_exit() destroyed underneath it */ static pthread_once_t hid_hotplug_init_once = PTHREAD_ONCE_INIT; -/* Avoids locking during startup. Darwin may clear this TLS before application - thread-specific destructors, so a zero marker requires checking the record. */ +/* Fast-path hint. Darwin may clear this TLS before application thread-specific + destructors, so a zero marker requires checking the published OS thread ID. */ static __thread unsigned char hid_hotplug_event_thread; -static int hid_internal_hotplug_init(void); - /* HIDAPI's public API contract (see hidapi.h) is that HIDAPI calls made from within a hotplug callback do not update the global error string: the callback runs on this internal event thread, and an application cannot serialize a @@ -637,31 +635,22 @@ static int hid_internal_hotplug_init(void); failure paths report global errors; those writes are suppressed via this check in register_global_error()[_format](). Returns non-zero when the caller is the hotplug event thread, including during - its thread-specific destructors. The fallback takes the hotplug mutex; the - thread record is retained until a successful join is published under it. */ + its thread-specific destructors. The fallback reads the OS thread ID with + acquire/release atomics, independently of the hotplug mutex: an application + may hold its own serialization mutex while a callback waits for it. The ID + remains published until a successful join, and does not depend on pthread_t + storage that the join may already have freed. Compiler atomics also support + building this file as C++. */ static int hid_internal_on_event_thread(void) { - int on_event_thread = 0; - uint64_t thread_id; + uint64_t event_thread_id, thread_id; if (hid_hotplug_event_thread) return 1; - if (hid_internal_hotplug_init() != 0) - return 0; - - pthread_mutex_lock(&hid_hotplug_context.mutex); - if (hid_hotplug_context.thread_needs_join) { - /* pthread_join() can free the pthread_t before the collector reacquires - this mutex. During a join, compare only on the still-living event - thread, identified by its OS ID, never on a thread reusing its handle. */ - if (!hid_hotplug_context.join_in_progress - || (pthread_threadid_np(NULL, &thread_id) == 0 && thread_id == hid_hotplug_context.thread_id)) { - on_event_thread = pthread_equal(pthread_self(), hid_hotplug_context.thread); - } - } - pthread_mutex_unlock(&hid_hotplug_context.mutex); - return on_event_thread; + event_thread_id = __atomic_load_n(&hid_hotplug_context.thread_id, __ATOMIC_ACQUIRE); + return event_thread_id != 0 && pthread_threadid_np(NULL, &thread_id) == 0 + && thread_id == event_thread_id; } static void hid_internal_hotplug_remove_postponed(void) @@ -753,8 +742,10 @@ static int hid_internal_hotplug_collect_thread(void) pthread_mutex_lock(&hid_hotplug_context.mutex); hid_hotplug_context.join_in_progress = 0; - if (result == 0) + if (result == 0) { + __atomic_store_n(&hid_hotplug_context.thread_id, 0, __ATOMIC_RELEASE); hid_hotplug_context.thread_needs_join = 0; + } /* Wake the threads waiting for this join to complete */ pthread_cond_broadcast(&hid_hotplug_context.join_done); @@ -1784,16 +1775,18 @@ static void hid_internal_hotplug_thread_epilogue(void) static void* hotplug_thread(void* user_data) { + uint64_t thread_id = 0; (void) user_data; /* Mark this event thread as the very first action, before anything here can attempt a global-error write, so that any such write on this internal event thread - notably from a user callback that re-enters hid_hotplug_(de)register_callback() - is suppressed (see - hid_internal_on_event_thread()). The marker avoids locking before the - startup barrier; destructor re-entry can fall back to the thread record. */ + hid_internal_on_event_thread()). Destructor re-entry can fall back to the + atomically published OS thread ID if Darwin has cleared the TLS marker. */ hid_hotplug_event_thread = 1; - pthread_threadid_np(NULL, &hid_hotplug_context.thread_id); + pthread_threadid_np(NULL, &thread_id); + __atomic_store_n(&hid_hotplug_context.thread_id, thread_id, __ATOMIC_RELEASE); /* Startup phase: the registering thread holds the hotplug mutex and is parked at the startup barrier, so this thread has exclusive access to the diff --git a/src/tests/test_hotplug_mac.c b/src/tests/test_hotplug_mac.c index e517d0ab9..807feffb5 100644 --- a/src/tests/test_hotplug_mac.c +++ b/src/tests/test_hotplug_mac.c @@ -124,12 +124,13 @@ static void test_concurrent_registration(void) CHECK(pthread_mutex_destroy(&gate.mutex) == 0); } -enum callback_action { REMOVE_BY_RETURN, REMOVE_EXPLICITLY, WAIT_FOR_RELEASE, REMOVE_THEN_EXIT }; +enum callback_action { REMOVE_BY_RETURN, REMOVE_EXPLICITLY, WAIT_FOR_RELEASE, REMOVE_THEN_EXIT, WAIT_FOR_APP_MUTEX }; static pthread_key_t callback_key; struct callback_state { pthread_mutex_t mutex; + pthread_mutex_t api_mutex; pthread_cond_t cond; enum callback_action action; int entered; @@ -179,6 +180,25 @@ static int HID_API_CALL lifecycle_callback(hid_hotplug_callback_handle handle, return state->action == REMOVE_BY_RETURN || state->action == REMOVE_THEN_EXIT; } +static int HID_API_CALL serialized_callback(hid_hotplug_callback_handle handle, + struct hid_device_info *device, hid_hotplug_event event, void *user_data) +{ + struct callback_state *state = (struct callback_state *)user_data; + (void)handle; + (void)device; + CHECK(event == HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED); + CHECK(pthread_mutex_lock(&state->mutex) == 0); + state->entered = 1; + CHECK(pthread_cond_broadcast(&state->cond) == 0); + CHECK(pthread_mutex_unlock(&state->mutex) == 0); + /* The owner holds api_mutex across hid_init(); this callback already holds + HIDAPI's hotplug mutex. General API calls are serialized by api_mutex. */ + CHECK(pthread_mutex_lock(&state->api_mutex) == 0); + hid_close(NULL); + CHECK(pthread_mutex_unlock(&state->api_mutex) == 0); + return 0; +} + static void *deregister_worker(void *arg) { struct callback_state *state = (struct callback_state *)arg; @@ -197,7 +217,7 @@ static void *deregister_worker(void *arg) static void test_device_callback(enum callback_action action, const char *name) { - struct callback_state state = { PTHREAD_MUTEX_INITIALIZER, PTHREAD_COND_INITIALIZER, + struct callback_state state = { PTHREAD_MUTEX_INITIALIZER, PTHREAD_MUTEX_INITIALIZER, PTHREAD_COND_INITIALIZER, action, 0, 0, 0, 0, 0, 0, 0 }; struct hid_device_info *devices = hid_enumerate(0, 0); unsigned short vendor_id, product_id; @@ -210,14 +230,23 @@ static void test_device_callback(enum callback_action action, const char *name) product_id = devices->product_id; hid_free_enumeration(devices); printf("%s (VID %04hx, PID %04hx)\n", name, vendor_id, product_id); + if (action == WAIT_FOR_APP_MUTEX) + CHECK(pthread_mutex_lock(&state.api_mutex) == 0); CHECK(hid_hotplug_register_callback(vendor_id, product_id, HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED, HID_API_HOTPLUG_ENUMERATE, - lifecycle_callback, &state, &state.handle) == 0); + action == WAIT_FOR_APP_MUTEX ? serialized_callback : lifecycle_callback, + &state, &state.handle) == 0); CHECK(pthread_mutex_lock(&state.mutex) == 0); wait_flag(&state.cond, &state.mutex, &state.entered); CHECK(pthread_mutex_unlock(&state.mutex) == 0); - if (action == WAIT_FOR_RELEASE) { + if (action == WAIT_FOR_APP_MUTEX) { + /* The process alarm bounds this call if its error-clearing path tries + to take the hotplug mutex held by the callback waiting for us. */ + CHECK(hid_init() == 0); + CHECK(pthread_mutex_unlock(&state.api_mutex) == 0); + CHECK(hid_hotplug_deregister_callback(state.handle) == 0); + } else if (action == WAIT_FOR_RELEASE) { pthread_t worker; struct timespec deadline; int result = 0; @@ -254,6 +283,7 @@ static void test_device_callback(enum callback_action action, const char *name) } } CHECK(pthread_cond_destroy(&state.cond) == 0); + CHECK(pthread_mutex_destroy(&state.api_mutex) == 0); CHECK(pthread_mutex_destroy(&state.mutex) == 0); } @@ -275,6 +305,7 @@ int main(void) test_device_callback(REMOVE_EXPLICITLY, "Last callback explicit deregistration and restart"); test_device_callback(REMOVE_THEN_EXIT, "Last callback removal followed by owner-thread hid_exit"); test_device_callback(WAIT_FOR_RELEASE, "External deregistration waits for an in-flight callback"); + test_device_callback(WAIT_FOR_APP_MUTEX, "Owner hid_init while a callback waits for the application mutex"); CHECK(pthread_key_delete(callback_key) == 0); CHECK(hid_exit() == 0); puts("Repeated owner-thread hid_init/register/hid_exit"); From 685a85004bf635756a20cb62ea74dec1224b97b1 Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Tue, 8 Sep 2026 23:41:53 +0300 Subject: [PATCH 10/10] mac: refuse to start the hotplug thread without a published thread id pthread_threadid_np() never fails for the calling thread, but its result was ignored; a zero id would silently disable the destructor-phase identity fallback. Fail startup cleanly instead. Also document the (practically unreachable) non-recoverable state after a failed join in hid_exit() and hid_hotplug_deregister_callback(). Assisted-by: claude-code:claude-fable-5-1 --- mac/hid.c | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/mac/hid.c b/mac/hid.c index 03a387597..1c717027d 100644 --- a/mac/hid.c +++ b/mac/hid.c @@ -878,7 +878,14 @@ static int hid_internal_hotplug_exit(void) hid_internal_hotplug_cleanup(); pthread_mutex_unlock(&hid_hotplug_context.mutex); - /* Join the stopped event thread, with the hotplug mutex released */ + /* Join the stopped event thread, with the hotplug mutex released. + A failed join is a non-recoverable internal error: `exiting` stays set + (hotplug registration keeps failing with "hid_exit() is in progress") + and hid_exit() leaves hid_mgr alive, because the thread record must be + retained until pthread_join() succeeds (see hid_internal_hotplug_collect_thread()) + and the unjoined thread may still use the run loop resources. It is + practically unreachable: the thread is joinable, never detached, and + the identity check keeps the event thread from joining itself. */ if (hid_internal_hotplug_collect_thread() != 0) return -1; @@ -1785,7 +1792,16 @@ static void* hotplug_thread(void* user_data) hid_internal_on_event_thread()). Destructor re-entry can fall back to the atomically published OS thread ID if Darwin has cleared the TLS marker. */ hid_hotplug_event_thread = 1; - pthread_threadid_np(NULL, &thread_id); + if (pthread_threadid_np(NULL, &thread_id) != 0 || thread_id == 0) { + /* Without a published OS thread id the destructor-phase identity + fallback would silently be disabled (see hid_internal_on_event_thread()), + so refuse to start rather than run with a weaker identity protocol. + Apple's libpthread never fails this call for the calling thread; the + registrant joins this thread on failure, and no callback exists yet + that could observe the missing id. */ + thread_id = 0; + hid_hotplug_context.startup_error = "failed to read the event thread id"; + } __atomic_store_n(&hid_hotplug_context.thread_id, thread_id, __ATOMIC_RELEASE); /* Startup phase: the registering thread holds the hotplug mutex and is @@ -1808,7 +1824,11 @@ static void* hotplug_thread(void* user_data) hid_hotplug_context.run_loop_mode = CFStringCreateWithCString(NULL, str, kCFStringEncodingASCII); } - if (hid_hotplug_context.run_loop_mode) { + if (hid_hotplug_context.startup_error) { + /* Thread id publication failed above: skip the startup, the epilogue + below hands the reason to the registrant. */ + } + else if (hid_hotplug_context.run_loop_mode) { hid_hotplug_context.manager = IOHIDManagerCreate(kCFAllocatorDefault, kIOHIDOptionsTypeNone); if (!hid_hotplug_context.manager) { hid_hotplug_context.startup_error = "failed to create the HID manager"; @@ -2256,6 +2276,11 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_call cannot join itself): its epilogue releases the resources, and the next application-thread collector joins it before reusing the thread record. */ if (hid_internal_hotplug_collect_thread() != 0) { + /* Reported as a failure even when the callback itself was removed above + (its handle is dead either way): a failed join of the library's own + thread is worth surfacing over the deregistration result, and the + next collector retries the join. Practically unreachable, see + hid_internal_hotplug_exit(). */ register_global_error("hid_hotplug_deregister_callback: failed to join the hotplug events thread"); return -1; }