[Spice-devel] [PATCH 2/5] channel: do not free rcc->stream in red_channel_client_disconnect

[Frediano Ziglio]

> 
> From: Marc-André Lureau <marcandre.lureau at gmail.com>
> 
> This fixes the following scary racy corruption after glib loop switch:
> 
> ==28173==
> ==28173== Debugger has detached.  Valgrind regains control.  We
> continue.
> ==28173== Invalid read of size 8
> ==28173==    at 0x4C7871E: reds_stream_read (reds.c:4521)
> ==28173==    by 0x4C2F9D7: red_peer_receive (red_channel.c:209)
> ==28173==    by 0x4C2FB59: red_peer_handle_incoming (red_channel.c:255)
> ==28173==    by 0x4C2FF36:
> red_channel_client_receive (red_channel.c:329)
> ==28173==    by 0x4C33D6D: red_channel_client_event (red_channel.c:1577)
> ==28173==    by 0x4C65098: watch_func (red_worker.c:10292)
> ==28173==    by 0x504DDAB: g_io_unix_dispatch (giounix.c:167)
> ==28173==    by 0x4FFACB6: g_main_dispatch (gmain.c:3065)
> ==28173==    by 0x4FFBA0D: g_main_context_dispatch (gmain.c:3641)
> ==28173==    by 0x4FFBBFF: g_main_context_iterate (gmain.c:3712)
> ==28173==    by 0x4FFC028: g_main_loop_run (gmain.c:3906)
> ==28173==    by 0x4C6ABF2: red_worker_main (red_worker.c:12180)
> ==28173==  Address 0x7d688e0 is 32 bytes inside a block of size 160
> free'd
> ==28173==    at 0x4A074C4: free (in
> /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==28173==    by 0x4C78A84: reds_stream_free (reds.c:4594)
> ==28173==    by 0x4C34E2E:
> red_channel_client_disconnect (red_channel.c:1865)
> ==28173==    by 0x4C30363:
> red_channel_client_default_peer_on_error (red_channel.c:417)
> ==28173==    by 0x4C3011B: red_peer_handle_outgoing (red_channel.c:372)
> ==28173==    by 0x4C3305F: red_channel_client_send (red_channel.c:1298)
> ==28173==    by 0x4C33FB6:
> red_channel_client_begin_send_message (red_channel.c:1616)
> ==28173==    by 0x4C5F4B4:
> display_begin_send_message (red_worker.c:8360)
> ==28173==    by 0x4C61DE8: display_channel_send_item (red_worker.c:9164)
> ==28173==    by 0x4C30C69:
> red_channel_client_send_item (red_channel.c:599)
> ==28173==    by 0x4C332BB: red_channel_client_push (red_channel.c:1351)
> ==28173==    by 0x4C33C3A:
> red_channel_client_handle_message (red_channel.c:1545)
> ---
>  server/red_channel.c | 39 +++++++++++++++++++++------------------
>  server/red_worker.c  |  2 ++
>  2 files changed, 23 insertions(+), 18 deletions(-)
> 
> diff --git a/server/red_channel.c b/server/red_channel.c
> index 8db3d6e..4d5066a 100644
> --- a/server/red_channel.c
> +++ b/server/red_channel.c
> @@ -1233,22 +1233,25 @@ static void red_channel_client_ref(RedChannelClient
> *rcc)
>  
>  static void red_channel_client_unref(RedChannelClient *rcc)
>  {
> -    if (!--rcc->refs) {
> -        spice_debug("destroy rcc=%p", rcc);
> -        if (rcc->send_data.main.marshaller) {
> -            spice_marshaller_destroy(rcc->send_data.main.marshaller);
> -        }
> +    spice_debug("rcc=%p", rcc);
>  
> -        if (rcc->send_data.urgent.marshaller) {
> -            spice_marshaller_destroy(rcc->send_data.urgent.marshaller);
> -        }
> +    if (--rcc->refs)
> +        return;
>  
> -        red_channel_client_destroy_remote_caps(rcc);
> -        if (rcc->channel) {
> -            red_channel_unref(rcc->channel);
> -        }
> -        free(rcc);
> -    }
> +    reds_stream_free(rcc->stream);
> +    rcc->stream = NULL;
> +
> +    if (rcc->send_data.main.marshaller)
> +        spice_marshaller_destroy(rcc->send_data.main.marshaller);
> +
> +    if (rcc->send_data.urgent.marshaller)
> +        spice_marshaller_destroy(rcc->send_data.urgent.marshaller);
> +
> +    red_channel_client_destroy_remote_caps(rcc);
> +    if (rcc->channel)
> +        red_channel_unref(rcc->channel);
> +
> +    free(rcc);
>  }
>  

This part looks quite a big change but mainly is style one.
However the debug string is printed at every reference change and not only
when really released.
I would rollback all these style changes.

>  void red_channel_client_destroy(RedChannelClient *rcc)
> @@ -1758,7 +1761,7 @@ void red_channel_pipes_add_empty_msg(RedChannel
> *channel, int msg_type)
>  int red_channel_client_is_connected(RedChannelClient *rcc)
>  {
>      if (!rcc->dummy) {
> -        return rcc->stream != NULL;
> +        return (rcc->stream != NULL) &&
> ring_item_is_linked(&rcc->channel_link);
>      } else {
>          return rcc->dummy_connected;
>      }
> @@ -1813,8 +1816,10 @@ static void red_channel_remove_client(RedChannelClient
> *rcc)
>                        rcc->channel->type, rcc->channel->id,
>                        rcc->channel->thread_id, pthread_self());
>      }
> +    spice_return_if_fail(ring_item_is_linked(&rcc->channel_link));
> +
>      ring_remove(&rcc->channel_link);
> -    spice_assert(rcc->channel->clients_num > 0);
> +    spice_return_if_fail(rcc->channel->clients_num > 0);

I think this WAS detecting an invalid internal state and the check was basically
made silent. Any reason?

>      rcc->channel->clients_num--;
>      // TODO: should we set rcc->channel to NULL???
>  }
> @@ -1854,8 +1859,6 @@ void red_channel_client_disconnect(RedChannelClient
> *rcc)
>          rcc->channel->core->watch_remove(rcc->stream->watch);
>          rcc->stream->watch = NULL;
>      }
> -    reds_stream_free(rcc->stream);
> -    rcc->stream = NULL;
>      if (rcc->latency_monitor.timer) {
>          rcc->channel->core->timer_remove(rcc->latency_monitor.timer);
>          rcc->latency_monitor.timer = NULL;
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 6cc5a35..742edfc 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -10135,6 +10135,8 @@ static gboolean watch_func(GIOChannel *source,
> GIOCondition condition,
>      SpiceWatch *watch = data;
>      int fd = g_io_channel_unix_get_fd(source);
>  
> +    spice_return_val_if_fail(!g_source_is_destroyed(watch->source), FALSE);
> +

Should glib call this function if the object is destroyed?

>      watch->func(fd, giocondition_to_spice_event(condition), watch->rcc);
>  
>      return TRUE;

Do we have a repro of this bug?

Frediano