9:58 a.m.
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(a)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
10:06 a.m.
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(a)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 :|
11:14 a.m.
2010/1/18 Jim Meyering <jim(a)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(a)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
11:28 a.m.
Matthias Bolte wrote:
2010/1/18 Jim Meyering <jim(a)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(a)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.
5422
days inactive
5422
days old
4 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Jim Meyering -
Matthias Bolte