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

[Hans de Goede]

Hi,

On 30-06-16 18:02, Christophe Fergeau wrote:
> On Thu, Jun 30, 2016 at 04:54:53PM +0200, Hans de Goede wrote:
>> 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.
>
> We need to set event_thread_run to FALSE before calling
> libusb_hotplug_deregister_callback() otherwise the thread will not exit
> properly libusb_handle_events() will return because
> deregister_callback() was called, but event_thread_run is still TRUE, so
> the thread does not exit, and we'll be back to waiting on
> libusb_handle_events()...

Ah right, good point.

>> 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.
>
> However, for the !USE_LIBUSB_HOTPLUG case, we don't need to force set
> event_thread_run to FALSE if we remove the event_thread_run test from
> the 'if' condition. We can just add a warning there if event_thread_run
> was not FALSE. However, if this triggers, the g_thread_join() is likely
> to hang as we don't have anything in dispose() which would force
> libusb_handle_events() to return.
>
> Proposed change to squash in this patch:
>
>
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index 806af74..aa48a01 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -390,7 +391,8 @@ static void spice_usb_device_manager_dispose(GObject *gobject)
>          priv->hp_handle = 0;
>      }
>  #endif
> -    if (priv->event_thread && !g_atomic_int_get(&priv->event_thread_run)) {
> +    if (priv->event_thread) {
> +        g_warn_if_fail(g_atomic_int_get(&priv->event_thread_run) == FALSE);
>          g_thread_join(priv->event_thread);
>          priv->event_thread = NULL;
>      }

Yeah that is probably the best we can do.

So with this squashed in the entire series is:

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

Regards,

Hans