From cd6eecae2e813cf8ca733ddca7137f2114f204eb Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Tue, 14 Jul 2026 01:40:09 +0300 Subject: [PATCH 1/9] windows: deliver the hotplug ENUMERATE pass asynchronously on the event context Implement the hotplug contract documented in #790 for the windows backend: The HID_API_HOTPLUG_ENUMERATE initial pass is now a deep-copied registration-time snapshot of the matching connected devices, replayed on a threadpool work item (the same critical-section-serialized event context that delivers live events) instead of running synchronously inside hid_hotplug_register_callback on the application thread. A callback's pending snapshot is flushed before any live event is delivered to it, so each device connection is reported exactly once and the pass always precedes live events; a non-zero callback return stops the remainder of the pass and deregisters the callback. Also per the contract: - hid_hotplug_deregister_callback returns -1 for unknown or stale handles (0 only when the callback was found and deregistered) - every register/deregister failure path sets the global error string, and *callback_handle is zeroed on registration failure - undelivered snapshots are freed on deregistration and hid_exit - CM_Unregister_Notification is no longer called from within the notification callback (forbidden, deadlocks) nor while holding the hotplug critical section (deadlocks against in-flight notifications): the teardown is detached under the lock and performed outside it, deferred to the work item when triggered from the notification itself Addresses #793 for the windows backend. Assisted-by: claude-code:claude-fable-5 --- windows/hid.c | 375 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 298 insertions(+), 77 deletions(-) diff --git a/windows/hid.c b/windows/hid.c index fceaaa05c..3f00f88dd 100644 --- a/windows/hid.c +++ b/windows/hid.c @@ -30,6 +30,14 @@ extern "C" { #undef WIN32_LEAN_AND_MEAN #endif +/* The native threadpool API (CreateThreadpoolWork & co), used to deliver + * the HID_API_HOTPLUG_ENUMERATE pass asynchronously, is only declared + * for Windows Vista and up. */ +#if !defined(_WIN32_WINNT) || (_WIN32_WINNT < 0x0600) +#undef _WIN32_WINNT +#define _WIN32_WINNT 0x0600 +#endif + #include "hidapi_winapi.h" #include @@ -48,6 +56,7 @@ typedef LONG NTSTATUS; #include #include #define _wcsdup wcsdup +#define _strdup strdup #define _stricmp strcasecmp #endif @@ -220,6 +229,11 @@ static struct hid_hotplug_context { /* Win32 notification handle */ HCMNOTIFICATION notify_handle; + /* Threadpool work item: delivers pending HID_API_HOTPLUG_ENUMERATE + passes and performs cleanup deferred from the notification callback. + Created with the first callback registration, closed by hid_exit(). */ + PTP_WORK event_work; + /* Critical section (faster mutex substitute), for both cached device list and callback list changes */ CRITICAL_SECTION critical_section; @@ -451,10 +465,41 @@ struct hid_hotplug_callback { void *user_data; hid_hotplug_callback_fn callback; + /* HID_API_HOTPLUG_ENUMERATE snapshot taken at registration time, + still to be replayed to this callback as synthetic + HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED events on the event context */ + struct hid_device_info *replay; + /* Pointer to the next notification */ struct hid_hotplug_callback *next; }; +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 = *src; + dst->next = NULL; + dst->path = NULL; + dst->serial_number = NULL; + dst->manufacturer_string = NULL; + dst->product_string = NULL; + + if ((src->path && (dst->path = _strdup(src->path)) == NULL) + || (src->serial_number && (dst->serial_number = _wcsdup(src->serial_number)) == NULL) + || (src->manufacturer_string && (dst->manufacturer_string = _wcsdup(src->manufacturer_string)) == NULL) + || (src->product_string && (dst->product_string = _wcsdup(src->product_string)) == NULL)) { + hid_free_enumeration(dst); + return NULL; + } + + return dst; +} + static void hid_internal_hotplug_remove_postponed() { /* Unregister the callbacks whose removal was postponed */ @@ -470,29 +515,49 @@ static void hid_internal_hotplug_remove_postponed() struct hid_hotplug_callback *callback = *current; if (!callback->events) { *current = (*current)->next; + hid_free_enumeration(callback->replay); free(callback); continue; } current = &callback->next; } - + /* Clear the flag so we don't start the cycle unless necessary */ hid_hotplug_context.cb_list_dirty = 0; } -static void hid_internal_hotplug_cleanup() +static void hid_internal_hotplug_unregister_notification(HCMNOTIFICATION notify_handle) { - if (!hid_hotplug_context.mutex_ready || hid_hotplug_context.mutex_in_use) { + if (notify_handle == NULL) { return; } + if (CM_Unregister_Notification(notify_handle) != CR_SUCCESS) { + register_global_error(L"CM_Unregister_Notification failed for Hotplug notification"); + } +} + +/* Always called inside a locked mutex. + When the last callback is gone, tears the machinery down and returns the + Win32 notification handle: CM_Unregister_Notification waits for in-progress + notification callbacks, which in turn may be blocked on our critical + section, so the caller must invoke it only after leaving the critical + section (and never from the notification callback itself: the notification + callback defers the teardown to the threadpool work item instead). */ +static HCMNOTIFICATION hid_internal_hotplug_cleanup() +{ + HCMNOTIFICATION notify_handle; + + if (!hid_hotplug_context.mutex_ready || hid_hotplug_context.mutex_in_use) { + return NULL; + } + /* Before checking if the list is empty, clear any entries whose removal was postponed first */ hid_internal_hotplug_remove_postponed(); /* Unregister the HID device connection notification when removing the last callback */ - /* This function is always called inside a locked mutex */ if (hid_hotplug_context.hotplug_cbs != NULL) { - return; + return NULL; } if (hid_hotplug_context.devs) { @@ -501,32 +566,102 @@ static void hid_internal_hotplug_cleanup() hid_hotplug_context.devs = NULL; } - if (hid_hotplug_context.notify_handle) { - if (CM_Unregister_Notification(hid_hotplug_context.notify_handle) != CR_SUCCESS) { - /* We mark an error, but we proceed with the cleanup */ - register_global_error(L"CM_Unregister_Notification failed for Hotplug notification"); + notify_handle = hid_hotplug_context.notify_handle; + hid_hotplug_context.notify_handle = NULL; + return notify_handle; +} + +/* Deliver (and consume) a callback's pending HID_API_HOTPLUG_ENUMERATE + snapshot. Always called inside a locked mutex, with mutex_in_use set. */ +static void hid_internal_hotplug_replay_flush(struct hid_hotplug_callback *callback) +{ + while (callback->replay != NULL) { + struct hid_device_info *device = callback->replay; + callback->replay = device->next; + device->next = NULL; + + if (!callback->events) { + /* The callback was deregistered while the pass was pending */ + hid_free_enumeration(device); + continue; + } + + int result = (*callback->callback)(callback->handle, device, HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED, callback->user_data); + hid_free_enumeration(device); + + /* A non-zero result stops the remainder of the pass and deregisters the callback */ + if (result) { + callback->events = 0; + hid_hotplug_context.cb_list_dirty = 1; + hid_free_enumeration(callback->replay); + callback->replay = NULL; } } +} - hid_hotplug_context.notify_handle = NULL; +/* Threadpool work item: delivers pending HID_API_HOTPLUG_ENUMERATE passes + (unless a live event got to them first) and performs the cleanup the + notification callback is not allowed to perform itself. */ +static VOID CALLBACK hid_internal_hotplug_event_work(PTP_CALLBACK_INSTANCE instance, PVOID context, PTP_WORK work) +{ + HCMNOTIFICATION notify_handle; + + (void)instance; + (void)context; + (void)work; + + EnterCriticalSection(&hid_hotplug_context.critical_section); + + hid_hotplug_context.mutex_in_use = 1; + for (struct hid_hotplug_callback *callback = hid_hotplug_context.hotplug_cbs; callback != NULL; callback = callback->next) { + hid_internal_hotplug_replay_flush(callback); + } + hid_hotplug_context.mutex_in_use = 0; + + notify_handle = hid_internal_hotplug_cleanup(); + + LeaveCriticalSection(&hid_hotplug_context.critical_section); + + hid_internal_hotplug_unregister_notification(notify_handle); } static void hid_internal_hotplug_exit() { + HCMNOTIFICATION notify_handle; + if (!hid_hotplug_context.mutex_ready) { /* If the critical section is not initialized, we are safe to assume nothing else is */ return; } EnterCriticalSection(&hid_hotplug_context.critical_section); struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; - /* Remove all callbacks from the list */ + /* Remove all callbacks from the list, including their undelivered HID_API_HOTPLUG_ENUMERATE snapshots */ while (*current) { struct hid_hotplug_callback *next = (*current)->next; + hid_free_enumeration((*current)->replay); free(*current); *current = next; } - hid_internal_hotplug_cleanup(); + notify_handle = hid_internal_hotplug_cleanup(); LeaveCriticalSection(&hid_hotplug_context.critical_section); + + hid_internal_hotplug_unregister_notification(notify_handle); + + if (hid_hotplug_context.event_work) { + /* Wait for any in-flight work item before the critical section goes away. + hid_exit() must not be called from a callback, so this cannot deadlock. */ + WaitForThreadpoolWorkCallbacks(hid_hotplug_context.event_work, FALSE); + CloseThreadpoolWork(hid_hotplug_context.event_work); + hid_hotplug_context.event_work = NULL; + } + + /* A last-gasp notification callback may have re-added a device between + the cleanup and the unregistration; nothing can race us anymore here */ + if (hid_hotplug_context.devs != NULL) { + hid_free_enumeration(hid_hotplug_context.devs); + hid_hotplug_context.devs = NULL; + } + hid_hotplug_context.mutex_ready = 0; DeleteCriticalSection(&hid_hotplug_context.critical_section); } @@ -1151,14 +1286,25 @@ DWORD WINAPI hid_internal_notify_callback(HCMNOTIFICATION notify, PVOID context, if (device) { /* Mark the critical section as IN USE, to prevent callback removal from inside a callback */ hid_hotplug_context.mutex_in_use = 1; - + + /* Callbacks registered from inside a callback are appended to the list + and see this device in their registration-time HID_API_HOTPLUG_ENUMERATE + snapshot (or don't, for a removal): the live dispatch is bound to the + callbacks present at event time, so the connection is reported exactly once */ + struct hid_hotplug_callback *last_at_event = hid_hotplug_context.hotplug_cbs; + while (last_at_event != NULL && last_at_event->next != NULL) { + last_at_event = last_at_event->next; + } + /* Call the notifications for the device */ - struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; - while (*current) { - struct hid_hotplug_callback *callback = *current; + for (struct hid_hotplug_callback *callback = hid_hotplug_context.hotplug_cbs; callback != NULL; callback = callback->next) { + /* The registration-time enumeration pass is always delivered + before any live events for the callback */ + hid_internal_hotplug_replay_flush(callback); + if ((callback->events & hotplug_event) && hid_internal_match_device_id(device->vendor_id, device->product_id, callback->vendor_id, callback->product_id)) { int result = (callback->callback)(callback->handle, device, hotplug_event, callback->user_data); - + /* If the result is non-zero, we MARK the callback for future removal and proceed */ /* We avoid changing the list until we are done calling the callbacks to simplify the process */ if (result) { @@ -1166,7 +1312,10 @@ DWORD WINAPI hid_internal_notify_callback(HCMNOTIFICATION notify, PVOID context, hid_hotplug_context.cb_list_dirty = 1; } } - current = &callback->next; + + if (callback == last_at_event) { + break; + } } hid_hotplug_context.mutex_in_use = 0; @@ -1176,8 +1325,13 @@ DWORD WINAPI hid_internal_notify_callback(HCMNOTIFICATION notify, PVOID context, hid_free_enumeration(device); } - /* Remove any callbacks that were marked for removal and stop the notification if none are left */ - hid_internal_hotplug_cleanup(); + /* Remove any callbacks that were marked for removal; if none are left, + defer the teardown to the threadpool work item: unregistering the + notification from its own callback is not allowed (deadlock) */ + hid_internal_hotplug_remove_postponed(); + if (hid_hotplug_context.hotplug_cbs == NULL && hid_hotplug_context.event_work != NULL) { + SubmitThreadpoolWork(hid_hotplug_context.event_work); + } } LeaveCriticalSection(&hid_hotplug_context.critical_section); @@ -1189,17 +1343,30 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven { struct hid_hotplug_callback* hotplug_cb; + /* No events can be delivered before the handle is written */ + if (callback_handle != NULL) { + *callback_handle = 0; + } + /* Check params */ + if (callback == NULL) { + register_global_error(L"Callback function is NULL"); + return -1; + } if (events == 0 - || (events & ~(HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED | HID_API_HOTPLUG_EVENT_DEVICE_LEFT)) - || (flags & ~(HID_API_HOTPLUG_ENUMERATE)) - || callback == NULL) { + || (events & ~(HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED | HID_API_HOTPLUG_EVENT_DEVICE_LEFT))) { + register_global_error(L"Invalid events mask"); + return -1; + } + if (flags & ~(HID_API_HOTPLUG_ENUMERATE)) { + register_global_error(L"Invalid flags"); return -1; } hotplug_cb = (struct hid_hotplug_callback*)calloc(1, sizeof(struct hid_hotplug_callback)); if (hotplug_cb == NULL) { + register_global_error(L"Failed to allocate memory for a hotplug callback"); return -1; } @@ -1210,6 +1377,7 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven hotplug_cb->events = events; hotplug_cb->user_data = user_data; hotplug_cb->callback = callback; + hotplug_cb->replay = NULL; /* Ensure we are ready to actually use the mutex */ hid_internal_hotplug_init(); @@ -1225,71 +1393,109 @@ 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; - } + /* Start the machinery with the first callback */ + if (hid_hotplug_context.hotplug_cbs == NULL) { + if (hid_init() < 0) { + /* register_global_error: global error is already set by hid_init */ + LeaveCriticalSection(&hid_hotplug_context.critical_section); + free(hotplug_cb); + return -1; + } - /* Append a new callback to the end */ - if (hid_hotplug_context.hotplug_cbs != NULL) { - struct hid_hotplug_callback *last = hid_hotplug_context.hotplug_cbs; - while (last->next != NULL) { - last = last->next; + if (hid_hotplug_context.event_work == NULL) { + hid_hotplug_context.event_work = CreateThreadpoolWork(hid_internal_hotplug_event_work, NULL, NULL); + if (hid_hotplug_context.event_work == NULL) { + LeaveCriticalSection(&hid_hotplug_context.critical_section); + free(hotplug_cb); + register_global_error(L"hid_hotplug_register_callback/CreateThreadpoolWork"); + return -1; + } } - last->next = hotplug_cb; - } - else { - GUID interface_class_guid; - CM_NOTIFY_FILTER notify_filter = { 0 }; - /* Fill already connected devices so we can use this info in disconnection notification */ - hid_hotplug_context.devs = hid_enumerate(0, 0); + /* Refresh the device cache: a teardown deferred to the work item may not have run yet */ + if (hid_hotplug_context.devs != NULL) { + hid_free_enumeration(hid_hotplug_context.devs); + hid_hotplug_context.devs = NULL; + } - hid_hotplug_context.hotplug_cbs = hotplug_cb; + /* Fill already connected devices so we can use this info in disconnection + notifications and HID_API_HOTPLUG_ENUMERATE passes (hid_enumerate also + implicitly initializes the library, as if by hid_init) */ + hid_hotplug_context.devs = hid_enumerate(0, 0); - if (hid_hotplug_context.notify_handle != NULL) { - register_global_error(L"Device notification have already been registered"); - LeaveCriticalSection(&hid_hotplug_context.critical_section); - return -1; - } + if (hid_hotplug_context.notify_handle == NULL) { + GUID interface_class_guid; + CM_NOTIFY_FILTER notify_filter = { 0 }; - /* Retrieve HID Interface Class GUID - https://docs.microsoft.com/windows-hardware/drivers/install/guid-devinterface-hid */ - HidD_GetHidGuid(&interface_class_guid); + /* Retrieve HID Interface Class GUID + https://docs.microsoft.com/windows-hardware/drivers/install/guid-devinterface-hid */ + HidD_GetHidGuid(&interface_class_guid); - notify_filter.cbSize = sizeof(notify_filter); - notify_filter.FilterType = CM_NOTIFY_FILTER_TYPE_DEVICEINTERFACE; - notify_filter.u.DeviceInterface.ClassGuid = interface_class_guid; + notify_filter.cbSize = sizeof(notify_filter); + notify_filter.FilterType = CM_NOTIFY_FILTER_TYPE_DEVICEINTERFACE; + notify_filter.u.DeviceInterface.ClassGuid = interface_class_guid; - /* Register for a HID device notification when adding the first callback */ - if (CM_Register_Notification(¬ify_filter, NULL, hid_internal_notify_callback, &hid_hotplug_context.notify_handle) != CR_SUCCESS) { - register_global_error(L"hid_hotplug_register_callback/CM_Register_Notification"); - LeaveCriticalSection(&hid_hotplug_context.critical_section); - return -1; + /* Register for a HID device notification when adding the first callback */ + if (CM_Register_Notification(¬ify_filter, NULL, hid_internal_notify_callback, &hid_hotplug_context.notify_handle) != CR_SUCCESS) { + hid_hotplug_context.notify_handle = NULL; + if (hid_hotplug_context.devs != NULL) { + hid_free_enumeration(hid_hotplug_context.devs); + hid_hotplug_context.devs = NULL; + } + LeaveCriticalSection(&hid_hotplug_context.critical_section); + free(hotplug_cb); + register_global_error(L"hid_hotplug_register_callback/CM_Register_Notification"); + return -1; + } } } - /* Mark the critical section 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; - + /* Take the registration-time snapshot to be replayed asynchronously + on the event context, one exact copy per matching connected device */ 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); + struct hid_device_info **replay_tail = &hotplug_cb->replay; + for (struct hid_device_info *device = hid_hotplug_context.devs; device != NULL; device = device->next) { + if (!hid_internal_match_device_id(device->vendor_id, device->product_id, vendor_id, product_id)) { + continue; + } + *replay_tail = hid_internal_copy_device_info(device); + if (*replay_tail == NULL) { + HCMNOTIFICATION notify_handle; + hid_free_enumeration(hotplug_cb->replay); + /* Tear the machinery down if this would-be-first callback was starting it */ + notify_handle = hid_internal_hotplug_cleanup(); + LeaveCriticalSection(&hid_hotplug_context.critical_section); + hid_internal_hotplug_unregister_notification(notify_handle); + free(hotplug_cb); + register_global_error(L"Failed to allocate memory for a device info snapshot"); + return -1; } + replay_tail = &(*replay_tail)->next; + } + } - device = device->next; + /* Append the new callback to the end of the list */ + if (hid_hotplug_context.hotplug_cbs != NULL) { + struct hid_hotplug_callback *last = hid_hotplug_context.hotplug_cbs; + while (last->next != NULL) { + last = last->next; } + last->next = hotplug_cb; + } + else { + hid_hotplug_context.hotplug_cbs = hotplug_cb; + } + + /* Return allocated handle */ + if (callback_handle != NULL) { + *callback_handle = hotplug_cb->handle; } - hid_hotplug_context.mutex_in_use = old_state; + /* Have the snapshot delivered on the event context; never from within this call */ + if (hotplug_cb->replay != NULL) { + SubmitThreadpoolWork(hid_hotplug_context.event_work); + } - /* Remove any callbacks that were marked for removal and stop the notification if none are left */ - hid_internal_hotplug_cleanup(); - LeaveCriticalSection(&hid_hotplug_context.critical_section); return 0; @@ -1297,21 +1503,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) { + int result = -1; + HCMNOTIFICATION notify_handle; + if (callback_handle <= 0 || !hid_hotplug_context.mutex_ready) { + register_global_error(L"Invalid or unknown hotplug callback handle"); return -1; } /* Lock the mutex to avoid race conditions */ EnterCriticalSection(&hid_hotplug_context.critical_section); - if (!hid_hotplug_context.hotplug_cbs) { - LeaveCriticalSection(&hid_hotplug_context.critical_section); - return -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) { + /* A callback already marked for removal counts as deregistered */ + if (!(*current)->events) { + break; + } + + /* Undelivered HID_API_HOTPLUG_ENUMERATE events must never fire after deregistration */ + hid_free_enumeration((*current)->replay); + (*current)->replay = NULL; + /* Check if we were already in the critical section, as we are NOT allowed to remove any callbacks if we are */ if (hid_hotplug_context.mutex_in_use) { /* If we are not allowed to remove the callback, we mark it as pending removal */ @@ -1322,15 +1536,22 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_call free(*current); *current = next; } + result = 0; break; } } - hid_internal_hotplug_cleanup(); + notify_handle = hid_internal_hotplug_cleanup(); LeaveCriticalSection(&hid_hotplug_context.critical_section); - return 0; + hid_internal_hotplug_unregister_notification(notify_handle); + + if (result != 0) { + register_global_error(L"Invalid or unknown hotplug callback handle"); + } + + return result; } HID_API_EXPORT hid_device * HID_API_CALL hid_open(unsigned short vendor_id, unsigned short product_id, const wchar_t *serial_number) From a3350c45f1c1675f932a2dcc84bda0f9611d7527 Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Tue, 14 Jul 2026 02:22:14 +0300 Subject: [PATCH 2/9] windows: address round-1 review findings on the async hotplug ENUMERATE pass - Arm the CM notification BEFORE taking the registration-time device snapshot, and deduplicate arrivals by path in the notification callback: a device connecting between the two is now caught by the notification and absorbed by the dedupe instead of being missed by both (it was reported by neither before) - Only (re)build the device cache when actually arming a notification; while one is attached the cache is kept current by its callbacks (re-enumerating during a deferred teardown could double-report) - Serialize new-notification arming against in-flight CM_Unregister_Notification calls (pending counter + condition variable): a detached handle stays live at the OS until its unregistration completes, and overlapping registrations would deliver events twice; hid_exit waits for the same quiescence before destroying state - Treat a failed CM_Unregister_Notification as a poisoned epoch: hid_exit reports the failure, returns -1 and deliberately leaks the critical section, work item and loaded DLLs rather than let a still-live notification touch destroyed state - Guard the machinery bootstrap with a statically-initialized SRWLOCK (two racing first registrations could both initialize the critical section), and serialize all mutations of the global error string with another SRWLOCK (concurrent thread-safe failures could double-free it) - Distinguish a genuinely failed enumeration from an empty system at registration (fail the registration on the former), and clear the stale "No HID devices found" error on successful registration - Fail registration with an error when the callback handle space is exhausted instead of signed-overflowing and wrapping onto live handles - Replace { 0 } struct initializers with memset in hid.c for -Wmissing-field-initializers cleanliness when built as C++ with GNU compilers; use the WinAPI error helper for CreateThreadpoolWork failures Addresses round-1 review of the windows part of #793. Assisted-by: claude-code:claude-fable-5 --- windows/hid.c | 260 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 206 insertions(+), 54 deletions(-) diff --git a/windows/hid.c b/windows/hid.c index 3f00f88dd..e35116b62 100644 --- a/windows/hid.c +++ b/windows/hid.c @@ -66,6 +66,7 @@ typedef LONG NTSTATUS; #include "hidapi_hidclass.h" #include "hidapi_hidsdi.h" +#include #include #include #include @@ -234,6 +235,20 @@ static struct hid_hotplug_context { Created with the first callback registration, closed by hid_exit(). */ PTP_WORK event_work; + /* Number of notification handles detached from the context whose + CM_Unregister_Notification call has not completed yet, and the condition + used to wait for that to drop to zero (a zero-initialized + CONDITION_VARIABLE is valid). Guarded by the critical section. + A new notification is only armed once this is zero: the OS keeps a + detached handle live until the unregistration completes, and two live + registrations would deliver every event twice. */ + LONG pending_unregistrations; + CONDITION_VARIABLE unregistration_cv; + + /* Set when CM_Unregister_Notification failed: the OS-side registration may + still be live, so the state its callbacks can touch is never destroyed */ + unsigned char unregistration_failed; + /* Critical section (faster mutex substitute), for both cached device list and callback list changes */ CRITICAL_SECTION critical_section; @@ -389,14 +404,22 @@ static void register_string_error(hid_device *dev, const WCHAR *string_error) static wchar_t *last_global_error_str = NULL; +/* Serializes mutations of last_global_error_str: the hotplug API is + thread-safe and its failure paths may write the global error concurrently */ +static SRWLOCK global_error_lock = SRWLOCK_INIT; + static void register_global_winapi_error(const WCHAR *op) { + AcquireSRWLockExclusive(&global_error_lock); register_winapi_error_to_buffer(&last_global_error_str, op); + ReleaseSRWLockExclusive(&global_error_lock); } static void register_global_error(const WCHAR *string_error) { + AcquireSRWLockExclusive(&global_error_lock); register_string_error_to_buffer(&last_global_error_str, string_error); + ReleaseSRWLockExclusive(&global_error_lock); } static HANDLE open_device(const wchar_t *path, BOOL open_rw) @@ -426,8 +449,13 @@ HID_API_EXPORT const char* HID_API_CALL hid_version_str(void) return HID_API_VERSION_STR; } +/* Serializes the bootstrap of the hotplug machinery: two racing first + registrations must not both initialize the critical section */ +static SRWLOCK hotplug_init_lock = SRWLOCK_INIT; + static void hid_internal_hotplug_init() { + AcquireSRWLockExclusive(&hotplug_init_lock); if (!hid_hotplug_context.mutex_ready) { InitializeCriticalSection(&hid_hotplug_context.critical_section); @@ -435,9 +463,11 @@ static void hid_internal_hotplug_init() hid_hotplug_context.mutex_ready = 1; hid_hotplug_context.mutex_in_use = 0; hid_hotplug_context.cb_list_dirty = 0; + hid_hotplug_context.pending_unregistrations = 0; if (hid_hotplug_context.next_handle < FIRST_HOTPLUG_CALLBACK_HANDLE) hid_hotplug_context.next_handle = FIRST_HOTPLUG_CALLBACK_HANDLE; } + ReleaseSRWLockExclusive(&hotplug_init_lock); } int HID_API_EXPORT hid_init(void) @@ -526,24 +556,43 @@ static void hid_internal_hotplug_remove_postponed() hid_hotplug_context.cb_list_dirty = 0; } -static void hid_internal_hotplug_unregister_notification(HCMNOTIFICATION notify_handle) +/* Completes the unregistration of a notification handle detached by + hid_internal_hotplug_cleanup. Must be called OUTSIDE the critical section: + CM_Unregister_Notification waits for in-progress notification callbacks, + which may themselves be blocked on the critical section. */ +static void hid_internal_hotplug_finish_unregistration(HCMNOTIFICATION notify_handle) { + CONFIGRET cr; + if (notify_handle == NULL) { return; } - if (CM_Unregister_Notification(notify_handle) != CR_SUCCESS) { + cr = CM_Unregister_Notification(notify_handle); + + EnterCriticalSection(&hid_hotplug_context.critical_section); + if (cr != CR_SUCCESS) { + /* The OS-side registration may still be live: taint the epoch so + hid_exit() never destroys the state a late notification can touch */ + hid_hotplug_context.unregistration_failed = 1; + } + hid_hotplug_context.pending_unregistrations--; + if (hid_hotplug_context.pending_unregistrations == 0) { + WakeAllConditionVariable(&hid_hotplug_context.unregistration_cv); + } + LeaveCriticalSection(&hid_hotplug_context.critical_section); + + if (cr != CR_SUCCESS) { register_global_error(L"CM_Unregister_Notification failed for Hotplug notification"); } } /* Always called inside a locked mutex. When the last callback is gone, tears the machinery down and returns the - Win32 notification handle: CM_Unregister_Notification waits for in-progress - notification callbacks, which in turn may be blocked on our critical - section, so the caller must invoke it only after leaving the critical - section (and never from the notification callback itself: the notification - callback defers the teardown to the threadpool work item instead). */ + Win32 notification handle, which the caller MUST hand to + hid_internal_hotplug_finish_unregistration after leaving the critical + section (never under it, and never from the notification callback itself: + the notification callback defers the teardown to the threadpool work item). */ static HCMNOTIFICATION hid_internal_hotplug_cleanup() { HCMNOTIFICATION notify_handle; @@ -568,6 +617,10 @@ static HCMNOTIFICATION hid_internal_hotplug_cleanup() notify_handle = hid_hotplug_context.notify_handle; hid_hotplug_context.notify_handle = NULL; + if (notify_handle != NULL) { + /* Balanced by hid_internal_hotplug_finish_unregistration */ + hid_hotplug_context.pending_unregistrations++; + } return notify_handle; } @@ -622,16 +675,17 @@ static VOID CALLBACK hid_internal_hotplug_event_work(PTP_CALLBACK_INSTANCE insta LeaveCriticalSection(&hid_hotplug_context.critical_section); - hid_internal_hotplug_unregister_notification(notify_handle); + hid_internal_hotplug_finish_unregistration(notify_handle); } -static void hid_internal_hotplug_exit() +static int hid_internal_hotplug_exit() { HCMNOTIFICATION notify_handle; + int failed_epoch; if (!hid_hotplug_context.mutex_ready) { /* If the critical section is not initialized, we are safe to assume nothing else is */ - return; + return 0; } EnterCriticalSection(&hid_hotplug_context.critical_section); struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; @@ -645,7 +699,29 @@ static void hid_internal_hotplug_exit() notify_handle = hid_internal_hotplug_cleanup(); LeaveCriticalSection(&hid_hotplug_context.critical_section); - hid_internal_hotplug_unregister_notification(notify_handle); + hid_internal_hotplug_finish_unregistration(notify_handle); + + /* Quiescence: unregistrations started by concurrent (contract-legal) + hid_hotplug_deregister_callback calls must complete before the state + their notifications can still touch is destroyed. The sleep releases + the critical section while waiting. */ + EnterCriticalSection(&hid_hotplug_context.critical_section); + while (hid_hotplug_context.pending_unregistrations > 0) { + if (!SleepConditionVariableCS(&hid_hotplug_context.unregistration_cv, &hid_hotplug_context.critical_section, INFINITE)) { + /* Should never happen; treat like a failed unregistration rather than spinning */ + break; + } + } + failed_epoch = (hid_hotplug_context.unregistration_failed != 0) || (hid_hotplug_context.pending_unregistrations > 0); + LeaveCriticalSection(&hid_hotplug_context.critical_section); + + if (failed_epoch) { + /* At least one notification may still be live at the OS level: keep the + critical section and the work item alive (deliberately leaked), so a + late callback touches valid, empty state instead of freed memory */ + register_global_error(L"hid_exit: a hotplug notification could not be unregistered"); + return -1; + } if (hid_hotplug_context.event_work) { /* Wait for any in-flight work item before the critical section goes away. @@ -664,11 +740,19 @@ static void hid_internal_hotplug_exit() hid_hotplug_context.mutex_ready = 0; DeleteCriticalSection(&hid_hotplug_context.critical_section); + + return 0; } int HID_API_EXPORT hid_exit(void) { - hid_internal_hotplug_exit(); + if (hid_internal_hotplug_exit() < 0) { + /* A hotplug notification could not be unregistered and may still fire: + keep the resolved DLLs loaded (deliberately leaked) so a late + callback does not call into unloaded code. + register_global_error: set by hid_internal_hotplug_exit */ + return -1; + } #ifndef HIDAPI_USE_DDK free_library_handles(); @@ -909,7 +993,9 @@ static hid_internal_detect_bus_type_result hid_internal_detect_bus_type(const wc wchar_t *device_id = NULL, *compatible_ids = NULL; CONFIGRET cr; DEVINST dev_node; - hid_internal_detect_bus_type_result result = { 0 }; + hid_internal_detect_bus_type_result result; + + memset(&result, 0, sizeof(result)); /* Get the device id from interface path */ device_id = (wchar_t *)hid_internal_get_device_interface_property(interface_path, &DEVPKEY_Device_InstanceId, DEVPROP_TYPE_STRING); @@ -1099,7 +1185,9 @@ static struct hid_device_info *hid_internal_get_device_info(const wchar_t *path, return dev; } -struct hid_device_info HID_API_EXPORT * HID_API_CALL hid_enumerate(unsigned short vendor_id, unsigned short product_id) +/* Same as hid_enumerate, but distinguishes a genuine failure (*failure set + to 1, error registered) from an empty result (NULL with *failure left 0) */ +static struct hid_device_info *hid_internal_enumerate(unsigned short vendor_id, unsigned short product_id, int *failure) { struct hid_device_info *root = NULL; /* return object */ struct hid_device_info *cur_dev = NULL; @@ -1108,6 +1196,8 @@ struct hid_device_info HID_API_EXPORT * HID_API_CALL hid_enumerate(unsigned shor wchar_t* device_interface_list = NULL; DWORD len; + *failure = 1; + if (hid_init() < 0) { /* register_global_error: global error is reset by hid_init */ return NULL; @@ -1146,6 +1236,8 @@ struct hid_device_info HID_API_EXPORT * HID_API_CALL hid_enumerate(unsigned shor goto end_of_function; } + *failure = 0; + /* Iterate over each device interface in the HID class, looking for the right one. */ for (wchar_t* device_interface = device_interface_list; *device_interface; device_interface += wcslen(device_interface) + 1) { HANDLE device_handle = INVALID_HANDLE_VALUE; @@ -1203,6 +1295,12 @@ struct hid_device_info HID_API_EXPORT * HID_API_CALL hid_enumerate(unsigned shor return root; } +struct hid_device_info HID_API_EXPORT * HID_API_CALL hid_enumerate(unsigned short vendor_id, unsigned short product_id) +{ + int failure = 0; + return hid_internal_enumerate(vendor_id, product_id, &failure); +} + void HID_API_EXPORT HID_API_CALL hid_free_enumeration(struct hid_device_info *devs) { /* TODO: Merge this with the Linux version. This function is platform-independent. */ @@ -1235,29 +1333,49 @@ DWORD WINAPI hid_internal_notify_callback(HCMNOTIFICATION notify, PVOID context, EnterCriticalSection(&hid_hotplug_context.critical_section); if (action == CM_NOTIFY_ACTION_DEVICEINTERFACEARRIVAL) { - HANDLE read_handle; + char *path; + int known = 0; hotplug_event = HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED; - /* Open read-only handle to the device */ - read_handle = open_device(event_data->u.DeviceInterface.SymbolicLink, FALSE); + /* An arrival for a path already in the cache is a duplicate: the + registration-time enumeration overlaps with the already-armed + notification, and a notification being unregistered may briefly + coexist with its replacement. Deduplicating here keeps each + connection reported (and cached) exactly once. */ + path = hid_internal_UTF16toUTF8(event_data->u.DeviceInterface.SymbolicLink); + if (path != NULL) { + for (struct hid_device_info *current = hid_hotplug_context.devs; current != NULL; current = current->next) { + /* Case-independent path comparison is mandatory */ + if (current->path != NULL && _stricmp(current->path, path) == 0) { + known = 1; + break; + } + } + free(path); + } - /* Check validity of read_handle. */ - if (read_handle != INVALID_HANDLE_VALUE) { - device = hid_internal_get_device_info(event_data->u.DeviceInterface.SymbolicLink, read_handle); + if (!known) { + /* Open read-only handle to the device */ + HANDLE read_handle = open_device(event_data->u.DeviceInterface.SymbolicLink, FALSE); - /* Append to the end of the device list */ - if (hid_hotplug_context.devs != NULL) { - struct hid_device_info *last = hid_hotplug_context.devs; - while (last->next != NULL) { - last = last->next; + /* Check validity of read_handle. */ + if (read_handle != INVALID_HANDLE_VALUE) { + device = hid_internal_get_device_info(event_data->u.DeviceInterface.SymbolicLink, read_handle); + + /* Append to the end of the device list */ + if (hid_hotplug_context.devs != NULL) { + struct hid_device_info *last = hid_hotplug_context.devs; + while (last->next != NULL) { + last = last->next; + } + last->next = device; + } else { + hid_hotplug_context.devs = device; } - last->next = device; - } else { - hid_hotplug_context.devs = device; - } - CloseHandle(read_handle); + CloseHandle(read_handle); + } } } else if (action == CM_NOTIFY_ACTION_DEVICEINTERFACEREMOVAL) { char *path; @@ -1385,12 +1503,27 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven /* Lock the mutex to avoid race conditions */ EnterCriticalSection(&hid_hotplug_context.critical_section); + /* Handle values are never reused while the library remains initialized */ + if (hid_hotplug_context.next_handle >= INT_MAX) { + LeaveCriticalSection(&hid_hotplug_context.critical_section); + free(hotplug_cb); + register_global_error(L"Hotplug callback handles exhausted"); + return -1; + } hotplug_cb->handle = hid_hotplug_context.next_handle++; - /* handle the unlikely case of handle overflow */ - if (hid_hotplug_context.next_handle < 0) - { - hid_hotplug_context.next_handle = 1; + /* A notification detached by a concurrent deregistration may still be live + at the OS until its unregistration completes; arming a replacement in + that window would deliver every event twice. The sleep releases the + critical section, so on wake the state is re-evaluated from scratch. */ + while (hid_hotplug_context.hotplug_cbs == NULL && hid_hotplug_context.notify_handle == NULL + && hid_hotplug_context.pending_unregistrations > 0) { + if (!SleepConditionVariableCS(&hid_hotplug_context.unregistration_cv, &hid_hotplug_context.critical_section, INFINITE)) { + LeaveCriticalSection(&hid_hotplug_context.critical_section); + free(hotplug_cb); + register_global_winapi_error(L"hid_hotplug_register_callback/SleepConditionVariableCS"); + return -1; + } } /* Start the machinery with the first callback */ @@ -1407,25 +1540,17 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven if (hid_hotplug_context.event_work == NULL) { LeaveCriticalSection(&hid_hotplug_context.critical_section); free(hotplug_cb); - register_global_error(L"hid_hotplug_register_callback/CreateThreadpoolWork"); + register_global_winapi_error(L"hid_hotplug_register_callback/CreateThreadpoolWork"); return -1; } } - /* Refresh the device cache: a teardown deferred to the work item may not have run yet */ - if (hid_hotplug_context.devs != NULL) { - hid_free_enumeration(hid_hotplug_context.devs); - hid_hotplug_context.devs = NULL; - } - - /* Fill already connected devices so we can use this info in disconnection - notifications and HID_API_HOTPLUG_ENUMERATE passes (hid_enumerate also - implicitly initializes the library, as if by hid_init) */ - hid_hotplug_context.devs = hid_enumerate(0, 0); - if (hid_hotplug_context.notify_handle == NULL) { GUID interface_class_guid; - CM_NOTIFY_FILTER notify_filter = { 0 }; + CM_NOTIFY_FILTER notify_filter; + int enumerate_failure = 0; + + memset(¬ify_filter, 0, sizeof(notify_filter)); /* Retrieve HID Interface Class GUID https://docs.microsoft.com/windows-hardware/drivers/install/guid-devinterface-hid */ @@ -1435,19 +1560,42 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven notify_filter.FilterType = CM_NOTIFY_FILTER_TYPE_DEVICEINTERFACE; notify_filter.u.DeviceInterface.ClassGuid = interface_class_guid; - /* Register for a HID device notification when adding the first callback */ + /* Register for a HID device notification when adding the first callback. + Armed BEFORE the device cache is filled: a device connecting in + between is then caught by the notification and deduplicated against + the cache, instead of being missed by both. */ if (CM_Register_Notification(¬ify_filter, NULL, hid_internal_notify_callback, &hid_hotplug_context.notify_handle) != CR_SUCCESS) { hid_hotplug_context.notify_handle = NULL; - if (hid_hotplug_context.devs != NULL) { - hid_free_enumeration(hid_hotplug_context.devs); - hid_hotplug_context.devs = NULL; - } LeaveCriticalSection(&hid_hotplug_context.critical_section); free(hotplug_cb); register_global_error(L"hid_hotplug_register_callback/CM_Register_Notification"); return -1; } + + /* Normally NULL already; an old notification may have appended + entries between its detachment and the completion of its + unregistration (or, after a failed unregistration, at any time) */ + if (hid_hotplug_context.devs != NULL) { + hid_free_enumeration(hid_hotplug_context.devs); + hid_hotplug_context.devs = NULL; + } + + /* Fill already connected devices so we can use this info in disconnection + notifications and HID_API_HOTPLUG_ENUMERATE passes */ + hid_hotplug_context.devs = hid_internal_enumerate(0, 0, &enumerate_failure); + if (enumerate_failure) { + /* An empty system is fine; a failed enumeration is not: the device + cache and the ENUMERATE snapshot would misrepresent the system */ + HCMNOTIFICATION notify_handle = hid_internal_hotplug_cleanup(); + LeaveCriticalSection(&hid_hotplug_context.critical_section); + hid_internal_hotplug_finish_unregistration(notify_handle); + free(hotplug_cb); + /* register_global_error: set by hid_internal_enumerate */ + return -1; + } } + /* else: reuse the still-attached notification (its deferred teardown has + not run yet); the device cache is kept current by that notification */ } /* Take the registration-time snapshot to be replayed asynchronously @@ -1465,7 +1613,7 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven /* Tear the machinery down if this would-be-first callback was starting it */ notify_handle = hid_internal_hotplug_cleanup(); LeaveCriticalSection(&hid_hotplug_context.critical_section); - hid_internal_hotplug_unregister_notification(notify_handle); + hid_internal_hotplug_finish_unregistration(notify_handle); free(hotplug_cb); register_global_error(L"Failed to allocate memory for a device info snapshot"); return -1; @@ -1498,6 +1646,10 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven LeaveCriticalSection(&hid_hotplug_context.critical_section); + /* A successful registration leaves no stale error behind (the internal + enumeration of an empty system registers "No HID devices found") */ + register_global_error(NULL); + return 0; } @@ -1545,7 +1697,7 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_call LeaveCriticalSection(&hid_hotplug_context.critical_section); - hid_internal_hotplug_unregister_notification(notify_handle); + hid_internal_hotplug_finish_unregistration(notify_handle); if (result != 0) { register_global_error(L"Invalid or unknown hotplug callback handle"); From 1f621c12ab8bd863d5c6adc34f7b9c57fe9a5084 Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Tue, 14 Jul 2026 20:57:20 +0300 Subject: [PATCH 3/9] windows: address round-2 review findings on the async hotplug ENUMERATE pass Critical section lifetime. hid_exit() used to delete the critical section (and reset mutex_ready) with nothing holding it, while a registration sleeping on a pending unregistration, a concurrent deregistration or a late notification could still be about to enter it. The critical section and the quiescence event are now created once and kept for the process lifetime, and an explicit `exiting` state, set under the lock, makes registration and deregistration fail instead of arming a notification behind a teardown. `exiting` is only cleared once hid_exit() has unloaded the libraries, which also closes the window in which a thread-safe (and implicitly initializing) hid_hotplug_register_callback() could call into hid.dll/cfgmgr32.dll while they were being unloaded. Global error string. The internal event context no longer writes it: hid_error(NULL) hands the raw pointer to the application without a lock, so a threadpool work item freeing it underneath an application thread is a use-after-free the application cannot serialize away. The unregistration failure is recorded in internal state and reported by the next hid_exit() instead. The writer-side lock is kept. Failed unregistration. Scoped to the teardown that reports it, so hid_exit() no longer fails forever after one failure. A leaked notification is still live at the OS level, so no second notification is ever armed next to it (both would deliver every event) - hotplug registration fails with an explicit error instead - the libraries it calls into stay loaded, and the module that contains the callback is pinned so that it cannot be unmapped underneath the OS. Arrival dedupe. It only ever existed to absorb the overlap between arming the notification and taking the registration-time snapshot, but it suppressed every arrival whose path was cached - and an interface path is reused when a device is replugged into the same port. It is now scoped to that window: the enumerated paths are recorded, each swallows at most one arrival, and a path is dropped as soon as its device leaves. Outside the window an arrival for a cached path is a new connection, and it replaces the stale record rather than being shadowed by it. Enumeration and cache consistency. A per-device allocation failure is reported through the failure channel instead of passing for a successful (but silently short) enumeration; a device_info is never handed out half-built, so the cache can no longer hold a record with a NULL path - which the removal walk dereferenced - and the bus-specific fixups can no longer read a NULL string. Threadpool. Resolved dynamically from kernel32 (and hotplug registration fails cleanly when it is unavailable) instead of forcing _WIN32_WINNT to 0x0600, which turned the threadpool, SRWLOCK and condition-variable calls into load-time Vista-only imports for every user of the library, including those that never touch hotplug. Assisted-by: claude-code:claude-opus-4-8 --- windows/hid.c | 767 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 599 insertions(+), 168 deletions(-) diff --git a/windows/hid.c b/windows/hid.c index e35116b62..ed304b0db 100644 --- a/windows/hid.c +++ b/windows/hid.c @@ -30,14 +30,6 @@ extern "C" { #undef WIN32_LEAN_AND_MEAN #endif -/* The native threadpool API (CreateThreadpoolWork & co), used to deliver - * the HID_API_HOTPLUG_ENUMERATE pass asynchronously, is only declared - * for Windows Vista and up. */ -#if !defined(_WIN32_WINNT) || (_WIN32_WINNT < 0x0600) -#undef _WIN32_WINNT -#define _WIN32_WINNT 0x0600 -#endif - #include "hidapi_winapi.h" #include @@ -226,29 +218,68 @@ struct hid_device_ { DWORD write_timeout_ms; }; +/* The threadpool work item that delivers the HID_API_HOTPLUG_ENUMERATE pass is + Windows Vista and up, so - like every other API this file uses above its + minimum target - it is resolved dynamically: a static import would raise the + Windows version hidapi can be loaded on for every user, including those that + never use hotplug. Hotplug registration fails with an error when it is not + available; nothing else in the library depends on it. + + The threadpool handles are declared as opaque pointers (which is what they are + in the SDK, too), so that none of this needs headers newer than the file's + minimum target. */ +typedef VOID (WINAPI *hid_internal_tp_work_callback)(PVOID instance, PVOID context, PVOID work); +typedef PVOID (WINAPI *CreateThreadpoolWork_)(hid_internal_tp_work_callback callback, PVOID context, PVOID callback_environ); +typedef VOID (WINAPI *SubmitThreadpoolWork_)(PVOID work); +typedef VOID (WINAPI *CloseThreadpoolWork_)(PVOID work); +typedef VOID (WINAPI *WaitForThreadpoolWorkCallbacks_)(PVOID work, BOOL cancel_pending); + +static CreateThreadpoolWork_ hid_internal_CreateThreadpoolWork = NULL; +static SubmitThreadpoolWork_ hid_internal_SubmitThreadpoolWork = NULL; +static CloseThreadpoolWork_ hid_internal_CloseThreadpoolWork = NULL; +static WaitForThreadpoolWorkCallbacks_ hid_internal_WaitForThreadpoolWorkCallbacks = NULL; + +/* An interface path captured by the registration-time enumeration whose arrival + notification may still be in flight. See hid_internal_hotplug_record_pending_arrivals. */ +struct hid_hotplug_path { + char *path; + struct hid_hotplug_path *next; +}; + static struct hid_hotplug_context { /* Win32 notification handle */ HCMNOTIFICATION notify_handle; - /* Threadpool work item: delivers pending HID_API_HOTPLUG_ENUMERATE + /* Threadpool work item (a PTP_WORK): delivers pending HID_API_HOTPLUG_ENUMERATE passes and performs cleanup deferred from the notification callback. Created with the first callback registration, closed by hid_exit(). */ - PTP_WORK event_work; + PVOID event_work; /* Number of notification handles detached from the context whose - CM_Unregister_Notification call has not completed yet, and the condition - used to wait for that to drop to zero (a zero-initialized - CONDITION_VARIABLE is valid). Guarded by the critical section. - A new notification is only armed once this is zero: the OS keeps a + CM_Unregister_Notification call has not completed yet, and a manual-reset + event that is signaled exactly while that count is zero. Both are guarded + by the critical section (the event is only ever waited on without it). + A new notification is only armed once the count is zero: the OS keeps a detached handle live until the unregistration completes, and two live registrations would deliver every event twice. */ LONG pending_unregistrations; - CONDITION_VARIABLE unregistration_cv; + HANDLE quiescent_event; /* Set when CM_Unregister_Notification failed: the OS-side registration may - still be live, so the state its callbacks can touch is never destroyed */ + still be live and call into this module at any time. Sticky for the whole + process - the state such a notification can reach is never destroyed, the + libraries it calls into are never unloaded, the module is pinned, and no + second notification is ever armed next to it (every event would be + delivered twice). */ + unsigned char notification_leaked; + + /* Same failure, scoped to the teardown that has to report it (hid_exit) */ unsigned char unregistration_failed; + /* Set while hid_exit() is tearing the machinery down: registration and + deregistration fail instead of re-arming into a context being destroyed */ + unsigned char exiting; + /* Critical section (faster mutex substitute), for both cached device list and callback list changes */ CRITICAL_SECTION critical_section; @@ -265,6 +296,10 @@ static struct hid_hotplug_context { /* Linked list of the device infos (mandatory when the device is disconnected) */ struct hid_device_info *devs; + + /* Paths whose arrival notification may still be in flight while it has + already been reported by the registration-time enumeration */ + struct hid_hotplug_path *pending_arrivals; } hid_hotplug_context; /* zero-initialized (static storage); next_handle set on first init */ static hid_device *new_hid_device() @@ -312,7 +347,7 @@ static void free_hid_device(hid_device *dev) free(dev); } -static void register_winapi_error_to_buffer(wchar_t **error_buffer, const WCHAR *op) +static void register_winapi_error_code_to_buffer(wchar_t **error_buffer, const WCHAR *op, DWORD error_code) { free(*error_buffer); *error_buffer = NULL; @@ -323,7 +358,6 @@ static void register_winapi_error_to_buffer(wchar_t **error_buffer, const WCHAR } WCHAR system_err_buf[1024]; - DWORD error_code = GetLastError(); DWORD system_err_len = FormatMessageW( FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, @@ -369,6 +403,15 @@ static void register_winapi_error_to_buffer(wchar_t **error_buffer, const WCHAR } } +static void register_winapi_error_to_buffer(wchar_t **error_buffer, const WCHAR *op) +{ + /* Capture the error code first: free() (and, for the global error buffer, + acquiring its lock) is not required to preserve it */ + DWORD error_code = GetLastError(); + + register_winapi_error_code_to_buffer(error_buffer, op, error_code); +} + #if defined(__GNUC__) # pragma GCC diagnostic push # pragma GCC diagnostic ignored "-Warray-bounds" @@ -402,24 +445,54 @@ static void register_string_error(hid_device *dev, const WCHAR *string_error) register_string_error_to_buffer(&dev->last_error_str, string_error); } +/* A minimal exclusive lock that needs no initialization. + + Deliberately not an SRWLOCK (nor a CONDITION_VARIABLE, nor the CRITICAL_SECTION + that cannot be initialized statically): those APIs are Windows Vista and up, so + using them would turn hidapi into a load-time importer of Vista-only kernel32 + symbols - for every user of the library, including those that never touch + hotplug. Everything this file uses above its minimum target is resolved + dynamically instead (see lookup_functions and hid_internal_hotplug_resolve_threadpool). + The regions guarded by this lock are a handful of instructions long. */ +typedef volatile LONG hid_internal_lock; + +static void hid_internal_lock_acquire(hid_internal_lock *lock) +{ + while (InterlockedCompareExchange(lock, 1, 0) != 0) { + /* The holder is never descheduled for long: yield and retry */ + Sleep(0); + } +} + +static void hid_internal_lock_release(hid_internal_lock *lock) +{ + InterlockedExchange(lock, 0); +} + static wchar_t *last_global_error_str = NULL; /* Serializes mutations of last_global_error_str: the hotplug API is - thread-safe and its failure paths may write the global error concurrently */ -static SRWLOCK global_error_lock = SRWLOCK_INIT; + thread-safe and its failure paths may write the global error concurrently. + Note that this only protects writers against each other: HIDAPI's internal + event context must never write the global error at all, because hid_error(NULL) + hands the raw string pointer out to the application without any lock. */ +static hid_internal_lock global_error_lock = 0; static void register_global_winapi_error(const WCHAR *op) { - AcquireSRWLockExclusive(&global_error_lock); - register_winapi_error_to_buffer(&last_global_error_str, op); - ReleaseSRWLockExclusive(&global_error_lock); + /* Capture the error code before the lock: acquiring it may clobber it */ + DWORD error_code = GetLastError(); + + hid_internal_lock_acquire(&global_error_lock); + register_winapi_error_code_to_buffer(&last_global_error_str, op, error_code); + hid_internal_lock_release(&global_error_lock); } static void register_global_error(const WCHAR *string_error) { - AcquireSRWLockExclusive(&global_error_lock); + hid_internal_lock_acquire(&global_error_lock); register_string_error_to_buffer(&last_global_error_str, string_error); - ReleaseSRWLockExclusive(&global_error_lock); + hid_internal_lock_release(&global_error_lock); } static HANDLE open_device(const wchar_t *path, BOOL open_rw) @@ -450,24 +523,98 @@ HID_API_EXPORT const char* HID_API_CALL hid_version_str(void) } /* Serializes the bootstrap of the hotplug machinery: two racing first - registrations must not both initialize the critical section */ -static SRWLOCK hotplug_init_lock = SRWLOCK_INIT; + registrations must not both initialize the critical section, and every read of + mutex_ready that is not already made under the critical section is made under + this lock (it is what publishes the critical section to other threads). */ +static hid_internal_lock hotplug_init_lock = 0; + +/* Resolves the threadpool API used as the hotplug event context. + Always called inside the critical section. Returns -1 when it is unavailable + (the OS predates it), in which case hotplug is not available either. */ +static int hid_internal_hotplug_resolve_threadpool(void) +{ + HMODULE kernel32; -static void hid_internal_hotplug_init() + if (hid_internal_CreateThreadpoolWork != NULL) { + /* Already resolved: resolved last, so it doubles as the "all set" flag */ + return 0; + } + + /* kernel32.dll is mapped into every process and is never unloaded, so its + handle needs neither LoadLibrary nor FreeLibrary */ + kernel32 = GetModuleHandleW(L"kernel32.dll"); + if (kernel32 == NULL) { + return -1; + } + +/* Avoid direct function-pointer cast from FARPROC to typed callback pointer. + Using memcpy keeps this warning-free regardless of the compiler and compiler settings. */ +#define RESOLVE_TP(x) do { \ + FARPROC proc_addr = GetProcAddress(kernel32, #x); \ + if (!proc_addr) return -1; \ + memcpy(&hid_internal_##x, &proc_addr, sizeof(hid_internal_##x)); \ +} while (0) + + RESOLVE_TP(SubmitThreadpoolWork); + RESOLVE_TP(CloseThreadpoolWork); + RESOLVE_TP(WaitForThreadpoolWorkCallbacks); + RESOLVE_TP(CreateThreadpoolWork); + +#undef RESOLVE_TP + + return 0; +} + +/* Bootstraps the hotplug machinery. The critical section and the quiescence event + are created once and are NEVER destroyed: they are the only things that can + serialize against a notification callback, and no caller - hid_exit() included - + can prove that no callback is about to enter them. Keeping them for the process + lifetime costs one critical section and one event handle, and removes an entire + class of use-after-free (a callback, a threadpool work item or a concurrent + deregistration entering a deleted critical section). + Returns -1 if the machinery cannot be initialized. */ +static int hid_internal_hotplug_init(void) { - AcquireSRWLockExclusive(&hotplug_init_lock); + int result = 0; + + hid_internal_lock_acquire(&hotplug_init_lock); if (!hid_hotplug_context.mutex_ready) { - InitializeCriticalSection(&hid_hotplug_context.critical_section); + /* Manual reset, initially signaled: nothing is pending yet */ + hid_hotplug_context.quiescent_event = CreateEvent(NULL, TRUE, TRUE, NULL); + if (hid_hotplug_context.quiescent_event == NULL) { + register_global_winapi_error(L"hid_hotplug_register_callback/CreateEvent"); + result = -1; + } else { + InitializeCriticalSection(&hid_hotplug_context.critical_section); - /* Set state to Ready */ - hid_hotplug_context.mutex_ready = 1; - hid_hotplug_context.mutex_in_use = 0; - hid_hotplug_context.cb_list_dirty = 0; - hid_hotplug_context.pending_unregistrations = 0; - if (hid_hotplug_context.next_handle < FIRST_HOTPLUG_CALLBACK_HANDLE) - hid_hotplug_context.next_handle = FIRST_HOTPLUG_CALLBACK_HANDLE; + hid_hotplug_context.mutex_in_use = 0; + hid_hotplug_context.cb_list_dirty = 0; + hid_hotplug_context.pending_unregistrations = 0; + if (hid_hotplug_context.next_handle < FIRST_HOTPLUG_CALLBACK_HANDLE) + hid_hotplug_context.next_handle = FIRST_HOTPLUG_CALLBACK_HANDLE; + + /* Set state to Ready. Published last: a thread that observes this + under hotplug_init_lock also observes everything above. */ + hid_hotplug_context.mutex_ready = 1; + } } - ReleaseSRWLockExclusive(&hotplug_init_lock); + hid_internal_lock_release(&hotplug_init_lock); + + return result; +} + +/* Whether the critical section exists and may be entered. Once it does, it stays + valid for the process lifetime (see hid_internal_hotplug_init), so the answer + can never go stale between the check and the EnterCriticalSection. */ +static int hid_internal_hotplug_ready(void) +{ + int ready; + + hid_internal_lock_acquire(&hotplug_init_lock); + ready = hid_hotplug_context.mutex_ready; + hid_internal_lock_release(&hotplug_init_lock); + + return ready; } int HID_API_EXPORT hid_init(void) @@ -530,6 +677,108 @@ static struct hid_device_info *hid_internal_copy_device_info(const struct hid_de return dst; } +/* Pins the module this code lives in, so that it stays mapped even if the + application unloads hidapi: a notification that could not be unregistered is + still live at the OS level and will call hid_internal_notify_callback. + Any address inside the module identifies it; hid_hotplug_context is in the very + same translation unit as the callback. */ +static void hid_internal_hotplug_pin_module(void) +{ + HMODULE module = NULL; + + GetModuleHandleExW(GET_MODULE_HANDLE_EX_FLAG_PIN | GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS, + (LPCWSTR)(void *)&hid_hotplug_context, &module); +} + +/* Always called inside a locked mutex */ +static void hid_internal_hotplug_free_pending_arrivals(void) +{ + struct hid_hotplug_path *current = hid_hotplug_context.pending_arrivals; + + hid_hotplug_context.pending_arrivals = NULL; + + while (current != NULL) { + struct hid_hotplug_path *next = current->next; + free(current->path); + free(current); + current = next; + } +} + +/* Records the interface paths the registration-time enumeration has just captured. + Always called inside a locked mutex; returns -1 on allocation failure. + + The notification is armed BEFORE the enumeration runs (a device connecting in + between must not be missed by both), so a device that arrives in that window is + picked up by the enumeration AND has an arrival notification in flight. That + notification must not report - or cache - the same connection a second time. + + This window is the only place where a duplicate arrival can occur, so the + suppression is scoped to it: a recorded path swallows at most one arrival and is + dropped as soon as the device leaves. Outside of it, an arrival for a path that + is still cached is NOT a duplicate: an interface path is reused when a device is + replugged into the same port, so suppressing it would swallow a real connection. */ +static int hid_internal_hotplug_record_pending_arrivals(void) +{ + for (struct hid_device_info *device = hid_hotplug_context.devs; device != NULL; device = device->next) { + struct hid_hotplug_path *entry = (struct hid_hotplug_path *)calloc(1, sizeof(struct hid_hotplug_path)); + + if (entry == NULL) { + return -1; + } + + /* Cached devices always have a path (hid_internal_get_device_info fails without one) */ + entry->path = _strdup(device->path); + if (entry->path == NULL) { + free(entry); + return -1; + } + + entry->next = hid_hotplug_context.pending_arrivals; + hid_hotplug_context.pending_arrivals = entry; + } + + return 0; +} + +/* Consumes the pending-arrival record for this path, if there is one. + Always called inside a locked mutex. + Returns 1 when this arrival was already reported by the registration-time + enumeration (see hid_internal_hotplug_record_pending_arrivals) and must be + dropped, 0 when it is a genuine new connection. */ +static int hid_internal_hotplug_take_pending_arrival(const char *path) +{ + for (struct hid_hotplug_path **current = &hid_hotplug_context.pending_arrivals; *current != NULL; current = &(*current)->next) { + /* Case-independent path comparison is mandatory */ + if (_stricmp((*current)->path, path) == 0) { + struct hid_hotplug_path *entry = *current; + *current = entry->next; + free(entry->path); + free(entry); + return 1; + } + } + + return 0; +} + +/* Unlinks the cached device with this path, if there is one, and hands it to the + caller (who owns it). Always called inside a locked mutex. */ +static struct hid_device_info *hid_internal_hotplug_take_cached_device(const char *path) +{ + for (struct hid_device_info **current = &hid_hotplug_context.devs; *current != NULL; current = &(*current)->next) { + /* Case-independent path comparison is mandatory */ + if ((*current)->path != NULL && _stricmp((*current)->path, path) == 0) { + struct hid_device_info *device = *current; + *current = device->next; + device->next = NULL; + return device; + } + } + + return NULL; +} + static void hid_internal_hotplug_remove_postponed() { /* Unregister the callbacks whose removal was postponed */ @@ -572,18 +821,45 @@ static void hid_internal_hotplug_finish_unregistration(HCMNOTIFICATION notify_ha EnterCriticalSection(&hid_hotplug_context.critical_section); if (cr != CR_SUCCESS) { - /* The OS-side registration may still be live: taint the epoch so - hid_exit() never destroys the state a late notification can touch */ + /* The OS-side registration may still be live and call into this module at + any time. Everything it can reach has to survive: the critical section and + the quiescence event are never destroyed anyway, the resolved libraries are + never unloaded (see hid_exit), the module is pinned, and no second + notification is ever armed next to this one - the two would deliver every + event twice. hid_exit() reports the failure; this function also runs on the + internal event context, which must never touch the global error string (an + application thread may be reading it through hid_error(NULL)). */ + hid_hotplug_context.notification_leaked = 1; hid_hotplug_context.unregistration_failed = 1; + hid_internal_hotplug_pin_module(); } hid_hotplug_context.pending_unregistrations--; if (hid_hotplug_context.pending_unregistrations == 0) { - WakeAllConditionVariable(&hid_hotplug_context.unregistration_cv); + SetEvent(hid_hotplug_context.quiescent_event); } LeaveCriticalSection(&hid_hotplug_context.critical_section); +} - if (cr != CR_SUCCESS) { - register_global_error(L"CM_Unregister_Notification failed for Hotplug notification"); +/* Waits until every detached notification handle has been unregistered. + Must be called WITHOUT the critical section: the unregistration completes on + another thread, which needs it. */ +static void hid_internal_hotplug_wait_quiescent(void) +{ + for (;;) { + int pending; + + EnterCriticalSection(&hid_hotplug_context.critical_section); + pending = (hid_hotplug_context.pending_unregistrations > 0); + LeaveCriticalSection(&hid_hotplug_context.critical_section); + + if (!pending) { + return; + } + + if (WaitForSingleObject(hid_hotplug_context.quiescent_event, INFINITE) == WAIT_FAILED) { + /* Should never happen; do not spin forever on a broken event */ + return; + } } } @@ -615,11 +891,14 @@ static HCMNOTIFICATION hid_internal_hotplug_cleanup() hid_hotplug_context.devs = NULL; } + hid_internal_hotplug_free_pending_arrivals(); + notify_handle = hid_hotplug_context.notify_handle; hid_hotplug_context.notify_handle = NULL; if (notify_handle != NULL) { /* Balanced by hid_internal_hotplug_finish_unregistration */ hid_hotplug_context.pending_unregistrations++; + ResetEvent(hid_hotplug_context.quiescent_event); } return notify_handle; } @@ -655,7 +934,7 @@ static void hid_internal_hotplug_replay_flush(struct hid_hotplug_callback *callb /* Threadpool work item: delivers pending HID_API_HOTPLUG_ENUMERATE passes (unless a live event got to them first) and performs the cleanup the notification callback is not allowed to perform itself. */ -static VOID CALLBACK hid_internal_hotplug_event_work(PTP_CALLBACK_INSTANCE instance, PVOID context, PTP_WORK work) +static VOID WINAPI hid_internal_hotplug_event_work(PVOID instance, PVOID context, PVOID work) { HCMNOTIFICATION notify_handle; @@ -678,86 +957,162 @@ static VOID CALLBACK hid_internal_hotplug_event_work(PTP_CALLBACK_INSTANCE insta hid_internal_hotplug_finish_unregistration(notify_handle); } -static int hid_internal_hotplug_exit() +/* Whether a notification could not be unregistered at some point in this process + and may still be live at the OS level. Sticky: see hid_hotplug_context. */ +static int hid_internal_hotplug_notification_leaked(void) { - HCMNOTIFICATION notify_handle; - int failed_epoch; + int leaked; - if (!hid_hotplug_context.mutex_ready) { - /* If the critical section is not initialized, we are safe to assume nothing else is */ + if (!hid_internal_hotplug_ready()) { return 0; } + EnterCriticalSection(&hid_hotplug_context.critical_section); - struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; - /* Remove all callbacks from the list, including their undelivered HID_API_HOTPLUG_ENUMERATE snapshots */ - while (*current) { - struct hid_hotplug_callback *next = (*current)->next; - hid_free_enumeration((*current)->replay); - free(*current); - *current = next; + leaked = (hid_hotplug_context.notification_leaked != 0); + LeaveCriticalSection(&hid_hotplug_context.critical_section); + + return leaked; +} + +/* Ends the teardown started by hid_internal_hotplug_exit: see the `exiting` flag. + Called by hid_exit() once the resolved libraries are gone, too. */ +static void hid_internal_hotplug_exit_done(void) +{ + if (!hid_internal_hotplug_ready()) { + return; } - notify_handle = hid_internal_hotplug_cleanup(); + + EnterCriticalSection(&hid_hotplug_context.critical_section); + hid_hotplug_context.exiting = 0; LeaveCriticalSection(&hid_hotplug_context.critical_section); +} - hid_internal_hotplug_finish_unregistration(notify_handle); +static int hid_internal_hotplug_exit(void) +{ + HCMNOTIFICATION notify_handle; + PVOID event_work; + int failed; + + /* Not a no-op even when hotplug was never used: `exiting` is what keeps a + concurrent hid_hotplug_register_callback() - which is thread-safe, and + initializes the library implicitly - from calling into hid.dll/cfgmgr32.dll + while hid_exit() is unloading them. It needs the critical section to be there. */ + if (hid_internal_hotplug_init() < 0) { + /* Nothing can be armed if the machinery cannot even be initialized */ + return 0; + } - /* Quiescence: unregistrations started by concurrent (contract-legal) - hid_hotplug_deregister_callback calls must complete before the state - their notifications can still touch is destroyed. The sleep releases - the critical section while waiting. */ EnterCriticalSection(&hid_hotplug_context.critical_section); - while (hid_hotplug_context.pending_unregistrations > 0) { - if (!SleepConditionVariableCS(&hid_hotplug_context.unregistration_cv, &hid_hotplug_context.critical_section, INFINITE)) { - /* Should never happen; treat like a failed unregistration rather than spinning */ - break; + + /* Nothing may arm the machinery from here on. Without this, a registration + waiting for a pending unregistration to complete could wake up behind this + teardown, arm a fresh notification and append a callback to a context that + is being dismantled - leaving a live notification and a "registered" + callback behind hid_exit(). */ + hid_hotplug_context.exiting = 1; + + /* Remove all callbacks from the list, including their undelivered HID_API_HOTPLUG_ENUMERATE snapshots */ + { + struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; + while (*current) { + struct hid_hotplug_callback *next = (*current)->next; + hid_free_enumeration((*current)->replay); + free(*current); + *current = next; } } - failed_epoch = (hid_hotplug_context.unregistration_failed != 0) || (hid_hotplug_context.pending_unregistrations > 0); + notify_handle = hid_internal_hotplug_cleanup(); LeaveCriticalSection(&hid_hotplug_context.critical_section); - if (failed_epoch) { - /* At least one notification may still be live at the OS level: keep the - critical section and the work item alive (deliberately leaked), so a - late callback touches valid, empty state instead of freed memory */ - register_global_error(L"hid_exit: a hotplug notification could not be unregistered"); - return -1; + hid_internal_hotplug_finish_unregistration(notify_handle); + + /* Quiescence: unregistrations started by concurrent (contract-legal) + hid_hotplug_deregister_callback calls must complete before anything a + notification can still reach is released */ + hid_internal_hotplug_wait_quiescent(); + + EnterCriticalSection(&hid_hotplug_context.critical_section); + if (hid_hotplug_context.pending_unregistrations > 0) { + /* Only reachable if waiting itself failed: treat it as a live notification */ + hid_hotplug_context.notification_leaked = 1; + hid_hotplug_context.unregistration_failed = 1; + hid_internal_hotplug_pin_module(); } + /* Scoped to this teardown: a later hid_exit() has nothing of its own to report + (a hotplug callback can no longer be registered once a notification leaked) */ + failed = (hid_hotplug_context.unregistration_failed != 0); + hid_hotplug_context.unregistration_failed = 0; - if (hid_hotplug_context.event_work) { - /* Wait for any in-flight work item before the critical section goes away. - hid_exit() must not be called from a callback, so this cannot deadlock. */ - WaitForThreadpoolWorkCallbacks(hid_hotplug_context.event_work, FALSE); - CloseThreadpoolWork(hid_hotplug_context.event_work); + event_work = hid_hotplug_context.event_work; + if (!failed && !hid_hotplug_context.notification_leaked) { hid_hotplug_context.event_work = NULL; + } else { + /* A notification may still fire and submit to the work item: keep it + (deliberately leaked), along with the critical section and the device + cache it works on */ + event_work = NULL; } + LeaveCriticalSection(&hid_hotplug_context.critical_section); - /* A last-gasp notification callback may have re-added a device between - the cleanup and the unregistration; nothing can race us anymore here */ - if (hid_hotplug_context.devs != NULL) { - hid_free_enumeration(hid_hotplug_context.devs); - hid_hotplug_context.devs = NULL; + if (event_work != NULL) { + /* No notification is registered anymore (CM_Unregister_Notification only + returns once its callbacks have finished) and nothing can submit new work + while `exiting` is set, so the work item can be drained and closed. + Not under the critical section: the work item takes it. + hid_exit() must not be called from a hotplug callback, so the wait cannot + deadlock on the calling thread itself. */ + hid_internal_WaitForThreadpoolWorkCallbacks(event_work, FALSE); + hid_internal_CloseThreadpoolWork(event_work); } - hid_hotplug_context.mutex_ready = 0; - DeleteCriticalSection(&hid_hotplug_context.critical_section); + EnterCriticalSection(&hid_hotplug_context.critical_section); + /* A last-gasp notification callback may have re-added a device between the + cleanup and the completion of the unregistration */ + hid_free_enumeration(hid_hotplug_context.devs); + hid_hotplug_context.devs = NULL; + hid_internal_hotplug_free_pending_arrivals(); + LeaveCriticalSection(&hid_hotplug_context.critical_section); + + /* `exiting` stays set until hid_exit() is done unloading the libraries: + see hid_internal_hotplug_exit_done. + The critical section and the quiescence event are kept for the process + lifetime on purpose: see hid_internal_hotplug_init */ + + if (failed) { + register_global_error(L"hid_exit: a hotplug notification could not be unregistered"); + return -1; + } return 0; } int HID_API_EXPORT hid_exit(void) { - if (hid_internal_hotplug_exit() < 0) { - /* A hotplug notification could not be unregistered and may still fire: - keep the resolved DLLs loaded (deliberately leaked) so a late - callback does not call into unloaded code. - register_global_error: set by hid_internal_hotplug_exit */ - return -1; - } + int result = hid_internal_hotplug_exit(); #ifndef HIDAPI_USE_DDK - free_library_handles(); - hidapi_initialized = FALSE; + if (!hid_internal_hotplug_notification_leaked()) { + /* Still under `exiting`: a concurrent (thread-safe) hotplug registration + fails instead of calling into libraries that are being unloaded here. + Every other call that goes through them either holds the critical section + or is accounted for by pending_unregistrations, which the teardown above + has already waited out. */ + free_library_handles(); + hidapi_initialized = FALSE; + } + /* else: a hotplug notification is still registered at the OS level and its + callback calls into hid.dll/cfgmgr32.dll through these handles: they stay + loaded (deliberately leaked, and the module is pinned) so that a late + notification never calls into unloaded code */ #endif + + hid_internal_hotplug_exit_done(); + + if (result < 0) { + /* register_global_error: set by hid_internal_hotplug_exit */ + return -1; + } + register_global_error(NULL); return 0; @@ -1118,6 +1473,12 @@ static struct hid_device_info *hid_internal_get_device_info(const wchar_t *path, /* Fill out the record */ dev->next = NULL; dev->path = hid_internal_UTF16toUTF8(path); + if (dev->path == NULL) { + /* A record without a path is useless to the caller and unusable as the key + of the hotplug device cache (where it would crash the removal lookup) */ + free(dev); + return NULL; + } dev->interface_number = -1; attrib.Size = sizeof(HIDD_ATTRIBUTES); @@ -1163,6 +1524,13 @@ static struct hid_device_info *hid_internal_get_device_info(const wchar_t *path, HidD_GetProductString(handle, string, size); dev->product_string = _wcsdup(string); + if (dev->serial_number == NULL || dev->manufacturer_string == NULL || dev->product_string == NULL) { + /* Out of memory. A half-built record is not a device (and the bus-specific + fixups right below dereference these strings) */ + hid_free_enumeration(dev); + return NULL; + } + /* now, the portion that depends on string descriptors */ switch (dev->bus_type) { case HID_API_BUS_USB: @@ -1265,7 +1633,16 @@ static struct hid_device_info *hid_internal_enumerate(unsigned short vendor_id, struct hid_device_info *tmp = hid_internal_get_device_info(device_interface, device_handle); if (tmp == NULL) { - goto cont_close; + /* Out of memory. Report a failure rather than a partial list: a + list that silently misses devices is indistinguishable from the + system not having them, and the hotplug device cache and the + HID_API_HOTPLUG_ENUMERATE snapshot are built from it. */ + register_global_error(L"Failed to allocate memory for a device info"); + *failure = 1; + CloseHandle(device_handle); + hid_free_enumeration(root); + root = NULL; + goto end_of_function; } if (cur_dev) { @@ -1333,35 +1710,33 @@ DWORD WINAPI hid_internal_notify_callback(HCMNOTIFICATION notify, PVOID context, EnterCriticalSection(&hid_hotplug_context.critical_section); if (action == CM_NOTIFY_ACTION_DEVICEINTERFACEARRIVAL) { - char *path; - int known = 0; + char *path = hid_internal_UTF16toUTF8(event_data->u.DeviceInterface.SymbolicLink); hotplug_event = HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED; - /* An arrival for a path already in the cache is a duplicate: the - registration-time enumeration overlaps with the already-armed - notification, and a notification being unregistered may briefly - coexist with its replacement. Deduplicating here keeps each - connection reported (and cached) exactly once. */ - path = hid_internal_UTF16toUTF8(event_data->u.DeviceInterface.SymbolicLink); - if (path != NULL) { - for (struct hid_device_info *current = hid_hotplug_context.devs; current != NULL; current = current->next) { - /* Case-independent path comparison is mandatory */ - if (current->path != NULL && _stricmp(current->path, path) == 0) { - known = 1; - break; - } - } - free(path); - } - - if (!known) { + if (path == NULL) { + /* Out of memory: there is nothing to report the connection with, and + nothing is cached, so the cache stays consistent */ + } else if (hid_internal_hotplug_take_pending_arrival(path)) { + /* Already reported (and cached) by the registration-time enumeration: + see hid_internal_hotplug_record_pending_arrivals */ + } else { /* Open read-only handle to the device */ HANDLE read_handle = open_device(event_data->u.DeviceInterface.SymbolicLink, FALSE); /* Check validity of read_handle. */ if (read_handle != INVALID_HANDLE_VALUE) { device = hid_internal_get_device_info(event_data->u.DeviceInterface.SymbolicLink, read_handle); + CloseHandle(read_handle); + } + + if (device != NULL) { + /* An interface path is reused when a device is replugged into the + same port, so an arrival for a path that is still cached means the + removal of the previous connection was missed. Drop the stale record + instead of shadowing it: a second record for the same path would + never be removed, and the connection is reported as the new one it is. */ + hid_free_enumeration(hid_internal_hotplug_take_cached_device(device->path)); /* Append to the end of the device list */ if (hid_hotplug_context.devs != NULL) { @@ -1373,10 +1748,17 @@ DWORD WINAPI hid_internal_notify_callback(HCMNOTIFICATION notify, PVOID context, } else { hid_hotplug_context.devs = device; } - - CloseHandle(read_handle); } + /* else: the interface could not be opened or described. The usual cause + is that it disconnected again before we got to it - no connection is + owed then: nothing is cached, so the removal notification that follows + finds nothing and reports nothing either, which is consistent. An + out-of-memory failure does lose the connection, but there is nothing to + report it with; the cache is left consistent either way, as only fully + described devices are ever cached. */ } + + free(path); } else if (action == CM_NOTIFY_ACTION_DEVICEINTERFACEREMOVAL) { char *path; @@ -1385,17 +1767,12 @@ DWORD WINAPI hid_internal_notify_callback(HCMNOTIFICATION notify, PVOID context, path = hid_internal_UTF16toUTF8(event_data->u.DeviceInterface.SymbolicLink); if (path != NULL) { + /* The device is gone: a later arrival on the same path is a new + connection and must not be mistaken for a pending duplicate */ + hid_internal_hotplug_take_pending_arrival(path); + /* Get and remove this device from the device list */ - for (struct hid_device_info **current = &hid_hotplug_context.devs; *current; current = &(*current)->next) { - /* Case-independent path comparison is mandatory */ - if (_stricmp((*current)->path, path) == 0) { - struct hid_device_info *next = (*current)->next; - device = *current; - device->next = NULL; - *current = next; - break; - } - } + device = hid_internal_hotplug_take_cached_device(path); free(path); } @@ -1448,7 +1825,7 @@ DWORD WINAPI hid_internal_notify_callback(HCMNOTIFICATION notify, PVOID context, notification from its own callback is not allowed (deadlock) */ hid_internal_hotplug_remove_postponed(); if (hid_hotplug_context.hotplug_cbs == NULL && hid_hotplug_context.event_work != NULL) { - SubmitThreadpoolWork(hid_hotplug_context.event_work); + hid_internal_SubmitThreadpoolWork(hid_hotplug_context.event_work); } } @@ -1498,49 +1875,88 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven hotplug_cb->replay = NULL; /* Ensure we are ready to actually use the mutex */ - hid_internal_hotplug_init(); + if (hid_internal_hotplug_init() < 0) { + /* register_global_error: set by hid_internal_hotplug_init */ + free(hotplug_cb); + return -1; + } /* Lock the mutex to avoid race conditions */ EnterCriticalSection(&hid_hotplug_context.critical_section); + for (;;) { + if (hid_hotplug_context.exiting) { + /* hid_exit() is tearing the machinery down: arming it again behind its + back would leave a live notification and a registered callback with no + context to run in */ + register_global_error(L"hid_exit() is in progress"); + LeaveCriticalSection(&hid_hotplug_context.critical_section); + free(hotplug_cb); + return -1; + } + + /* A notification detached by a concurrent deregistration may still be live + at the OS until its unregistration completes; arming a replacement in + that window would deliver every event twice. */ + if (hid_hotplug_context.hotplug_cbs != NULL || hid_hotplug_context.notify_handle != NULL + || hid_hotplug_context.pending_unregistrations == 0) { + break; + } + + /* The unregistration completes on another thread (or on the event context), + which needs the critical section, so the wait must not hold it. On wake the + state is re-evaluated from scratch. */ + LeaveCriticalSection(&hid_hotplug_context.critical_section); + if (WaitForSingleObject(hid_hotplug_context.quiescent_event, INFINITE) == WAIT_FAILED) { + register_global_winapi_error(L"hid_hotplug_register_callback/WaitForSingleObject"); + free(hotplug_cb); + return -1; + } + EnterCriticalSection(&hid_hotplug_context.critical_section); + } + + if (hid_hotplug_context.notification_leaked) { + /* A notification could not be unregistered and may still be live at the OS + level: a second one would deliver every event twice, to every callback. + The condition is sticky (and unreachable in practice), so hotplug stays + unavailable for the rest of the process. */ + register_global_error(L"A hotplug notification could not be unregistered: hotplug is no longer available"); + LeaveCriticalSection(&hid_hotplug_context.critical_section); + free(hotplug_cb); + return -1; + } + /* Handle values are never reused while the library remains initialized */ if (hid_hotplug_context.next_handle >= INT_MAX) { + register_global_error(L"Hotplug callback handles exhausted"); LeaveCriticalSection(&hid_hotplug_context.critical_section); free(hotplug_cb); - register_global_error(L"Hotplug callback handles exhausted"); return -1; } hotplug_cb->handle = hid_hotplug_context.next_handle++; - /* A notification detached by a concurrent deregistration may still be live - at the OS until its unregistration completes; arming a replacement in - that window would deliver every event twice. The sleep releases the - critical section, so on wake the state is re-evaluated from scratch. */ - while (hid_hotplug_context.hotplug_cbs == NULL && hid_hotplug_context.notify_handle == NULL - && hid_hotplug_context.pending_unregistrations > 0) { - if (!SleepConditionVariableCS(&hid_hotplug_context.unregistration_cv, &hid_hotplug_context.critical_section, INFINITE)) { + /* Start the machinery with the first callback */ + if (hid_hotplug_context.hotplug_cbs == NULL) { + if (hid_init() < 0) { + /* register_global_error: global error is already set by hid_init */ LeaveCriticalSection(&hid_hotplug_context.critical_section); free(hotplug_cb); - register_global_winapi_error(L"hid_hotplug_register_callback/SleepConditionVariableCS"); return -1; } - } - /* Start the machinery with the first callback */ - if (hid_hotplug_context.hotplug_cbs == NULL) { - if (hid_init() < 0) { - /* register_global_error: global error is already set by hid_init */ + if (hid_internal_hotplug_resolve_threadpool() < 0) { + register_global_error(L"Hotplug is not supported: the threadpool API is unavailable"); LeaveCriticalSection(&hid_hotplug_context.critical_section); free(hotplug_cb); return -1; } if (hid_hotplug_context.event_work == NULL) { - hid_hotplug_context.event_work = CreateThreadpoolWork(hid_internal_hotplug_event_work, NULL, NULL); + hid_hotplug_context.event_work = hid_internal_CreateThreadpoolWork(hid_internal_hotplug_event_work, NULL, NULL); if (hid_hotplug_context.event_work == NULL) { + register_global_winapi_error(L"hid_hotplug_register_callback/CreateThreadpoolWork"); LeaveCriticalSection(&hid_hotplug_context.critical_section); free(hotplug_cb); - register_global_winapi_error(L"hid_hotplug_register_callback/CreateThreadpoolWork"); return -1; } } @@ -1561,36 +1977,39 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven notify_filter.u.DeviceInterface.ClassGuid = interface_class_guid; /* Register for a HID device notification when adding the first callback. - Armed BEFORE the device cache is filled: a device connecting in - between is then caught by the notification and deduplicated against - the cache, instead of being missed by both. */ + Armed BEFORE the device cache is filled: a device connecting in between + is then caught by the notification instead of being missed by both, and + the duplicate that this creates is suppressed exactly once + (see hid_internal_hotplug_record_pending_arrivals). */ if (CM_Register_Notification(¬ify_filter, NULL, hid_internal_notify_callback, &hid_hotplug_context.notify_handle) != CR_SUCCESS) { + register_global_error(L"hid_hotplug_register_callback/CM_Register_Notification"); hid_hotplug_context.notify_handle = NULL; LeaveCriticalSection(&hid_hotplug_context.critical_section); free(hotplug_cb); - register_global_error(L"hid_hotplug_register_callback/CM_Register_Notification"); return -1; } - /* Normally NULL already; an old notification may have appended - entries between its detachment and the completion of its - unregistration (or, after a failed unregistration, at any time) */ - if (hid_hotplug_context.devs != NULL) { - hid_free_enumeration(hid_hotplug_context.devs); - hid_hotplug_context.devs = NULL; - } + /* Normally empty already; an old notification may have appended entries + between its detachment and the completion of its unregistration */ + hid_free_enumeration(hid_hotplug_context.devs); + hid_hotplug_context.devs = NULL; + hid_internal_hotplug_free_pending_arrivals(); /* Fill already connected devices so we can use this info in disconnection notifications and HID_API_HOTPLUG_ENUMERATE passes */ hid_hotplug_context.devs = hid_internal_enumerate(0, 0, &enumerate_failure); + if (!enumerate_failure && hid_internal_hotplug_record_pending_arrivals() < 0) { + register_global_error(L"Failed to allocate memory for the hotplug device cache"); + enumerate_failure = 1; + } if (enumerate_failure) { /* An empty system is fine; a failed enumeration is not: the device - cache and the ENUMERATE snapshot would misrepresent the system */ + cache and the ENUMERATE snapshot would misrepresent the system. + register_global_error: set above or by hid_internal_enumerate */ HCMNOTIFICATION notify_handle = hid_internal_hotplug_cleanup(); LeaveCriticalSection(&hid_hotplug_context.critical_section); hid_internal_hotplug_finish_unregistration(notify_handle); free(hotplug_cb); - /* register_global_error: set by hid_internal_enumerate */ return -1; } } @@ -1609,13 +2028,13 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven *replay_tail = hid_internal_copy_device_info(device); if (*replay_tail == NULL) { HCMNOTIFICATION notify_handle; + register_global_error(L"Failed to allocate memory for a device info snapshot"); hid_free_enumeration(hotplug_cb->replay); /* Tear the machinery down if this would-be-first callback was starting it */ notify_handle = hid_internal_hotplug_cleanup(); LeaveCriticalSection(&hid_hotplug_context.critical_section); hid_internal_hotplug_finish_unregistration(notify_handle); free(hotplug_cb); - register_global_error(L"Failed to allocate memory for a device info snapshot"); return -1; } replay_tail = &(*replay_tail)->next; @@ -1641,15 +2060,17 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven /* Have the snapshot delivered on the event context; never from within this call */ if (hotplug_cb->replay != NULL) { - SubmitThreadpoolWork(hid_hotplug_context.event_work); + hid_internal_SubmitThreadpoolWork(hid_hotplug_context.event_work); } - LeaveCriticalSection(&hid_hotplug_context.critical_section); - /* A successful registration leaves no stale error behind (the internal - enumeration of an empty system registers "No HID devices found") */ + enumeration of an empty system registers "No HID devices found"). + Under the critical section: outside of it this would race with - and wipe - + the error string of a concurrently failing registration. */ register_global_error(NULL); + LeaveCriticalSection(&hid_hotplug_context.critical_section); + return 0; } @@ -1658,7 +2079,7 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_call int result = -1; HCMNOTIFICATION notify_handle; - if (callback_handle <= 0 || !hid_hotplug_context.mutex_ready) { + if (callback_handle <= 0 || !hid_internal_hotplug_ready()) { register_global_error(L"Invalid or unknown hotplug callback handle"); return -1; } @@ -1666,6 +2087,14 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_call /* Lock the mutex to avoid race conditions */ EnterCriticalSection(&hid_hotplug_context.critical_section); + if (hid_hotplug_context.exiting) { + /* hid_exit() deregisters every callback and invalidates every handle: + this one is (or is about to be) one of them */ + register_global_error(L"Invalid or unknown hotplug callback handle"); + LeaveCriticalSection(&hid_hotplug_context.critical_section); + return -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) { @@ -1693,16 +2122,18 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_call } } + if (result != 0) { + /* Under the critical section: outside of it this would race with - and wipe - + the error string of a concurrently failing hotplug call */ + register_global_error(L"Invalid or unknown hotplug callback handle"); + } + notify_handle = hid_internal_hotplug_cleanup(); LeaveCriticalSection(&hid_hotplug_context.critical_section); hid_internal_hotplug_finish_unregistration(notify_handle); - if (result != 0) { - register_global_error(L"Invalid or unknown hotplug callback handle"); - } - return result; } From 0ddecf2fd6b507fd8d023cc2f706039cb041d603 Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Tue, 14 Jul 2026 22:00:55 +0300 Subject: [PATCH 4/9] windows: address round-3 review findings on the async hotplug ENUMERATE pass Machinery lifetime, take three. Round 2 made hid_exit() bootstrap the hotplug machinery just to raise the `exiting` flag, and never destroyed it - so every program, including ones that never touch hotplug, had hid_exit() create a kernel event and a critical section it could not free: one leaked handle per load-use-unload cycle in a plugin-style host. The flag does not need the critical section: `hotplug_exiting` now lives under the statically initialized bootstrap spinlock (which costs no OS object), so hid_exit() can raise it before knowing whether the machinery even exists and return without creating anything. On top of that, public hotplug calls are now counted into the machinery through the same lock (enter/leave), which gives hid_exit() the proof round 2 lacked: once `exiting` cuts off new entries, every notification is unregistered, the work item is drained and closed, and the user count has hit zero, nothing can be inside or blocked on the critical section - so the clean path destroys the event and the critical section it created instead of keeping them for the process lifetime. When a notification could not be unregistered, everything is still kept, exactly as in round 2: a live OS callback must never find a deleted critical section. 100 x (LoadLibrary, hid_init, hid_enumerate, hid_exit, FreeLibrary) now leaks 0 handles (was +1 per cycle), with or without hotplug use in the loop. Pending-arrivals dedupe, scoped in time as well. The registration-time records suppressed the one in-flight duplicate they were made for, but a record whose device never re-arrived outlived that window - so a device whose removal notification was missed had its genuine re-arrival swallowed by its own stale record, and the stale-record recovery could never run for exactly the devices it was written for. The list now expires (10 s, far beyond any notification delivery): an expired record no longer marks a duplicate, and the failure modes are asymmetric in the right direction - an over-aged duplicate produces a spurious left+arrived pair instead of a silently dropped connection. The owed DEVICE_LEFT. Replacing a stale cache record on a same-path arrival reported the new connection but silently discarded the previous one, although the header promises a "left" event for every matching device that disconnects while a callback is registered. The live dispatch loop is factored out and the stale record is now dispatched as a synthetic DEVICE_LEFT - before the new device is cached, so a callback registered from within sees the connection exactly once. Spinlock discipline. The global error string is now built and freed outside the spinlock - only the pointer swap is guarded - and the acquire loop escalates from Sleep(0) to Sleep(1) after a few attempts: Sleep(0) yields only to equal-or-higher priority threads, so spinning on a lower-priority holder on a single CPU made no progress until the balance-set manager intervened. Enumeration failure taxonomy. hid_internal_UTF16toUTF8() returns NULL both for invalid UTF-16 (e.g. an unpaired surrogate in an interface path) and on allocation failure, and round 2 escalated both to a full enumeration failure reported as out-of-memory - one malformed path lost every other device. The two are now told apart: an unrepresentable device is skipped (consistently with the hotplug notifications, which can neither report nor cache it), and only genuine allocation failures abort the pass. Also corrects the comment on the global error lock (a user callback calling the public API from the event context does write the global error; the header's serialization requirement covers it), removes a dead `failed` term that notification_leaked always implies, and documents why the registration wait loop may release the critical section unconditionally. Assisted-by: claude-code:claude-fable-5 --- windows/hid.c | 634 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 448 insertions(+), 186 deletions(-) diff --git a/windows/hid.c b/windows/hid.c index ed304b0db..ff37cdcb3 100644 --- a/windows/hid.c +++ b/windows/hid.c @@ -246,6 +246,14 @@ struct hid_hotplug_path { struct hid_hotplug_path *next; }; +/* How long (in milliseconds) the pending-arrivals list stays authoritative. + The duplicate arrival it suppresses was already queued by the OS when the + list was recorded and is delivered about as soon as the registration releases + the critical section, so anything this old is not that duplicate - it is a + genuine re-arrival on a path whose removal notification was missed (see + hid_internal_hotplug_take_pending_arrival). */ +#define HID_HOTPLUG_PENDING_ARRIVALS_TTL_MS 10000 + static struct hid_hotplug_context { /* Win32 notification handle */ HCMNOTIFICATION notify_handle; @@ -276,10 +284,6 @@ static struct hid_hotplug_context { /* Same failure, scoped to the teardown that has to report it (hid_exit) */ unsigned char unregistration_failed; - /* Set while hid_exit() is tearing the machinery down: registration and - deregistration fail instead of re-arming into a context being destroyed */ - unsigned char exiting; - /* Critical section (faster mutex substitute), for both cached device list and callback list changes */ CRITICAL_SECTION critical_section; @@ -298,8 +302,11 @@ static struct hid_hotplug_context { struct hid_device_info *devs; /* Paths whose arrival notification may still be in flight while it has - already been reported by the registration-time enumeration */ + already been reported by the registration-time enumeration, and the + GetTickCount() timestamp of the moment they were recorded (see + HID_HOTPLUG_PENDING_ARRIVALS_TTL_MS) */ struct hid_hotplug_path *pending_arrivals; + DWORD pending_arrivals_tick; } hid_hotplug_context; /* zero-initialized (static storage); next_handle set on first init */ static hid_device *new_hid_device() @@ -453,14 +460,26 @@ static void register_string_error(hid_device *dev, const WCHAR *string_error) symbols - for every user of the library, including those that never touch hotplug. Everything this file uses above its minimum target is resolved dynamically instead (see lookup_functions and hid_internal_hotplug_resolve_threadpool). - The regions guarded by this lock are a handful of instructions long. */ + The regions guarded by this lock are kept deliberately small - pointer swaps + and flag/counter updates; the one-time bootstrap of the hotplug machinery is + the largest. */ typedef volatile LONG hid_internal_lock; static void hid_internal_lock_acquire(hid_internal_lock *lock) { + unsigned int attempts = 0; + while (InterlockedCompareExchange(lock, 1, 0) != 0) { - /* The holder is never descheduled for long: yield and retry */ - Sleep(0); + /* Sleep(0) yields the rest of the quantum, but only to threads of equal + or higher priority: if the holder is lower-priority (or starved on a + single-CPU system), spinning on it can burn quanta without any + progress. After a few attempts, Sleep(1) instead: it yields to any + ready thread. */ + if (++attempts < 16) { + Sleep(0); + } else { + Sleep(1); + } } } @@ -473,26 +492,47 @@ static wchar_t *last_global_error_str = NULL; /* Serializes mutations of last_global_error_str: the hotplug API is thread-safe and its failure paths may write the global error concurrently. - Note that this only protects writers against each other: HIDAPI's internal - event context must never write the global error at all, because hid_error(NULL) - hands the raw string pointer out to the application without any lock. */ + Note that this only protects writers against each other: hid_error(NULL) + hands the raw string pointer out to the application without any lock, which + is why the header requires the application to serialize hid_error(NULL) + against the hotplug API. HIDAPI's own code on the internal event context + never writes the global error; a user callback that calls the public hotplug + API from the event context does, and that same serialization requirement in + the header covers it. */ static hid_internal_lock global_error_lock = 0; -static void register_global_winapi_error(const WCHAR *op) +/* Publishes a message (built by the caller, ownership taken) as the global + error string. Only the pointer swap is under the lock: it is a spinlock, and + building or freeing a message is far too heavy for a spin-guarded region. */ +static void register_global_error_message(wchar_t *msg) { - /* Capture the error code before the lock: acquiring it may clobber it */ - DWORD error_code = GetLastError(); + wchar_t *old_msg; hid_internal_lock_acquire(&global_error_lock); - register_winapi_error_code_to_buffer(&last_global_error_str, op, error_code); + old_msg = last_global_error_str; + last_global_error_str = msg; hid_internal_lock_release(&global_error_lock); + + free(old_msg); +} + +static void register_global_winapi_error_code(DWORD error_code, const WCHAR *op) +{ + wchar_t *msg = NULL; + + register_winapi_error_code_to_buffer(&msg, op, error_code); + register_global_error_message(msg); +} + +static void register_global_winapi_error(const WCHAR *op) +{ + /* Capture the error code first: building the message may clobber it */ + register_global_winapi_error_code(GetLastError(), op); } static void register_global_error(const WCHAR *string_error) { - hid_internal_lock_acquire(&global_error_lock); - register_string_error_to_buffer(&last_global_error_str, string_error); - hid_internal_lock_release(&global_error_lock); + register_global_error_message(string_error ? _wcsdup(string_error) : NULL); } static HANDLE open_device(const wchar_t *path, BOOL open_rw) @@ -522,12 +562,33 @@ HID_API_EXPORT const char* HID_API_CALL hid_version_str(void) return HID_API_VERSION_STR; } -/* Serializes the bootstrap of the hotplug machinery: two racing first - registrations must not both initialize the critical section, and every read of - mutex_ready that is not already made under the critical section is made under - this lock (it is what publishes the critical section to other threads). */ +/* Serializes the bootstrap and the teardown of the hotplug machinery (and the + flag and counter below): two racing first registrations must not both + initialize the critical section, and every read of mutex_ready that is not + already made under the critical section is made under this lock (it is what + publishes the critical section to other threads). Statically initialized: + guarding state with it costs no OS object. */ static hid_internal_lock hotplug_init_lock = 0; +/* Set while hid_exit() is tearing the hotplug machinery down (and unloading the + resolved libraries): hotplug registration and deregistration fail instead of + arming a context that is being destroyed, or calling into libraries that are + being unloaded. Guarded by hotplug_init_lock, NOT by the critical section: + hid_exit() must be able to raise it before it can know whether the machinery + (and with it the critical section) even exists - and without creating it, as + a program that never uses hotplug must not have hid_exit() create OS objects + on its behalf. */ +static LONG hotplug_exiting = 0; + +/* Number of threads currently counted into the hotplug machinery by + hid_internal_hotplug_enter, i.e. inside - or blocked on - the critical + section from a public hotplug call. Guarded by hotplug_init_lock. + hid_exit() destroys the critical section and the quiescence event only after + `hotplug_exiting` has cut off new entries and this count has drained to zero: + deleting a critical section another thread is blocked on (or about to enter) + is undefined behavior. */ +static LONG hotplug_machinery_users = 0; + /* Resolves the threadpool API used as the hotplug event context. Always called inside the critical section. Returns -1 when it is unavailable (the OS predates it), in which case hotplug is not available either. */ @@ -565,47 +626,105 @@ static int hid_internal_hotplug_resolve_threadpool(void) return 0; } -/* Bootstraps the hotplug machinery. The critical section and the quiescence event - are created once and are NEVER destroyed: they are the only things that can - serialize against a notification callback, and no caller - hid_exit() included - - can prove that no callback is about to enter them. Keeping them for the process - lifetime costs one critical section and one event handle, and removes an entire - class of use-after-free (a callback, a threadpool work item or a concurrent - deregistration entering a deleted critical section). - Returns -1 if the machinery cannot be initialized. */ -static int hid_internal_hotplug_init(void) +/* Bootstraps the hotplug machinery: the critical section that guards all + hotplug state and the manual-reset quiescence event. Must be called with + hotplug_init_lock held. On failure nothing is created and *create_error + receives the CreateEvent error code (reporting it is left to the caller: + this runs under a spinlock). + Once created, the machinery stays valid for as long as anything can enter it: + hid_exit() destroys it only after proving nothing can (see + hid_internal_hotplug_enter and hid_internal_hotplug_exit), and when a + notification could not be unregistered it is never destroyed at all, so that + a live OS callback can never enter a deleted critical section. */ +static int hid_internal_hotplug_init_under_lock(DWORD *create_error) { - int result = 0; + if (hid_hotplug_context.mutex_ready) { + return 0; + } - hid_internal_lock_acquire(&hotplug_init_lock); - if (!hid_hotplug_context.mutex_ready) { - /* Manual reset, initially signaled: nothing is pending yet */ - hid_hotplug_context.quiescent_event = CreateEvent(NULL, TRUE, TRUE, NULL); - if (hid_hotplug_context.quiescent_event == NULL) { - register_global_winapi_error(L"hid_hotplug_register_callback/CreateEvent"); - result = -1; - } else { - InitializeCriticalSection(&hid_hotplug_context.critical_section); + /* Manual reset, initially signaled: nothing is pending yet */ + hid_hotplug_context.quiescent_event = CreateEvent(NULL, TRUE, TRUE, NULL); + if (hid_hotplug_context.quiescent_event == NULL) { + *create_error = GetLastError(); + return -1; + } - hid_hotplug_context.mutex_in_use = 0; - hid_hotplug_context.cb_list_dirty = 0; - hid_hotplug_context.pending_unregistrations = 0; - if (hid_hotplug_context.next_handle < FIRST_HOTPLUG_CALLBACK_HANDLE) - hid_hotplug_context.next_handle = FIRST_HOTPLUG_CALLBACK_HANDLE; + InitializeCriticalSection(&hid_hotplug_context.critical_section); - /* Set state to Ready. Published last: a thread that observes this - under hotplug_init_lock also observes everything above. */ - hid_hotplug_context.mutex_ready = 1; - } + hid_hotplug_context.mutex_in_use = 0; + hid_hotplug_context.cb_list_dirty = 0; + hid_hotplug_context.pending_unregistrations = 0; + if (hid_hotplug_context.next_handle < FIRST_HOTPLUG_CALLBACK_HANDLE) + hid_hotplug_context.next_handle = FIRST_HOTPLUG_CALLBACK_HANDLE; + + /* Set state to Ready. Published last: a thread that observes this + under hotplug_init_lock also observes everything above. */ + hid_hotplug_context.mutex_ready = 1; + + return 0; +} + +/* Result codes of hid_internal_hotplug_enter (0 is success) */ +#define HID_HOTPLUG_ENTER_EXITING 1 /* hid_exit() is in progress */ +#define HID_HOTPLUG_ENTER_NOT_READY 2 /* no machinery and bootstrap not requested */ +#define HID_HOTPLUG_ENTER_FAILED 3 /* bootstrap failed (global error registered) */ + +/* Counts the calling thread into the hotplug machinery, bootstrapping it first + when `bootstrap` is set. While a thread is counted in, the critical section + and the quiescence event exist and stay valid: hid_exit() destroys them only + after `hotplug_exiting` has cut off new entries AND the count has drained to + zero. A successful call (and only a successful call) MUST be balanced with + hid_internal_hotplug_leave(). */ +static int hid_internal_hotplug_enter(int bootstrap) +{ + int result = 0; + DWORD create_error = 0; + + hid_internal_lock_acquire(&hotplug_init_lock); + if (hotplug_exiting) { + result = HID_HOTPLUG_ENTER_EXITING; + } else if (!bootstrap && !hid_hotplug_context.mutex_ready) { + result = HID_HOTPLUG_ENTER_NOT_READY; + } else if (hid_internal_hotplug_init_under_lock(&create_error) < 0) { + result = HID_HOTPLUG_ENTER_FAILED; + } else { + hotplug_machinery_users++; } hid_internal_lock_release(&hotplug_init_lock); + if (result == HID_HOTPLUG_ENTER_FAILED) { + register_global_winapi_error_code(create_error, L"hid_hotplug_register_callback/CreateEvent"); + } + return result; } -/* Whether the critical section exists and may be entered. Once it does, it stays - valid for the process lifetime (see hid_internal_hotplug_init), so the answer - can never go stale between the check and the EnterCriticalSection. */ +static void hid_internal_hotplug_leave(void) +{ + hid_internal_lock_acquire(&hotplug_init_lock); + hotplug_machinery_users--; + hid_internal_lock_release(&hotplug_init_lock); +} + +/* Whether hid_exit() is currently tearing the machinery down. Re-checked under + the critical section by callers that were already counted in when hid_exit() + started: `hotplug_exiting` may be raised while they hold - or wait on - the + critical section, and they must not arm anything behind the teardown. */ +static int hid_internal_hotplug_exiting(void) +{ + int exiting; + + hid_internal_lock_acquire(&hotplug_init_lock); + exiting = (hotplug_exiting != 0); + hid_internal_lock_release(&hotplug_init_lock); + + return exiting; +} + +/* Whether the critical section exists and may be entered. Only used by + hid_exit() itself (via hid_internal_hotplug_notification_leaked), on the same + thread that is the only one allowed to destroy the machinery, so the answer + cannot go stale between the check and the EnterCriticalSection. */ static int hid_internal_hotplug_ready(void) { int ready; @@ -720,6 +839,8 @@ static void hid_internal_hotplug_free_pending_arrivals(void) replugged into the same port, so suppressing it would swallow a real connection. */ static int hid_internal_hotplug_record_pending_arrivals(void) { + hid_hotplug_context.pending_arrivals_tick = GetTickCount(); + for (struct hid_device_info *device = hid_hotplug_context.devs; device != NULL; device = device->next) { struct hid_hotplug_path *entry = (struct hid_hotplug_path *)calloc(1, sizeof(struct hid_hotplug_path)); @@ -748,6 +869,21 @@ static int hid_internal_hotplug_record_pending_arrivals(void) dropped, 0 when it is a genuine new connection. */ static int hid_internal_hotplug_take_pending_arrival(const char *path) { + /* An expired list suppresses nothing: the duplicate it exists for would have + long been delivered (see HID_HOTPLUG_PENDING_ARRIVALS_TTL_MS). Devices + whose records outlive the window are devices whose removal notification + was missed - consuming the record on their eventual re-arrival would + swallow a genuine connection and keep the stale-record recovery in + hid_internal_notify_callback from ever running. Between the two failure + modes the expiry errs towards the recoverable one: a duplicate that + somehow outlives the window is reported as a spurious LEFT+ARRIVED pair + instead of a connection being silently dropped. */ + if (hid_hotplug_context.pending_arrivals != NULL + && GetTickCount() - hid_hotplug_context.pending_arrivals_tick > HID_HOTPLUG_PENDING_ARRIVALS_TTL_MS) { + hid_internal_hotplug_free_pending_arrivals(); + return 0; + } + for (struct hid_hotplug_path **current = &hid_hotplug_context.pending_arrivals; *current != NULL; current = &(*current)->next) { /* Case-independent path comparison is mandatory */ if (_stricmp((*current)->path, path) == 0) { @@ -974,17 +1110,13 @@ static int hid_internal_hotplug_notification_leaked(void) return leaked; } -/* Ends the teardown started by hid_internal_hotplug_exit: see the `exiting` flag. - Called by hid_exit() once the resolved libraries are gone, too. */ +/* Ends the teardown started by hid_internal_hotplug_exit: see `hotplug_exiting`. + Called unconditionally by hid_exit() once the resolved libraries are gone. */ static void hid_internal_hotplug_exit_done(void) { - if (!hid_internal_hotplug_ready()) { - return; - } - - EnterCriticalSection(&hid_hotplug_context.critical_section); - hid_hotplug_context.exiting = 0; - LeaveCriticalSection(&hid_hotplug_context.critical_section); + hid_internal_lock_acquire(&hotplug_init_lock); + hotplug_exiting = 0; + hid_internal_lock_release(&hotplug_init_lock); } static int hid_internal_hotplug_exit(void) @@ -992,25 +1124,34 @@ static int hid_internal_hotplug_exit(void) HCMNOTIFICATION notify_handle; PVOID event_work; int failed; + int keep_machinery; + int machinery_ready; + + /* Nothing may enter or arm the machinery from here on. Without this, a + registration waiting for a pending unregistration to complete could wake up + behind this teardown, arm a fresh notification and append a callback to a + context that is being dismantled - leaving a live notification and a + "registered" callback behind hid_exit(). It is also what keeps a concurrent + hid_hotplug_register_callback() - which is thread-safe, and initializes the + library implicitly - from calling into hid.dll/cfgmgr32.dll while hid_exit() + is unloading them, and what makes the destruction at the end of this + function safe. Lowered again by hid_internal_hotplug_exit_done(). */ + hid_internal_lock_acquire(&hotplug_init_lock); + hotplug_exiting = 1; + machinery_ready = (hid_hotplug_context.mutex_ready != 0); + hid_internal_lock_release(&hotplug_init_lock); - /* Not a no-op even when hotplug was never used: `exiting` is what keeps a - concurrent hid_hotplug_register_callback() - which is thread-safe, and - initializes the library implicitly - from calling into hid.dll/cfgmgr32.dll - while hid_exit() is unloading them. It needs the critical section to be there. */ - if (hid_internal_hotplug_init() < 0) { - /* Nothing can be armed if the machinery cannot even be initialized */ + if (!machinery_ready) { + /* Hotplug was never used (or a previous hid_exit() already destroyed the + machinery): nothing can be armed, and nothing was created that would + have to be freed. A registration bootstrapping the machinery right now + fails on `hotplug_exiting` before it arms anything or calls into the + libraries hid_exit() is about to unload. */ return 0; } EnterCriticalSection(&hid_hotplug_context.critical_section); - /* Nothing may arm the machinery from here on. Without this, a registration - waiting for a pending unregistration to complete could wake up behind this - teardown, arm a fresh notification and append a callback to a context that - is being dismantled - leaving a live notification and a "registered" - callback behind hid_exit(). */ - hid_hotplug_context.exiting = 1; - /* Remove all callbacks from the list, including their undelivered HID_API_HOTPLUG_ENUMERATE snapshots */ { struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; @@ -1044,12 +1185,16 @@ static int hid_internal_hotplug_exit(void) hid_hotplug_context.unregistration_failed = 0; event_work = hid_hotplug_context.event_work; - if (!failed && !hid_hotplug_context.notification_leaked) { + /* `failed` implies notification_leaked: they are only ever set together. + Stable at this point: no unregistration is pending anymore, and nothing + that could start one can enter behind `hotplug_exiting`. */ + keep_machinery = (hid_hotplug_context.notification_leaked != 0); + if (!keep_machinery) { hid_hotplug_context.event_work = NULL; } else { /* A notification may still fire and submit to the work item: keep it - (deliberately leaked), along with the critical section and the device - cache it works on */ + (deliberately leaked), along with the critical section, the quiescence + event and the device cache it works on */ event_work = NULL; } LeaveCriticalSection(&hid_hotplug_context.critical_section); @@ -1073,10 +1218,44 @@ static int hid_internal_hotplug_exit(void) hid_internal_hotplug_free_pending_arrivals(); LeaveCriticalSection(&hid_hotplug_context.critical_section); - /* `exiting` stays set until hid_exit() is done unloading the libraries: - see hid_internal_hotplug_exit_done. - The critical section and the quiescence event are kept for the process - lifetime on purpose: see hid_internal_hotplug_init */ + if (!keep_machinery) { + /* hid_exit() frees what the machinery created - one kernel event and one + critical section: a plugin-style host that repeatedly loads, uses and + unloads the library must not accumulate OS objects. Destroying them is + safe only once nothing can be inside the critical section: every + notification is unregistered and the work item is drained and closed + (established above), `hotplug_exiting` (still raised) keeps new callers + out, so only callers already counted in remain - wait those out. They + fail out promptly on their `exiting` checks. */ + for (;;) { + int busy; + + hid_internal_lock_acquire(&hotplug_init_lock); + busy = (hotplug_machinery_users != 0); + if (!busy) { + /* Unpublished under the same acquisition that proved the machinery + idle: with `hotplug_exiting` raised nothing can be counted back + in, so the destruction below cannot race anything. */ + hid_hotplug_context.mutex_ready = 0; + } + hid_internal_lock_release(&hotplug_init_lock); + + if (!busy) { + break; + } + Sleep(1); + } + + DeleteCriticalSection(&hid_hotplug_context.critical_section); + CloseHandle(hid_hotplug_context.quiescent_event); + hid_hotplug_context.quiescent_event = NULL; + } + /* else: the machinery outlives hid_exit() on purpose - a leaked notification + may still enter the critical section at any time + (see hid_internal_hotplug_init_under_lock) */ + + /* `hotplug_exiting` stays raised until hid_exit() is done unloading the + libraries: see hid_internal_hotplug_exit_done */ if (failed) { register_global_error(L"hid_exit: a hotplug notification could not be unregistered"); @@ -1422,13 +1601,20 @@ static hid_internal_detect_bus_type_result hid_internal_detect_bus_type(const wc return result; } -static char *hid_internal_UTF16toUTF8(const wchar_t *src) +/* Returns NULL both when `src` is not valid UTF-16 and on allocation failure. + Callers that need to tell the two apart (an invalid string is the string's + problem; running out of memory is a library failure) pass `oom`, which is set + to 1 on allocation failure and left untouched otherwise. */ +static char *hid_internal_UTF16toUTF8(const wchar_t *src, int *oom) { char *dst = NULL; int len = WideCharToMultiByte(CP_UTF8, WC_ERR_INVALID_CHARS, src, -1, NULL, 0, NULL, NULL); if (len) { dst = (char*)calloc(len, sizeof(char)); if (dst == NULL) { + if (oom) { + *oom = 1; + } return NULL; } WideCharToMultiByte(CP_UTF8, WC_ERR_INVALID_CHARS, src, -1, dst, len, NULL, NULL); @@ -1452,7 +1638,11 @@ static wchar_t *hid_internal_UTF8toUTF16(const char *src) return dst; } -static struct hid_device_info *hid_internal_get_device_info(const wchar_t *path, HANDLE handle) +/* Returns NULL when the device cannot be described: because the process is out + of memory (`oom` - when passed - is set to 1; a library failure) or because + `path` is not valid UTF-16 (`oom` is left untouched; the device is simply not + representable and callers skip it). */ +static struct hid_device_info *hid_internal_get_device_info(const wchar_t *path, HANDLE handle, int *oom) { struct hid_device_info *dev = NULL; /* return object */ HIDD_ATTRIBUTES attrib; @@ -1467,12 +1657,15 @@ static struct hid_device_info *hid_internal_get_device_info(const wchar_t *path, dev = (struct hid_device_info*)calloc(1, sizeof(struct hid_device_info)); if (dev == NULL) { + if (oom) { + *oom = 1; + } return NULL; } /* Fill out the record */ dev->next = NULL; - dev->path = hid_internal_UTF16toUTF8(path); + dev->path = hid_internal_UTF16toUTF8(path, oom); if (dev->path == NULL) { /* A record without a path is useless to the caller and unusable as the key of the hotplug device cache (where it would crash the removal lookup) */ @@ -1527,6 +1720,9 @@ static struct hid_device_info *hid_internal_get_device_info(const wchar_t *path, if (dev->serial_number == NULL || dev->manufacturer_string == NULL || dev->product_string == NULL) { /* Out of memory. A half-built record is not a device (and the bus-specific fixups right below dereference these strings) */ + if (oom) { + *oom = 1; + } hid_free_enumeration(dev); return NULL; } @@ -1630,7 +1826,16 @@ static struct hid_device_info *hid_internal_enumerate(unsigned short vendor_id, device to the enumeration list. */ if (hid_internal_match_device_id(attrib.VendorID, attrib.ProductID, vendor_id, product_id)) { /* VID/PID match. Create the record. */ - struct hid_device_info *tmp = hid_internal_get_device_info(device_interface, device_handle); + int oom = 0; + struct hid_device_info *tmp = hid_internal_get_device_info(device_interface, device_handle, &oom); + + if (tmp == NULL && !oom) { + /* The interface path is not valid UTF-16: the device cannot be + represented to the caller. Skip it - consistently with the + hotplug notifications, which cannot report (or cache) such a + device either, so the device cache stays in step with this list. */ + goto cont_close; + } if (tmp == NULL) { /* Out of memory. Report a failure rather than a partial list: a @@ -1693,6 +1898,47 @@ void HID_API_EXPORT HID_API_CALL hid_free_enumeration(struct hid_device_info *d } } +/* Delivers one live event to every matching callback registered at this moment. + Always called inside a locked mutex. Does not consume `device`. */ +static void hid_internal_hotplug_dispatch(struct hid_device_info *device, hid_hotplug_event hotplug_event) +{ + /* Mark the critical section as IN USE, to prevent callback removal from inside a callback */ + hid_hotplug_context.mutex_in_use = 1; + + /* Callbacks registered from inside a callback are appended to the list + and see this device in their registration-time HID_API_HOTPLUG_ENUMERATE + snapshot (or don't, for a removal): the live dispatch is bound to the + callbacks present at event time, so the connection is reported exactly once */ + struct hid_hotplug_callback *last_at_event = hid_hotplug_context.hotplug_cbs; + while (last_at_event != NULL && last_at_event->next != NULL) { + last_at_event = last_at_event->next; + } + + /* Call the notifications for the device */ + for (struct hid_hotplug_callback *callback = hid_hotplug_context.hotplug_cbs; callback != NULL; callback = callback->next) { + /* The registration-time enumeration pass is always delivered + before any live events for the callback */ + hid_internal_hotplug_replay_flush(callback); + + if ((callback->events & hotplug_event) && hid_internal_match_device_id(device->vendor_id, device->product_id, callback->vendor_id, callback->product_id)) { + int result = (callback->callback)(callback->handle, device, hotplug_event, callback->user_data); + + /* If the result is non-zero, we MARK the callback for future removal and proceed */ + /* We avoid changing the list until we are done calling the callbacks to simplify the process */ + if (result) { + callback->events = 0; + hid_hotplug_context.cb_list_dirty = 1; + } + } + + if (callback == last_at_event) { + break; + } + } + + hid_hotplug_context.mutex_in_use = 0; +} + DWORD WINAPI hid_internal_notify_callback(HCMNOTIFICATION notify, PVOID context, CM_NOTIFY_ACTION action, PCM_NOTIFY_EVENT_DATA event_data, DWORD event_data_size) { struct hid_device_info *device = NULL; @@ -1710,7 +1956,7 @@ DWORD WINAPI hid_internal_notify_callback(HCMNOTIFICATION notify, PVOID context, EnterCriticalSection(&hid_hotplug_context.critical_section); if (action == CM_NOTIFY_ACTION_DEVICEINTERFACEARRIVAL) { - char *path = hid_internal_UTF16toUTF8(event_data->u.DeviceInterface.SymbolicLink); + char *path = hid_internal_UTF16toUTF8(event_data->u.DeviceInterface.SymbolicLink, NULL); hotplug_event = HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED; @@ -1726,17 +1972,29 @@ DWORD WINAPI hid_internal_notify_callback(HCMNOTIFICATION notify, PVOID context, /* Check validity of read_handle. */ if (read_handle != INVALID_HANDLE_VALUE) { - device = hid_internal_get_device_info(event_data->u.DeviceInterface.SymbolicLink, read_handle); + device = hid_internal_get_device_info(event_data->u.DeviceInterface.SymbolicLink, read_handle, NULL); CloseHandle(read_handle); } if (device != NULL) { /* An interface path is reused when a device is replugged into the same port, so an arrival for a path that is still cached means the - removal of the previous connection was missed. Drop the stale record - instead of shadowing it: a second record for the same path would - never be removed, and the connection is reported as the new one it is. */ - hid_free_enumeration(hid_internal_hotplug_take_cached_device(device->path)); + removal of the previous connection was missed. The previous + connection is owed a DEVICE_LEFT (the header promises one for every + matching device that disconnects while a callback is registered): + deliver it synthetically from the stale record, then drop that + record - a second record for the same path would never be removed - + and report the new connection as the arrival it is. */ + struct hid_device_info *stale_device = hid_internal_hotplug_take_cached_device(device->path); + if (stale_device != NULL) { + /* Dispatched before the new device is cached: a callback + registered from inside one of these callbacks takes its + HID_API_HOTPLUG_ENUMERATE snapshot from the cache, and must + not see the new connection there AND as the live arrival + dispatched below. */ + hid_internal_hotplug_dispatch(stale_device, HID_API_HOTPLUG_EVENT_DEVICE_LEFT); + hid_free_enumeration(stale_device); + } /* Append to the end of the device list */ if (hid_hotplug_context.devs != NULL) { @@ -1764,7 +2022,7 @@ DWORD WINAPI hid_internal_notify_callback(HCMNOTIFICATION notify, PVOID context, hotplug_event = HID_API_HOTPLUG_EVENT_DEVICE_LEFT; - path = hid_internal_UTF16toUTF8(event_data->u.DeviceInterface.SymbolicLink); + path = hid_internal_UTF16toUTF8(event_data->u.DeviceInterface.SymbolicLink, NULL); if (path != NULL) { /* The device is gone: a later arrival on the same path is a new @@ -1779,41 +2037,7 @@ DWORD WINAPI hid_internal_notify_callback(HCMNOTIFICATION notify, PVOID context, } if (device) { - /* Mark the critical section as IN USE, to prevent callback removal from inside a callback */ - hid_hotplug_context.mutex_in_use = 1; - - /* Callbacks registered from inside a callback are appended to the list - and see this device in their registration-time HID_API_HOTPLUG_ENUMERATE - snapshot (or don't, for a removal): the live dispatch is bound to the - callbacks present at event time, so the connection is reported exactly once */ - struct hid_hotplug_callback *last_at_event = hid_hotplug_context.hotplug_cbs; - while (last_at_event != NULL && last_at_event->next != NULL) { - last_at_event = last_at_event->next; - } - - /* Call the notifications for the device */ - for (struct hid_hotplug_callback *callback = hid_hotplug_context.hotplug_cbs; callback != NULL; callback = callback->next) { - /* The registration-time enumeration pass is always delivered - before any live events for the callback */ - hid_internal_hotplug_replay_flush(callback); - - if ((callback->events & hotplug_event) && hid_internal_match_device_id(device->vendor_id, device->product_id, callback->vendor_id, callback->product_id)) { - int result = (callback->callback)(callback->handle, device, hotplug_event, callback->user_data); - - /* If the result is non-zero, we MARK the callback for future removal and proceed */ - /* We avoid changing the list until we are done calling the callbacks to simplify the process */ - if (result) { - callback->events = 0; - hid_hotplug_context.cb_list_dirty = 1; - } - } - - if (callback == last_at_event) { - break; - } - } - - hid_hotplug_context.mutex_in_use = 0; + hid_internal_hotplug_dispatch(device, hotplug_event); /* Free removed device */ if (hotplug_event == HID_API_HOTPLUG_EVENT_DEVICE_LEFT) { @@ -1834,61 +2058,21 @@ DWORD WINAPI hid_internal_notify_callback(HCMNOTIFICATION notify, PVOID context, return ERROR_SUCCESS; } -int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short vendor_id, unsigned short product_id, int events, int flags, hid_hotplug_callback_fn callback, void* user_data, hid_hotplug_callback_handle* callback_handle) +/* The registration steps that run inside the machinery. The caller has already + counted itself in with hid_internal_hotplug_enter - which is what keeps the + critical section alive for the whole call - and balances that with + hid_internal_hotplug_leave afterwards. Takes ownership of hotplug_cb: it is + freed on failure. */ +static int hid_internal_hotplug_register_counted(struct hid_hotplug_callback *hotplug_cb, int flags, hid_hotplug_callback_handle *callback_handle) { - struct hid_hotplug_callback* hotplug_cb; - - /* No events can be delivered before the handle is written */ - if (callback_handle != NULL) { - *callback_handle = 0; - } - - /* Check params */ - if (callback == NULL) { - register_global_error(L"Callback function is NULL"); - return -1; - } - if (events == 0 - || (events & ~(HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED | HID_API_HOTPLUG_EVENT_DEVICE_LEFT))) { - register_global_error(L"Invalid events mask"); - return -1; - } - if (flags & ~(HID_API_HOTPLUG_ENUMERATE)) { - register_global_error(L"Invalid flags"); - return -1; - } - - hotplug_cb = (struct hid_hotplug_callback*)calloc(1, sizeof(struct hid_hotplug_callback)); - - if (hotplug_cb == NULL) { - register_global_error(L"Failed to allocate memory for a hotplug callback"); - return -1; - } - - /* Fill out the record */ - hotplug_cb->next = NULL; - hotplug_cb->vendor_id = vendor_id; - hotplug_cb->product_id = product_id; - hotplug_cb->events = events; - hotplug_cb->user_data = user_data; - hotplug_cb->callback = callback; - hotplug_cb->replay = NULL; - - /* Ensure we are ready to actually use the mutex */ - if (hid_internal_hotplug_init() < 0) { - /* register_global_error: set by hid_internal_hotplug_init */ - free(hotplug_cb); - return -1; - } - /* Lock the mutex to avoid race conditions */ EnterCriticalSection(&hid_hotplug_context.critical_section); for (;;) { - if (hid_hotplug_context.exiting) { - /* hid_exit() is tearing the machinery down: arming it again behind its - back would leave a live notification and a registered callback with no - context to run in */ + if (hid_internal_hotplug_exiting()) { + /* hid_exit() started tearing the machinery down after this call was + counted in: arming it again behind its back would leave a live + notification and a registered callback with no context to run in */ register_global_error(L"hid_exit() is in progress"); LeaveCriticalSection(&hid_hotplug_context.critical_section); free(hotplug_cb); @@ -1905,7 +2089,12 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven /* The unregistration completes on another thread (or on the event context), which needs the critical section, so the wait must not hold it. On wake the - state is re-evaluated from scratch. */ + state is re-evaluated from scratch. + This LeaveCriticalSection releases the critical section completely only + because it cannot be held recursively here: on the event context (the one + place this function runs with the critical section already held, via a + user callback registering a callback) hotplug_cbs is never NULL, so the + loop has already exited above. */ LeaveCriticalSection(&hid_hotplug_context.critical_section); if (WaitForSingleObject(hid_hotplug_context.quiescent_event, INFINITE) == WAIT_FAILED) { register_global_winapi_error(L"hid_hotplug_register_callback/WaitForSingleObject"); @@ -2019,10 +2208,10 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven /* Take the registration-time snapshot to be replayed asynchronously on the event context, one exact copy per matching connected device */ - if ((flags & HID_API_HOTPLUG_ENUMERATE) && (events & HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED)) { + if ((flags & HID_API_HOTPLUG_ENUMERATE) && (hotplug_cb->events & HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED)) { struct hid_device_info **replay_tail = &hotplug_cb->replay; for (struct hid_device_info *device = hid_hotplug_context.devs; device != NULL; device = device->next) { - if (!hid_internal_match_device_id(device->vendor_id, device->product_id, vendor_id, product_id)) { + if (!hid_internal_match_device_id(device->vendor_id, device->product_id, hotplug_cb->vendor_id, hotplug_cb->product_id)) { continue; } *replay_tail = hid_internal_copy_device_info(device); @@ -2074,12 +2263,82 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven return 0; } +int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short vendor_id, unsigned short product_id, int events, int flags, hid_hotplug_callback_fn callback, void* user_data, hid_hotplug_callback_handle* callback_handle) +{ + struct hid_hotplug_callback* hotplug_cb; + int result; + + /* No events can be delivered before the handle is written */ + if (callback_handle != NULL) { + *callback_handle = 0; + } + + /* Check params */ + if (callback == NULL) { + register_global_error(L"Callback function is NULL"); + return -1; + } + if (events == 0 + || (events & ~(HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED | HID_API_HOTPLUG_EVENT_DEVICE_LEFT))) { + register_global_error(L"Invalid events mask"); + return -1; + } + if (flags & ~(HID_API_HOTPLUG_ENUMERATE)) { + register_global_error(L"Invalid flags"); + return -1; + } + + hotplug_cb = (struct hid_hotplug_callback*)calloc(1, sizeof(struct hid_hotplug_callback)); + + if (hotplug_cb == NULL) { + register_global_error(L"Failed to allocate memory for a hotplug callback"); + return -1; + } + + /* Fill out the record */ + hotplug_cb->next = NULL; + hotplug_cb->vendor_id = vendor_id; + hotplug_cb->product_id = product_id; + hotplug_cb->events = events; + hotplug_cb->user_data = user_data; + hotplug_cb->callback = callback; + hotplug_cb->replay = NULL; + + /* Ensure the machinery is ready to be used, and keep hid_exit() from + destroying it while this call is inside */ + switch (hid_internal_hotplug_enter(1 /* bootstrap on first use */)) { + case 0: + break; + case HID_HOTPLUG_ENTER_EXITING: + /* hid_exit() is tearing the machinery down: arming it again behind its + back would leave a live notification and a registered callback with no + context to run in */ + register_global_error(L"hid_exit() is in progress"); + free(hotplug_cb); + return -1; + default: + /* register_global_error: set by hid_internal_hotplug_enter */ + free(hotplug_cb); + return -1; + } + + result = hid_internal_hotplug_register_counted(hotplug_cb, flags, callback_handle); + + hid_internal_hotplug_leave(); + + return result; +} + int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_callback_handle callback_handle) { int result = -1; HCMNOTIFICATION notify_handle; - if (callback_handle <= 0 || !hid_internal_hotplug_ready()) { + /* Never bootstraps the machinery: with no machinery there is nothing this + handle could belong to. And when hid_exit() is in progress, it deregisters + every callback and invalidates every handle - this one is (or is about to + be) one of them. */ + if (callback_handle <= 0 || hid_internal_hotplug_enter(0) != 0) { register_global_error(L"Invalid or unknown hotplug callback handle"); return -1; } @@ -2087,11 +2346,12 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_call /* Lock the mutex to avoid race conditions */ EnterCriticalSection(&hid_hotplug_context.critical_section); - if (hid_hotplug_context.exiting) { - /* hid_exit() deregisters every callback and invalidates every handle: - this one is (or is about to be) one of them */ + if (hid_internal_hotplug_exiting()) { + /* hid_exit() started tearing the machinery down after this call was + counted in: same as above */ register_global_error(L"Invalid or unknown hotplug callback handle"); LeaveCriticalSection(&hid_hotplug_context.critical_section); + hid_internal_hotplug_leave(); return -1; } @@ -2134,6 +2394,8 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_call hid_internal_hotplug_finish_unregistration(notify_handle); + hid_internal_hotplug_leave(); + return result; } @@ -2250,7 +2512,7 @@ HID_API_EXPORT hid_device * HID_API_CALL hid_open_path(const char *path) dev->input_report_length = caps.InputReportByteLength; dev->feature_report_length = caps.FeatureReportByteLength; dev->read_buf = (char*) malloc(dev->input_report_length); - dev->device_info = hid_internal_get_device_info(interface_path, dev->device_handle); + dev->device_info = hid_internal_get_device_info(interface_path, dev->device_handle, NULL); end_of_function: free(interface_path); From 4667fabc63d2da2e16ba59ac4994487c3967c87b Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Wed, 15 Jul 2026 01:18:00 +0300 Subject: [PATCH 5/9] windows: dedupe hotplug arrivals against the cache, not the wall clock The registration-time arrival dedupe was scoped by a 10-second wall-clock TTL. An arrival whose pending-arrival marker had expired was reclassified as a new connection on an already-cached path, which synthesized a DEVICE_LEFT for the cached record before reporting a second DEVICE_ARRIVED. Elapsed time is not a sound basis for that decision. A user callback is only ADVISED to be short, so a perfectly legal slow callback that holds the critical section for longer than the TTL let the still-queued arrival of a device that was present at registration expire: one physical connection was then reported as ARRIVED -> LEFT -> ARRIVED, breaking the documented exactly-once guarantee. Dedupe against the live device cache instead, as the linux and mac backends do. That needs no timing assumption anywhere: - The OS delivers exactly one arrival notification per interface instance, so the only way an arrival can be a duplicate is the arm-before-enumerate window at registration - and there the enumeration has, by construction, already put the device in the cache. An arrival for a path that is still cached is therefore always a duplicate: skip the append and the dispatch. - The removal notification is authoritative and drops the cache entry, so a genuine re-plug - even onto the same, reused interface path - is not in the cache and is reported. The missed-removal recovery that the TTL existed to reach is removed along with it, rather than re-gated on some other signal: a missed CM removal is not a real scenario (removals arrive through the very notification that delivers arrivals), and a spurious LEFT+ARRIVED pair for a device that never disconnected is a far worse failure than the hypothetical duplicate it guarded against. Assisted-by: claude-code:claude-opus-4-8 --- windows/hid.c | 168 ++++++++++---------------------------------------- 1 file changed, 31 insertions(+), 137 deletions(-) diff --git a/windows/hid.c b/windows/hid.c index ff37cdcb3..7311b350b 100644 --- a/windows/hid.c +++ b/windows/hid.c @@ -239,21 +239,6 @@ static SubmitThreadpoolWork_ hid_internal_SubmitThreadpoolWork = NULL; static CloseThreadpoolWork_ hid_internal_CloseThreadpoolWork = NULL; static WaitForThreadpoolWorkCallbacks_ hid_internal_WaitForThreadpoolWorkCallbacks = NULL; -/* An interface path captured by the registration-time enumeration whose arrival - notification may still be in flight. See hid_internal_hotplug_record_pending_arrivals. */ -struct hid_hotplug_path { - char *path; - struct hid_hotplug_path *next; -}; - -/* How long (in milliseconds) the pending-arrivals list stays authoritative. - The duplicate arrival it suppresses was already queued by the OS when the - list was recorded and is delivered about as soon as the registration releases - the critical section, so anything this old is not that duplicate - it is a - genuine re-arrival on a path whose removal notification was missed (see - hid_internal_hotplug_take_pending_arrival). */ -#define HID_HOTPLUG_PENDING_ARRIVALS_TTL_MS 10000 - static struct hid_hotplug_context { /* Win32 notification handle */ HCMNOTIFICATION notify_handle; @@ -298,15 +283,10 @@ static struct hid_hotplug_context { /* Linked list of the hotplug callbacks */ struct hid_hotplug_callback *hotplug_cbs; - /* Linked list of the device infos (mandatory when the device is disconnected) */ + /* Linked list of the device infos (mandatory when the device is disconnected). + Doubles as the arrival dedupe set: an arrival for a path that is already in + here has already been reported (see hid_internal_notify_callback). */ struct hid_device_info *devs; - - /* Paths whose arrival notification may still be in flight while it has - already been reported by the registration-time enumeration, and the - GetTickCount() timestamp of the moment they were recorded (see - HID_HOTPLUG_PENDING_ARRIVALS_TTL_MS) */ - struct hid_hotplug_path *pending_arrivals; - DWORD pending_arrivals_tick; } hid_hotplug_context; /* zero-initialized (static storage); next_handle set on first init */ static hid_device *new_hid_device() @@ -809,88 +789,26 @@ static void hid_internal_hotplug_pin_module(void) (LPCWSTR)(void *)&hid_hotplug_context, &module); } -/* Always called inside a locked mutex */ -static void hid_internal_hotplug_free_pending_arrivals(void) -{ - struct hid_hotplug_path *current = hid_hotplug_context.pending_arrivals; - - hid_hotplug_context.pending_arrivals = NULL; - - while (current != NULL) { - struct hid_hotplug_path *next = current->next; - free(current->path); - free(current); - current = next; - } -} - -/* Records the interface paths the registration-time enumeration has just captured. - Always called inside a locked mutex; returns -1 on allocation failure. +/* Tells whether an interface path is already in the device cache - i.e. whether + its connection has already been reported. Always called inside a locked mutex. - The notification is armed BEFORE the enumeration runs (a device connecting in - between must not be missed by both), so a device that arrives in that window is - picked up by the enumeration AND has an arrival notification in flight. That - notification must not report - or cache - the same connection a second time. + This is the whole arrival dedupe. The notification is armed BEFORE the + registration-time enumeration runs (a device connecting in between must not be + missed by both), so a device that arrives in that window is captured by the + enumeration AND has an arrival notification in flight; that notification must + not report - or cache - the same connection a second time. - This window is the only place where a duplicate arrival can occur, so the - suppression is scoped to it: a recorded path swallows at most one arrival and is - dropped as soon as the device leaves. Outside of it, an arrival for a path that - is still cached is NOT a duplicate: an interface path is reused when a device is - replugged into the same port, so suppressing it would swallow a real connection. */ -static int hid_internal_hotplug_record_pending_arrivals(void) + The cache decides it, with no timing assumption anywhere: the OS delivers + exactly one arrival notification per interface instance, so that overlap is the + only way an arrival can be a duplicate - and in that case the enumeration has, + by construction, already put the device in the cache. Conversely the removal + notification is authoritative and drops the cache entry, so a genuine re-plug + (even onto the same, reused path) is NOT cached and IS reported. */ +static int hid_internal_hotplug_is_cached(const char *path) { - hid_hotplug_context.pending_arrivals_tick = GetTickCount(); - for (struct hid_device_info *device = hid_hotplug_context.devs; device != NULL; device = device->next) { - struct hid_hotplug_path *entry = (struct hid_hotplug_path *)calloc(1, sizeof(struct hid_hotplug_path)); - - if (entry == NULL) { - return -1; - } - - /* Cached devices always have a path (hid_internal_get_device_info fails without one) */ - entry->path = _strdup(device->path); - if (entry->path == NULL) { - free(entry); - return -1; - } - - entry->next = hid_hotplug_context.pending_arrivals; - hid_hotplug_context.pending_arrivals = entry; - } - - return 0; -} - -/* Consumes the pending-arrival record for this path, if there is one. - Always called inside a locked mutex. - Returns 1 when this arrival was already reported by the registration-time - enumeration (see hid_internal_hotplug_record_pending_arrivals) and must be - dropped, 0 when it is a genuine new connection. */ -static int hid_internal_hotplug_take_pending_arrival(const char *path) -{ - /* An expired list suppresses nothing: the duplicate it exists for would have - long been delivered (see HID_HOTPLUG_PENDING_ARRIVALS_TTL_MS). Devices - whose records outlive the window are devices whose removal notification - was missed - consuming the record on their eventual re-arrival would - swallow a genuine connection and keep the stale-record recovery in - hid_internal_notify_callback from ever running. Between the two failure - modes the expiry errs towards the recoverable one: a duplicate that - somehow outlives the window is reported as a spurious LEFT+ARRIVED pair - instead of a connection being silently dropped. */ - if (hid_hotplug_context.pending_arrivals != NULL - && GetTickCount() - hid_hotplug_context.pending_arrivals_tick > HID_HOTPLUG_PENDING_ARRIVALS_TTL_MS) { - hid_internal_hotplug_free_pending_arrivals(); - return 0; - } - - for (struct hid_hotplug_path **current = &hid_hotplug_context.pending_arrivals; *current != NULL; current = &(*current)->next) { /* Case-independent path comparison is mandatory */ - if (_stricmp((*current)->path, path) == 0) { - struct hid_hotplug_path *entry = *current; - *current = entry->next; - free(entry->path); - free(entry); + if (device->path != NULL && _stricmp(device->path, path) == 0) { return 1; } } @@ -1027,8 +945,6 @@ static HCMNOTIFICATION hid_internal_hotplug_cleanup() hid_hotplug_context.devs = NULL; } - hid_internal_hotplug_free_pending_arrivals(); - notify_handle = hid_hotplug_context.notify_handle; hid_hotplug_context.notify_handle = NULL; if (notify_handle != NULL) { @@ -1215,7 +1131,6 @@ static int hid_internal_hotplug_exit(void) cleanup and the completion of the unregistration */ hid_free_enumeration(hid_hotplug_context.devs); hid_hotplug_context.devs = NULL; - hid_internal_hotplug_free_pending_arrivals(); LeaveCriticalSection(&hid_hotplug_context.critical_section); if (!keep_machinery) { @@ -1963,9 +1878,13 @@ DWORD WINAPI hid_internal_notify_callback(HCMNOTIFICATION notify, PVOID context, if (path == NULL) { /* Out of memory: there is nothing to report the connection with, and nothing is cached, so the cache stays consistent */ - } else if (hid_internal_hotplug_take_pending_arrival(path)) { - /* Already reported (and cached) by the registration-time enumeration: - see hid_internal_hotplug_record_pending_arrivals */ + } else if (hid_internal_hotplug_is_cached(path)) { + /* This connection is already known - and therefore already reported. + The only way that happens is the arm-before-enumerate window at + registration, where the enumeration cached the device and the OS had + already queued this arrival for it (see + hid_internal_hotplug_is_cached). Drop it whole: no second cache + entry, no second dispatch. */ } else { /* Open read-only handle to the device */ HANDLE read_handle = open_device(event_data->u.DeviceInterface.SymbolicLink, FALSE); @@ -1977,25 +1896,6 @@ DWORD WINAPI hid_internal_notify_callback(HCMNOTIFICATION notify, PVOID context, } if (device != NULL) { - /* An interface path is reused when a device is replugged into the - same port, so an arrival for a path that is still cached means the - removal of the previous connection was missed. The previous - connection is owed a DEVICE_LEFT (the header promises one for every - matching device that disconnects while a callback is registered): - deliver it synthetically from the stale record, then drop that - record - a second record for the same path would never be removed - - and report the new connection as the arrival it is. */ - struct hid_device_info *stale_device = hid_internal_hotplug_take_cached_device(device->path); - if (stale_device != NULL) { - /* Dispatched before the new device is cached: a callback - registered from inside one of these callbacks takes its - HID_API_HOTPLUG_ENUMERATE snapshot from the cache, and must - not see the new connection there AND as the live arrival - dispatched below. */ - hid_internal_hotplug_dispatch(stale_device, HID_API_HOTPLUG_EVENT_DEVICE_LEFT); - hid_free_enumeration(stale_device); - } - /* Append to the end of the device list */ if (hid_hotplug_context.devs != NULL) { struct hid_device_info *last = hid_hotplug_context.devs; @@ -2025,11 +1925,10 @@ DWORD WINAPI hid_internal_notify_callback(HCMNOTIFICATION notify, PVOID context, path = hid_internal_UTF16toUTF8(event_data->u.DeviceInterface.SymbolicLink, NULL); if (path != NULL) { - /* The device is gone: a later arrival on the same path is a new - connection and must not be mistaken for a pending duplicate */ - hid_internal_hotplug_take_pending_arrival(path); - - /* Get and remove this device from the device list */ + /* Get and remove this device from the device list. Dropping the entry is + what makes a later arrival on the same path (an interface path is + reused when a device is replugged into the same port) a new connection + rather than a duplicate - see hid_internal_hotplug_is_cached. */ device = hid_internal_hotplug_take_cached_device(path); free(path); @@ -2168,8 +2067,8 @@ static int hid_internal_hotplug_register_counted(struct hid_hotplug_callback *ho /* Register for a HID device notification when adding the first callback. Armed BEFORE the device cache is filled: a device connecting in between is then caught by the notification instead of being missed by both, and - the duplicate that this creates is suppressed exactly once - (see hid_internal_hotplug_record_pending_arrivals). */ + the duplicate that this creates is suppressed by the cache itself + (see hid_internal_hotplug_is_cached). */ if (CM_Register_Notification(¬ify_filter, NULL, hid_internal_notify_callback, &hid_hotplug_context.notify_handle) != CR_SUCCESS) { register_global_error(L"hid_hotplug_register_callback/CM_Register_Notification"); hid_hotplug_context.notify_handle = NULL; @@ -2182,15 +2081,10 @@ static int hid_internal_hotplug_register_counted(struct hid_hotplug_callback *ho between its detachment and the completion of its unregistration */ hid_free_enumeration(hid_hotplug_context.devs); hid_hotplug_context.devs = NULL; - hid_internal_hotplug_free_pending_arrivals(); /* Fill already connected devices so we can use this info in disconnection notifications and HID_API_HOTPLUG_ENUMERATE passes */ hid_hotplug_context.devs = hid_internal_enumerate(0, 0, &enumerate_failure); - if (!enumerate_failure && hid_internal_hotplug_record_pending_arrivals() < 0) { - register_global_error(L"Failed to allocate memory for the hotplug device cache"); - enumerate_failure = 1; - } if (enumerate_failure) { /* An empty system is fine; a failed enumeration is not: the device cache and the ENUMERATE snapshot would misrepresent the system. From d1e8e7e29015ea0eca7131734cd72ec4c8884ce5 Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Wed, 15 Jul 2026 01:39:27 +0300 Subject: [PATCH 6/9] windows: make the hotplug cache lookups allocation-free Deduping arrivals against the live cache made that cache the single source of truth for what has been reported, and removed the stale-record recovery that used to paper over an inconsistent cache. That is sound only if the cache can never be left inconsistent - and one path could still leave it so. Both cache lookups took the interface path a CM notification carries (UTF-16) and converted it to UTF-8 before searching, which allocates. In the REMOVAL lookup that allocation is load-bearing: if it fails under memory pressure the removal is processed but its cache record survives. Because the cache is now the whole arrival dedupe, a surviving record is unrecoverable - interface paths are reused when a device is replugged into the same port, so the next connection on that path is classified as a duplicate and suppressed forever, for every callback, including later HID_API_HOTPLUG_ENUMERATE passes. The device becomes invisible. The removal notification did arrive; only its local processing failed, so the "removals come through the same notification stream" argument does not save it. Compare the cached UTF-8 path against the notification's UTF-16 symbolic link directly, encoding the UTF-16 to UTF-8 one code point at a time and matching it against the cached bytes (ASCII case-independent, exactly as the _stricmp() it replaces was). Both lookups - the arrival dedupe and the removal - are now allocation-free, so an out-of-memory condition can no longer desynchronize the cache from the system. The only allocation left on the notification path is the one that describes an arriving device, where failing it simply does not cache the device - and nothing was reported for it either, so the cache stays consistent. This is preferred over caching a second, wide copy of each path as a comparison key: it adds no per-device allocation to keep and free, no field to keep in sync with dev->path, and no change to the cached record type - the one remaining allocation is the one whose failure was always handled cleanly by dropping the arrival. Assisted-by: claude-code:claude-opus-4-8 --- windows/hid.c | 125 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 97 insertions(+), 28 deletions(-) diff --git a/windows/hid.c b/windows/hid.c index 7311b350b..679abc74e 100644 --- a/windows/hid.c +++ b/windows/hid.c @@ -49,7 +49,6 @@ typedef LONG NTSTATUS; #include #define _wcsdup wcsdup #define _strdup strdup -#define _stricmp strcasecmp #endif /*#define HIDAPI_USE_DDK*/ @@ -789,6 +788,88 @@ static void hid_internal_hotplug_pin_module(void) (LPCWSTR)(void *)&hid_hotplug_context, &module); } +/* ASCII case folding: locale-independent, and all an interface path needs + (this is also all the _stricmp() below used to do, in the C locale) */ +static unsigned char hid_internal_ascii_tolower(unsigned char c) +{ + return (c >= 'A' && c <= 'Z') ? (unsigned char)(c - 'A' + 'a') : c; +} + +/* Compares a cached (UTF-8) interface path with the (UTF-16) one a notification + carries, WITHOUT ALLOCATING: it encodes the UTF-16 path to UTF-8 on the fly, one + code point at a time, and matches it against the bytes of the cached one. + + Both hotplug cache lookups need this comparison, and both used to convert the + notification's symbolic link to UTF-8 first - which allocates. That allocation + failing in the REMOVAL lookup would process the removal but leave its cache + record behind, and a stale record is unrecoverable: it is the arrival dedupe, so + the next connection on that interface path (paths are reused when a device is + replugged into the same port) would be classified as a duplicate and suppressed + forever, for every callback, including later HID_API_HOTPLUG_ENUMERATE passes. + With no allocation on either lookup, an out-of-memory condition can no longer + desynchronize the cache from the system: the only allocation left is the one that + describes an arriving device, and failing it simply does not cache the device + (nothing was reported for it either - see hid_internal_notify_callback). + + The comparison is case-independent for ASCII, exactly like the _stricmp() it + replaces. A cached path is always a WC_ERR_INVALID_CHARS conversion of an + interface path (see hid_internal_UTF16toUTF8 and hid_internal_get_device_info), + so it always encodes well-formed UTF-16: an ill-formed symbolic link cannot be in + the cache, and comparing unequal is the correct answer for it - which is what the + failing conversion used to yield as well. */ +static int hid_internal_path_equals(const char *cached_path, const wchar_t *interface_path) +{ + const unsigned char *cached = (const unsigned char *)cached_path; + + while (*interface_path != L'\0') { + unsigned long code_point = (unsigned long)*interface_path++; + unsigned char utf8[4]; + size_t len, i; + + if (code_point >= 0xD800UL && code_point <= 0xDBFFUL) { + /* A high surrogate must be followed by a low one */ + if (*interface_path < 0xDC00 || *interface_path > 0xDFFF) { + return 0; + } + code_point = 0x10000UL + ((code_point - 0xD800UL) << 10) + (unsigned long)(*interface_path++ - 0xDC00); + } else if (code_point >= 0xDC00UL && code_point <= 0xDFFFUL) { + /* An unpaired low surrogate */ + return 0; + } + + if (code_point < 0x80UL) { + utf8[0] = (unsigned char)code_point; + len = 1; + } else if (code_point < 0x800UL) { + utf8[0] = (unsigned char)(0xC0UL | (code_point >> 6)); + utf8[1] = (unsigned char)(0x80UL | (code_point & 0x3FUL)); + len = 2; + } else if (code_point < 0x10000UL) { + utf8[0] = (unsigned char)(0xE0UL | (code_point >> 12)); + utf8[1] = (unsigned char)(0x80UL | ((code_point >> 6) & 0x3FUL)); + utf8[2] = (unsigned char)(0x80UL | (code_point & 0x3FUL)); + len = 3; + } else { + utf8[0] = (unsigned char)(0xF0UL | (code_point >> 18)); + utf8[1] = (unsigned char)(0x80UL | ((code_point >> 12) & 0x3FUL)); + utf8[2] = (unsigned char)(0x80UL | ((code_point >> 6) & 0x3FUL)); + utf8[3] = (unsigned char)(0x80UL | (code_point & 0x3FUL)); + len = 4; + } + + /* No byte of an encoded code point is ever '\0', so a cached path that ends + early simply compares unequal here: the walk cannot run past its end */ + for (i = 0; i < len; ++i) { + if (hid_internal_ascii_tolower(*cached) != hid_internal_ascii_tolower(utf8[i])) { + return 0; + } + ++cached; + } + } + + return *cached == '\0'; +} + /* Tells whether an interface path is already in the device cache - i.e. whether its connection has already been reported. Always called inside a locked mutex. @@ -804,11 +885,11 @@ static void hid_internal_hotplug_pin_module(void) by construction, already put the device in the cache. Conversely the removal notification is authoritative and drops the cache entry, so a genuine re-plug (even onto the same, reused path) is NOT cached and IS reported. */ -static int hid_internal_hotplug_is_cached(const char *path) +static int hid_internal_hotplug_is_cached(const wchar_t *interface_path) { for (struct hid_device_info *device = hid_hotplug_context.devs; device != NULL; device = device->next) { /* Case-independent path comparison is mandatory */ - if (device->path != NULL && _stricmp(device->path, path) == 0) { + if (device->path != NULL && hid_internal_path_equals(device->path, interface_path)) { return 1; } } @@ -816,13 +897,14 @@ static int hid_internal_hotplug_is_cached(const char *path) return 0; } -/* Unlinks the cached device with this path, if there is one, and hands it to the - caller (who owns it). Always called inside a locked mutex. */ -static struct hid_device_info *hid_internal_hotplug_take_cached_device(const char *path) +/* Unlinks the cached device with this interface path, if there is one, and hands + it to the caller (who owns it). Always called inside a locked mutex. Allocates + nothing: see hid_internal_path_equals. */ +static struct hid_device_info *hid_internal_hotplug_take_cached_device(const wchar_t *interface_path) { for (struct hid_device_info **current = &hid_hotplug_context.devs; *current != NULL; current = &(*current)->next) { /* Case-independent path comparison is mandatory */ - if ((*current)->path != NULL && _stricmp((*current)->path, path) == 0) { + if ((*current)->path != NULL && hid_internal_path_equals((*current)->path, interface_path)) { struct hid_device_info *device = *current; *current = device->next; device->next = NULL; @@ -1871,14 +1953,9 @@ DWORD WINAPI hid_internal_notify_callback(HCMNOTIFICATION notify, PVOID context, EnterCriticalSection(&hid_hotplug_context.critical_section); if (action == CM_NOTIFY_ACTION_DEVICEINTERFACEARRIVAL) { - char *path = hid_internal_UTF16toUTF8(event_data->u.DeviceInterface.SymbolicLink, NULL); - hotplug_event = HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED; - if (path == NULL) { - /* Out of memory: there is nothing to report the connection with, and - nothing is cached, so the cache stays consistent */ - } else if (hid_internal_hotplug_is_cached(path)) { + if (hid_internal_hotplug_is_cached(event_data->u.DeviceInterface.SymbolicLink)) { /* This connection is already known - and therefore already reported. The only way that happens is the arm-before-enumerate window at registration, where the enumeration cached the device and the OS had @@ -1915,24 +1992,16 @@ DWORD WINAPI hid_internal_notify_callback(HCMNOTIFICATION notify, PVOID context, report it with; the cache is left consistent either way, as only fully described devices are ever cached. */ } - - free(path); } else if (action == CM_NOTIFY_ACTION_DEVICEINTERFACEREMOVAL) { - char *path; - hotplug_event = HID_API_HOTPLUG_EVENT_DEVICE_LEFT; - path = hid_internal_UTF16toUTF8(event_data->u.DeviceInterface.SymbolicLink, NULL); - - if (path != NULL) { - /* Get and remove this device from the device list. Dropping the entry is - what makes a later arrival on the same path (an interface path is - reused when a device is replugged into the same port) a new connection - rather than a duplicate - see hid_internal_hotplug_is_cached. */ - device = hid_internal_hotplug_take_cached_device(path); - - free(path); - } + /* Get and remove this device from the device list. Dropping the entry is + what makes a later arrival on the same path (an interface path is reused + when a device is replugged into the same port) a new connection rather + than a duplicate - see hid_internal_hotplug_is_cached. The lookup cannot + fail for lack of memory (it allocates nothing), so a removal can never + leave its record behind. */ + device = hid_internal_hotplug_take_cached_device(event_data->u.DeviceInterface.SymbolicLink); } if (device) { From 7827d8fe8ae2afed50252f82003d20af69f90bc6 Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Wed, 15 Jul 2026 03:37:33 +0300 Subject: [PATCH 7/9] windows: keep the internal event context from writing the global error The hotplug callback runs on HIDAPI's internal event context (a threadpool work item or the CM notification callback). A user callback that re-enters the public hid_hotplug_register/deregister_callback from there wrote the global error string on that context, which an application cannot serialize its lock-free hid_error(NULL) read against. Drop such writes at the global-error chokepoint while a dispatch is in progress (mutex_in_use), matching the other backends; per-device hid_error(dev) writes are unaffected. App-thread failures still set the global error as before. Also guard hid_winapi_get_container_id against a NULL dev->device_info, like its sibling accessors: hid_open_path stores hid_internal_get_device_info() without a NULL check and that helper can now return NULL. Assisted-by: claude-code:claude-opus-4-8 --- windows/hid.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/windows/hid.c b/windows/hid.c index 679abc74e..97b4a2be3 100644 --- a/windows/hid.c +++ b/windows/hid.c @@ -475,9 +475,11 @@ static wchar_t *last_global_error_str = NULL; hands the raw string pointer out to the application without any lock, which is why the header requires the application to serialize hid_error(NULL) against the hotplug API. HIDAPI's own code on the internal event context - never writes the global error; a user callback that calls the public hotplug - API from the event context does, and that same serialization requirement in - the header covers it. */ + never writes the global error - not even a user callback that re-enters the + public hotplug API from that context: register_global_error_message() drops + those writes (see the mutex_in_use guard there). An application therefore + never has to serialize hid_error(NULL) against a write it could not see + coming, matching the other backends (libusb, linux, mac). */ static hid_internal_lock global_error_lock = 0; /* Publishes a message (built by the caller, ownership taken) as the global @@ -487,6 +489,19 @@ static void register_global_error_message(wchar_t *msg) { wchar_t *old_msg; + /* A user callback runs on HIDAPI's internal event context (a threadpool + work item or the CM notification callback), where mutex_in_use is set for + the whole dispatch. A nested public hotplug call made from such a callback + must not touch last_global_error_str - success clear included: the write + happens on the event context, and the application cannot serialize its + lock-free hid_error(NULL) read against it. Drop the write instead. This is + a cheap byte read of a flag the dispatching thread itself set (and the same + thread holds the critical section), so it adds no lock-order edge. */ + if (hid_hotplug_context.mutex_in_use) { + free(msg); + return; + } + hid_internal_lock_acquire(&global_error_lock); old_msg = last_global_error_str; last_global_error_str = msg; @@ -2950,6 +2965,11 @@ int HID_API_EXPORT_CALL hid_winapi_get_container_id(hid_device *dev, GUID *conta return -1; } + if (!dev->device_info) { + register_string_error(dev, L"NULL device info"); + return -1; + } + register_string_error(dev, NULL); interface_path = hid_internal_UTF8toUTF16(dev->device_info->path); From a1a0319febb1ef1bf2301cddeaa8cdceeac63bce Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Wed, 15 Jul 2026 03:55:19 +0300 Subject: [PATCH 8/9] windows: detect the hotplug event context with a thread-local flag The previous guard suppressed global-error writes based on mutex_in_use, which is process-global ("a dispatch is in progress somewhere"). That wrongly dropped the genuine error of a concurrent application thread calling the hotplug API while a callback ran on the event context, and it read the flag without synchronization (a data race). Use a thread-local flag set only around each user-callback invocation in both dispatch paths instead, so suppression applies to the dispatching thread alone: an application thread always records its errors, while a callback's nested register/deregister writes (failure and the success-path clear) are dropped. This matches how the other backends key off the event thread's identity. Assisted-by: claude-code:claude-opus-4-8 --- windows/hid.c | 43 ++++++++++++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 11 deletions(-) diff --git a/windows/hid.c b/windows/hid.c index 97b4a2be3..66d3d14f5 100644 --- a/windows/hid.c +++ b/windows/hid.c @@ -469,6 +469,15 @@ static void hid_internal_lock_release(hid_internal_lock *lock) static wchar_t *last_global_error_str = NULL; +/* Thread-local: true only while THIS thread is inside a user hotplug callback, + i.e. it is HIDAPI's internal event context. Thread-specific on purpose - the + event context is not a single fixed thread (a threadpool work item or the CM + notification callback), so a stored thread id would be fragile, and the flag + must be true for the dispatching thread alone, never for a concurrent + application thread. This is how mac/libusb detect the event context (they + compare its thread identity). */ +static __declspec(thread) int hid_in_hotplug_callback = 0; + /* Serializes mutations of last_global_error_str: the hotplug API is thread-safe and its failure paths may write the global error concurrently. Note that this only protects writers against each other: hid_error(NULL) @@ -477,9 +486,9 @@ static wchar_t *last_global_error_str = NULL; against the hotplug API. HIDAPI's own code on the internal event context never writes the global error - not even a user callback that re-enters the public hotplug API from that context: register_global_error_message() drops - those writes (see the mutex_in_use guard there). An application therefore - never has to serialize hid_error(NULL) against a write it could not see - coming, matching the other backends (libusb, linux, mac). */ + those writes (see hid_in_hotplug_callback). An application therefore never + has to serialize hid_error(NULL) against a write it could not see coming, + matching the other backends (libusb, linux, mac). */ static hid_internal_lock global_error_lock = 0; /* Publishes a message (built by the caller, ownership taken) as the global @@ -490,14 +499,15 @@ static void register_global_error_message(wchar_t *msg) wchar_t *old_msg; /* A user callback runs on HIDAPI's internal event context (a threadpool - work item or the CM notification callback), where mutex_in_use is set for - the whole dispatch. A nested public hotplug call made from such a callback - must not touch last_global_error_str - success clear included: the write - happens on the event context, and the application cannot serialize its - lock-free hid_error(NULL) read against it. Drop the write instead. This is - a cheap byte read of a flag the dispatching thread itself set (and the same - thread holds the critical section), so it adds no lock-order edge. */ - if (hid_hotplug_context.mutex_in_use) { + work item or the CM notification callback). A nested public hotplug call + made from such a callback must not touch last_global_error_str - success + clear included: the write happens on the event context, and the + application cannot serialize its lock-free hid_error(NULL) read against it. + hid_in_hotplug_callback is thread-local and set only around the callback + invocation, so this drops writes on the dispatching thread alone. A + concurrent application thread (its own flag is 0) still records its errors, + and reading a thread-local is not a shared-memory data race. */ + if (hid_in_hotplug_callback) { free(msg); return; } @@ -1067,7 +1077,13 @@ static void hid_internal_hotplug_replay_flush(struct hid_hotplug_callback *callb continue; } + /* Mark this thread as the event context for the duration of the call, so a + nested public hotplug call does not write the global error (save/restore + keeps nested callbacks composing correctly). */ + int prev_in_cb = hid_in_hotplug_callback; + hid_in_hotplug_callback = 1; int result = (*callback->callback)(callback->handle, device, HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED, callback->user_data); + hid_in_hotplug_callback = prev_in_cb; hid_free_enumeration(device); /* A non-zero result stops the remainder of the pass and deregisters the callback */ @@ -1933,7 +1949,12 @@ static void hid_internal_hotplug_dispatch(struct hid_device_info *device, hid_ho hid_internal_hotplug_replay_flush(callback); if ((callback->events & hotplug_event) && hid_internal_match_device_id(device->vendor_id, device->product_id, callback->vendor_id, callback->product_id)) { + /* Mark this thread as the event context for the duration of the call + (see hid_in_hotplug_callback); save/restore for nested callbacks. */ + int prev_in_cb = hid_in_hotplug_callback; + hid_in_hotplug_callback = 1; int result = (callback->callback)(callback->handle, device, hotplug_event, callback->user_data); + hid_in_hotplug_callback = prev_in_cb; /* If the result is non-zero, we MARK the callback for future removal and proceed */ /* We avoid changing the list until we are done calling the callbacks to simplify the process */ From 9c0b299c0c794c3554ee3d11c76528c15641980b Mon Sep 17 00:00:00 2001 From: Ihor Dutchak Date: Wed, 15 Jul 2026 04:04:26 +0300 Subject: [PATCH 9/9] windows: use a portable thread-local for the hotplug-callback flag __declspec(thread) is silently ignored by MinGW/Cygwin GCC (a -Werror=attributes failure), breaking the fedora-mingw and windows-cmake-mingw builds. Select __declspec(thread) for MSVC/clang-cl and __thread for GCC/Clang. Assisted-by: claude-code:claude-opus-4-8 --- windows/hid.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/windows/hid.c b/windows/hid.c index 66d3d14f5..44fafbdd4 100644 --- a/windows/hid.c +++ b/windows/hid.c @@ -476,7 +476,12 @@ static wchar_t *last_global_error_str = NULL; must be true for the dispatching thread alone, never for a concurrent application thread. This is how mac/libusb detect the event context (they compare its thread identity). */ -static __declspec(thread) int hid_in_hotplug_callback = 0; +#if defined(_MSC_VER) +#define HID_THREAD_LOCAL __declspec(thread) +#else +#define HID_THREAD_LOCAL __thread /* GCC/Clang (MinGW, Cygwin): __declspec(thread) is ignored there */ +#endif +static HID_THREAD_LOCAL int hid_in_hotplug_callback = 0; /* Serializes mutations of last_global_error_str: the hotplug API is thread-safe and its failure paths may write the global error concurrently.