>
> 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@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
> > >
> >
> > 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?
> > Is not an output variable, is also an input one we need the
> > value passed by red_qxl_update_area.
>
> Do you mean red_qxl_update_area_complete() in handle_dev_update_async()?
> or something else?
>
Yes, the new value is passed back so in this case we need it.
> Christophe
>
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel