[Spice-devel] [PATCH spice-server 1/8] red_channel: prevent adding and pushing pipe items after a channel_client has diconnected

[Alon Levy]

On Fri, 2013-07-26 at 14:08 -0400, Yonit Halperin wrote:

ACK series, looks good.

> Fixes: leaks of pipe items & "red_client_destroy: assertion `rcc->send_data.size == 0'"
> 
> red_channel_disconnect clears the pipe. It is called only once. After,
> it was called, not items should be added to the pipe.
> 
> An example of when this assert can occur:
> on_new_cursor_channel (red_worker.c), pushes 2 pipe items.
> When it pushes the first pipe item, if the client has disconnected,
> it can hit a socket error, and then, red_channel_client_disconnect is called.
> The second call to adding a pipe item, will add the item to
> the pipe. red_channel_client_pipe_add_type also calls
> red_channel_client_push, which will update the send_data.size.
> Then, the push will also hit a socket error, but red_channel_client_disconnect
> won't clear the pending pipe item again, since it was already called.
> When red_client_destory is called, we hit assertion `rcc->send_data.size
> == 0'.
> Note that if a pipe item is added to the pipe after
> red_channel_client_disconnect was called, but without pushing it,
> we should hit "spice_assert(rcc->pipe_size == 0)".
> ---
>  server/red_channel.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/server/red_channel.c b/server/red_channel.c
> index 31c991b..a35da9b 100644
> --- a/server/red_channel.c
> +++ b/server/red_channel.c
> @@ -1524,9 +1524,23 @@ void red_channel_pipe_item_init(RedChannel *channel, PipeItem *item, int type)
>      item->type = type;
>  }
>  
> -void red_channel_client_pipe_add(RedChannelClient *rcc, PipeItem *item)
> +static inline int validate_pipe_add(RedChannelClient *rcc, PipeItem *item)
>  {
>      spice_assert(rcc && item);
> +    if (SPICE_UNLIKELY(!red_channel_client_is_connected(rcc))) {
> +        spice_debug("rcc is disconnected %p", rcc);
> +        red_channel_client_release_item(rcc, item, FALSE);
> +        return FALSE;
> +    }
> +    return TRUE;
> +}
> +
> +void red_channel_client_pipe_add(RedChannelClient *rcc, PipeItem *item)
> +{
> +
> +    if (!validate_pipe_add(rcc, item)) {
> +        return;
> +    }
>      rcc->pipe_size++;
>      ring_add(&rcc->pipe, &item->link);
>  }
> @@ -1540,10 +1554,10 @@ void red_channel_client_pipe_add_push(RedChannelClient *rcc, PipeItem *item)
>  void red_channel_client_pipe_add_after(RedChannelClient *rcc,
>                                         PipeItem *item, PipeItem *pos)
>  {
> -    spice_assert(rcc);
>      spice_assert(pos);
> -    spice_assert(item);
> -
> +    if (!validate_pipe_add(rcc, item)) {
> +        return;
> +    }
>      rcc->pipe_size++;
>      ring_add_after(&item->link, &pos->link);
>  }
> @@ -1557,14 +1571,18 @@ int red_channel_client_pipe_item_is_linked(RedChannelClient *rcc,
>  void red_channel_client_pipe_add_tail_no_push(RedChannelClient *rcc,
>                                                PipeItem *item)
>  {
> -    spice_assert(rcc);
> +    if (!validate_pipe_add(rcc, item)) {
> +        return;
> +    }
>      rcc->pipe_size++;
>      ring_add_before(&item->link, &rcc->pipe);
>  }
>  
>  void red_channel_client_pipe_add_tail(RedChannelClient *rcc, PipeItem *item)
>  {
> -    spice_assert(rcc);
> +    if (!validate_pipe_add(rcc, item)) {
> +        return;
> +    }
>      rcc->pipe_size++;
>      ring_add_before(&item->link, &rcc->pipe);
>      red_channel_client_push(rcc);