Hi, On 29-06-16 17:42, Christophe Fergeau wrote:
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
Erm no, it is the caller's responsibility to make sure that all spice_usbredir_channel's are properly torn down before the usbdevicemanager gets torn down. Otherwise you're not just having the problem of the thread continuing to run, but also you still have open usbfs file-descriptors and any redirected devices will not become available to the client-os again (think e.g. virt-manager with multiple spice ontexts). The proper fix is to fix the channel already being set to NULL, without first calling spice_usbredir_channel_disconnect_device(). Regards, Hans rather than calling
spice_usb_device_manager_stop_event_listening() which will only set it to FALSE if this was the only user of the event thread. This fixes 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. --- src/usb-device-manager.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c index 1912b62..2b10be2 100644 --- a/src/usb-device-manager.c +++ b/src/usb-device-manager.c @@ -374,7 +374,9 @@ 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); + /* Force termination of the event thread even if there were some mismatched + * spice_usb_device_manager_{start,stop}_event_listening calls */ + 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;
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel