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

[Christophe Fergeau]

On Mon, Sep 04, 2017 at 10:35:59AM -0400, Frediano Ziglio wrote:
> > 
> > On Mon, Sep 04, 2017 at 09:15:55AM -0400, Frediano Ziglio wrote:
> > > > 
> > > > On Mon, Sep 04, 2017 at 07:35:46AM -0400, Frediano Ziglio wrote:
> > > > > > 
> > > > > > 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.
> > > > 
> > > > Yes, _UPDATE_ASYNC will call display_channel_update() and uses the
> > > > rectangle list.
> > > > _UPDATE (not async) also calls display_channel_update(), gets a
> > > > rectangle list, but does not make use of it as far as I can tell.
> > > > So I'm wondering if it would be enough to call display_channel_draw()
> > > > rather than display_channel_update() in the sync case.
> > > > 
> > > 
> > > Now I got it. I tried to inline the function however is
> > > msg->clear_dirty_region
> > > is 1 we need to access to the surface which is private to DisplayChannel.
> > > 
> > > I think if we pass a dummy single rectangle display_channel_update won't
> > > allocate any memory. Does it sound reasonable?
> > 
> > Maybe this:
> > 
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index c745790f8..bcdc32a66 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -2031,12 +2031,16 @@ void display_channel_update(DisplayChannel *display,
> >      display_channel_draw(display, &rect, surface_id);
> >  
> >      surface = &display->priv->surfaces[surface_id];
> > -    if (*qxl_dirty_rects == NULL) {
> > -        *num_dirty_rects =
> > pixman_region32_n_rects(&surface->draw_dirty_region);
> > -        *qxl_dirty_rects = spice_new0(QXLRect, *num_dirty_rects);
> > +
> > +    if (qxl_dirty_rects != NULL) {
> > +        if (*qxl_dirty_rects == NULL) {
> > +            *num_dirty_rects =
> > pixman_region32_n_rects(&surface->draw_dirty_region);
> > +            *qxl_dirty_rects = spice_new0(QXLRect, *num_dirty_rects);
> > +        }
> > +
> > +        region_to_qxlrects(&surface->draw_dirty_region, *qxl_dirty_rects,
> > *num_dirty_rects);
> >      }
> >  
> > -    region_to_qxlrects(&surface->draw_dirty_region, *qxl_dirty_rects,
> > *num_dirty_rects);
> >      if (clear_dirty)
> >          region_clear(&surface->draw_dirty_region);
> >  }
> > diff --git a/server/red-worker.c b/server/red-worker.c
> > index 1e1eb7052..3291f3644 100644
> > --- a/server/red-worker.c
> > +++ b/server/red-worker.c
> > @@ -443,7 +443,7 @@ static void handle_dev_update(void *opaque, void
> > *payload)
> >      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);
> > +                           NULL, NULL);
> 
> I think this should be
> 
>    msg->qxl_dirty_rects ? &msg->qxl_dirty_rects : NULL, &msg->num_dirty_rects);
> 
> yes, looks like complicated but we need to write into the passed array.

Ah yeah, something might expect it was modified. Oh well, your initial
suggestion is probably good then, though I'd explicitly check for a NULL
msg->qxl_dirty_rects I think rather than msg->qxl_dirty_rects !=
qxl_dirty_rects.

Christophe