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

[Jonathon Jongsma]

On Tue, 2016-09-20 at 08:18 -0400, Frediano Ziglio wrote:
> > 
> > 
> > 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).

Yes, I see now. A bit non-obvious, to be honest, but it works. If we
also allocate the the cmd variable with replay_malloc, we could just
omit the extra g_free(), I think. I'll send a possible patch. Feel free
to use it or not.

Jonathon