[Spice-devel] [PATCH spice-server 1/5] Fix crash attempting to connect duplicate channels

[Christophe Fergeau]

On Fri, Aug 25, 2017 at 11:04:35AM -0400, Frediano Ziglio wrote:
> > 
> > On Thu, Aug 24, 2017 at 03:37:16PM +0100, Frediano Ziglio wrote:
> > > You could easily trigger this issue using multiple monitors and
> > > a modified client with this patch:
> > > 
> > > --- a/src/channel-main.c
> > > +++ b/src/channel-main.c
> > > @@ -1699,6 +1699,7 @@ static gboolean _channel_new(channel_new_t *c)
> > >  {
> > >      g_return_val_if_fail(c != NULL, FALSE);
> > > 
> > > +    if (c->type == SPICE_CHANNEL_DISPLAY) c->id = 0;
> > >      spice_channel_new(c->session, c->type, c->id);
> > > 
> > >      g_object_unref(c->session);
> > > 
> > > 
> > > which cause a crash like
> > > 
> > > 
> > > (process:28742): Spice-WARNING **: Failed to create channel client: Client
> > > 0x40246f5d0: duplicate channel type 2 id 0
> > > 2017-08-24 09:36:57.451+0000: shutting down, reason=crashed
> > > 
> > > One issue is that g_initable_new in this case returns NULL (dcc.c).
> > 
> > I would split this commit in 2 parts, one simple NULL dereference fix,
> > and a second one with your watch changes.
> > 
> 
> Make sense.
> Actually I discovered the watch part trying to reproduce the NULL
> pointer dereference.
> 
> > > 
> > > The other is that the watch in RedsStream is supposed to be bound
> > > to the Qemu provided SpiceCoreInterface while RedChannelClient bound
> > > it to Glib one causing the crash when the watch is deleted from
> > > RedsStream. To avoid this a new watch is saved in
> > > RedChannelClientPrivate.
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  server/dcc.c                |  4 +++-
> > >  server/red-channel-client.c | 39 ++++++++++++++++++++++++++++-----------
> > >  2 files changed, 31 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/server/dcc.c b/server/dcc.c
> > > index 4490507b..2e1acd23 100644
> > > --- a/server/dcc.c
> > > +++ b/server/dcc.c
> > > @@ -516,7 +516,9 @@ DisplayChannelClient *dcc_new(DisplayChannel *display,
> > >                           NULL);
> > >      spice_debug("New display (client %p) dcc %p stream %p", client, dcc,
> > >      stream);
> > >      common_graphics_channel_set_during_target_migrate(COMMON_GRAPHICS_CHANNEL(display),
> > >      mig_target);
> > > -    dcc->priv->id =
> > > common_graphics_channel_get_qxl(COMMON_GRAPHICS_CHANNEL(display))->id;
> > > +    if (dcc) {
> > > +        dcc->priv->id =
> > > common_graphics_channel_get_qxl(COMMON_GRAPHICS_CHANNEL(display))->id;
> > > +    }
> > >  
> > >      return dcc;
> > >  }
> > > diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> > > index f1042dd4..1162ddca 100644
> > > --- a/server/red-channel-client.c
> > > +++ b/server/red-channel-client.c
> > > @@ -120,6 +120,7 @@ struct RedChannelClientPrivate
> > >      RedChannel *channel;
> > >      RedClient  *client;
> > >      RedsStream *stream;
> > > +    SpiceWatch *watch;
> > >      gboolean monitor_latency;
> > >  
> > >      struct {
> > > @@ -339,6 +340,20 @@ red_channel_client_finalize(GObject *object)
> > >  {
> > >      RedChannelClient *self = RED_CHANNEL_CLIENT(object);
> > >  
> > > +    SpiceCoreInterfaceInternal *core =
> > > red_channel_get_core_interface(self->priv->channel);
> > > +    if (self->priv->watch) {
> > > +        core->watch_remove(core, self->priv->watch);
> > > +        self->priv->watch = NULL;
> > > +    }
> > > +    if (self->priv->latency_monitor.timer) {
> > > +        core->timer_remove(core, self->priv->latency_monitor.timer);
> > > +        self->priv->latency_monitor.timer = NULL;
> > > +    }
> > > +    if (self->priv->connectivity_monitor.timer) {
> > > +        core->timer_remove(core, self->priv->connectivity_monitor.timer);
> > > +        self->priv->connectivity_monitor.timer = NULL;
> > > +    }
> > > +
> > 
> > These latency_monitor/connectivity_monitor hunks seem unrelated to this
> > patch?
> 
> When a double channel is registered the second get build, register
> watches/timers then fail to be inserted into RedClient and got freed.
> As the timers/watches points to the object if you don't unregister
> them they will use a freed object.
> 
> Maybe all these unregistering should be on a single function?

This still sounds like a separate commit
"Stop latency/connectivity monitors when SpiceChannel is destroyed". And
yes this could be factored with the code in _disconnect() if this is
what you are asking.

> 
> > Apart from this, looks good to me, though I don't really like this whole
> > RedsStream API :-/
> > 
> > Christophe
> > 
> 
> Fully agree I think the RedsStream API should be a low core API not
> related to RedChannel. Actually I think should not even know what is
> RedsState/SpiceServer.
> 
> On the other side an authenticated client can cause the crash of
> the VM with this method. Still authenticated but the shutdown
> may require admin privileges.
> 
> One option would be to pass also (and allows to change) the
> SpiceCoreInterface to RedsStream so it can release the watch
> more safely with the right interface used to build the watch.
> 
> Not sure how easy is to remove watch from RedsStream (if make
> sense).
> 
> Any suggestion are welcome on the API.

Solution might be to gut all of that and to use GSocket or GIOChannel ;)
Not sure it's worth spending a lot of time on RedsStream right now,
the way you address things should be good enough for now.

Christophe