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

[Frediano Ziglio]

> 
> 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 at redhat.com>
> > ---
> >  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
> 

In case of async message (used by Linux drivers) display_channel_draw
fills the rectangle list and the list is passed back to Qemu.
Is not an output variable, is also an input one we need the
value passed by red_qxl_update_area.
Maybe I didn't get what you suggested.

Frediano