[Spice-devel] [spice-gtk v1] channel-usbredir: Fix crash on channel-up

[Christophe Fergeau]

On Tue, Nov 29, 2016 at 10:46:24PM +0100, Victor Toso wrote:
> From: Victor Toso <me at victortoso.com>
> 
> Seems that usbredirhost initialization can take longer then
> the channel_up call from spice-channel.
> 
> As this seems to be a race, a simple check for NULL pointer should be
> enough. Similar fix was done in 5b252b0f499601bcf387c02a4dd35d27ed34c

Too many "seems" for my taste in this commit log when this mentions a
race condition :(

priv->host is initialized in spice_usbredir_channel_set_context() which
is called from SpiceUsbDeviceManager::channel_new. SpiceUsbDeviceManager
will only start listening for the "channel-new" signal in
spice_usb_device_manager_initable_init().

I think what you are suggesting is that
spice_usb_device_manager_initable_init() is taking too long, and the
usbredir is up and running before we get a chance to set priv->host?

NB: channel_up is missing from the backtrace but is what is called
between #0 and #1.

Can we miss some data if we skip this flushing of pending writes? I
guess we cannot have any since priv->host does not exist yet?

Christophe

> 
>  #0 in usbredirhost_write_guest_data (host=0x0) at usbredirhost.c:876
>  #1 in spice_channel_recv_auth (channel=0x55af5f3b8f50 [SpiceUsbredirChannel]) at spice-channel.c:1225
>  #2 in spice_channel_coroutine (data=0x55af5f3b8f50) at spice-channel.c:2580
>  #3 in coroutine_trampoline (cc=0x55af5f3b85e0) at coroutine_ucontext.c:63
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1399838
> 
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> ---
>  src/channel-usbredir.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> index 4837d68..561f486 100644
> --- a/src/channel-usbredir.c
> +++ b/src/channel-usbredir.c
> @@ -817,6 +817,9 @@ static void spice_usbredir_channel_up(SpiceChannel *c)
>      SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(c);
>      SpiceUsbredirChannelPrivate *priv = channel->priv;
>  
> +    if (priv->host == NULL)
> +      return;
> +
>      /* Flush any pending writes */
>      usbredirhost_write_guest_data(priv->host);
>  }
> -- 
> 2.9.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20161130/b1c7dd4d/attachment.sig>