From 977a70e88992bfe56a03294d76b8478bf7dd7020 Mon Sep 17 00:00:00 2001 From: Yonit Halperin 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