[PATCH 0/1] MR1503: wineusb.sys: Move event handling to a single thread.
Also fixes a segmentation fault when exiting winedevice.exe. I can file a separate bug report, but it seems related to Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52213 On my system the core dump can be provoked when using the bus_udev backend in winebus.sys ("Enable SDL" 0). It creates two hid devices - a mouse and a keyboard. Since it resides in the same winedevice.exe process as wineusb.sys, something similar to the above bug occurs when handling the SIGQUIT signal. The actual crash happens in the libusb event thread while waiting in pthread_cond_wait(). I'm using this for months without any issues (with a device driver which uses wineusb.sys for device access, etc.). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1503
From: Ivo Ivanov <logos128(a)gmail.com> Fixes a segmentation fault on exit of winedevice.exe. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52213 --- dlls/wineusb.sys/unixlib.c | 57 +++++++++++++++++++------------------- dlls/wineusb.sys/unixlib.h | 5 ++-- dlls/wineusb.sys/wineusb.c | 37 +++++++++---------------- 3 files changed, 43 insertions(+), 56 deletions(-) diff --git a/dlls/wineusb.sys/unixlib.c b/dlls/wineusb.sys/unixlib.c index f269d3e96fd..278750c37f6 100644 --- a/dlls/wineusb.sys/unixlib.c +++ b/dlls/wineusb.sys/unixlib.c @@ -51,10 +51,7 @@ struct unix_device static libusb_hotplug_callback_handle hotplug_cb_handle; -static bool thread_shutdown; - -static pthread_cond_t event_cond = PTHREAD_COND_INITIALIZER; -static pthread_mutex_t event_mutex = PTHREAD_MUTEX_INITIALIZER; +static volatile bool thread_shutdown; static struct usb_event *usb_events; static size_t usb_event_count, usb_events_capacity; @@ -92,28 +89,21 @@ static bool array_reserve(void **elements, size_t *capacity, size_t count, size_ static void queue_event(const struct usb_event *event) { - pthread_mutex_lock(&event_mutex); if (array_reserve((void **)&usb_events, &usb_events_capacity, usb_event_count + 1, sizeof(*usb_events))) usb_events[usb_event_count++] = *event; else ERR("Failed to queue event.\n"); - pthread_mutex_unlock(&event_mutex); - pthread_cond_signal(&event_cond); } -static NTSTATUS usb_get_event(void *args) +static bool get_event(struct usb_event *event) { - const struct usb_get_event_params *params = args; + if (!usb_event_count) return false; - pthread_mutex_lock(&event_mutex); - while (!usb_event_count) - pthread_cond_wait(&event_cond, &event_mutex); - *params->event = usb_events[0]; + *event = usb_events[0]; if (--usb_event_count) memmove(usb_events, usb_events + 1, usb_event_count * sizeof(*usb_events)); - pthread_mutex_unlock(&event_mutex); - return STATUS_SUCCESS; + return true; } static void add_usb_device(libusb_device *libusb_device) @@ -239,10 +229,30 @@ static int LIBUSB_CALL hotplug_cb(libusb_context *context, libusb_device *device static NTSTATUS usb_main_loop(void *args) { - static const struct usb_event shutdown_event = {.type = USB_EVENT_SHUTDOWN}; + const struct usb_main_loop_params *params = args; int ret; - TRACE("Starting libusb event thread.\n"); + while (!thread_shutdown) + { + if (get_event(params->event)) return STATUS_PENDING; + + if ((ret = libusb_handle_events(NULL))) + ERR("Error handling events: %s\n", libusb_strerror(ret)); + } + + libusb_exit(NULL); + free(usb_events); + usb_events = NULL; + usb_event_count = usb_events_capacity = 0; + thread_shutdown = false; + + TRACE("USB main loop exiting.\n"); + return STATUS_SUCCESS; +} + +static NTSTATUS usb_init(void *args) +{ + int ret; if ((ret = libusb_init(NULL))) { @@ -260,17 +270,6 @@ static NTSTATUS usb_main_loop(void *args) return STATUS_UNSUCCESSFUL; } - while (!thread_shutdown) - { - if ((ret = libusb_handle_events(NULL))) - ERR("Error handling events: %s\n", libusb_strerror(ret)); - } - - libusb_exit(NULL); - - queue_event(&shutdown_event); - - TRACE("Shutting down libusb event thread.\n"); return STATUS_SUCCESS; } @@ -566,8 +565,8 @@ const unixlib_entry_t __wine_unix_call_funcs[] = { #define X(name) [unix_ ## name] = name X(usb_main_loop), + X(usb_init), X(usb_exit), - X(usb_get_event), X(usb_submit_urb), X(usb_cancel_transfer), X(usb_destroy_device), diff --git a/dlls/wineusb.sys/unixlib.h b/dlls/wineusb.sys/unixlib.h index 0c3319969b7..eee16c8fc2b 100644 --- a/dlls/wineusb.sys/unixlib.h +++ b/dlls/wineusb.sys/unixlib.h @@ -31,7 +31,6 @@ enum usb_event_type USB_EVENT_ADD_DEVICE, USB_EVENT_REMOVE_DEVICE, USB_EVENT_TRANSFER_COMPLETE, - USB_EVENT_SHUTDOWN, }; struct usb_event @@ -53,7 +52,7 @@ struct usb_event } u; }; -struct usb_get_event_params +struct usb_main_loop_params { struct usb_event *event; }; @@ -77,8 +76,8 @@ struct usb_destroy_device_params enum unix_funcs { unix_usb_main_loop, + unix_usb_init, unix_usb_exit, - unix_usb_get_event, unix_usb_submit_urb, unix_usb_cancel_transfer, unix_usb_destroy_device, diff --git a/dlls/wineusb.sys/wineusb.c b/dlls/wineusb.sys/wineusb.c index 63b723503c9..a5eaa450609 100644 --- a/dlls/wineusb.sys/wineusb.c +++ b/dlls/wineusb.sys/wineusb.c @@ -167,14 +167,7 @@ static void remove_unix_device(struct unix_device *unix_device) IoInvalidateDeviceRelations(bus_pdo, BusRelations); } -static HANDLE libusb_event_thread, event_thread; - -static DWORD CALLBACK libusb_event_thread_proc(void *arg) -{ - __wine_unix_call(unix_handle, unix_usb_main_loop, NULL); - - return 0; -} +static HANDLE event_thread; static void complete_irp(IRP *irp) { @@ -189,18 +182,18 @@ static void complete_irp(IRP *irp) static DWORD CALLBACK event_thread_proc(void *arg) { struct usb_event event; - - TRACE("Starting client event thread.\n"); - - for (;;) + struct usb_main_loop_params params = { - struct usb_get_event_params params = - { - .event = &event, - }; + .event = &event, + }; - __wine_unix_call(unix_handle, unix_usb_get_event, ¶ms); + TRACE("Starting event thread.\n"); + if (__wine_unix_call(unix_handle, unix_usb_init, NULL) != STATUS_SUCCESS) + return 0; + + while (__wine_unix_call(unix_handle, unix_usb_main_loop, ¶ms) == STATUS_PENDING) + { switch (event.type) { case USB_EVENT_ADD_DEVICE: @@ -214,12 +207,11 @@ static DWORD CALLBACK event_thread_proc(void *arg) case USB_EVENT_TRANSFER_COMPLETE: complete_irp(event.u.completed_irp); break; - - case USB_EVENT_SHUTDOWN: - TRACE("Shutting down client event thread.\n"); - return 0; } } + + TRACE("Shutting down event thread.\n"); + return 0; } static NTSTATUS fdo_pnp(IRP *irp) @@ -268,7 +260,6 @@ static NTSTATUS fdo_pnp(IRP *irp) } case IRP_MN_START_DEVICE: - libusb_event_thread = CreateThread(NULL, 0, libusb_event_thread_proc, NULL, 0, NULL); event_thread = CreateThread(NULL, 0, event_thread_proc, NULL, 0, NULL); irp->IoStatus.Status = STATUS_SUCCESS; @@ -283,8 +274,6 @@ static NTSTATUS fdo_pnp(IRP *irp) struct usb_device *device, *cursor; __wine_unix_call(unix_handle, unix_usb_exit, NULL); - WaitForSingleObject(libusb_event_thread, INFINITE); - CloseHandle(libusb_event_thread); WaitForSingleObject(event_thread, INFINITE); CloseHandle(event_thread); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1503
Is it guaranteed that libusb_handle_events() will always return immediately after running an event callback? I'm also disturbed by the fact that this fixes anything. Partly because I don't see what's special about that thread, and partly because I don't know why we're getting SIGQUIT? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1503#note_17091
Is it guaranteed that libusb_handle_events() will always return immediately after running an event callback?
Yes. According to the libusb_handle_events_timeout_completed() documentation, which is the main function used internally by all other libusb_handle_events variant functions - _"If a non-zero timeval is passed and no events are currently pending, this function will block waiting for events to handle up until the specified timeout. If an event arrives or a signal is raised, this function will return early."_ libusb_handle_events() basically calls libusb_handle_events_timeout_completed() with a 60s timeval, so it'll return either when an event occurs (hotplug, transfer, err, exit, etc.) or the timeout expires. I observed all the libusb code related to libusb_handle_events(), and found it does exactly that.
I'm also disturbed by the fact that this fixes anything. Partly because I don't see what's special about that thread, and partly because I don't know why we're getting SIGQUIT?
This core dump occurs when the last application quits, so a SIGQUIT is sent to all remaining services to quit. This particular winedevice.exe (which shares both winebus.sys and wineusb.sys) receives the signal, and when each thread's SIGQUIT handler gets called, the usb event handling thread segfaults somewhere in pthread_cond_wait() while being canceled by pthread_exit(). Normally this should work, but something happens when the library is implemented in Wine's Windows/Unix style. According to the bug report, when the execution flow is in the Unix library, it resets the stack to the exit_frame and calls the pthread_exit(), which fails to unwind properly. It's well described in this and the following post of the report: https://bugs.winehq.org/show_bug.cgi?id=52213#c11 So, to workaround this I just avoided using pthread_cond_wait() and moved to single-threaded event handling, since the libusb_handle_events() suits this approach quite well. Followed similar logic to the winebus.sys event thread implementation. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1503#note_17119
participants (3)
-
Ivo Ivanov -
Ivo Ivanov (＠logos128)
-
Zebediah Figura (＠zfigura)