[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);
 }
 
 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);
     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);
+
     watch->func(fd, giocondition_to_spice_event(condition), watch->rcc);
 
     return TRUE;
-- 
2.4.3