On Wed, Aug 30, 2017 at 01:51:26PM +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.
I'd expect a detailed description (ie at least function names) of the
potential race in the log.
>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> server/red-client.c | 29 ++++++++++++++++++++++++-----
> 1 file changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/server/red-client.c b/server/red-client.c
> index ddfc5400..800b1a5d 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,23 +199,45 @@ void red_client_destroy(RedClient *client)
> client->thread_id,
> pthread_self());
> }
> - red_client_set_disconnecting(client);
> - 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
"This makes sure that we won't try to add new RedChannelClient instances
to the RedClient::channels list while iterating it"?
> + client->disconnecting = TRUE;
> + while (client->channels) {
> RedChannel *channel;
> + RedChannelClient *rcc = client->channels->data;
> +
> + // Remove the list to the RedChannelClient we are processing
"Remove the RedChannelClient we are processing from the list"?
> + // 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)
I'd expect some details here on in the commit log about the deadlock
scenario.
> + 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);
This call is
red_channel_client_set_destroying(rcc);
red_channel_client_disconnect(rcc);
both of which have been done earlier in this function.
Christophe
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel