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

[Markus Armbruster]

Benoît Canet <address@hidden> writes:

> The Tuesday 27 May 2014 à 21:11:45 (+0200), Markus Armbruster wrote :
>> 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?
>
> Clear from the start it fixes a potential double free.
> "This commits seems to fix two thing a leak and a double free."

Well, before the patch, the leak exists, but there is no double-free.

The patch fixes only one thing: the leak.  It takes care not to break
things by freeing when it shouldn't.

Do you still think the commit message should be amended?  How?

[...]