[PATCH v4 0/1] MR1503: wineusb.sys: Move event handling to a single thread.

[Ivo Ivanov (＠logos128)]

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.).

--
  v4: wineusb.sys: Move event handling to a single thread.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/1503

[PATCH v4 1/1] wineusb.sys: Move event handling to a single thread.

[Ivo Ivanov]

From: Ivo Ivanov <logos128(a)gmail.com>

Also 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 e9e91326a84..7848b31105e 100644
--- a/dlls/wineusb.sys/wineusb.c
+++ b/dlls/wineusb.sys/wineusb.c
@@ -165,14 +165,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_usb_main_loop, NULL);
-
-    return 0;
-}
+static HANDLE event_thread;
 
 static void complete_irp(IRP *irp)
 {
@@ -187,18 +180,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_usb_get_event, ¶ms);
+    TRACE("Starting event thread.\n");
 
+    if (WINE_UNIX_CALL(unix_usb_init, NULL) != STATUS_SUCCESS)
+        return 0;
+
+    while (WINE_UNIX_CALL(unix_usb_main_loop, ¶ms) == STATUS_PENDING)
+    {
         switch (event.type)
         {
             case USB_EVENT_ADD_DEVICE:
@@ -212,12 +205,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)
@@ -266,7 +258,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;
@@ -281,8 +272,6 @@ static NTSTATUS fdo_pnp(IRP *irp)
             struct usb_device *device, *cursor;
 
             WINE_UNIX_CALL(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