[Spice-devel] [PATCH spice-server 3/4] 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 till the client entirely disconnected.
Calling red_client_remove_channel from red_channel_client_disconnect
fix 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. 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 | 12 ++++++++++--
 server/red-client.c         | 10 +++++++++-
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/server/dcc.c b/server/dcc.c
index 2778bb88..769d13b1 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -574,6 +574,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);
@@ -590,6 +591,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 145ba43f..e1f4faa5 100644
--- a/server/red-channel-client.c
+++ b/server/red-channel-client.c
@@ -681,6 +681,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);
 
@@ -703,6 +705,7 @@ static void red_channel_client_ping_timer(void *opaque)
     /* More portable alternative code path (less accurate but avoids bogus ioctls)*/
     red_channel_client_push_ping(rcc);
 #endif /* ifdef HAVE_LINUX_SOCKIOS_H */
+    g_object_unref(rcc);
 }
 
 static inline int red_channel_client_waiting_for_ack(RedChannelClient *rcc)
@@ -734,6 +737,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)) {
@@ -778,6 +783,7 @@ static void red_channel_client_connectivity_timer(void *opaque)
                       rcc, type, id, monitor->timeout);
         red_channel_client_disconnect(rcc);
     }
+    g_object_unref(rcc);
 }
 
 void red_channel_client_start_connectivity_monitoring(RedChannelClient *rcc, uint32_t timeout_ms)
@@ -996,8 +1002,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)
@@ -1692,6 +1696,10 @@ void red_channel_client_disconnect(RedChannelClient *rcc)
     }
     red_channel_remove_client(channel, rcc);
     red_channel_on_disconnect(channel, 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 800b1a5d..36577dab 100644
--- a/server/red-client.c
+++ b/server/red-client.c
@@ -235,6 +235,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);
@@ -346,10 +347,17 @@ int red_client_during_migrate_at_target(RedClient *client)
 
 void red_client_remove_channel(RedChannelClient *rcc)
 {
+    GList *link;
     RedClient *client = red_channel_client_get_client(rcc);
     pthread_mutex_lock(&client->lock);
-    client->channels = g_list_remove(client->channels, rcc);
+    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.13.5