[spice-server,3/4] red-client: Avoid stale channel client after disconnection

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

Details

Message ID 20170830125128.9453-3-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.
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@redhat.com>
---
 server/dcc.c                |  2 ++
 server/red-channel-client.c | 12 ++++++++++--
 server/red-client.c         | 10 +++++++++-
 3 files changed, 21 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

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 */

Comments

On Wed, Aug 30, 2017 at 01:51:27PM +0100, Frediano Ziglio wrote:
> Disconnecting a single channel from the client caused the server to
> keep a stale channel client till the client entirely disconnected.

"stale" as in "the channel client is disconnected but still in the
list"? or something else?

> Calling red_client_remove_channel from red_channel_client_disconnect
> fix this last issue.

"fixes"

> 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

Let's end the sentence here so that the reader can breath :)

> 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.

Not fully clear about what you mean here, as this patch is not changing
red-channel.c.

Christophe