[Spice-devel] [spice-gtk v3 4/4] usb-device-manager: Avoid USB event thread leak

[Hans de Goede]

Hi,

The entire series looks good to me now, one remark wrt this patch,
with that fixed this series is:

Reviewed-by: Hans de Goede <hdegoede at redhat.com>


On 30-06-16 15:40, Christophe Fergeau wrote:
> This is a follow-up of the previous commit ('usb-channel: Really stop
> listening for USB events on disconnection').
>
> 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 even if
> spice_usb_device_manager_stop_event_listening() was not enough. When
> this happens, this means that there is a bug in the internal users of
> spice_usb_device_manager_start_event_listening(), but with this change,
> we'll at least warn about it, and avoid a thread leak/potential future
> crash.
> ---
> Unchanged since v2 apart from the commit log.
>
>
>  src/usb-device-manager.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index 808ec6c..33818c2 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -375,6 +375,15 @@ 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);
> +        if (g_atomic_int_get(&priv->event_thread_run)) {
> +            /* Force termination of the event thread even if there were some
> +             * mismatched spice_usb_device_manager_{start,stop}_event_listening
> +             * calls. Otherwise, the usb event thread will be leaked, and will
> +             * try to use the libusb context we destroy in finalize(), which would
> +             * cause a crash */
> +             g_warn_if_reached();
> +             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;
>

I would move this to outside the #ifdef USE_LIBUSB_HOTPLUG / if (priv->hp_handle) {}

You will also want the warn_if_reached and g_atomic_int_set(..., FALSE) when
these are not true.

You should also remove the !g_atomic_int_get(&priv->event_thread_run) from the
if below the #endif since you force that to false now, so that part of the
if is useless.

Regards,

Hans