When using USB redirection, it's fairly easy to leak the thread handling USB events, which will eventually cause problems in long lived apps. In particular, in virt-manager, one can: - start a VM - connect to it with SPICE - open the USB redirection window - redirect a device - close the SPICE window -> the SpiceUsbDeviceManager instance will be destroyed (including the USB context it owns), but the associated event thread will keep running. Since it's running a loop blocking on libusb_handle_events(priv->context), the loop will eventually try to use the USB context we just destroyed causing a crash. We can get in this situation when redirecting a USB device because we will call spice_usb_device_manager_start_event_listening() in spice_usbredir_channel_open_device(). The matching spice_usb_device_manager_stop_event_listening() call is supposed to happen in spice_usbredir_channel_disconnect_device(), however by the time it's called in the scenario described above, the session associated with the channel will already have been set to NULL in spice_session_channel_destroy(). Since the USB event thread has to be stopped when we destroy the associated SpiceUsbDeviceManager, spice_usb_device_manager_dispose() should force event_thread_run to FALSE even if spice_usb_device_manager_stop_event_listening() was not enough. When this happens, this means that there is a bug in the internal users of spice_usb_device_manager_start_event_listening(), but with this change, we'll at least warn about it, and avoid a thread leak/potential future crash. This should avoid the issues described in https://bugzilla.redhat.com/show_bug.cgi?id=1217202 (virt-manager) and most likely https://bugzilla.redhat.com/show_bug.cgi?id=1337007 (gnome-boxes) as well. --- Changes since v1: - make it very explicit that it's workarounding a bug in other parts of the code, both in the log and in the code (through g_warn_if_reached()) src/usb-device-manager.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c index 808ec6c..33818c2 100644 --- a/src/usb-device-manager.c +++ b/src/usb-device-manager.c @@ -375,6 +375,15 @@ static void spice_usb_device_manager_dispose(GObject *gobject) #ifdef USE_LIBUSB_HOTPLUG if (priv->hp_handle) { spice_usb_device_manager_stop_event_listening(self); + if (g_atomic_int_get(&priv->event_thread_run)) { + /* Force termination of the event thread even if there were some + * mismatched spice_usb_device_manager_{start,stop}_event_listening + * calls. Otherwise, the usb event thread will be leaked, and will + * try to use the libusb context we destroy in finalize(), which would + * cause a crash */ + g_warn_if_reached(); + g_atomic_int_set(&priv->event_thread_run, FALSE); + } /* This also wakes up the libusb_handle_events() in the event_thread */ libusb_hotplug_deregister_callback(priv->context, priv->hp_handle); priv->hp_handle = 0; -- 2.7.4 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel