[Spice-devel] [spice-gtk 3/3] usb-device-manager: Fix USB event thread leak

[Hans de Goede]

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;
>