From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001

From: Yonit Halperin <yhalperi@redhat.com>

Date: Thu, 25 Jul 2013 14:19:21 -0400

Subject: [PATCH] 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_channel.c    |  9 +++++----

 server/main_dispatcher.c | 37 +++++++++++++++++++++++++++++++++++++

 server/main_dispatcher.h |  7 +++++++

 server/reds.c            |  1 +

 server/reds.h            |  2 ++

 5 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/server/main_channel.c b/server/main_channel.c

index 233e633..fe032a6 100644

--- a/server/main_channel.c

+++ b/server/main_channel.c

@@ -46,6 +46,7 @@

 #include "red_common.h"

 #include "reds.h"

 #include "migration_protocol.h"

+#include "main_dispatcher.h"

 #define ZERO_BUF_SIZE 4096

@@ -175,13 +176,13 @@ int main_channel_is_connected(MainChannel *main_chan)

     return red_channel_is_connected(&main_chan->base);

 }

-// when disconnection occurs, let reds shutdown all channels. This will trigger the

-// real disconnection of main channel

+/*

+ * When the main channel is disconnected, disconnect the entire client.

+ */

 static void main_channel_client_on_disconnect(RedChannelClient *rcc)

 {

     spice_printerr("rcc=%p", rcc);

-    reds_client_disconnect(rcc->client);

-//    red_channel_client_disconnect(rcc);

+    main_dispatcher_client_disconnect(rcc->client);

 }

 RedClient *main_channel_get_client_by_link_id(MainChannel *main_chan, uint32_t connection_id)

diff --git a/server/main_dispatcher.c b/server/main_dispatcher.c

index bf160dd..dbe1037 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 */);

 }

diff --git a/server/main_dispatcher.h b/server/main_dispatcher.h

index 0c79ca8..522c7f9 100644

--- a/server/main_dispatcher.h

+++ b/server/main_dispatcher.h

@@ -7,6 +7,13 @@

 void main_dispatcher_channel_event(int event, SpiceChannelEventInfo *info);

 void main_dispatcher_seamless_migrate_dst_complete(RedClient *client);

 void main_dispatcher_set_mm_time_latency(RedClient *client, uint32_t latency);

+/*

+ * Disconnecting the client is always executed asynchronously,

+ * in order to protect from expired references in the routines

+ * that triggered the client destruction.

+ */

+void main_dispatcher_client_disconnect(RedClient *client);

+

 void main_dispatcher_init(SpiceCoreInterface *core);

 #endif //MAIN_DISPATCHER_H

diff --git a/server/reds.c b/server/reds.c

index 30d0652..c66ddc4 100644

--- a/server/reds.c

+++ b/server/reds.c

@@ -537,6 +537,7 @@ void reds_client_disconnect(RedClient *client)

     }

     if (!client || client->disconnecting) {

+        spice_debug("client %p already during disconnection", client);

         return;

     }

diff --git a/server/reds.h b/server/reds.h

index c5c557d..1c5ae84 100644

--- a/server/reds.h

+++ b/server/reds.h

@@ -136,6 +136,8 @@ void reds_handle_agent_mouse_event(const VDAgentMouseState *mouse_state); // use

 extern struct SpiceCoreInterface *core;

 // Temporary measures to make splitting reds.c to inputs_channel.c easier

+

+/* should be called only from main_dispatcher */

 void reds_client_disconnect(RedClient *client);

 // Temporary (?) for splitting main channel