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

[Frediano Ziglio]

This fixes a crash if red_channel_client disconnect is called
handling a message.
This can happen for instance handling SPICE_MSGC_ACK which calls
red_channel_client_push which try to write items to socket detecting
writing errors (for instance socket disconnection).
Messages are read in a loop and the red_channel_client_disconnect
would cause rcc->stream to be NULL which will turn in a use-after-free
problem (stream in red_peer_handle_incoming will use cached stream value).

Signed-off-by: Marc-André Lureau <marcandre.lureau at gmail.com>
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
---
 server/red-channel.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/server/red-channel.c b/server/red-channel.c
index 306c87d..51e8958 100644
--- a/server/red-channel.c
+++ b/server/red-channel.c
@@ -1231,22 +1231,28 @@ void red_channel_client_ref(RedChannelClient *rcc)
 
 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);
-        }
+    if (--rcc->refs != 0) {
+        return;
+    }
 
-        if (rcc->send_data.urgent.marshaller) {
-            spice_marshaller_destroy(rcc->send_data.urgent.marshaller);
-        }
+    spice_debug("destroy rcc=%p", rcc);
 
-        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)
@@ -1749,7 +1755,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 ring_item_is_linked(&rcc->channel_link);
     } else {
         return rcc->dummy_connected;
     }
@@ -1804,6 +1810,8 @@ 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);
     rcc->channel->clients_num--;
@@ -1845,8 +1853,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;
-- 
2.4.3