[Spice-devel] [spice-server v1] char-device: fix mismatch of client tokens

[Frediano Ziglio]

> 
> As the tokens counter were not being reset you could enter in a
> situation where client thinks it has more tokens then server which
> would eventually lead to client's disconnection from 0c5eca97f16ec6
> onwards (before it was crashing).
> 
> It is easy to check the above situation if you track the amount of
> tokens you have in the client and simply kill and restart the agent
> while doing some file transfer: the client could reach more then 13
> tokens which should not really be possible.
> 
> Based on patch from Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/char-device.c |  6 ++++++
>  server/reds.c        | 17 +++++------------
>  2 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/server/char-device.c b/server/char-device.c
> index cb35aa2..957fb26 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -851,6 +851,12 @@ void red_char_device_reset(RedCharDevice *dev)
>          dev_client->num_send_tokens +=
>          g_queue_get_length(dev_client->send_queue);
>          g_queue_foreach(dev_client->send_queue, (GFunc)red_pipe_item_unref,
>          NULL);
>          g_queue_clear(dev_client->send_queue);
> +
> +        /* If device is reset, we must reset the tokens counters as well as
> we
> +         * don't hold any data from client and upon agent's reconnection we
> send
> +         * SPICE_MSG_MAIN_AGENT_CONNECTED_TOKENS with all free tokens we
> have */
> +        dev_client->num_client_tokens += dev_client->num_client_tokens_free;
> +        dev_client->num_client_tokens_free = 0;
>      }
>      red_char_device_reset_dev_instance(dev, NULL);
>  }
> diff --git a/server/reds.c b/server/reds.c
> index 4fd1d35..27a5957 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -542,23 +542,16 @@ static void reds_reset_vdp(RedsState *reds)
>      dev->priv->write_filter.discard_all = TRUE;
>      dev->priv->client_agent_started = FALSE;
>  
> -    /* resetting and not destroying the dev as a workaround for a bad
> -     * tokens management in the vdagent protocol:
> -     *  The client tokens' are set only once, when the main channel is
> initialized.
> -     *  Instead, it would have been more appropriate to reset them upon
> AGENT_CONNECT.
> +    /*  The client's tokens are set once when the main channel is
> initialized
> +     *  and once upon agent's connection with
> SPICE_MSG_MAIN_AGENT_CONNECTED_TOKENS.
>       *  The client tokens are tracked as part of the RedCharDeviceClient.
>       Thus,
>       *  in order to be backward compatible with the client, we need to track
>       the tokens
>       *  even if the agent is detached. We don't destroy the char_device, and
>       *  instead we just reset it.
> -     *  In addition, there used to be a misshandling of AGENT_TOKENS message
> in spice-gtk: it
> -     *  overrides the amount of tokens, instead of adding the given amount.
> +     *  The tokens are also reset to avoid mismatch in upon agent
> reconnection.
>       */
> -    if (red_channel_test_remote_cap(&reds->main_channel->base,
> -                                    SPICE_MAIN_CAP_AGENT_CONNECTED_TOKENS))
> {
> -        dev->priv->agent_attached = FALSE;
> -    } else {
> -        red_char_device_reset(RED_CHAR_DEVICE(dev));
> -    }
> +    dev->priv->agent_attached = FALSE;
> +    red_char_device_reset(RED_CHAR_DEVICE(dev));
>  
>      sif = spice_char_device_get_interface(reds->vdagent);
>      if (sif->state) {
> --
> 2.5.5
> 
> 

I think this patch is an improve, fix an issue and work with very old clients.

I would say

Acked-by: Frediano Ziglio <fziglio at redhat.com>

Note that this do not mean I consider the patch that introduced all these
issues good. Having an object structure that represent an object (in this
case a Qemu character device) with entirely different life span and
multiplicity is confusing.

Frediano