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(). The session is only needed in order to get the SpiceUsbDeviceManager instance we need to call spice_usb_device_manager_stop_event_listening() on. This patch stores it in SpiceChannelUsbredir instead, this way we don't need SpiceChannel::session to be non-NULL during device disconnection. This should fix 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. --- src/channel-usbredir.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c index df18be2..6595508 100644 --- a/src/channel-usbredir.c +++ b/src/channel-usbredir.c @@ -79,6 +79,7 @@ struct _SpiceUsbredirChannelPrivate { SpiceUsbAclHelper *acl_helper; #endif GMutex device_connect_mutex; + SpiceUsbDeviceManager *usb_device_manager; }; static void channel_set_handlers(SpiceChannelClass *klass); @@ -188,6 +189,11 @@ static void spice_usbredir_channel_dispose(GObject *obj) SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(obj); spice_usbredir_channel_disconnect_device(channel); + /* This should have been set to NULL during device disconnection, + * but better not to leak it if this does not happen for some reason + */ + g_warn_if_fail(channel->priv->usb_device_manager == NULL); + g_clear_object(&channel->priv->usb_device_manager); /* Chain up to the parent class */ if (G_OBJECT_CLASS(spice_usbredir_channel_parent_class)->dispose) @@ -275,6 +281,7 @@ static gboolean spice_usbredir_channel_open_device( SpiceUsbredirChannel *channel, GError **err) { SpiceUsbredirChannelPrivate *priv = channel->priv; + SpiceSession *session; libusb_device_handle *handle = NULL; int rc, status; @@ -300,10 +307,9 @@ static gboolean spice_usbredir_channel_open_device( return FALSE; } - if (!spice_usb_device_manager_start_event_listening( - spice_usb_device_manager_get( - spice_channel_get_session(SPICE_CHANNEL(channel)), NULL), - err)) { + session = spice_channel_get_session(SPICE_CHANNEL(channel)); + priv->usb_device_manager = g_object_ref(spice_usb_device_manager_get(session, NULL)); + if (!spice_usb_device_manager_start_event_listening(priv->usb_device_manager, err)) { usbredirhost_set_device(priv->host, NULL); return FALSE; } @@ -481,12 +487,10 @@ void spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel) * usbredirhost_set_device NULL will interrupt the * libusb_handle_events call in the thread. */ - { - SpiceSession *session = spice_channel_get_session(SPICE_CHANNEL(channel)); - if (session != NULL) - spice_usb_device_manager_stop_event_listening( - spice_usb_device_manager_get(session, NULL)); - } + g_warn_if_fail(priv->usb_device_manager != NULL); + spice_usb_device_manager_stop_event_listening(priv->usb_device_manager); + g_clear_object(&priv->usb_device_manager); + /* This also closes the libusb handle we passed from open_device */ usbredirhost_set_device(priv->host, NULL); g_clear_pointer(&priv->device, libusb_unref_device); -- 2.7.4 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel