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@xxxxxxxxxx>
> > > ---
> > > 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel