[Spice-devel] [PATCH spice-server 5/5] red-client: Prevent some nasty threading problems disconnecting channels
Frediano Ziglio
fziglio at redhat.com
Thu Aug 24 14:37:20 UTC 2017
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.
Another 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.
Another issue was that 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.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
---
server/red-channel-client.c | 11 ++++++++++-
server/red-client.c | 48 ++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 53 insertions(+), 6 deletions(-)
diff --git a/server/red-channel-client.c b/server/red-channel-client.c
index 1162ddca..b236e3bd 100644
--- a/server/red-channel-client.c
+++ b/server/red-channel-client.c
@@ -696,6 +696,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);
@@ -718,6 +720,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)
@@ -749,6 +752,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)) {
@@ -793,6 +798,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)
@@ -1012,7 +1018,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)
@@ -1709,6 +1714,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 3ce09e33..7394bd1f 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,22 +199,46 @@ void red_client_destroy(RedClient *client)
client->thread_id,
pthread_self());
}
- 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);
+ g_object_unref(rcc);
+ pthread_mutex_lock(&client->lock);
}
+ pthread_mutex_unlock(&client->lock);
g_object_unref(client);
}
@@ -254,6 +275,16 @@ gboolean red_client_add_channel(RedClient *client, RedChannelClient *rcc, GError
pthread_mutex_lock(&client->lock);
g_object_get(channel, "channel-type", &type, "id", &id, NULL);
+ if (client->disconnecting) {
+ g_set_error(error,
+ SPICE_SERVER_ERROR,
+ SPICE_SERVER_ERROR_FAILED,
+ "Client %p got disconnected while connecting channel type %d id %d",
+ client, type, id);
+ result = FALSE;
+ goto cleanup;
+ }
+
if (red_client_get_channel(client, type, id)) {
g_set_error(error,
SPICE_SERVER_ERROR,
@@ -316,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_remove_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
More information about the Spice-devel
mailing list