From d9c9926b114a03cd91f579d09e9f42505db22814 Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Tue, 14 Jul 2026 01:39:58 +0300 Subject: [PATCH 1/5] 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 2/5] 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 3/5] 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 4/5] 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 5/5] 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