[Spice-devel] [PATCH spice-server 1/5] Fix crash attempting to connect duplicate channels
Frediano Ziglio
fziglio at redhat.com
Fri Aug 25 15:04:35 UTC 2017
>
> 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?
> 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.
> > reds_stream_free(self->priv->stream);
> > self->priv->stream = NULL;
> >
> > @@ -922,7 +937,7 @@ static gboolean
> > red_channel_client_initable_init(GInitable *initable,
> >
> > core = red_channel_get_core_interface(self->priv->channel);
> > if (self->priv->stream)
> > - self->priv->stream->watch =
> > + self->priv->watch =
> > core->watch_add(core, self->priv->stream->socket,
> > SPICE_WATCH_EVENT_READ,
> > red_channel_client_event,
> > @@ -1003,9 +1018,11 @@ void red_channel_client_destroy(RedChannelClient
> > *rcc)
> > void red_channel_client_shutdown(RedChannelClient *rcc)
> > {
> > if (rcc->priv->stream && !rcc->priv->stream->shutdown) {
> > - SpiceCoreInterfaceInternal *core =
> > red_channel_get_core_interface(rcc->priv->channel);
> > - core->watch_remove(core, rcc->priv->stream->watch);
> > - rcc->priv->stream->watch = NULL;
> > + if (rcc->priv->watch) {
> > + SpiceCoreInterfaceInternal *core =
> > red_channel_get_core_interface(rcc->priv->channel);
> > + core->watch_remove(core, rcc->priv->watch);
> > + rcc->priv->watch = NULL;
> > + }
> > shutdown(rcc->priv->stream->socket, SHUT_RDWR);
> > rcc->priv->stream->shutdown = TRUE;
> > }
> > @@ -1294,10 +1311,10 @@ void red_channel_client_push(RedChannelClient *rcc)
> > red_channel_client_send_item(rcc, pipe_item);
> > }
> > if (red_channel_client_no_item_being_sent(rcc) &&
> > g_queue_is_empty(&rcc->priv->pipe)
> > - && rcc->priv->stream->watch) {
> > + && rcc->priv->watch) {
> > SpiceCoreInterfaceInternal *core;
> > core = red_channel_get_core_interface(rcc->priv->channel);
> > - core->watch_update_mask(core, rcc->priv->stream->watch,
> > + core->watch_update_mask(core, rcc->priv->watch,
> > SPICE_WATCH_EVENT_READ);
> > }
> > rcc->priv->during_send = FALSE;
> > @@ -1511,10 +1528,10 @@ static inline gboolean
> > prepare_pipe_add(RedChannelClient *rcc, RedPipeItem *item
> > red_pipe_item_unref(item);
> > return FALSE;
> > }
> > - if (g_queue_is_empty(&rcc->priv->pipe) && rcc->priv->stream->watch) {
> > + if (g_queue_is_empty(&rcc->priv->pipe) && rcc->priv->watch) {
> > SpiceCoreInterfaceInternal *core;
> > core = red_channel_get_core_interface(rcc->priv->channel);
> > - core->watch_update_mask(core, rcc->priv->stream->watch,
> > + core->watch_update_mask(core, rcc->priv->watch,
> > SPICE_WATCH_EVENT_READ |
> > SPICE_WATCH_EVENT_WRITE);
> > }
> > return TRUE;
> > @@ -1678,9 +1695,9 @@ void red_channel_client_disconnect(RedChannelClient
> > *rcc)
> > spice_printerr("rcc=%p (channel=%p type=%d id=%d)", rcc, channel,
> > type, id);
> > red_channel_client_pipe_clear(rcc);
> > - if (rcc->priv->stream->watch) {
> > - core->watch_remove(core, rcc->priv->stream->watch);
> > - rcc->priv->stream->watch = NULL;
> > + if (rcc->priv->watch) {
> > + core->watch_remove(core, rcc->priv->watch);
> > + rcc->priv->watch = NULL;
> > }
> > if (rcc->priv->latency_monitor.timer) {
> > core->timer_remove(core, rcc->priv->latency_monitor.timer);
Frediano
More information about the Spice-devel
mailing list