From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Christophe Fergeau <cfergeau@redhat.com>
Date: Wed, 29 Jun 2016 16:57:36 +0200
Subject: [PATCH] usb-channel: Really stop listening for USB events on
 disconnection

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 4d669c4..bc32d6b 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);