[spice-server,2/4] red-client: Protect concurrent list accesses

Submitted by Frediano Ziglio on Aug. 30, 2017, 12:51 p.m.

Details

Message ID 20170830125128.9453-2-fziglio@redhat.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Aug. 30, 2017, 12:51 p.m.
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.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 server/red-client.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/red-client.c b/server/red-client.c
index ddfc5400..800b1a5d 100644
--- a/server/red-client.c
+++ b/server/red-client.c
@@ -192,9 +192,6 @@  void red_client_migrate(RedClient *client)
 
 void red_client_destroy(RedClient *client)
 {
-    RedChannelClient *rcc;
-
-    spice_printerr("destroy client %p with #channels=%d", client, g_list_length(client->channels));
     if (!pthread_equal(pthread_self(), client->thread_id)) {
         spice_warning("client->thread_id (0x%lx) != pthread_self (0x%lx)."
                       "If one of the threads is != io-thread && != vcpu-thread,"
@@ -202,23 +199,45 @@  void red_client_destroy(RedClient *client)
                       client->thread_id,
                       pthread_self());
     }
-    red_client_set_disconnecting(client);
-    FOREACH_CHANNEL_CLIENT(client, rcc) {
+
+    pthread_mutex_lock(&client->lock);
+    spice_printerr("destroy client %p with #channels=%d", client, g_list_length(client->channels));
+    // this make sure that possible RedChannelClient setting up
+    // to be added won't be added to the list
+    client->disconnecting = TRUE;
+    while (client->channels) {
         RedChannel *channel;
+        RedChannelClient *rcc = client->channels->data;
+
+        // Remove the list to the RedChannelClient we are processing
+        // note that we own the object so 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);
+
         // 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);
+
+        // 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. 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);
 }
 

Comments

On Wed, Aug 30, 2017 at 01:51:26PM +0100, Frediano Ziglio wrote:
> 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.

I'd expect a detailed description (ie at least function names) of the
potential race in the log.

> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  server/red-client.c | 29 ++++++++++++++++++++++++-----
>  1 file changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/server/red-client.c b/server/red-client.c
> index ddfc5400..800b1a5d 100644
> --- a/server/red-client.c
> +++ b/server/red-client.c
> @@ -192,9 +192,6 @@ void red_client_migrate(RedClient *client)
>  
>  void red_client_destroy(RedClient *client)
>  {
> -    RedChannelClient *rcc;
> -
> -    spice_printerr("destroy client %p with #channels=%d", client, g_list_length(client->channels));
>      if (!pthread_equal(pthread_self(), client->thread_id)) {
>          spice_warning("client->thread_id (0x%lx) != pthread_self (0x%lx)."
>                        "If one of the threads is != io-thread && != vcpu-thread,"
> @@ -202,23 +199,45 @@ void red_client_destroy(RedClient *client)
>                        client->thread_id,
>                        pthread_self());
>      }
> -    red_client_set_disconnecting(client);
> -    FOREACH_CHANNEL_CLIENT(client, rcc) {
> +
> +    pthread_mutex_lock(&client->lock);
> +    spice_printerr("destroy client %p with #channels=%d", client, g_list_length(client->channels));
> +    // this make sure that possible RedChannelClient setting up
> +    // to be added won't be added to the list

"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 list to the RedChannelClient we are processing

"Remove the RedChannelClient we are processing from the list"?

> +        // note that we own the object so 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);
> +
>          // 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);
> +
> +        // prevent dead lock disconnecting rcc (which can happen
> +        // in the same thread or synchronously on another one)

I'd expect some details here on in the commit log about the deadlock
scenario.

> +        pthread_mutex_unlock(&client->lock);
> +
>          // 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);

This call is
          red_channel_client_set_destroying(rcc);
          red_channel_client_disconnect(rcc);

both of which have been done earlier in this function.

Christophe