[Spice-devel] [spice-gtk v3 3/4] usb-channel: Really stop listening for USB events on disconnection

[Christophe Fergeau]

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