On Fri, Sep 01, 2017 at 04:05:25PM +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,10 @@ 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: {
> + QXLRect update = { 0, 0, 100, 100 };
> + worker->update_area(worker, 0, &update, NULL, 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
NB: inline patches in commit logs confuse git am :-/
>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> server/red-worker.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 4d8566be..594dec55 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 != qxl_dirty_rects) {
> + free(qxl_dirty_rects);
> + }
Given that the caller (red_qxl_update_area) does not care about the
msg->qxl_dirty_rects/msg->num_dirty_rects, I guess you could always pass
NULL, and update display_channel_update accordingly. Maybe it would even
be enough to call display_channel_draw() here instead?
Christophe
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel