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

[Frediano Ziglio]

> 
> > On 30 Aug 2017, at 10:19, Frediano Ziglio <fziglio at redhat.com> 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);
> 
> So what you are doing with this change is create multiple channels
> with the same id. Is that valid? Why would you want to do that
> legitimately?
> 

Because I'm the bad guy! :-)

If you expect that everybody is good with you... well...
then there are no security issue, right?
In this case an authenticated user (maybe a normal one) can
crash (so shutdown) the guest (which require admin privileges).

But beside that a robust code should handle unexpected
conditions, at the end anything can come from the network.

> If it’s not valid, shouldn’t we rather assert at creation time?
> Would that also fix the issue you were seeing?
> 

assert -> crash ?? why do I have to crash the VM ?

> I’m trying to understand if the condition your patch is addressing
> is legitimate today, or if there is a reason to make it legitimate later,
> or if it’s forever a “don’t go there” scenario :-)
> 
> > 
> >     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
> > 
> > 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.
> 
> For my education, could you explain why it’s harmless to move from a
> per-stream watch to a per-channel client watch?
> 
> 

Is just that RedsStream assume that the stream is attached to
the main SpiceCoreInterface which is different from the RCC one.
This difference basically causes the code to call a function
(provided by spice server user) passing an invalid pointer.
So storing the watch in RCC fix this assumption.

As RedsStream is attached to a single RCC both watches would
be in theory per-channel.

Another option would be to store the right SpiceCoreInterface
inside RedsStream.

> 
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> > server/red-channel-client.c | 39 ++++++++++++++++++++++++++++-----------
> > 1 file changed, 28 insertions(+), 11 deletions(-)
> > 
> > diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> > index 145ba43f..65392ed1 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;
> > +    }
> > +
> >     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