[Spice-devel,spice-gtk,v2,1/2] channel-usbredir: Fix crash on channel-up

Submitted by Victor Toso on Nov. 30, 2016, 5:36 p.m.

Details

Message ID 20161130173633.28854-1-victortoso@redhat.com
State Accepted
Headers show
Series "Series without cover letter" ( rev: 2 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Nov. 30, 2016, 5:36 p.m.
From: Victor Toso <me@victortoso.com>

SpiceSession does not initialize its SpiceUsbDeviceManager object on
startup that could lead to a race condition where channel-usbredir is
requested to flush data while it is uninitialized.

In a few places, spice_usb_device_manager_get() is called as in
usb-device-widget.c and spice-gtk-session.c but not used in
spicy-stats, making the tool to crash on startup.

 #0 in usbredirhost_write_guest_data (host=0x0) at usbredir/usbredirhost/usbredirhost.c:876
 #1 in spice_usbredir_channel_up (c=0x643830) at channel-usbredir.c:821
 #2 in spice_channel_up (channel=0x643830) at spice-channel.c:1238
 #3 in spice_channel_recv_auth (channel=0x643830) at spice-channel.c:1225
 #4 in spice_channel_coroutine (data=0x643830) at spice-channel.c:2580
 #5 in coroutine_trampoline (cc=0x642ec0) at coroutine_ucontext.c:63
 #6 in continuation_trampoline (i0=6565568, i1=0) at continuation.c:55

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1399838

Signed-off-by: Victor Toso <victortoso@redhat.com>
Reported-by: Michael Cullen <michael@cullen-online.com>
---
 src/spice-session.c | 7 +++++++
 1 file changed, 7 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/spice-session.c b/src/spice-session.c
index f900bd1..91e4f97 100644
--- a/src/spice-session.c
+++ b/src/spice-session.c
@@ -281,6 +281,7 @@  static void spice_session_init(SpiceSession *session)
 {
     SpiceSessionPrivate *s;
     gchar *channels;
+    GError *err = NULL;
 
     SPICE_DEBUG("New session (compiled from package " PACKAGE_STRING ")");
     s = session->priv = SPICE_SESSION_GET_PRIVATE(session);
@@ -293,6 +294,12 @@  static void spice_session_init(SpiceSession *session)
     s->images = cache_image_new((GDestroyNotify)pixman_image_unref);
     s->glz_window = glz_decoder_window_new();
     update_proxy(session, NULL);
+
+    spice_usb_device_manager_get(session, &err);
+    if (err != NULL) {
+        SPICE_DEBUG("Could not initialize SpiceUsbDeviceManager - %s", err->message);
+        g_clear_error(&err);
+    }
 }
 
 static void

Comments

On Wed, Nov 30, 2016 at 06:36:32PM +0100, Victor Toso wrote:
> From: Victor Toso <me@victortoso.com>
> 
> SpiceSession does not initialize its SpiceUsbDeviceManager object on
> startup that could lead to a race condition where channel-usbredir is
> requested to flush data while it is uninitialized.
> 
> In a few places, spice_usb_device_manager_get() is called as in
> usb-device-widget.c and spice-gtk-session.c but not used in
> spicy-stats, making the tool to crash on startup.

Just running spicy-stats when there is a usbredir channel is going to
cause a crash? Isn't this avoided by your next patch as well which makes
sure host is not NULL before trying to flush?

Christophe

> 
>  #0 in usbredirhost_write_guest_data (host=0x0) at usbredir/usbredirhost/usbredirhost.c:876
>  #1 in spice_usbredir_channel_up (c=0x643830) at channel-usbredir.c:821
>  #2 in spice_channel_up (channel=0x643830) at spice-channel.c:1238
>  #3 in spice_channel_recv_auth (channel=0x643830) at spice-channel.c:1225
>  #4 in spice_channel_coroutine (data=0x643830) at spice-channel.c:2580
>  #5 in coroutine_trampoline (cc=0x642ec0) at coroutine_ucontext.c:63
>  #6 in continuation_trampoline (i0=6565568, i1=0) at continuation.c:55
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1399838
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> Reported-by: Michael Cullen <michael@cullen-online.com>
> ---
>  src/spice-session.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/spice-session.c b/src/spice-session.c
> index f900bd1..91e4f97 100644
> --- a/src/spice-session.c
> +++ b/src/spice-session.c
> @@ -281,6 +281,7 @@ static void spice_session_init(SpiceSession *session)
>  {
>      SpiceSessionPrivate *s;
>      gchar *channels;
> +    GError *err = NULL;
>  
>      SPICE_DEBUG("New session (compiled from package " PACKAGE_STRING ")");
>      s = session->priv = SPICE_SESSION_GET_PRIVATE(session);
> @@ -293,6 +294,12 @@ static void spice_session_init(SpiceSession *session)
>      s->images = cache_image_new((GDestroyNotify)pixman_image_unref);
>      s->glz_window = glz_decoder_window_new();
>      update_proxy(session, NULL);
> +
> +    spice_usb_device_manager_get(session, &err);
> +    if (err != NULL) {
> +        SPICE_DEBUG("Could not initialize SpiceUsbDeviceManager - %s", err->message);
> +        g_clear_error(&err);
> +    }
>  }
>  
>  static void
> -- 
> 2.9.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
Hi,

On Thu, Dec 01, 2016 at 01:29:50PM +0100, Christophe Fergeau wrote:
> On Wed, Nov 30, 2016 at 06:36:32PM +0100, Victor Toso wrote:
> > From: Victor Toso <me@victortoso.com>
> > 
> > SpiceSession does not initialize its SpiceUsbDeviceManager object on
> > startup that could lead to a race condition where channel-usbredir is
> > requested to flush data while it is uninitialized.
> > 
> > In a few places, spice_usb_device_manager_get() is called as in
> > usb-device-widget.c and spice-gtk-session.c but not used in
> > spicy-stats, making the tool to crash on startup.
> 
> Just running spicy-stats when there is a usbredir channel is going to
> cause a crash?

Yes

> Isn't this avoided by your next patch as well

Yes

> which makes sure host is not NULL before trying to flush?

I see this as two different problems, really.

1-) chanel-usbredir should take in consideration that it might not be
    initialized (second patch)
2-) SpiceSession should initialize SpiceUsbDeviceManager (this patch)
    otherwise, situations like (1) could happen;

spice_usb_device_manager_initable_init() could fail here so, the
situation in (1) might as well happen.

> 
> Christophe
> 
> > 
> >  #0 in usbredirhost_write_guest_data (host=0x0) at usbredir/usbredirhost/usbredirhost.c:876
> >  #1 in spice_usbredir_channel_up (c=0x643830) at channel-usbredir.c:821
> >  #2 in spice_channel_up (channel=0x643830) at spice-channel.c:1238
> >  #3 in spice_channel_recv_auth (channel=0x643830) at spice-channel.c:1225
> >  #4 in spice_channel_coroutine (data=0x643830) at spice-channel.c:2580
> >  #5 in coroutine_trampoline (cc=0x642ec0) at coroutine_ucontext.c:63
> >  #6 in continuation_trampoline (i0=6565568, i1=0) at continuation.c:55
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1399838
> > 
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > Reported-by: Michael Cullen <michael@cullen-online.com>
> > ---
> >  src/spice-session.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/src/spice-session.c b/src/spice-session.c
> > index f900bd1..91e4f97 100644
> > --- a/src/spice-session.c
> > +++ b/src/spice-session.c
> > @@ -281,6 +281,7 @@ static void spice_session_init(SpiceSession *session)
> >  {
> >      SpiceSessionPrivate *s;
> >      gchar *channels;
> > +    GError *err = NULL;
> >  
> >      SPICE_DEBUG("New session (compiled from package " PACKAGE_STRING ")");
> >      s = session->priv = SPICE_SESSION_GET_PRIVATE(session);
> > @@ -293,6 +294,12 @@ static void spice_session_init(SpiceSession *session)
> >      s->images = cache_image_new((GDestroyNotify)pixman_image_unref);
> >      s->glz_window = glz_decoder_window_new();
> >      update_proxy(session, NULL);
> > +
> > +    spice_usb_device_manager_get(session, &err);
> > +    if (err != NULL) {
> > +        SPICE_DEBUG("Could not initialize SpiceUsbDeviceManager - %s", err->message);
> > +        g_clear_error(&err);
> > +    }
> >  }
> >  
> >  static void
> > -- 
> > 2.9.3
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Thu, Dec 01, 2016 at 02:20:29PM +0100, Victor Toso wrote:
> Hi,
> 
> On Thu, Dec 01, 2016 at 01:29:50PM +0100, Christophe Fergeau wrote:
> > On Wed, Nov 30, 2016 at 06:36:32PM +0100, Victor Toso wrote:
> > > From: Victor Toso <me@victortoso.com>
> > > 
> > > SpiceSession does not initialize its SpiceUsbDeviceManager object on
> > > startup that could lead to a race condition where channel-usbredir is
> > > requested to flush data while it is uninitialized.
> > > 
> > > In a few places, spice_usb_device_manager_get() is called as in
> > > usb-device-widget.c and spice-gtk-session.c but not used in
> > > spicy-stats, making the tool to crash on startup.
> > 
> > Just running spicy-stats when there is a usbredir channel is going to
> > cause a crash?
> 
> Yes

Ok, I'd mention this explicitly "making the tool crash on startup if
it encounters an usbredir channel"

> 
> > Isn't this avoided by your next patch as well
> 
> Yes
> 
> > which makes sure host is not NULL before trying to flush?
> 
> I see this as two different problems, really.
> 
> 1-) chanel-usbredir should take in consideration that it might not be
>     initialized (second patch)
> 2-) SpiceSession should initialize SpiceUsbDeviceManager (this patch)
>     otherwise, situations like (1) could happen;
> 
> spice_usb_device_manager_initable_init() could fail here so, the
> situation in (1) might as well happen.
> 
> > 
> > Christophe
> > 
> > > 
> > >  #0 in usbredirhost_write_guest_data (host=0x0) at usbredir/usbredirhost/usbredirhost.c:876
> > >  #1 in spice_usbredir_channel_up (c=0x643830) at channel-usbredir.c:821
> > >  #2 in spice_channel_up (channel=0x643830) at spice-channel.c:1238
> > >  #3 in spice_channel_recv_auth (channel=0x643830) at spice-channel.c:1225
> > >  #4 in spice_channel_coroutine (data=0x643830) at spice-channel.c:2580
> > >  #5 in coroutine_trampoline (cc=0x642ec0) at coroutine_ucontext.c:63
> > >  #6 in continuation_trampoline (i0=6565568, i1=0) at continuation.c:55
> > > 
> > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1399838
> > > 
> > > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > > Reported-by: Michael Cullen <michael@cullen-online.com>
> > > ---
> > >  src/spice-session.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/src/spice-session.c b/src/spice-session.c
> > > index f900bd1..91e4f97 100644
> > > --- a/src/spice-session.c
> > > +++ b/src/spice-session.c
> > > @@ -281,6 +281,7 @@ static void spice_session_init(SpiceSession *session)
> > >  {
> > >      SpiceSessionPrivate *s;
> > >      gchar *channels;
> > > +    GError *err = NULL;
> > >  
> > >      SPICE_DEBUG("New session (compiled from package " PACKAGE_STRING ")");
> > >      s = session->priv = SPICE_SESSION_GET_PRIVATE(session);
> > > @@ -293,6 +294,12 @@ static void spice_session_init(SpiceSession *session)
> > >      s->images = cache_image_new((GDestroyNotify)pixman_image_unref);
> > >      s->glz_window = glz_decoder_window_new();
> > >      update_proxy(session, NULL);
> > > +
> > > +    spice_usb_device_manager_get(session, &err);

I know this is going to be redundant, but
session->priv->usb_manager = spice_usb_device_manager_get(session, &err);
would look a bit better.

Either way,
Acked-by: Christophe Fergeau <cfergeau@redhat.com>