Re: [Qemu-stable] [Qemu-devel] [PATCH 09/14] blockdev: Plug memory leak in drive_init()

[Markus Armbruster]

Benoît Canet <address@hidden> writes:

> The Tuesday 27 May 2014 à 18:13:12 (+0200), Markus Armbruster wrote :
>> Benoît Canet <address@hidden> writes:
>> 
>> > The Monday 26 May 2014 à 19:37:10 (+0200), Markus Armbruster wrote :
>> >> Introduced in commit f298d07.  Spotted by Coverity.
>> >> 
>> >> Signed-off-by: Markus Armbruster <address@hidden>
>> >> ---
>> >>  blockdev.c | 2 ++
>> >>  1 file changed, 2 insertions(+)
>> >> 
>> >> diff --git a/blockdev.c b/blockdev.c
>> >> index 6460c70..7ec7d79 100644
>> >> --- a/blockdev.c
>> >> +++ b/blockdev.c
>> >> @@ -941,6 +941,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, 
>> >> BlockInterfaceType block_default_type)
>> >>  
>> >>      /* Actual block device init: Functionality shared with blockdev-add 
>> >> */
>> >>      dinfo = blockdev_init(filename, bs_opts, &local_err);
>> >> +    bs_opts = NULL;
>
> What is the purpose of this line ? I though it was to avoid double unref.

Before this patch, bs_opts gets leaked on any path from its qdict_new()
that doesn't go through blockdev_init().

The new line below frees it, but without the line above, it would free
it a second time on paths that go through blockdev_init().

Clear now?

>> >>      if (dinfo == NULL) {
>> >>          if (local_err) {
>> >>              qerror_report_err(local_err);
>> >> @@ -978,6 +979,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, 
>> >> BlockInterfaceType block_default_type)
>> >>  
>> >>  fail:
>> >>      qemu_opts_del(legacy_opts);
>> >> +    QDECREF(bs_opts);
>> >>      return dinfo;
>> >>  }
>> >>  
>> >> -- 
>> >> 1.9.3
>> >> 
>> >> 
>> >
>> > This commits seems to fix two thing a leak and a double free.
>> > Maybe reflecting this on the commit message would make the bs_opts =
>> > NULL; clearer.
>> 
>> I'm blind.  Can you explain the double free to me?