[Spice-devel] [PATCH spice-server 4/8] decouple disconnection of the main channel from client destruction
Yonit Halperin
yhalperi at redhat.com
Fri Jul 26 11:08:34 PDT 2013
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
--
1.8.1.4
More information about the Spice-devel
mailing list