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

[Jonathon Jongsma]

On Thu, 2017-09-07 at 10:28 +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
> 
> 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.

Although I think I wrote the above paragraph, when I read it now it
seems a bit awkward. Here's another attempt:

"Since RedChannelClient is a GInitable type, creating a new object can
fail. Internally, GInitable creates a new object, calls that object's
_init() function, and if init fails the newly-created object is
destroyed. As part of RedChannelClient's _init() function, we add a new
watch for the stream (using the core interface associated with the
channel). When _init() fails, the associated channel client will be
destroyed, which in turn frees the stream (since the channel client
took ownership of the stream). Unfortunately, reds_stream_free() tries
to remove the watch from the core interface associated with the
RedsState instead of the core interface that was used to add the watch.
In most channels, these two core interfaces are equivalent, but for the
Display and Cursor channels, the channel's core interface is the Glib-
based one that is owned by RedWorker."

Not sure if that's any better or not. Take it or leave it.

ACK series.

> 
> 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>
> ---
> Changes since v2:
> - improved commit message (Jonathon)
> ---
>  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 655e41c73..35bb8d922 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;
> +    }
> +
>      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);
> +    }
>  
>      if (self->priv->monitor_latency
>          && reds_stream_get_family(self->priv->stream) != AF_UNIX) {