[Spice-commits] 4 commits - server/dcc.c server/red-channel-client.c server/red-client.c server/red-client.h server/reds.c server/tests

[GitLab Mirror]

 server/dcc.c                  |    2 +
 server/red-channel-client.c   |   13 +++++++++--
 server/red-client.c           |   48 +++++++++++++++++++++++++++++++++---------
 server/red-client.h           |    2 -
 server/reds.c                 |    1 
 server/tests/test-channel.c   |    1 
 server/tests/test-smartcard.c |    1 
 7 files changed, 51 insertions(+), 17 deletions(-)

New commits:
commit 5f02dff835385d56d43b2dccd209918ebdeb80df
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Fri Aug 25 10:02:28 2017 +0100

    red-client: Automatically set MainChannelClient
    
    This make sure that the RedClient has always an attached
    MainChannelClient.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

diff --git a/server/red-client.c b/server/red-client.c
index 476f6f5b..6cbfa72b 100644
--- a/server/red-client.c
+++ b/server/red-client.c
@@ -295,6 +295,11 @@ gboolean red_client_add_channel(RedClient *client, RedChannelClient *rcc, GError
         goto cleanup;
     }
 
+    // first must be the main one
+    if (!client->mcc) {
+        client->mcc = g_object_ref(rcc);
+        spice_assert(MAIN_CHANNEL_CLIENT(rcc) != NULL);
+    }
     client->channels = g_list_prepend(client->channels, rcc);
     if (client->during_target_migrate && client->seamless_migrate) {
         if (red_channel_client_set_migration_seamless(rcc)) {
@@ -312,12 +317,6 @@ MainChannelClient *red_client_get_main(RedClient *client)
     return client->mcc;
 }
 
-void red_client_set_main(RedClient *client, MainChannelClient *mcc)
-{
-    spice_assert(client->mcc == NULL);
-    client->mcc = g_object_ref(mcc);
-}
-
 void red_client_semi_seamless_migrate_complete(RedClient *client)
 {
     RedChannelClient *rcc;
diff --git a/server/red-client.h b/server/red-client.h
index 3ee7accb..e3c04f43 100644
--- a/server/red-client.h
+++ b/server/red-client.h
@@ -39,8 +39,6 @@ gboolean red_client_add_channel(RedClient *client, RedChannelClient *rcc, GError
 void red_client_remove_channel(RedChannelClient *rcc);
 
 MainChannelClient *red_client_get_main(RedClient *client);
-// main should be set once before all the other channels are created
-void red_client_set_main(RedClient *client, MainChannelClient *mcc);
 
 /* called when the migration handshake results in seamless migration (dst side).
  * By default we assume semi-seamless */
diff --git a/server/reds.c b/server/reds.c
index b5eccd90..f0173613 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -1902,7 +1902,6 @@ static void reds_handle_main_link(RedsState *reds, RedLinkInfo *link)
     red_channel_capabilities_reset(&caps);
     spice_debug("NEW Client %p mcc %p connect-id %d", client, mcc, connection_id);
     g_free(link_mess);
-    red_client_set_main(client, mcc);
 
     if (reds->vdagent) {
         if (mig_target) {
diff --git a/server/tests/test-channel.c b/server/tests/test-channel.c
index 372c8d79..5923d314 100644
--- a/server/tests/test-channel.c
+++ b/server/tests/test-channel.c
@@ -289,7 +289,6 @@ static void channel_loop(void)
     mcc = main_channel_link(main_channel, client, create_dummy_stream(server, NULL),
                             0, FALSE, &caps);
     g_assert_nonnull(mcc);
-    red_client_set_main(client, mcc);
 
     // inject a trace into the core interface to count the events
     SpiceCoreInterfaceInternal *server_core = reds_get_core_interface(server);
diff --git a/server/tests/test-smartcard.c b/server/tests/test-smartcard.c
index c1e4786d..f102452f 100644
--- a/server/tests/test-smartcard.c
+++ b/server/tests/test-smartcard.c
@@ -337,7 +337,6 @@ static void test_smartcard(TestFixture *fixture, gconstpointer user_data)
     mcc = main_channel_link(main_channel, client, create_dummy_stream(server, NULL),
                             0, FALSE, &caps);
     g_assert_nonnull(mcc);
-    red_client_set_main(client, mcc);
 
     // create our testing RedChannelClient
     red_channel_connect(channel, client, create_dummy_stream(server, &client_socket),
commit 59be4f19c46cbeab0b8f405816b7bc4afe253187
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Thu Aug 24 21:43:18 2017 +0100

    red-client: Make sure MainChannelClient is freed as last
    
    MainChannelClient is used by other clients to store some data
    so should not disappear if other clients are still present.
    Keep a owning reference to it and release after RedClient is
    released.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

diff --git a/server/red-client.c b/server/red-client.c
index d73d0f8d..476f6f5b 100644
--- a/server/red-client.c
+++ b/server/red-client.c
@@ -106,6 +106,7 @@ red_client_finalize (GObject *object)
 {
     RedClient *self = RED_CLIENT(object);
 
+    g_clear_object(&self->mcc);
     spice_debug("release client=%p", self);
     pthread_mutex_destroy(&self->lock);
 
@@ -313,7 +314,8 @@ MainChannelClient *red_client_get_main(RedClient *client)
 
 void red_client_set_main(RedClient *client, MainChannelClient *mcc)
 {
-    client->mcc = mcc;
+    spice_assert(client->mcc == NULL);
+    client->mcc = g_object_ref(mcc);
 }
 
 void red_client_semi_seamless_migrate_complete(RedClient *client)
commit 2e1684eb66ef3610594007d190380abf63a82b0e
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Wed Aug 30 13:36:17 2017 +0100

    red-client: Avoid stale channel client after disconnection
    
    Disconnecting a single channel from the client caused the server to
    keep a stale channel client (RedChannelClient object) till the client
    (RedClient object) entirely disconnected, that is the channel client
    is disconnected but still in the list preventing new connections.
    Calling red_client_remove_channel from red_channel_client_disconnect
    fixes this last issue.
    An issue was that was not clear how the ownership were managed. When
    red_client_destroy was called red_channel_client_destroy was called
    which freed the RedChannelClient object so this should imply
    ownership.
    However same red_channel_client_destroy call was attempted by
    RedChannel using its list (red_channel_destroy). Basically the two
    pointers (the one from the channel and the one from the client) were
    considered as one ownership. To avoid the confusion now the client
    list always decrement the counter.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

diff --git a/server/dcc.c b/server/dcc.c
index 538f1f51..ba98331d 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -575,6 +575,7 @@ void dcc_start(DisplayChannelClient *dcc)
     if (!display_channel_client_wait_for_init(dcc))
         return;
 
+    g_object_ref(dcc);
     red_channel_client_ack_zero_messages_window(rcc);
     if (display->priv->surfaces[0].context.canvas) {
         display_channel_current_flush(display, 0);
@@ -591,6 +592,7 @@ void dcc_start(DisplayChannelClient *dcc)
         red_channel_client_pipe_add(rcc, dcc_gl_scanout_item_new(rcc, NULL, 0));
         dcc_push_monitors_config(dcc);
     }
+    g_object_unref(dcc);
 }
 
 static void dcc_destroy_stream_agents(DisplayChannelClient *dcc)
diff --git a/server/red-channel-client.c b/server/red-channel-client.c
index beb3a9a4..0b4096cd 100644
--- a/server/red-channel-client.c
+++ b/server/red-channel-client.c
@@ -678,6 +678,8 @@ static void red_channel_client_ping_timer(void *opaque)
 {
     RedChannelClient *rcc = opaque;
 
+    g_object_ref(rcc);
+
     spice_assert(rcc->priv->latency_monitor.state == PING_STATE_TIMER);
     red_channel_client_cancel_ping_timer(rcc);
 
@@ -692,11 +694,13 @@ static void red_channel_client_ping_timer(void *opaque)
     if (so_unsent_size > 0) {
         /* tcp send buffer is still occupied. rescheduling ping */
         red_channel_client_start_ping_timer(rcc, PING_TEST_IDLE_NET_TIMEOUT_MS);
+        g_object_unref(rcc);
         return;
     }
 #endif /* ifdef HAVE_LINUX_SOCKIOS_H */
     /* More portable alternative code path (less accurate but avoids bogus ioctls)*/
     red_channel_client_push_ping(rcc);
+    g_object_unref(rcc);
 }
 
 static inline int red_channel_client_waiting_for_ack(RedChannelClient *rcc)
@@ -728,6 +732,8 @@ static void red_channel_client_connectivity_timer(void *opaque)
     RedChannelClientConnectivityMonitor *monitor = &rcc->priv->connectivity_monitor;
     int is_alive = TRUE;
 
+    g_object_ref(rcc);
+
     if (monitor->state == CONNECTIVITY_STATE_BLOCKED) {
         if (!monitor->received_bytes && !monitor->sent_bytes) {
             if (!red_channel_client_is_blocked(rcc) && !red_channel_client_waiting_for_ack(rcc)) {
@@ -766,6 +772,7 @@ static void red_channel_client_connectivity_timer(void *opaque)
                             rcc, monitor->timeout);
         red_channel_client_disconnect(rcc);
     }
+    g_object_unref(rcc);
 }
 
 void red_channel_client_start_connectivity_monitoring(RedChannelClient *rcc, uint32_t timeout_ms)
@@ -1026,8 +1033,6 @@ void red_channel_client_destroy(RedChannelClient *rcc)
 {
     rcc->priv->destroying = TRUE;
     red_channel_client_disconnect(rcc);
-    red_client_remove_channel(rcc);
-    g_object_unref(rcc);
 }
 
 void red_channel_client_shutdown(RedChannelClient *rcc)
@@ -1736,6 +1741,10 @@ void red_channel_client_disconnect(RedChannelClient *rcc)
 
     red_channel_remove_client(channel, rcc);
     red_channel_client_on_disconnect(rcc);
+    // remove client from RedClient
+    // NOTE this may trigger the free of the object, if we are in a watch/timer
+    // we should make sure we keep a reference
+    red_client_remove_channel(rcc);
 }
 
 gboolean red_channel_client_is_blocked(RedChannelClient *rcc)
diff --git a/server/red-client.c b/server/red-client.c
index 019da5a2..d73d0f8d 100644
--- a/server/red-client.c
+++ b/server/red-client.c
@@ -234,6 +234,7 @@ void red_client_destroy(RedClient *client)
         spice_assert(red_channel_client_no_item_being_sent(rcc));
 
         red_channel_client_destroy(rcc);
+        g_object_unref(rcc);
         pthread_mutex_lock(&client->lock);
     }
     pthread_mutex_unlock(&client->lock);
@@ -347,8 +348,14 @@ void red_client_remove_channel(RedChannelClient *rcc)
 {
     RedClient *client = red_channel_client_get_client(rcc);
     pthread_mutex_lock(&client->lock);
-    client->channels = g_list_remove(client->channels, rcc);
+    GList *link = g_list_find(client->channels, rcc);
+    if (link) {
+        client->channels = g_list_delete_link(client->channels, link);
+    }
     pthread_mutex_unlock(&client->lock);
+    if (link) {
+        g_object_unref(rcc);
+    }
 }
 
 /* returns TRUE If all channels are finished migrating, FALSE otherwise */
commit f4a387f60d1f7d7c919e28e23f18ab2664e9c179
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Wed Aug 30 13:18:51 2017 +0100

    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 at redhat.com>

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);
 }