[Spice-devel] [PATCH spice-server v2 3/7] red-client: Avoid stale channel client after disconnection

[Frediano Ziglio]

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>
---
 server/dcc.c                |  2 ++
 server/red-channel-client.c | 13 +++++++++++--
 server/red-client.c         |  9 ++++++++-
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/server/dcc.c b/server/dcc.c
index 538f1f51a..ba98331df 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 c3ad68183..a4a57ce32 100644
--- a/server/red-channel-client.c
+++ b/server/red-channel-client.c
@@ -686,6 +686,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);
 
@@ -700,11 +702,13 @@ static void red_channel_client_ping_timer(void *opaque)
     if (so_unsent_size > 0) {
         /* tcp snd 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)
@@ -736,6 +740,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)) {
@@ -776,6 +782,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)
@@ -1039,8 +1046,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)
@@ -1753,6 +1758,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 019da5a2b..d73d0f8d0 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 */
-- 
2.21.0