Re: [Qemu-devel] [PATCH 18/34] qcow2: Fix memory leak in qcow2_update_options() error path

[Max Reitz]

On 13.05.2015 14:02, Kevin Wolf wrote:
> Am 13.05.2015 um 13:52 hat Max Reitz geschrieben:
> > On 08.05.2015 19:21, Kevin Wolf wrote:
> > > Signed-off-by: Kevin Wolf <address@hidden>
> > > ---
> > >   block/qcow2.c | 12 ++++++++++--
> > >   1 file changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/block/qcow2.c b/block/qcow2.c
> > > index abe22f3..84d6e0f 100644
> > > --- a/block/qcow2.c
> > > +++ b/block/qcow2.c
> > > @@ -546,8 +546,8 @@ static int qcow2_update_options(BlockDriverState *bs, QDict 
> > > *options,
> > >       const char *opt_overlap_check, *opt_overlap_check_template;
> > >       int overlap_check_template = 0;
> > >       uint64_t l2_cache_size, refcount_cache_size;
> > > -    Qcow2Cache* l2_table_cache;
> > > -    Qcow2Cache* refcount_block_cache;
> > > +    Qcow2Cache* l2_table_cache = NULL;
> > > +    Qcow2Cache* refcount_block_cache = NULL;
> > >       bool use_lazy_refcounts;
> > >       int i;
> > >       Error *local_err = NULL;
> > > @@ -670,6 +670,14 @@ static int qcow2_update_options(BlockDriverState *bs, 
> > > QDict *options,
> > >       ret = 0;
> > >   fail:
> > > +    if (ret < 0) {
> > > +        if (l2_table_cache) {
> > > +            qcow2_cache_destroy(bs, l2_table_cache);
> > > +        }
> > > +        if (refcount_block_cache) {
> > > +            qcow2_cache_destroy(bs, refcount_block_cache);
> > > +        }
> > > +    }
> > >       qemu_opts_del(opts);
> > >       opts = NULL;
> > Why don't you squash this into patch 17 (I guess there is a reason,
> > but I fail to see it)?
> It's a preexisting bug and its fix is unrelated to any of the
> refactoring or preparation for reopen. So I think it deserves its own
> commit, and if it doesn't, it's not clear to me why it should belong to
> patch 17 of all patches.

Because patch 17 is introducing the {l2_table,refcount_block}_cache as 
local variables.

> > Also, I think it might make sense to either set
> > {l2_table,refcount_block}_cache to NULL once they have been moved to
> > s->{l2_table,refcount_block}_cache, or, even better, exchange them
> > with their respective field in *s. Thus, you could drop the "if (ret
> > < 0)", I think I'd find the code easier to read, and with the
> > transation, we'd even be safe if the options had been set before and
> > are now to be updated.
> The if (ret < 0) disappears shortly after this patch when this is moved
> into the abort part of a transaction.

Right, I just saw this. :-)

Max