On Thu, Aug 24, 2017 at 03:37:20PM +0100, Frediano Ziglio wrote:
> 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.
Just reading the log, I get the impression there are 3 fixes together in
this commit. Given its title ("nasty threading issues"), this is really
not a nice trick to play to the person reviewing the patch :(
Christophe
>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> 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
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel