[Spice-devel] [PATCH v3 1/8] replay: Record allocations in a GList to handle errors

[Frediano Ziglio]

> 
> On Mon, 2016-09-19 at 18:35 +0200, Victor Toso wrote:
> > Hi,
> > 
> > On Fri, Sep 16, 2016 at 12:32:54PM +0100, Frediano Ziglio wrote:
> > > 
> > > Allocations are kept into a GList to be able to free in case some
> > > errors happened.
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  server/red-replay-qxl.c | 68
> > > ++++++++++++++++++++++++++++++++++++++++---------
> > >  1 file changed, 56 insertions(+), 12 deletions(-)
> > > 
...
> > > @@ -1234,9 +1257,28 @@ SPICE_GNUC_VISIBLE QXLCommandExt*
> > > spice_replay_next_cmd(SpiceReplay *replay,
> > >          info->id = (uintptr_t)cmd;
> > >      }
> > > 
> > > +    /* all buffer allocated will be used by the caller but
> > > +     * free the list of buffer allocated to avoid to free on next
> > > calls */
> > > +    if (replay->allocated) {
> > > +        g_list_free(replay->allocated);
> > > +        replay->allocated = NULL;
> > > +    }
> > > +
> > >      replay->counter++;
> > > 
> > >      return cmd;
> > > +
> > > +error:
> > > +    /* if there were some allocation during the read free all
> > > +     * buffers allocated to avoid leaks */
> > > +    if (replay->allocated) {
> > > +        g_list_free_full(replay->allocated, free);
> > > +        replay->allocated = NULL;
> > > +    }
> > > +    if (cmd) {
> > > +        g_free(cmd);
> 
> Shouldn't we use spice_replay_free_cmd() here?
> 
> 
> > > +    }
> > > +    return NULL;
> > >  }
> > > 

No, this would result in a double free.
The buffers are in an unstable state so you could free data pointer not initialized
(that's the reason of the list).

Frediano