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

[Christophe Fergeau]

On Wed, Jun 29, 2016 at 07:29:18PM +0200, Hans de Goede wrote:
> 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().

Yeah, I agree with most of this, however if the API is misused (and
actually it has been for years...), it still much better to cleanup the
thread rather than leaving it hanging around until it triggers a crash.

However, I should have made this commit more explicit that something bad
happened first, ie:

        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. This should not happen if the API is properly
             * used, but if it's not, this will avoid a crash/thread
             * leak */
            g_warn_if_reached();
            g_atomic_int_set(&priv->event_thread_run, FALSE);
        }
(I'll send a v2 doing that, and add more precisions to the commit log
that it's not a real fix)


In this case spice_usbredir_channel_disconnect_device() is called with a
non-NULL channel:

#0  0x00007fc80b449dc4 in spice_usbredir_channel_disconnect_device
(channel=0x55aa2bc55060 [SpiceUsbredirChannel]) at
channel-usbredir.c:459
#1  0x00007fc80b449fb3 in _disconnect_device_thread (task=0x55aa2c7ad910
[GTask], object=0x55aa2bc55060, task_data=0x0, cancellable=0x0) at
channel-usbredir.c:508
#2  0x00007fc83f0fab10 in g_task_thread_pool_thread
(thread_data=0x55aa2c7ad910, pool_data=<optimized out>) at gtask.c:1288
#3  0x00007fc83f875735 in g_thread_pool_thread_proxy (data=<optimized
out>) at gthreadpool.c:307
#4  0x00007fc83f874d38 in g_thread_proxy (data=0x55aa2c93fed0) at
gthread.c:780
#5  0x00007fc84244f5ca in start_thread (arg=0x7fc8193b3700) at
pthread_create.c:333
#6  0x00007fc841a77ead in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:109


but SpiceChannel::session has already been set to NULL, so this specific
code block is not going to run:

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

The session is set to NULL through:

#0  0x00007fc80b4221ed in spice_session_channel_destroy (session=0x55aa2ba69fa0 [SpiceSession], channel=0x55aa2bc55060 [SpiceUsbredirChannel]) at spice-session.c:2280
#1  0x00007fc80b41c70e in session_disconnect (self=0x55aa2ba69fa0 [SpiceSession], keep_main=0)
    at spice-session.c:314
#2  0x00007fc80b420a3b in session_disconnect_idle (self=0x55aa2ba69fa0 [SpiceSession])
    at spice-session.c:1908
#3  0x00007fc83f84e703 in g_main_context_dispatch (context=0x55aa2b09d320) at gmain.c:3154


I"m a bit wary of touching the device disconnection/channel teardown code paths
though as I'm not familiar with them :((

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160630/1fe594c4/attachment.sig>