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

[Jonathon Jongsma]

On Wed, 2017-08-30 at 10:36 +0100, Frediano Ziglio wrote:
> You could easily trigger this issue using multiple monitors and
> a modified spice-gtk 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

It took me quite a while to figure out how this crash is related to the
patch below. Perhaps it needs additional detail in the commit log? As
far as I can tell, it's like this:

RedChannelClient is an GInitable type, which means that the object is
constructed, and then the _init() function is called, which can fail.
If the _init() fails, the newly-created object will be destroyed. As
part of _init(), we add a new watch for the stream using the core
interface that is associated with the channel. After adding the watch,
our rcc creation fails (due to duplicate ID), and the rcc object is
unreffed. This results in a call to reds_stream_free() (since the rcc
now owns the stream). But in reds_stream_free, we were trying to remove
the watch from the core interface associated with the RedsState. For
most channels, these two core interfaces are equivalent. But for the
Display and Cursor channels, it is the core Glib-based interface
associated with the RedWorker.

Do I understand it correctly?

> 
> The watch in RedsStream by default is bound to the Qemu
> provided SpiceCoreInterface while RedChannelClient bound it
> to Glib one causing the crash when the watch is deleted from
> RedsStream. Change the bound interface.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/red-channel-client.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/server/red-channel-client.c b/server/red-channel-
> client.c
> index 145ba43f..acc2b4eb 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -339,6 +339,16 @@ 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->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;
> +    }
> +

I'm a little bit confused here. Is this part related at all to the
crash in the commit message? It doesn't seem to be.

>      reds_stream_free(self->priv->stream);
>      self->priv->stream = NULL;
>  
> @@ -921,12 +931,14 @@ static gboolean
> red_channel_client_initable_init(GInitable *initable,
>      }
>  
>      core = red_channel_get_core_interface(self->priv->channel);
> -    if (self->priv->stream)
> +    if (self->priv->stream) {
> +        reds_stream_set_core_interface(self->priv->stream, core);
>          self->priv->stream->watch =
>              core->watch_add(core, self->priv->stream->socket,
>                              SPICE_WATCH_EVENT_READ,
>                              red_channel_client_event,
>                              self);
> +    }

It seems a little bit weird that the RedChannelClient is poking into
the stream structure to set stream->watch...

What about moving this whole section down below the call to
red_client_add_channel() (which is the call that can fail) in order to
avoid creating the watch at all when our rcc creation fails?

>  
>      if (self->priv->monitor_latency
>          && reds_stream_get_family(self->priv->stream) != AF_UNIX) {