| Message ID | 1401125835-21765-10-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
The Monday 26 May 2014 à 19:37:10 (+0200), Markus Armbruster wrote : > Introduced in commit f298d07. Spotted by Coverity. > > Signed-off-by: Markus Armbruster <armbru@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; > 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. Best regards Benoît
Benoît Canet <benoit.canet@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 <armbru@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; >> 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?
Benoît Canet <benoit.canet@irqsave.net> writes: > The Tuesday 27 May 2014 à 18:13:12 (+0200), Markus Armbruster wrote : >> Benoît Canet <benoit.canet@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 <armbru@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? >> >> 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?
Benoît Canet <benoit.canet@irqsave.net> writes: > The Tuesday 27 May 2014 à 21:11:45 (+0200), Markus Armbruster wrote : >> Benoît Canet <benoit.canet@irqsave.net> writes: >> >> > The Tuesday 27 May 2014 à 18:13:12 (+0200), Markus Armbruster wrote : >> >> Benoît Canet <benoit.canet@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 <armbru@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? [...]
The Tuesday 27 May 2014 à 18:13:12 (+0200), Markus Armbruster wrote : > Benoît Canet <benoit.canet@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 <armbru@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. > >> 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?
The Tuesday 27 May 2014 à 21:11:45 (+0200), Markus Armbruster wrote : > Benoît Canet <benoit.canet@irqsave.net> writes: > > > The Tuesday 27 May 2014 à 18:13:12 (+0200), Markus Armbruster wrote : > >> Benoît Canet <benoit.canet@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 <armbru@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." Best regards Benoît > > >> >> 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? >
The Tuesday 27 May 2014 à 21:44:15 (+0200), Markus Armbruster wrote : > Benoît Canet <benoit.canet@irqsave.net> writes: > > > The Tuesday 27 May 2014 à 21:11:45 (+0200), Markus Armbruster wrote : > >> Benoît Canet <benoit.canet@irqsave.net> writes: > >> > >> > The Tuesday 27 May 2014 à 18:13:12 (+0200), Markus Armbruster wrote : > >> >> Benoît Canet <benoit.canet@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 <armbru@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. Best regards Benoît > > [...] >
Benoît Canet <benoit.canet@irqsave.net> writes: > The Tuesday 27 May 2014 à 21:44:15 (+0200), Markus Armbruster wrote : >> Benoît Canet <benoit.canet@irqsave.net> writes: >> >> > The Tuesday 27 May 2014 à 21:11:45 (+0200), Markus Armbruster wrote : >> >> Benoît Canet <benoit.canet@irqsave.net> writes: >> >> >> >> > The Tuesday 27 May 2014 à 18:13:12 (+0200), Markus Armbruster wrote : >> >> >> Benoît Canet <benoit.canet@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 <armbru@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!
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; 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; }
Introduced in commit f298d07. Spotted by Coverity. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- blockdev.c | 2 ++ 1 file changed, 2 insertions(+)