From 8490f83e1f51ac8dbcac3c68c97827e0acb9ec4e Mon Sep 17 00:00:00 2001
From: Yonit Halperin <yhalperi@redhat.com>
Date: Thu, 25 Jul 2013 14:19:21 -0400
Subject: decouple disconnection of the main channel from client destruction

Fixes rhbz#918169

Some channels make direct calls to reds/main_channel routines. If
these routines try to read/write to the socket, and they get socket
error, main_channel_client_on_disconnect is called, and triggers
red_client_destroy. In order to prevent accessing expired references
to RedClient, RedChannelClient, or other objects (inside the original call, after
red_client_destroy has been called) I made the call to
red_client_destroy asynchronous with respect to main_channel_client_on_disconnect.
I added MAIN_DISPATCHER_CLIENT_DISCONNECT to main_dispatcher.
main_channel_client_on_disconnect pushes this msg to the dispatcher,
instead of calling directly to reds_client_disconnect.

The patch uses RedClient ref-count in order to handle a case where
reds_client_disconnect is called directly (e.g., when a new client connects while
another one is connected), while there is already CLIENT_DISCONNECT msg
pending in the main_dispatcher.

Examples:
(1) snd_worker.c

    snd_disconnect_channel()
        channel->cleanup() //snd_playback_cleanup
            reds_enable_mm_timer()
                .
                .
                main_channel_push_multi_media_time()...socket_error
                    .
                    .
                    red_client_destory()
                        .
                        .
                        snd_disconnect_channel()
                            channel->cleanup()
                                celt051_encoder_destroy()
            celt051_encoder_destory() // double release

Note that this bug could have been solved by changing the order of
calls: e.g., channel->stream = NULL before calling cleanup, and
some other changes + reference counting. However, I found other
places in the code with similar problems, and I looked for a general
solution, at least till we redesign red_channel to handle reference
counting more consistently.

(2) inputs_channel.c

    inputs_connect()
        main_channel_client_push_notify()...socket_error
                .
                .
            red_client_destory()
                .
                .
        red_channel_client_create() // refers to client which is already destroyed

(3) reds.c

    reds_handle_main_link()
       main_channel_push_init() ...socket error
                .
                .
            red_client_destory()
                .
                .
       main_channel_client_start_net_test(mcc) // refers to mcc which is already destroyed

    This can explain the assert in rhbz#964136, comment #1 (but not the hang that occurred before).
---
 server/main_dispatcher.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

(limited to 'server/main_dispatcher.c')

diff --git a/server/main_dispatcher.c b/server/main_dispatcher.c
index bf160dd3..dbe10374 100644
--- a/server/main_dispatcher.c
+++ b/server/main_dispatcher.c
@@ -41,6 +41,7 @@ enum {
     MAIN_DISPATCHER_CHANNEL_EVENT = 0,
     MAIN_DISPATCHER_MIGRATE_SEAMLESS_DST_COMPLETE,
     MAIN_DISPATCHER_SET_MM_TIME_LATENCY,
+    MAIN_DISPATCHER_CLIENT_DISCONNECT,
 
     MAIN_DISPATCHER_NUM_MESSAGES
 };
@@ -59,6 +60,10 @@ typedef struct MainDispatcherMmTimeLatencyMessage {
     uint32_t latency;
 } MainDispatcherMmTimeLatencyMessage;
 
+typedef struct MainDispatcherClientDisconnectMessage {
+    RedClient *client;
+} MainDispatcherClientDisconnectMessage;
+
 /* channel_event - calls core->channel_event, must be done in main thread */
 static void main_dispatcher_self_handle_channel_event(
                                                 int event,
@@ -108,6 +113,16 @@ static void main_dispatcher_handle_mm_time_latency(void *opaque,
     red_client_unref(msg->client);
 }
 
+static void main_dispatcher_handle_client_disconnect(void *opaque,
+                                                     void *payload)
+{
+    MainDispatcherClientDisconnectMessage *msg = payload;
+
+    spice_debug("client=%p", msg->client);
+    reds_client_disconnect(msg->client);
+    red_client_unref(msg->client);
+}
+
 void main_dispatcher_seamless_migrate_dst_complete(RedClient *client)
 {
     MainDispatcherMigrateSeamlessDstCompleteMessage msg;
@@ -137,6 +152,20 @@ void main_dispatcher_set_mm_time_latency(RedClient *client, uint32_t latency)
                             &msg);
 }
 
+void main_dispatcher_client_disconnect(RedClient *client)
+{
+    MainDispatcherClientDisconnectMessage msg;
+
+    if (!client->disconnecting) {
+        spice_debug("client %p", client);
+        msg.client = red_client_ref(client);
+        dispatcher_send_message(&main_dispatcher.base, MAIN_DISPATCHER_CLIENT_DISCONNECT,
+                                &msg);
+    } else {
+        spice_debug("client %p already during disconnection", client);
+    }
+}
+
 static void dispatcher_handle_read(int fd, int event, void *opaque)
 {
     Dispatcher *dispatcher = opaque;
@@ -144,6 +173,11 @@ static void dispatcher_handle_read(int fd, int event, void *opaque)
     dispatcher_handle_recv_read(dispatcher);
 }
 
+/*
+ * FIXME:
+ * Reds routines shouldn't be exposed. Instead reds.c should register the callbacks,
+ * and the corresponding operations should be made only via main_dispatcher.
+ */
 void main_dispatcher_init(SpiceCoreInterface *core)
 {
     memset(&main_dispatcher, 0, sizeof(main_dispatcher));
@@ -160,4 +194,7 @@ void main_dispatcher_init(SpiceCoreInterface *core)
     dispatcher_register_handler(&main_dispatcher.base, MAIN_DISPATCHER_SET_MM_TIME_LATENCY,
                                 main_dispatcher_handle_mm_time_latency,
                                 sizeof(MainDispatcherMmTimeLatencyMessage), 0 /* no ack */);
+    dispatcher_register_handler(&main_dispatcher.base, MAIN_DISPATCHER_CLIENT_DISCONNECT,
+                                main_dispatcher_handle_client_disconnect,
+                                sizeof(MainDispatcherClientDisconnectMessage), 0 /* no ack */);
 }
-- 
cgit