[Spice-devel] [PATCH spice-server v3] red-worker: Fix leak processing update commands

[Christophe Fergeau]

On Mon, Sep 04, 2017 at 05:28:29PM +0100, Frediano Ziglio wrote:
> Is possible to have a leak processing update commands if
> the update command is synchronous and the rectangle list
> is empty. Note that Qemu always pass an empty list.
> 
> If the list is empty display_channel_update fill the list.
> This is used to send back the list in case of asynchronous
> requests. But in handle_dev_update_async (the callback that
> handle the asynchronous case) the list is correctly freed.
> 
> This was discovered by accident looking at the code.
> 
> Reproduced with a Windows recording file using GCC address
> sanitizer and this patch to spice-server-replay:
> 
>   --- a/server/red-replay-qxl.c
>   +++ b/server/red-replay-qxl.c
>   @@ -1280,7 +1280,13 @@ static void replay_handle_dev_input(QXLWorker *worker, SpiceReplay *replay,
>            replay->created_primary = FALSE;
>            worker->destroy_surfaces(worker);
>            break;
>   -    case RED_WORKER_MESSAGE_UPDATE:
>   +    case RED_WORKER_MESSAGE_UPDATE: {
>   +        static uint8_t count = 0;
>   +        QXLRect dummy;
>   +        QXLRect update = { 0, 0, 100, 100 };
>   +        count ^= 1;
>   +        worker->update_area(worker, 0, &update, count ? &dummy : NULL, count ? 1 : 0, 0);
>   +        } break;
>            // XXX do anything? we record the correct bitmaps already.
>        case RED_WORKER_MESSAGE_DISPLAY_CONNECT:
>            // we want to ignore this one - it is sent on client connection, we
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/red-worker.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> Changes since v2:
> - better check for leak (thanks Christophe);
> - more complete test patch in comment.
> 
> Changes since v1:
> - indented patch in comment to avoid am problems.
> 
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 4d8566be..675c232e 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -437,13 +437,17 @@ static void handle_dev_update(void *opaque, void *payload)
>  {
>      RedWorker *worker = opaque;
>      RedWorkerMessageUpdate *msg = payload;
> +    QXLRect *qxl_dirty_rects = msg->qxl_dirty_rects;
>  
>      spice_return_if_fail(worker->running);
>  
>      flush_display_commands(worker);
>      display_channel_update(worker->display_channel,
>                             msg->surface_id, msg->qxl_area, msg->clear_dirty_region,
> -                           &msg->qxl_dirty_rects, &msg->num_dirty_rects);
> +                           &qxl_dirty_rects, &msg->num_dirty_rects);
> +    if (msg->qxl_dirty_rects == NULL) {
> +        free(qxl_dirty_rects);
> +    }

Acked-by: Christophe Fergeau <cfergeau at redhat.com>