[libvirt] [PATCH] qemu_driver: don't leak a virDomainDeviceDef buffer
At first I was going to call virDomainDeviceDefFree only "if (dev)", but saw that it handles a NULL "dev" just fine, so it's better to skip the test altogether, just as we do for many other free-like functions.
From ea8511d709492f5cdc152a1eaccbccd05f036648 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 18 Jan 2010 16:55:36 +0100 Subject: [PATCH] qemu_driver: don't leak a virDomainDeviceDef buffer
* src/qemu/qemu_driver.c (qemudDomainAttachDevice): Don't leak "dev". --- src/qemu/qemu_driver.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 365921f..1aa8af6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6062,11 +6062,11 @@ cleanup: if (cgroup) virCgroupFree(&cgroup); - if (ret < 0 && dev != NULL) { + if (ret < 0 && dev != NULL) if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0) VIR_WARN0("Fail to restore disk device ownership"); - virDomainDeviceDefFree(dev); - } + virDomainDeviceDefFree(dev); + if (vm) virDomainObjUnlock(vm); qemuDriverUnlock(driver); -- 1.6.6.638.g2bc54
On Mon, Jan 18, 2010 at 04:58:37PM +0100, Jim Meyering wrote:
At first I was going to call virDomainDeviceDefFree only "if (dev)", but saw that it handles a NULL "dev" just fine, so it's better to skip the test altogether, just as we do for many other free-like functions.
From ea8511d709492f5cdc152a1eaccbccd05f036648 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 18 Jan 2010 16:55:36 +0100 Subject: [PATCH] qemu_driver: don't leak a virDomainDeviceDef buffer
* src/qemu/qemu_driver.c (qemudDomainAttachDevice): Don't leak "dev". --- src/qemu/qemu_driver.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 365921f..1aa8af6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6062,11 +6062,11 @@ cleanup: if (cgroup) virCgroupFree(&cgroup);
- if (ret < 0 && dev != NULL) { + if (ret < 0 && dev != NULL) if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0) VIR_WARN0("Fail to restore disk device ownership"); - virDomainDeviceDefFree(dev); - } + virDomainDeviceDefFree(dev); + if (vm) virDomainObjUnlock(vm); qemuDriverUnlock(driver); --
Your fix is reasonable, but can you leave it out of GIT, because there are a huge number of other related problems in this bit code which I've got patches almost ready for. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
2010/1/18 Jim Meyering <jim@meyering.net>:
At first I was going to call virDomainDeviceDefFree only "if (dev)", but saw that it handles a NULL "dev" just fine, so it's better to skip the test altogether, just as we do for many other free-like functions.
From ea8511d709492f5cdc152a1eaccbccd05f036648 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 18 Jan 2010 16:55:36 +0100 Subject: [PATCH] qemu_driver: don't leak a virDomainDeviceDef buffer
* src/qemu/qemu_driver.c (qemudDomainAttachDevice): Don't leak "dev". --- src/qemu/qemu_driver.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 365921f..1aa8af6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6062,11 +6062,11 @@ cleanup: if (cgroup) virCgroupFree(&cgroup);
- if (ret < 0 && dev != NULL) { + if (ret < 0 && dev != NULL) if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0) VIR_WARN0("Fail to restore disk device ownership"); - virDomainDeviceDefFree(dev); - } + virDomainDeviceDefFree(dev); + if (vm) virDomainObjUnlock(vm); qemuDriverUnlock(driver); -- 1.6.6.638.g2bc54
NACK. This will probably result in a segfault because you are freeing memory that is still in use. Yes the toplevel dev leaks here, but for example qemudDomainAttachNetDevice some lines above takes parts from the dev struct an assigns them to other structs _without_ copying them. I found this leak some time ago too, but gave up on fixing it as I noticed how entangled this code is. Matthias
Matthias Bolte wrote:
2010/1/18 Jim Meyering <jim@meyering.net>:
At first I was going to call virDomainDeviceDefFree only "if (dev)", but saw that it handles a NULL "dev" just fine, so it's better to skip the test altogether, just as we do for many other free-like functions.
From ea8511d709492f5cdc152a1eaccbccd05f036648 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 18 Jan 2010 16:55:36 +0100 Subject: [PATCH] qemu_driver: don't leak a virDomainDeviceDef buffer
* src/qemu/qemu_driver.c (qemudDomainAttachDevice): Don't leak "dev". ...
NACK. This will probably result in a segfault because you are freeing memory that is still in use.
Yes the toplevel dev leaks here, but for example qemudDomainAttachNetDevice some lines above takes parts from the dev struct an assigns them to other structs _without_ copying them.
Thanks. That is nastily unintuitive and sounds a lot like a bug. I hope it's on the list of things to be fixed by Dan's patch.
I found this leak some time ago too, but gave up on fixing it as I noticed how entangled this code is.
participants (3)
-
Daniel P. Berrange -
Jim Meyering -
Matthias Bolte