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

Submitted by Victor Toso on Nov. 29, 2016, 9:46 p.m.

Details

Message ID 20161129214624.20210-1-victortoso@redhat.com
State Superseded
Headers show
Series "channel-usbredir: Fix crash on channel-up" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Nov. 29, 2016, 9:46 p.m.
From: Victor Toso <me@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

 #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@redhat.com>
---
 src/channel-usbredir.c | 3 +++
 1 file changed, 3 insertions(+)

Patch hide | download patch | download mbox

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

Comments

On Tue, Nov 29, 2016 at 10:46:24PM +0100, Victor Toso wrote:
> From: Victor Toso <me@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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
Hi,

On Wed, Nov 30, 2016 at 11:20:42AM +0100, Christophe Fergeau wrote:
> On Tue, Nov 29, 2016 at 10:46:24PM +0100, Victor Toso wrote:
> > From: Victor Toso <me@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 :(

Ah, yeah, sorry about that. I wrote it as I was thinking about other
possibilities too, leading the commit log to not be so certain about the
issue.

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

Hm, it is a bit too hard to say for sure what is slow without more debug
info.

As you said, what does initialize priv->host is:
1-) channel-new [signal] -> channel_new() [usb-device-manager]
2-) spice_usbredir_channel_set_context()
3-) priv->host = usbredirhost_open_full(...)

As this did not happen at the time of the crash, yes, maybe
spice_usb_device_manager_initable_init() failed or was not called
which would mean that spice_usb_device_manager_get() was not called.

Not sure who should be calling spice_usb_device_manager_get() first?
- src/usb-device-widget.c at spice_usb_device_widget_constructor()
- src/spice-option.c .... at spice_set_session_option()
- src/spice-gtk-session.c at spice_gtk_session_request_auto_usbredir()

But I'm sure it can't be on src/channel-usbredir.c at
spice_usbredir_channel_open_device() as this needs priv->host already :)

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

Indeed, I wonder why is it missing?

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

I don't think we can miss anything if the device is not even open yet.
Maybe fixing this crash can trigger a different issue later on,
hopefully it would be easier to track.

Let me know if I should improve the commit log but after writing this
email, the 'seems' are likely correct as I'm not 100% sure how this was
triggered.

>
> Christophe

Cheers,
  toso

> 
> > 
> >  #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@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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
Hi,

On Wed, Nov 30, 2016 at 03:33:13PM +0100, Victor Toso wrote:
> Hi,
>
> On Wed, Nov 30, 2016 at 11:20:42AM +0100, Christophe Fergeau wrote:
> > On Tue, Nov 29, 2016 at 10:46:24PM +0100, Victor Toso wrote:
> > > From: Victor Toso <me@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 :(
> 
> Ah, yeah, sorry about that. I wrote it as I was thinking about other
> possibilities too, leading the commit log to not be so certain about the
> issue.
> 
> > 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?
> 
> Hm, it is a bit too hard to say for sure what is slow without more debug
> info.
> 
> As you said, what does initialize priv->host is:
> 1-) channel-new [signal] -> channel_new() [usb-device-manager]
> 2-) spice_usbredir_channel_set_context()
> 3-) priv->host = usbredirhost_open_full(...)
> 
> As this did not happen at the time of the crash, yes, maybe
> spice_usb_device_manager_initable_init() failed or was not called
> which would mean that spice_usb_device_manager_get() was not called.
> 
> Not sure who should be calling spice_usb_device_manager_get() first?
> - src/usb-device-widget.c at spice_usb_device_widget_constructor()
> - src/spice-option.c .... at spice_set_session_option()
> - src/spice-gtk-session.c at spice_gtk_session_request_auto_usbredir()
> 
> But I'm sure it can't be on src/channel-usbredir.c at
> spice_usbredir_channel_open_device() as this needs priv->host already :)
> 
> > NB: channel_up is missing from the backtrace but is what is called
> > between #0 and #1.
> 
> Indeed, I wonder why is it missing?
> 
> > 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?
> 
> I don't think we can miss anything if the device is not even open yet.
> Maybe fixing this crash can trigger a different issue later on,
> hopefully it would be easier to track.
> 
> Let me know if I should improve the commit log but after writing this
> email, the 'seems' are likely correct as I'm not 100% sure how this was
> triggered.
> 
> >
> > Christophe
> 
> Cheers,
>   toso

I missed a information on bugzilla, see:

Additional info:
(...)
  cmdline:        spicy-stats --uri=spice://localhost:5900
  crash_function: usbredirhost_write_guest_data
  executable:     /usr/bin/spicy-stats

That should narrow down how the bug was triggered...

> 
> > 
> > > 
> > >  #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@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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> 



> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel