diff --git a/hidtest/test.c b/hidtest/test.c index 9fbf2321b..3667a5756 100644 --- a/hidtest/test.c +++ b/hidtest/test.c @@ -19,13 +19,17 @@ #include #include #include +#include // for "tolower()" #include -// Headers needed for sleeping. +// Headers needed for sleeping and console management (wait for a keypress) #ifdef _WIN32 #include + #include #else + #include + #include #include #endif @@ -50,8 +54,46 @@ #if defined(USING_HIDAPI_LIBUSB) && HID_API_VERSION >= HID_API_MAKE_VERSION(0, 12, 0) #include #endif + // +// A function that waits for a key to be pressed and reports it's code +// Used for immediate response in interactive subroutines +// Taken from: +// https://cboard.cprogramming.com/c-programming/63166-kbhit-linux.html +int waitkey(void) +{ +#ifdef _WIN32 + return _getch(); +#else + struct termios oldt, newt; + int ch; + int oldf; + + tcgetattr(STDIN_FILENO, &oldt); + newt = oldt; + newt.c_lflag &= ~(ICANON | ECHO); + tcsetattr(STDIN_FILENO, TCSANOW, &newt); + oldf = fcntl(STDIN_FILENO, F_GETFL, 0); + fcntl(STDIN_FILENO, F_SETFL, oldf | O_NONBLOCK); + + do + { + usleep(1); + ch = getchar(); + } + while (EOF == ch); + + tcsetattr(STDIN_FILENO, TCSANOW, &oldt); + fcntl(STDIN_FILENO, F_SETFL, oldf); + return ch; +#endif +} + +// + +// +// Report Device info const char *hid_bus_name(hid_bus_type bus_type) { static const char *const HidBusTypeName[] = { "Unknown", @@ -126,80 +168,28 @@ void print_devices_with_descriptor(struct hid_device_info *cur_dev) { } } -int device_callback( - hid_hotplug_callback_handle callback_handle, - struct hid_device_info* device, - hid_hotplug_event event, - void* user_data) +// +// Default static testing +void test_static(void) { - (void)user_data; - - if (event & HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED) - printf("Handle %d: New device is connected: %s.\n", callback_handle, device->path); - else - printf("Handle %d: Device was disconnected: %s.\n", callback_handle, device->path); - - printf("type: %04hx %04hx\n serial_number: %ls", device->vendor_id, device->product_id, device->serial_number); - printf("\n"); - printf(" Manufacturer: %ls\n", device->manufacturer_string); - printf(" Product: %ls\n", device->product_string); - printf(" Release: %hx\n", device->release_number); - printf(" Interface: %d\n", device->interface_number); - printf(" Usage (page): 0x%hx (0x%hx)\n", device->usage, device->usage_page); - printf("\n"); - - /* Printed data might not show on the screen - force it out */ - fflush(stdout); + struct hid_device_info *devs; - return 0; + devs = hid_enumerate(0x0, 0x0); + print_devices_with_descriptor(devs); + hid_free_enumeration(devs); } -int main(int argc, char* argv[]) -{ - (void)argc; - (void)argv; +// +// Fixed device testing +void test_device(void) +{ int res; unsigned char buf[256]; - #define MAX_STR 255 +#define MAX_STR 255 wchar_t wstr[MAX_STR]; hid_device *handle; int i; - hid_hotplug_callback_handle token1, token2; - - struct hid_device_info *devs; - - printf("hidapi test/example tool. Compiled with hidapi version %s, runtime version %s.\n", HID_API_VERSION_STR, hid_version_str()); - if (HID_API_VERSION == HID_API_MAKE_VERSION(hid_version()->major, hid_version()->minor, hid_version()->patch)) { - printf("Compile-time version matches runtime version of hidapi.\n\n"); - } - else { - printf("Compile-time version is different than runtime version of hidapi.\n]n"); - } - - if (hid_init()) - return -1; - -#if defined(__APPLE__) && HID_API_VERSION >= HID_API_MAKE_VERSION(0, 12, 0) - // To work properly needs to be called before hid_open/hid_open_path after hid_init. - // Best/recommended option - call it right after hid_init. - hid_darwin_set_open_exclusive(0); -#endif - - devs = hid_enumerate(0x0, 0x0); - print_devices_with_descriptor(devs); - hid_free_enumeration(devs); - - hid_hotplug_register_callback(0, 0, HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED | HID_API_HOTPLUG_EVENT_DEVICE_LEFT, HID_API_HOTPLUG_ENUMERATE, device_callback, NULL, &token1); - hid_hotplug_register_callback(0x054c, 0x0ce6, HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED | HID_API_HOTPLUG_EVENT_DEVICE_LEFT, HID_API_HOTPLUG_ENUMERATE, device_callback, NULL, &token2); - - while (1) - { - - } - - hid_hotplug_deregister_callback(token2); - hid_hotplug_deregister_callback(token1); // Set up the command buffer. memset(buf,0x00,sizeof(buf)); @@ -213,8 +203,7 @@ int main(int argc, char* argv[]) handle = hid_open(0x4d8, 0x3f, NULL); if (!handle) { printf("unable to open device\n"); - hid_exit(); - return 1; + return; } // Read the Manufacturer String @@ -344,13 +333,221 @@ int main(int argc, char* argv[]) } hid_close(handle); +} - /* Free static HIDAPI objects. */ - hid_exit(); +// +// Normal hotplug testing +int device_callback( + hid_hotplug_callback_handle callback_handle, + struct hid_device_info* device, + hid_hotplug_event event, + void* user_data) +{ + (void)user_data; -#ifdef _WIN32 - system("pause"); + if (event & HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED) + printf("Handle %d: New device is connected: %s.\n", callback_handle, device->path); + else + printf("Handle %d: Device was disconnected: %s.\n", callback_handle, device->path); + + printf("type: %04hx %04hx\n serial_number: %ls", device->vendor_id, device->product_id, device->serial_number); + printf("\n"); + printf(" Manufacturer: %ls\n", device->manufacturer_string); + printf(" Product: %ls\n", device->product_string); + printf(" Release: %hx\n", device->release_number); + printf(" Interface: %d\n", device->interface_number); + printf(" Usage (page): 0x%hx (0x%hx)\n", device->usage, device->usage_page); + printf("(Press Q to exit the test)\n"); + printf("\n"); + + return 0; +} + + +void test_hotplug(void) +{ + printf("Starting the Hotplug test\n"); + printf("(Press Q to exit the test)\n"); + + hid_hotplug_callback_handle token1, token2; + + hid_hotplug_register_callback(0, 0, HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED | HID_API_HOTPLUG_EVENT_DEVICE_LEFT, HID_API_HOTPLUG_ENUMERATE, device_callback, NULL, &token1); + hid_hotplug_register_callback(0x054c, 0x0ce6, HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED | HID_API_HOTPLUG_EVENT_DEVICE_LEFT, HID_API_HOTPLUG_ENUMERATE, device_callback, NULL, &token2); + + while (1) + { + int command = tolower(waitkey()); + if ('q' == command) + { + break; + } + } + + hid_hotplug_deregister_callback(token2); + hid_hotplug_deregister_callback(token1); + + printf("\n\nHotplug test stopped\n"); +} + +// +// Stress-testing weird edge cases in hotplugs +int cb1_handle; +int cb2_handle; +int cb_test1_triggered; + +int cb2_func(hid_hotplug_callback_handle callback_handle, + struct hid_device_info *device, + hid_hotplug_event event, + void *user_data) +{ + (void) callback_handle; + (void) device; + (void) event; + (void) user_data; + // TIP: only perform the test once + if(cb_test1_triggered) + { + return 1; + } + + printf("Callback 2 fired\n"); + + // Deregister the first callback + // It should be placed in the list at an index prior to the current one, which will make the pointer to the current one invalid on some implementations + hid_hotplug_deregister_callback(cb1_handle); + + cb_test1_triggered = 1; + + // As long as we are inside this callback, nothing goes wrong; however, returning from here will cause a use-after-free error on flawed implementations + // as to retrieve the next element (or to check for it's presence) it will look those dereference a pointer located in an already freed area + // Undefined behavior + return 1; +} + +int cb1_func(hid_hotplug_callback_handle callback_handle, + struct hid_device_info *device, + hid_hotplug_event event, + void *user_data) +{ + (void) callback_handle; + (void) device; + (void) event; + (void) user_data; + + // TIP: only perform the test once + if(cb_test1_triggered) + { + return 1; + } + + printf("Callback 1 fired\n"); + + // Register the second callback and make it be called immediately by enumeration attempt + // Will cause a deadlock on Linux immediately + hid_hotplug_register_callback(0, 0, HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED | HID_API_HOTPLUG_EVENT_DEVICE_LEFT, HID_API_HOTPLUG_ENUMERATE, cb2_func, NULL, &cb2_handle); + return 1; +} + +void test_hotplug_deadlocks(void) +{ + cb_test1_triggered = 0; + printf("Starting the Hotplug callbacks deadlocks test\n"); + printf("TIP: if you don't see a message that it succeeded, it means the test failed and the system is now deadlocked\n"); + // Register the first callback and make it be called immediately by enumeration attempt (if at least 1 device is present) + hid_hotplug_register_callback(0, 0, HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED | HID_API_HOTPLUG_EVENT_DEVICE_LEFT, HID_API_HOTPLUG_ENUMERATE, cb1_func, NULL, &cb1_handle); + + printf("Test finished successfully (at least no deadlocks were found)\n"); + + // Intentionally leave a callback registered to test how hid_exit handles it + //hid_hotplug_deregister_callback(cb2_handle); +} + + +// +// CLI + +void print_version_check(void) +{ + printf("hidapi test/example tool. Compiled with hidapi version %s, runtime version %s.\n", HID_API_VERSION_STR, hid_version_str()); + if (HID_API_VERSION == HID_API_MAKE_VERSION(hid_version()->major, hid_version()->minor, hid_version()->patch)) { + printf("Compile-time version matches runtime version of hidapi.\n\n"); + } + else { + printf("Compile-time version is different than runtime version of hidapi.\n]n"); + } +} + +void interactive_loop(void) +{ + int command = 0; + + print_version_check(); + + do { + printf("Interactive HIDAPI testing utility\n"); + printf(" 1: List connected devices\n"); + printf(" 2: Dynamic hotplug test\n"); + printf(" 3: Test specific device [04d8:003f]\n"); + printf(" 4: Test hotplug callback management deadlocking scenario\n"); + printf(" Q: Quit\n"); + printf("Please enter command:"); + + /* Printed data might not show on the screen when the command is done - force it out */ + fflush(stdout); + + command = toupper(waitkey()); + + printf("%c\n\n========================================\n\n", command); + + // GET COMMAND + switch (command) { + case '1': + test_static(); + break; + case '2': + test_hotplug(); + break; + case '3': + test_device(); + break; + case '4': + test_hotplug_deadlocks(); + break; + case 'Q': + printf("Quitting.\n"); + return; + default: + printf("Command not recognized\n"); + break; + } + + /* Printed data might not show on the screen when the command is done - force it out */ + fflush(stdout); + + printf("\n\n========================================\n\n"); + } while(command != 'Q'); +} + +// +// Main +int main(int argc, char* argv[]) +{ + (void)argc; + (void)argv; + + if (hid_init()) + return -1; + +#if defined(__APPLE__) && HID_API_VERSION >= HID_API_MAKE_VERSION(0, 12, 0) + // To work properly needs to be called before hid_open/hid_open_path after hid_init. + // Best/recommended option - call it right after hid_init. + hid_darwin_set_open_exclusive(0); #endif + interactive_loop(); + + /* Free static HIDAPI objects. */ + hid_exit(); + return 0; } diff --git a/libusb/hid.c b/libusb/hid.c index 22e1234fe..47077a0f4 100644 --- a/libusb/hid.c +++ b/libusb/hid.c @@ -160,11 +160,12 @@ static struct hid_hotplug_context { hidapi_thread_state callback_thread; /* This mutex prevents changes to the callback list */ - pthread_mutex_t cb_mutex; + pthread_mutex_t mutex; - int mutex_ready; - - int thread_running; + /* Boolean flags */ + unsigned char mutex_ready; + unsigned char mutex_in_use; + unsigned char cb_list_dirty; struct hid_hotplug_queue* queue; @@ -176,7 +177,6 @@ static struct hid_hotplug_context { } hid_hotplug_context = { .next_handle = FIRST_HOTPLUG_CALLBACK_HANDLE, .mutex_ready = 0, - .thread_running = 0, .queue = NULL, .hotplug_cbs = NULL, .devs = NULL, @@ -541,28 +541,46 @@ struct hid_hotplug_callback hid_hotplug_callback_handle handle; }; +static void hid_internal_hotplug_remove_postponed() +{ + /* Unregister the callbacks whose removal was postponed */ + /* This function is always called inside a locked mutex */ + /* However, any actions are only allowed if the mutex is NOT in use and if the DIRTY flag is set */ + if (!hid_hotplug_context.mutex_ready || hid_hotplug_context.mutex_in_use || !hid_hotplug_context.cb_list_dirty) { + return; + } + + /* Traverse the list of callbacks and check if any were marked for removal */ + struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; + while (*current) { + struct hid_hotplug_callback *callback = *current; + if (!callback->events) { + *current = (*current)->next; + 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() { - if (hid_hotplug_context.hotplug_cbs != NULL) { + if (!hid_hotplug_context.mutex_ready || hid_hotplug_context.mutex_in_use) { return; } - /* Mark the threads as stopped */ - hid_hotplug_context.thread_running = 0; + /* Before checking if the list is empty, clear any entries whose removal was postponed first */ + hid_internal_hotplug_remove_postponed(); - /* Forcibly wake up the thread so it can shut down immediately */ - hidapi_thread_cond_signal(&hid_hotplug_context.callback_thread); + if (hid_hotplug_context.hotplug_cbs != NULL) { + return; + } /* Wait for both threads to stop */ hidapi_thread_join(&hid_hotplug_context.libusb_thread); - hidapi_thread_join(&hid_hotplug_context.callback_thread); - - /* Cleanup connected device list */ - hid_free_enumeration(hid_hotplug_context.devs); - hid_hotplug_context.devs = NULL; - /* Disarm the libusb listener */ - libusb_hotplug_deregister_callback(usb_context, hid_hotplug_context.callback_handle); - libusb_exit(hid_hotplug_context.context); } static void hid_internal_hotplug_init() @@ -570,8 +588,18 @@ static void hid_internal_hotplug_init() if (!hid_hotplug_context.mutex_ready) { hidapi_thread_state_init(&hid_hotplug_context.libusb_thread); hidapi_thread_state_init(&hid_hotplug_context.callback_thread); - pthread_mutex_init(&hid_hotplug_context.cb_mutex, NULL); + + /* Initialize the mutex as recursive */ + pthread_mutexattr_t attr; + pthread_mutexattr_init(&attr); + pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); + pthread_mutex_init(&hid_hotplug_context.mutex, &attr); + pthread_mutexattr_destroy(&attr); + + /* Set state to Ready */ hid_hotplug_context.mutex_ready = 1; + hid_hotplug_context.mutex_in_use = 0; + hid_hotplug_context.cb_list_dirty = 0; } } @@ -581,7 +609,7 @@ static void hid_internal_hotplug_exit() return; } - pthread_mutex_lock(&hid_hotplug_context.cb_mutex); + pthread_mutex_lock(&hid_hotplug_context.mutex); struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; /* Remove all callbacks from the list */ while (*current) { @@ -590,9 +618,9 @@ static void hid_internal_hotplug_exit() *current = next; } hid_internal_hotplug_cleanup(); - pthread_mutex_unlock(&hid_hotplug_context.cb_mutex); + pthread_mutex_unlock(&hid_hotplug_context.mutex); hid_hotplug_context.mutex_ready = 0; - pthread_mutex_destroy(&hid_hotplug_context.cb_mutex); + pthread_mutex_destroy(&hid_hotplug_context.mutex); hidapi_thread_state_destroy(&hid_hotplug_context.callback_thread); hidapi_thread_state_destroy(&hid_hotplug_context.libusb_thread); @@ -1071,25 +1099,27 @@ static int match_libusb_to_info(libusb_device *device, struct hid_device_info* i static void hid_internal_invoke_callbacks(struct hid_device_info* info, hid_hotplug_event event) { - pthread_mutex_lock(&hid_hotplug_context.cb_mutex); + pthread_mutex_lock(&hid_hotplug_context.mutex); + hid_hotplug_context.mutex_in_use = 1; struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; while (*current) { struct hid_hotplug_callback *callback = *current; if ((callback->events & event) && hid_internal_match_device_id(info->vendor_id, info->product_id, callback->vendor_id, callback->product_id)) { int result = callback->callback(callback->handle, info, event, callback->user_data); - /* If the result is non-zero, we remove the callback and proceed */ - /* Do not use the deregister call as it locks the mutex, and we are currently in a lock */ + /* If the result is non-zero, we mark the callback for removal and proceed */ if (result) { - struct hid_hotplug_callback *callback = *current; - *current = (*current)->next; - free(callback); + (*current)->events = 0; + hid_hotplug_context.cb_list_dirty = 1; continue; } } current = &callback->next; } - pthread_mutex_unlock(&hid_hotplug_context.cb_mutex); + + hid_hotplug_context.mutex_in_use = 0; + hid_internal_hotplug_remove_postponed(); + pthread_mutex_unlock(&hid_hotplug_context.mutex); } static int hid_libusb_hotplug_callback(libusb_context *ctx, libusb_device *device, libusb_hotplug_event event, void * user_data) @@ -1128,23 +1158,6 @@ static int hid_libusb_hotplug_callback(libusb_context *ctx, libusb_device *devic return 0; } -static void* hotplug_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 */ - struct timeval tv; - tv.tv_sec = 0; - tv.tv_usec = 5000; - - while (hid_hotplug_context.thread_running) { - /* This will allow libusb to call the callbacks, which will fill up the queue */ - libusb_handle_events_timeout_completed(hid_hotplug_context.context, &tv, NULL); - } - return NULL; -} - static void process_hotplug_event(struct hid_hotplug_queue* msg) { if (msg->event == LIBUSB_HOTPLUG_EVENT_DEVICE_ARRIVED) { @@ -1190,10 +1203,9 @@ static void process_hotplug_event(struct hid_hotplug_queue* msg) /* Release the libusb device - we are done with it */ libusb_unref_device(msg->device); - /* Clean up if the last callback was removed */ - pthread_mutex_lock(&hid_hotplug_context.cb_mutex); - hid_internal_hotplug_cleanup(); - pthread_mutex_unlock(&hid_hotplug_context.cb_mutex); + /* Cleanup note: this function is called inside a thread that the clenup function would be waiting to finish */ + /* Any callbacks that await removal are removed in hid_internal_invoke_callbacks */ + /* No further cleaning is needed */ } static void* callback_thread(void* user_data) @@ -1207,25 +1219,67 @@ static void* callback_thread(void* user_data) ts.tv_nsec = 5000000; hidapi_thread_mutex_lock(&hid_hotplug_context.callback_thread); - while (hid_hotplug_context.thread_running) { + + /* 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); + /* We use this thread's mutex to protect the queue */ hidapi_thread_mutex_lock(&hid_hotplug_context.libusb_thread); while (hid_hotplug_context.queue) { - process_hotplug_event(hid_hotplug_context.queue); + struct hid_hotplug_queue *cur_event = hid_hotplug_context.queue; + process_hotplug_event(cur_event); /* Empty the queue */ - hid_hotplug_context.queue = hid_hotplug_context.queue->next; + cur_event = cur_event->next; + free(hid_hotplug_context.queue); + hid_hotplug_context.queue = cur_event; } hidapi_thread_mutex_unlock(&hid_hotplug_context.libusb_thread); - - /* Make the tread fall asleep and wait for a condition to wake it up */ - hidapi_thread_cond_timedwait(&hid_hotplug_context.callback_thread, &ts); + 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; } +static void* hotplug_thread(void* user_data) +{ + (void) user_data; + + hidapi_thread_create(&hid_hotplug_context.callback_thread, callback_thread, NULL); + + /* 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 */ + struct timeval tv; + tv.tv_sec = 0; + tv.tv_usec = 5000; + + while (hid_hotplug_context.hotplug_cbs) { + /* This will allow libusb to call the callbacks, which will fill up the queue */ + libusb_handle_events_timeout_completed(hid_hotplug_context.context, &tv, NULL); + } + + /* Disarm the libusb listener */ + 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 */ + hidapi_thread_cond_signal(&hid_hotplug_context.callback_thread); + + hidapi_thread_join(&hid_hotplug_context.callback_thread); + + return NULL; +} + 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) { if (!libusb_has_capability(LIBUSB_CAP_HAS_HOTPLUG)) { @@ -1258,7 +1312,7 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven hid_internal_hotplug_init(); /* Lock the mutex to avoid race itions */ - pthread_mutex_lock(&hid_hotplug_context.cb_mutex); + pthread_mutex_lock(&hid_hotplug_context.mutex); hotplug_cb->handle = hid_hotplug_context.next_handle++; @@ -1284,7 +1338,7 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven /* Fill already connected devices so we can use this info in disconnection notification */ if (libusb_init(&hid_hotplug_context.context)) { free(hotplug_cb); - pthread_mutex_unlock(&hid_hotplug_context.cb_mutex); + pthread_mutex_unlock(&hid_hotplug_context.mutex); return -1; } @@ -1300,16 +1354,18 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven libusb_exit(hid_hotplug_context.context); free(hotplug_cb); hid_hotplug_context.hotplug_cbs = NULL; - pthread_mutex_unlock(&hid_hotplug_context.cb_mutex); + pthread_mutex_unlock(&hid_hotplug_context.mutex); return -1; } /* Initialization succeeded! We run the threads now */ - hid_hotplug_context.thread_running = 1; hidapi_thread_create(&hid_hotplug_context.libusb_thread, hotplug_thread, NULL); - hidapi_thread_create(&hid_hotplug_context.callback_thread, callback_thread, NULL); } + /* Mark the mutex as IN USE, to prevent callback removal from inside a callback */ + unsigned char old_state = hid_hotplug_context.mutex_in_use; + hid_hotplug_context.mutex_in_use = 1; + if ((flags & HID_API_HOTPLUG_ENUMERATE) && (events & HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED)) { struct hid_device_info* device = hid_hotplug_context.devs; /* Notify about already connected devices, if asked so */ @@ -1322,42 +1378,52 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven } } - pthread_mutex_unlock(&hid_hotplug_context.cb_mutex); + hid_hotplug_context.mutex_in_use = old_state; + + hid_internal_hotplug_cleanup(); + + pthread_mutex_unlock(&hid_hotplug_context.mutex); return 0; } int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_callback_handle callback_handle) { - struct hid_hotplug_callback *hotplug_cb = NULL; - if (!libusb_has_capability(LIBUSB_CAP_HAS_HOTPLUG) || !hid_hotplug_context.mutex_ready || callback_handle <= 0) { return -1; } - pthread_mutex_lock(&hid_hotplug_context.cb_mutex); + pthread_mutex_lock(&hid_hotplug_context.mutex); if (hid_hotplug_context.hotplug_cbs == NULL) { - pthread_mutex_unlock(&hid_hotplug_context.cb_mutex); + pthread_mutex_unlock(&hid_hotplug_context.mutex); return -1; } + int result = -1; + /* Remove this notification */ for (struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; *current != NULL; current = &(*current)->next) { if ((*current)->handle == callback_handle) { - struct hid_hotplug_callback *next = (*current)->next; - hotplug_cb = *current; - *current = next; - free(hotplug_cb); + /* Check if we were already in a locked state, as we are NOT allowed to remove any callbacks if we are */ + if (hid_hotplug_context.mutex_in_use) { + (*current)->events = 0; + hid_hotplug_context.cb_list_dirty = 1; + } else { + struct hid_hotplug_callback *next = (*current)->next; + free(*current); + *current = next; + } + result = 0; break; } } hid_internal_hotplug_cleanup(); - pthread_mutex_unlock(&hid_hotplug_context.cb_mutex); + pthread_mutex_unlock(&hid_hotplug_context.mutex); - return 0; + return result; } hid_device * hid_open(unsigned short vendor_id, unsigned short product_id, const wchar_t *serial_number) diff --git a/linux/hid.c b/linux/hid.c index 06b32a60b..e77bf28ba 100644 --- a/linux/hid.c +++ b/linux/hid.c @@ -899,8 +899,11 @@ static struct hid_hotplug_context { pthread_t thread; pthread_mutex_t mutex; - - int mutex_ready; + + /* Boolean flags */ + unsigned char mutex_ready; + unsigned char mutex_in_use; + unsigned char cb_list_dirty; /* HIDAPI unique callback handle counter */ hid_hotplug_callback_handle next_handle; @@ -931,27 +934,61 @@ struct hid_hotplug_callback { struct hid_hotplug_callback *next; }; +static void hid_internal_hotplug_remove_postponed() +{ + /* Unregister the callbacks whose removal was postponed */ + /* This function is always called inside a locked mutex */ + /* However, any actions are only allowed if the mutex is NOT in use and if the DIRTY flag is set */ + if (!hid_hotplug_context.mutex_ready || hid_hotplug_context.mutex_in_use || !hid_hotplug_context.cb_list_dirty) { + return; + } + + /* Traverse the list of callbacks and check if any were marked for removal */ + struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; + while (*current) { + struct hid_hotplug_callback *callback = *current; + if (!callback->events) { + *current = (*current)->next; + 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() { + if (!hid_hotplug_context.mutex_ready || hid_hotplug_context.mutex_in_use) { + return; + } + + /* Before checking if the list is empty, clear any entries whose removal was postponed first */ + hid_internal_hotplug_remove_postponed(); + if (hid_hotplug_context.hotplug_cbs != NULL) { return; } pthread_join(hid_hotplug_context.thread, NULL); - - /* Cleanup connected device list */ - hid_free_enumeration(hid_hotplug_context.devs); - hid_hotplug_context.devs = NULL; - /* Disarm the udev monitor */ - udev_monitor_unref(hid_hotplug_context.mon); - udev_unref(hid_hotplug_context.udev_ctx); } static void hid_internal_hotplug_init() { if (!hid_hotplug_context.mutex_ready) { - pthread_mutex_init(&hid_hotplug_context.mutex, NULL); + /* Initialize the mutex as recursive */ + pthread_mutexattr_t attr; + pthread_mutexattr_init(&attr); + pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); + pthread_mutex_init(&hid_hotplug_context.mutex, &attr); + pthread_mutexattr_destroy(&attr); + + /* Set state to Ready */ hid_hotplug_context.mutex_ready = 1; + hid_hotplug_context.mutex_in_use = 0; + hid_hotplug_context.cb_list_dirty = 0; } } @@ -1109,23 +1146,28 @@ void HID_API_EXPORT hid_free_enumeration(struct hid_device_info *devs) static void hid_internal_invoke_callbacks(struct hid_device_info *info, hid_hotplug_event event) { + pthread_mutex_lock(&hid_hotplug_context.mutex); + hid_hotplug_context.mutex_in_use = 1; + struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; while (*current) { struct hid_hotplug_callback *callback = *current; if ((callback->events & event) && hid_internal_match_device_id(info->vendor_id, info->product_id, callback->vendor_id, callback->product_id)) { int result = callback->callback(callback->handle, info, event, callback->user_data); - /* If the result is non-zero, we remove the callback and proceed */ - /* Do not use the deregister call as it locks the mutex, and we are currently in a lock */ + /* If the result is non-zero, we mark the callback for removal and proceed */ if (result) { - struct hid_hotplug_callback *callback = *current; - *current = (*current)->next; - free(callback); + (*current)->events = 0; + hid_hotplug_context.cb_list_dirty = 1; continue; } } current = &callback->next; } + + hid_hotplug_context.mutex_in_use = 0; + hid_internal_hotplug_remove_postponed(); + pthread_mutex_unlock(&hid_hotplug_context.mutex); } static int match_udev_to_info(struct udev_device* raw_dev, struct hid_device_info *info) @@ -1141,11 +1183,19 @@ static void* hotplug_thread(void* user_data) { (void) user_data; + /* Note: the cleanup sequence is always executed with the mutex locked, so we shoud never lock the mutex without checking if we need to stop */ + while (hid_hotplug_context.monitor_fd > 0) { fd_set fds; struct timeval tv; int ret; + /* On every iteration, check if we still have any callbacks left and leave if none are left */ + /* NOTE: the check is performed UNLOCKED and the value CAN change in the background */ + if (!hid_hotplug_context.hotplug_cbs) { + break; + } + FD_ZERO(&fds); FD_SET(hid_hotplug_context.monitor_fd, &fds); /* 5 msec timeout seems reasonable; don't set too low to avoid high CPU usage */ @@ -1155,6 +1205,11 @@ static void* hotplug_thread(void* user_data) ret = select(hid_hotplug_context.monitor_fd+1, &fds, NULL, NULL, &tv); + /* An extra check, just in case within those 5msec the thread was told to stop */ + if (!hid_hotplug_context.hotplug_cbs) { + break; + } + /* Check if our file descriptor has received data. */ if (ret > 0 && FD_ISSET(hid_hotplug_context.monitor_fd, &fds)) { @@ -1162,9 +1217,7 @@ static void* hotplug_thread(void* user_data) select() ensured that this will not block. */ struct udev_device *raw_dev = udev_monitor_receive_device(hid_hotplug_context.mon); if (raw_dev) { - /* Lock the mutex so callback/device lists don't change elsewhere from here on */ pthread_mutex_lock(&hid_hotplug_context.mutex); - const char* action = udev_device_get_action(raw_dev); if (!strcmp(action, "add")) { // We create a list of all usages on this UDEV device @@ -1209,6 +1262,14 @@ static void* hotplug_thread(void* user_data) } } } + + /* Cleanup connected device list */ + hid_free_enumeration(hid_hotplug_context.devs); + hid_hotplug_context.devs = NULL; + /* Disarm the udev monitor */ + udev_monitor_unref(hid_hotplug_context.mon); + udev_unref(hid_hotplug_context.udev_ctx); + return NULL; } @@ -1268,7 +1329,7 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven else { // Prepare a UDEV context to run monitoring on hid_hotplug_context.udev_ctx = udev_new(); - if(!hid_hotplug_context.udev_ctx) + if (!hid_hotplug_context.udev_ctx) { pthread_mutex_unlock(&hid_hotplug_context.mutex); return -1; @@ -1289,6 +1350,26 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven pthread_create(&hid_hotplug_context.thread, NULL, &hotplug_thread, NULL); } + /* Mark the mutex as IN USE, to prevent callback removal from inside a callback */ + unsigned char old_state = hid_hotplug_context.mutex_in_use; + hid_hotplug_context.mutex_in_use = 1; + + if ((flags & HID_API_HOTPLUG_ENUMERATE) && (events & HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED)) { + struct hid_device_info* device = hid_hotplug_context.devs; + /* Notify about already connected devices, if asked so */ + while (device != NULL) { + if (hid_internal_match_device_id(device->vendor_id, device->product_id, hotplug_cb->vendor_id, hotplug_cb->product_id)) { + (*hotplug_cb->callback)(hotplug_cb->handle, device, HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED, hotplug_cb->user_data); + } + + device = device->next; + } + } + + hid_hotplug_context.mutex_in_use = old_state; + + hid_internal_hotplug_cleanup(); + pthread_mutex_unlock(&hid_hotplug_context.mutex); return 0; @@ -1296,7 +1377,7 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_callback_handle callback_handle) { - if (!hid_hotplug_context.mutex_ready) { + if (!hid_hotplug_context.mutex_ready || callback_handle <= 0) { return -1; } @@ -1312,9 +1393,15 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_call /* Remove this notification */ for (struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; *current != NULL; current = &(*current)->next) { if ((*current)->handle == callback_handle) { - struct hid_hotplug_callback *next = (*current)->next; - free(*current); - *current = next; + /* Check if we were already in a locked state, as we are NOT allowed to remove any callbacks if we are */ + if (hid_hotplug_context.mutex_in_use) { + (*current)->events = 0; + hid_hotplug_context.cb_list_dirty = 1; + } else { + struct hid_hotplug_callback *next = (*current)->next; + free(*current); + *current = next; + } result = 0; break; } diff --git a/mac/hid.c b/mac/hid.c index 75038b471..910747632 100644 --- a/mac/hid.c +++ b/mac/hid.c @@ -497,7 +497,10 @@ static struct hid_hotplug_context { pthread_mutex_t mutex; - int mutex_ready; + /* Boolean flags */ + unsigned char mutex_ready; + unsigned char mutex_in_use; + unsigned char cb_list_dirty; /* Linked list of the hotplug callbacks */ struct hid_hotplug_callback *hotplug_cbs; @@ -516,8 +519,40 @@ static struct hid_hotplug_context { .devs = NULL }; -static void hid_internal_hotplug_cleanup() +static void hid_internal_hotplug_remove_postponed(void) { + /* Unregister the callbacks whose removal was postponed */ + /* This function is always called inside a locked mutex */ + /* However, any actions are only allowed if the mutex is NOT in use and if the DIRTY flag is set */ + if (!hid_hotplug_context.mutex_ready || hid_hotplug_context.mutex_in_use || !hid_hotplug_context.cb_list_dirty) { + return; + } + + /* Traverse the list of callbacks and check if any were marked for removal */ + struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; + while (*current) { + struct hid_hotplug_callback *callback = *current; + if (!callback->events) { + *current = (*current)->next; + 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(void) +{ + if (!hid_hotplug_context.mutex_ready || hid_hotplug_context.mutex_in_use) { + return; + } + + /* Before checking if the list is empty, clear any entries whose removal was postponed first */ + hid_internal_hotplug_remove_postponed(); + if (hid_hotplug_context.hotplug_cbs != NULL) { return; } @@ -537,15 +572,24 @@ static void hid_internal_hotplug_cleanup() pthread_join(hid_hotplug_context.thread, NULL); } -static void hid_internal_hotplug_init() +static void hid_internal_hotplug_init(void) { if (!hid_hotplug_context.mutex_ready) { - pthread_mutex_init(&hid_hotplug_context.mutex, NULL); + /* Initialize the mutex as recursive */ + pthread_mutexattr_t attr; + pthread_mutexattr_init(&attr); + pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); + pthread_mutex_init(&hid_hotplug_context.mutex, &attr); + pthread_mutexattr_destroy(&attr); + + /* Set state to Ready */ hid_hotplug_context.mutex_ready = 1; + hid_hotplug_context.mutex_in_use = 0; + hid_hotplug_context.cb_list_dirty = 0; } } -static void hid_internal_hotplug_exit() +static void hid_internal_hotplug_exit(void) { if (!hid_hotplug_context.mutex_ready) { return; @@ -604,11 +648,12 @@ static int hid_internal_match_device_id(unsigned short vendor_id, unsigned short return (expected_vendor_id == 0x0 || vendor_id == expected_vendor_id) && (expected_product_id == 0x0 || product_id == expected_product_id); } -static void process_pending_events(void) { +static void process_pending_events(void) +{ SInt32 res; do { res = CFRunLoopRunInMode(kCFRunLoopDefaultMode, 0.001, FALSE); - } while(res != kCFRunLoopRunFinished && res != kCFRunLoopRunTimedOut); + } while (res != kCFRunLoopRunFinished && res != kCFRunLoopRunTimedOut); } static int read_usb_interface_from_hid_service_parent(io_service_t hid_service) @@ -925,23 +970,29 @@ void HID_API_EXPORT hid_free_enumeration(struct hid_device_info *devs) static void hid_internal_invoke_callbacks(struct hid_device_info *info, hid_hotplug_event event) { + pthread_mutex_lock(&hid_hotplug_context.mutex); + hid_hotplug_context.mutex_in_use = 1; + struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; while (*current) { struct hid_hotplug_callback *callback = *current; if ((callback->events & event) && hid_internal_match_device_id(info->vendor_id, info->product_id, callback->vendor_id, callback->product_id)) { int result = callback->callback(callback->handle, info, event, callback->user_data); - /* If the result is non-zero, we remove the callback and proceed */ + /* If the result is non-zero, we mark the callback for removal and proceed */ /* Do not use the deregister call as it locks the mutex, and we are currently in a lock */ if (result) { - struct hid_hotplug_callback *callback = *current; - *current = (*current)->next; - free(callback); + (*current)->events = 0; + hid_hotplug_context.cb_list_dirty = 1; continue; } } current = &callback->next; } + + hid_hotplug_context.mutex_in_use = 0; + hid_internal_hotplug_remove_postponed(); + pthread_mutex_unlock(&hid_hotplug_context.mutex); } static void hid_internal_hotplug_connect_callback(void *context, IOReturn result, void *sender, IOHIDDeviceRef device) @@ -951,19 +1002,19 @@ static void hid_internal_hotplug_connect_callback(void *context, IOReturn result (void) sender; struct hid_device_info* info = create_device_info(device); - if(!info) { + if (!info) { return; } struct hid_device_info* info_cur = info; - /* NOTE: we don't call any callbacks and we don't lock the mutex during initialization: the mutex is held by the main thread, but it's waiting by a barrier*/ + /* NOTE: we don't call any callbacks and we don't lock the mutex during initialization: the mutex is held by the main thread, but it's waiting by a barrier */ if (hid_hotplug_context.thread_state > 0) { /* Lock the mutex to avoid race conditions */ pthread_mutex_lock(&hid_hotplug_context.mutex); /* Invoke all callbacks */ - while(info_cur) + while (info_cur) { hid_internal_invoke_callbacks(info_cur, HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED); info_cur = info_cur->next; @@ -1036,10 +1087,10 @@ static void hid_internal_hotplug_disconnect_callback(void *context, IOReturn res } } -static void hotplug_stop_callback(void *context) +static void hotplug_stop_callback(void* context) { - hid_device *dev = (hid_device*) context; - CFRunLoopStop(dev->run_loop); /*TODO: CFRunLoopGetCurrent()*/ + (void) context; + CFRunLoopStop(hid_hotplug_context.run_loop); } static void* hotplug_thread(void* user_data) @@ -1049,7 +1100,7 @@ static void* hotplug_thread(void* user_data) hid_hotplug_context.thread_state = 0; hid_hotplug_context.manager = IOHIDManagerCreate(kCFAllocatorDefault, kIOHIDOptionsTypeNone); - if(!hid_hotplug_context.run_loop_mode) { + if (!hid_hotplug_context.run_loop_mode) { const char *str = "HIDAPI_hotplug"; hid_hotplug_context.run_loop_mode = CFStringCreateWithCString(NULL, str, kCFStringEncodingASCII); } @@ -1060,7 +1111,7 @@ static void* hotplug_thread(void* user_data) hid_hotplug_context.run_loop = CFRunLoopGetCurrent(); /* Create the RunLoopSource which is used to signal the - event loop to stop when hid_close() is called. */ + event loop to stop when hid_internal_hotplug_cleanup() is called. */ CFRunLoopSourceContext ctx; memset(&ctx, 0, sizeof(ctx)); ctx.version = 0; @@ -1178,6 +1229,10 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven hid_hotplug_context.hotplug_cbs = hotplug_cb; } + /* Mark the mutex as IN USE, to prevent callback removal from inside a callback */ + unsigned char old_state = hid_hotplug_context.mutex_in_use; + hid_hotplug_context.mutex_in_use = 1; + if ((flags & HID_API_HOTPLUG_ENUMERATE) && (events & HID_API_HOTPLUG_EVENT_DEVICE_ARRIVED)) { struct hid_device_info* device = hid_hotplug_context.devs; /* Notify about already connected devices, if asked so */ @@ -1189,7 +1244,11 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven device = device->next; } } - + + hid_hotplug_context.mutex_in_use = old_state; + + hid_internal_hotplug_cleanup(); + pthread_mutex_unlock(&hid_hotplug_context.mutex); return 0; @@ -1213,9 +1272,15 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_call /* Remove this notification */ for (struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; *current != NULL; current = &(*current)->next) { if ((*current)->handle == callback_handle) { - struct hid_hotplug_callback *next = (*current)->next; - free(*current); - *current = next; + /* Check if we were already in a locked state, as we are NOT allowed to remove any callbacks if we are */ + if (hid_hotplug_context.mutex_in_use) { + (*current)->events = 0; + hid_hotplug_context.cb_list_dirty = 1; + } else { + struct hid_hotplug_callback *next = (*current)->next; + free(*current); + *current = next; + } result = 0; break; } diff --git a/windows/hid.c b/windows/hid.c index f849c3038..d45c103cb 100644 --- a/windows/hid.c +++ b/windows/hid.c @@ -210,7 +210,10 @@ static struct hid_hotplug_context { /* Critical section (faster mutex substitute), for both cached device list and callback list changes */ CRITICAL_SECTION critical_section; - BOOL critical_section_ready; + /* Boolean flags */ + unsigned char mutex_ready; + unsigned char mutex_in_use; + unsigned char cb_list_dirty; /* HIDAPI unique callback handle counter */ hid_hotplug_callback_handle next_handle; @@ -222,7 +225,7 @@ static struct hid_hotplug_context { struct hid_device_info *devs; } hid_hotplug_context = { .notify_handle = NULL, - .critical_section_ready = 0, + .mutex_ready = 0, .next_handle = FIRST_HOTPLUG_CALLBACK_HANDLE, .hotplug_cbs = NULL, .devs = NULL @@ -318,7 +321,7 @@ static void register_winapi_error_to_buffer(wchar_t **error_buffer, const WCHAR /* Get rid of the CR and LF that FormatMessage() sticks at the end of the message. Thanks Microsoft! */ - while(msg[msg_len-1] == L'\r' || msg[msg_len-1] == L'\n' || msg[msg_len-1] == L' ') + while (msg[msg_len-1] == L'\r' || msg[msg_len-1] == L'\n' || msg[msg_len-1] == L' ') { msg[msg_len-1] = L'\0'; msg_len--; @@ -399,9 +402,13 @@ HID_API_EXPORT const char* HID_API_CALL hid_version_str(void) static void hid_internal_hotplug_init() { - if (!hid_hotplug_context.critical_section_ready) { + if (!hid_hotplug_context.mutex_ready) { InitializeCriticalSection(&hid_hotplug_context.critical_section); - hid_hotplug_context.critical_section_ready = 1; + + /* Set state to Ready */ + hid_hotplug_context.mutex_ready = 1; + hid_hotplug_context.mutex_in_use = 0; + hid_hotplug_context.cb_list_dirty = 0; } } @@ -422,8 +429,52 @@ int HID_API_EXPORT hid_init(void) return 0; } +struct hid_hotplug_callback { + hid_hotplug_callback_handle handle; + unsigned short vendor_id; + unsigned short product_id; + hid_hotplug_event events; + void *user_data; + hid_hotplug_callback_fn callback; + + /* Pointer to the next notification */ + struct hid_hotplug_callback *next; +}; + +static void hid_internal_hotplug_remove_postponed() +{ + /* Unregister the callbacks whose removal was postponed */ + /* This function is always called inside a locked mutex */ + /* However, any actions are only allowed if the mutex is NOT in use and if the DIRTY flag is set */ + if (!hid_hotplug_context.mutex_ready || hid_hotplug_context.mutex_in_use || !hid_hotplug_context.cb_list_dirty) { + return; + } + + /* Traverse the list of callbacks and check if any were marked for removal */ + struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; + while (*current) { + struct hid_hotplug_callback *callback = *current; + if (!callback->events) { + *current = (*current)->next; + 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() { + if (!hid_hotplug_context.mutex_ready || hid_hotplug_context.mutex_in_use) { + return; + } + + /* 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) { @@ -446,21 +497,9 @@ static void hid_internal_hotplug_cleanup() hid_hotplug_context.notify_handle = NULL; } -struct hid_hotplug_callback { - hid_hotplug_callback_handle handle; - unsigned short vendor_id; - unsigned short product_id; - hid_hotplug_event events; - void *user_data; - hid_hotplug_callback_fn callback; - - /* Pointer to the next notification */ - struct hid_hotplug_callback *next; -}; - static void hid_internal_hotplug_exit() { - if (!hid_hotplug_context.critical_section_ready) { + if (!hid_hotplug_context.mutex_ready) { /* If the critical section is not initialized, we are safe to assume nothing else is */ return; } @@ -474,20 +513,20 @@ static void hid_internal_hotplug_exit() } hid_internal_hotplug_cleanup(); LeaveCriticalSection(&hid_hotplug_context.critical_section); - hid_hotplug_context.critical_section_ready = 0; + hid_hotplug_context.mutex_ready = 0; DeleteCriticalSection(&hid_hotplug_context.critical_section); } int HID_API_EXPORT hid_exit(void) { + hid_internal_hotplug_exit(); + #ifndef HIDAPI_USE_DDK free_library_handles(); hidapi_initialized = FALSE; #endif register_global_error(NULL); - hid_internal_hotplug_exit(); - return 0; } @@ -1094,28 +1133,34 @@ 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; + /* Call the notifications for the device */ struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; while (*current) { struct hid_hotplug_callback *callback = *current; 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 remove the callback and proceed */ - /* Do not use the deregister call as it locks the mutex, and we are currently in a lock */ + + /* 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) { - *current = (*current)->next; - free(callback); - continue; + callback->events = 0; + hid_hotplug_context.cb_list_dirty = 1; } } current = &callback->next; } + hid_hotplug_context.mutex_in_use = 0; + /* Free removed device */ if (hotplug_event == HID_API_HOTPLUG_EVENT_DEVICE_LEFT) { free(device); } + /* Remove any callbacks that were marked for removal and stop the notification if none are left */ hid_internal_hotplug_cleanup(); } @@ -1208,6 +1253,10 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven } } + /* 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; + 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 */ @@ -1220,6 +1269,11 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven } } + hid_hotplug_context.mutex_in_use = old_state; + + /* 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; @@ -1227,7 +1281,7 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_register_callback(unsigned short ven int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_callback_handle callback_handle) { - if (callback_handle <= 0 || !hid_hotplug_context.critical_section_ready) { + if (callback_handle <= 0 || !hid_hotplug_context.mutex_ready) { return -1; } @@ -1242,9 +1296,16 @@ int HID_API_EXPORT HID_API_CALL hid_hotplug_deregister_callback(hid_hotplug_call /* Remove this notification */ for (struct hid_hotplug_callback **current = &hid_hotplug_context.hotplug_cbs; *current != NULL; current = &(*current)->next) { if ((*current)->handle == callback_handle) { - struct hid_hotplug_callback *next = (*current)->next; - free(*current); - *current = next; + /* 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 */ + (*current)->events = 0; + hid_hotplug_context.cb_list_dirty = 1; + } else { + struct hid_hotplug_callback *next = (*current)->next; + free(*current); + *current = next; + } break; } }