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

[Frediano Ziglio]

> 
> 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
> 

I cannot just check for NULL, Qemu could decide to pass a not NULL pointer,
in this can we can't free the memory used by Qemu without causing a potential
invalid free (the pointer could be from stack or allocated with another
allocator). Actually I'm not 100% sure in all cases we get a NULL but
supposing NULL would be a API breakage.

Frediano