Benoît Canet <benoit.ca...@irqsave.net> writes:
> The Tuesday 27 May 2014 à 21:44:15 (+0200), Markus Armbruster wrote :
>> Benoît Canet <benoit.ca...@irqsave.net> writes:
>>
>> > The Tuesday 27 May 2014 à 21:11:45 (+0200), Markus Armbruster wrote :
>> >> Benoît Canet <benoit.ca...@irqsave.net> writes:
>> >>
>> >> > The Tuesday 27 May 2014 à 18:13:12 (+0200), Markus Armbruster wrote :
>> >> >> Benoît Canet <benoit.ca...@irqsave.net> 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 <arm...@redhat.com>
>> >> >> >> ---
>> >> >> >> 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?
>
> Maybe just says it also take care not to introduce a double free.
Adding this paragraph:
bs_opts is leaked on all paths from its qdev_new() that don't got
through blockdev_init(). Add the missing QDECREF(), and zap bs_opts
after blockdev_init(), so the new QDECREF() does nothing when we go
through blockdev_init().
Thanks!
