diff --git a/.gitignore b/.gitignore index a479f336..ee128dba 100644 --- a/.gitignore +++ b/.gitignore @@ -33,4 +33,6 @@ doxygen/html/ # Visual Studio Code + CMake .vscode/ + +# Local build directory build/ diff --git a/libusb/hid.c b/libusb/hid.c index ad0c4e87..b42d5265 100644 --- a/libusb/hid.c +++ b/libusb/hid.c @@ -172,7 +172,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). + * Protected by `mutex`: all reads, writes and the final free during teardown + * are performed while holding it. The teardown free runs in + * hid_internal_hotplug_cleanup() after the hotplug threads have been joined. */ struct hid_device_info *devs; } hid_hotplug_context = { .next_handle = FIRST_HOTPLUG_CALLBACK_HANDLE, @@ -584,6 +587,11 @@ static void hid_internal_hotplug_cleanup() /* Wait for both threads to stop */ hidapi_thread_join(&hid_hotplug_context.libusb_thread); + + /* Both hotplug threads have exited: we now have exclusive access to `devs` + * (the caller holds `mutex` and no hotplug event can reach process_hotplug_event). */ + hid_free_enumeration(hid_hotplug_context.devs); + hid_hotplug_context.devs = NULL; } static void hid_internal_hotplug_init() @@ -1142,8 +1150,8 @@ static int hid_libusb_hotplug_callback(libusb_context *ctx, libusb_device *devic msg->event = event; msg->next = NULL; - /* We use this thread's mutex to protect the queue */ - hidapi_thread_mutex_lock(&hid_hotplug_context.libusb_thread); + /* Use callback_thread's mutex to protect the queue and signal it */ + hidapi_thread_mutex_lock(&hid_hotplug_context.callback_thread); struct hid_hotplug_queue* end = hid_hotplug_context.queue; if (end) { while (end->next) { @@ -1153,16 +1161,21 @@ static int hid_libusb_hotplug_callback(libusb_context *ctx, libusb_device *devic } else { hid_hotplug_context.queue = msg; } - hidapi_thread_mutex_unlock(&hid_hotplug_context.libusb_thread); - /* Wake up the other thread so it can react to the new message immediately */ + /* Wake up the callback thread so it can react to the new message immediately */ hidapi_thread_cond_signal(&hid_hotplug_context.callback_thread); + hidapi_thread_mutex_unlock(&hid_hotplug_context.callback_thread); return 0; } static void process_hotplug_event(struct hid_hotplug_queue* msg) { + /* Lock the mutex to avoid race conditions with hid_hotplug_register_callback(), + * which may iterate devs during HID_API_HOTPLUG_ENUMERATE while holding this mutex. + * The mutex is recursive, so hid_internal_invoke_callbacks() can safely re-acquire it. */ + pthread_mutex_lock(&hid_hotplug_context.mutex); + if (msg->event == LIBUSB_HOTPLUG_EVENT_DEVICE_ARRIVED) { struct hid_device_info* info = hid_enumerate_from_libusb(msg->device, 0, 0); struct hid_device_info* info_cur = info; @@ -1203,6 +1216,8 @@ static void process_hotplug_event(struct hid_hotplug_queue* msg) } } + pthread_mutex_unlock(&hid_hotplug_context.mutex); + /* Release the libusb device - we are done with it */ libusb_unref_device(msg->device); @@ -1215,40 +1230,32 @@ static void* callback_thread(void* user_data) { (void) user_data; - /* 5 msec timeout seems reasonable; don't set too low to avoid high CPU usage */ - /* This timeout only affects how much time it takes to stop the thread */ - hidapi_timespec ts; - ts.tv_sec = 0; - ts.tv_nsec = 5000000; - hidapi_thread_mutex_lock(&hid_hotplug_context.callback_thread); - + /* We stop the thread if by the moment there are no events left in the queue there are no callbacks left */ while (1) { - /* Make the tread fall asleep and wait for a condition to wake it up */ - hidapi_thread_cond_timedwait(&hid_hotplug_context.callback_thread, &ts); + /* Wait for events to arrive or shutdown signal */ + while (!hid_hotplug_context.queue && hid_hotplug_context.hotplug_cbs) { + hidapi_thread_cond_wait(&hid_hotplug_context.callback_thread); + } - /* We use this thread's mutex to protect the queue */ - hidapi_thread_mutex_lock(&hid_hotplug_context.libusb_thread); + /* Process all pending events from the queue */ while (hid_hotplug_context.queue) { struct hid_hotplug_queue *cur_event = hid_hotplug_context.queue; - process_hotplug_event(cur_event); + hid_hotplug_context.queue = cur_event->next; - /* Empty the queue */ - cur_event = cur_event->next; - free(hid_hotplug_context.queue); - hid_hotplug_context.queue = cur_event; + /* Release the lock while processing to avoid blocking event producers */ + hidapi_thread_mutex_unlock(&hid_hotplug_context.callback_thread); + process_hotplug_event(cur_event); + free(cur_event); + hidapi_thread_mutex_lock(&hid_hotplug_context.callback_thread); } - hidapi_thread_mutex_unlock(&hid_hotplug_context.libusb_thread); + if (!hid_hotplug_context.hotplug_cbs) { break; } } - /* Cleanup connected device list */ - hid_free_enumeration(hid_hotplug_context.devs); - hid_hotplug_context.devs = NULL; - hidapi_thread_mutex_unlock(&hid_hotplug_context.callback_thread); return NULL; @@ -1275,8 +1282,12 @@ static void* hotplug_thread(void* user_data) libusb_hotplug_deregister_callback(hid_hotplug_context.context, hid_hotplug_context.callback_handle); libusb_exit(hid_hotplug_context.context); - /* Forcibly wake up the thread so it can shut down immediately and wait for it to stop */ + /* hotplug_cbs is already NULL here (the loop above exited because of that). + * Signal callback_thread under the mutex so it can observe the NULL hotplug_cbs + * and exit cleanly, rather than waiting indefinitely in cond_wait. */ + hidapi_thread_mutex_lock(&hid_hotplug_context.callback_thread); hidapi_thread_cond_signal(&hid_hotplug_context.callback_thread); + hidapi_thread_mutex_unlock(&hid_hotplug_context.callback_thread); hidapi_thread_join(&hid_hotplug_context.callback_thread); @@ -1353,8 +1364,12 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven LIBUSB_HOTPLUG_EVENT_DEVICE_ARRIVED | LIBUSB_HOTPLUG_EVENT_DEVICE_LEFT, 0, LIBUSB_HOTPLUG_MATCH_ANY, LIBUSB_HOTPLUG_MATCH_ANY, LIBUSB_HOTPLUG_MATCH_ANY, &hid_libusb_hotplug_callback, NULL, &hid_hotplug_context.callback_handle)) { - /* Major malfunction, failed to register a callback */ + /* Major malfunction, failed to register a callback. + * No hotplug thread was started, so we must unwind `devs` ourselves + * (hid_internal_hotplug_cleanup() would try to join a non-existent thread). */ libusb_exit(hid_hotplug_context.context); + hid_free_enumeration(hid_hotplug_context.devs); + hid_hotplug_context.devs = NULL; free(hotplug_cb); hid_hotplug_context.hotplug_cbs = NULL; pthread_mutex_unlock(&hid_hotplug_context.mutex);