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.
>
> 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?
Apart from this, looks good to me, though I don't really like this whole
RedsStream API :-/
Christophe
> 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);
> --
> 2.13.5
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel