The channels list was not protected by a lock in red_client_destroy. This could cause for instance a RedChannelClient object to be created while scanning the list so potentially modifying the list while scanning with all race issues. Consider a client which attempt to connect to a new channel and then for some reason close the main channel: - the new channel connection is handled by the main thread; - the relative RCC will be passed to red_channel_connect which can decide to continue the initialization in another thread (this happens currently for CursorChannelClient and DisplayChannelClient); - the main thread will catch the main channel disconnection and call main_dispatcher_client_disconnect (from main_channel_client_on_disconnect); - main thread calls reds_client_disconnect; - reds_client_disconnect calls red_client_destroy; - now we have 2 thread which will attempt to change RedClient::channels list, one protected by the mutex the other not. Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx> -- This version attempts to reply to https://lists.freedesktop.org/archives/spice-devel/2017-September/040124.html --- server/red-client.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/server/red-client.c b/server/red-client.c index a4c79a174..019da5a2b 100644 --- a/server/red-client.c +++ b/server/red-client.c @@ -190,8 +190,6 @@ void red_client_migrate(RedClient *client) void red_client_destroy(RedClient *client) { - RedChannelClient *rcc; - if (!pthread_equal(pthread_self(), client->thread_id)) { spice_warning("client->thread_id (%p) != " "pthread_self (%p)." @@ -200,23 +198,45 @@ void red_client_destroy(RedClient *client) (void*) client->thread_id, (void*) pthread_self()); } - red_client_set_disconnecting(client); - FOREACH_CHANNEL_CLIENT(client, rcc) { + + pthread_mutex_lock(&client->lock); + spice_debug("destroy client %p with #channels=%d", client, g_list_length(client->channels)); + // This makes sure that we won't try to add new RedChannelClient instances + // to the RedClient::channels list while iterating it + client->disconnecting = TRUE; + while (client->channels) { RedChannel *channel; + RedChannelClient *rcc = client->channels->data; + + // Remove the RedChannelClient we are processing from the list + // Note that we own the object so it is safe to do some operations on it. + // This manual scan of the list is done to have a thread safe + // iteration of the list + client->channels = g_list_delete_link(client->channels, client->channels); + + // prevent dead lock disconnecting rcc (which can happen + // in the same thread or synchronously on another one) + pthread_mutex_unlock(&client->lock); + // some channels may be in other threads, so disconnection // is not synchronous. channel = red_channel_client_get_channel(rcc); red_channel_client_set_destroying(rcc); + // some channels may be in other threads. However we currently // assume disconnect is synchronous (we changed the dispatcher // to wait for disconnection) // TODO: should we go back to async. For this we need to use // ref count for channel clients. red_channel_disconnect_client(channel, rcc); + spice_assert(red_channel_client_pipe_is_empty(rcc)); spice_assert(red_channel_client_no_item_being_sent(rcc)); + red_channel_client_destroy(rcc); + pthread_mutex_lock(&client->lock); } + pthread_mutex_unlock(&client->lock); g_object_unref(client); } -- 2.21.0 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel