diff options
author | Frediano Ziglio <fziglio@redhat.com> | 2017-08-30 13:18:51 +0100 |
---|---|---|
committer | Frediano Ziglio <fziglio@redhat.com> | 2019-12-11 12:47:41 +0000 |
commit | f4a387f60d1f7d7c919e28e23f18ab2664e9c179 (patch) | |
tree | 4d593496dfc2416d93d052954b02b1ca7b841ad1 | |
parent | 5c6108a5a43ff2da5cd07acf0ad4a83538117081 (diff) |
red-client: Protect concurrent list accesses
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@redhat.com>
-rw-r--r-- | server/red-client.c | 28 |
1 files changed, 24 insertions, 4 deletions
diff --git a/server/red-client.c b/server/red-client.c index a4c79a17..019da5a2 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); } |