[libvirt] [PATCH] revert commit baade4cd2bf partly
This is not a memory leak. See line 8029 and 8030 of qemu_driver.c. To ensure this, I tested twice following these steps: 1. set bandwidth lively (--live) 2. query bandwidth (--live) 3. set bandwidth lively (--live) The first time libvirtd crashed at step 2. The second time on step 2 I got strage data, and libvirtd crashed at step 3. --- src/qemu/qemu_driver.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e93fe87..4be36f5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7864,7 +7864,7 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, virDomainDefPtr persistentDef = NULL; int ret = -1; virDomainNetDefPtr net = NULL, persistentNet = NULL; - virNetDevBandwidthPtr bandwidth = NULL, newBandwidth = NULL; + virNetDevBandwidthPtr bandwidth = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -7986,6 +7986,8 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_LIVE) { + virNetDevBandwidthPtr newBandwidth = NULL; + if (VIR_ALLOC(newBandwidth) < 0) { virReportOOMError(); goto cleanup; @@ -8053,7 +8055,6 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, ret = 0; cleanup: virNetDevBandwidthFree(bandwidth); - virNetDevBandwidthFree(newBandwidth); virCgroupFree(&group); if (vm) virDomainObjUnlock(vm); -- 1.7.4.4
On Tue, Jan 03, 2012 at 11:14:15AM +0800, Hu Tao wrote:
This is not a memory leak. See line 8029 and 8030 of qemu_driver.c.
To ensure this, I tested twice following these steps:
1. set bandwidth lively (--live) 2. query bandwidth (--live) 3. set bandwidth lively (--live)
The first time libvirtd crashed at step 2. The second time on step 2 I got strage data, and libvirtd crashed at step 3.
--- src/qemu/qemu_driver.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e93fe87..4be36f5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7864,7 +7864,7 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, virDomainDefPtr persistentDef = NULL; int ret = -1; virDomainNetDefPtr net = NULL, persistentNet = NULL; - virNetDevBandwidthPtr bandwidth = NULL, newBandwidth = NULL; + virNetDevBandwidthPtr bandwidth = NULL;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -7986,6 +7986,8 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, }
if (flags & VIR_DOMAIN_AFFECT_LIVE) { + virNetDevBandwidthPtr newBandwidth = NULL; + if (VIR_ALLOC(newBandwidth) < 0) { virReportOOMError(); goto cleanup; @@ -8053,7 +8055,6 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, ret = 0; cleanup: virNetDevBandwidthFree(bandwidth); - virNetDevBandwidthFree(newBandwidth); virCgroupFree(&group); if (vm) virDomainObjUnlock(vm); -- 1.7.4.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
ping. -- Thanks, Hu Tao
On 01/05/2012 09:13 AM, Hu Tao wrote:
On Tue, Jan 03, 2012 at 11:14:15AM +0800, Hu Tao wrote:
This is not a memory leak. See line 8029 and 8030 of qemu_driver.c.
To ensure this, I tested twice following these steps:
1. set bandwidth lively (--live) 2. query bandwidth (--live) 3. set bandwidth lively (--live)
The first time libvirtd crashed at step 2. The second time on step 2 I got strage data, and libvirtd crashed at step 3. Yeah, I can reproduce this and libvirtd crashed at step 3 for me.
In addition, valgrind can't find this memory leak, it's a negative branch, coverity complains it, line 7994 called allocation function "virAlloc" on "newBandwidth", and line 7999 variable "newBandwidth" is not freed or pointed-to in function "memset", lines 8007 and 8017 variable "newBandwidth" going out of scope leaks the storage it points to, because 'cleanup' label hasn't freed allocated 'newBandwidth' variable memory. 7994 if (VIR_ALLOC(newBandwidth) < 0) { ...... 7999 memset(newBandwidth, 0, sizeof(newBandwidth)); ...... 8005 if (VIR_ALLOC(newBandwidth->in) < 0) { 8006 virReportOOMError(); 8007 goto cleanup; ...... 8015 if (VIR_ALLOC(newBandwidth->out) < 0) { 8016 virReportOOMError(); 8017 goto cleanup; ...... Regards, Alex
--- src/qemu/qemu_driver.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e93fe87..4be36f5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7864,7 +7864,7 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, virDomainDefPtr persistentDef = NULL; int ret = -1; virDomainNetDefPtr net = NULL, persistentNet = NULL; - virNetDevBandwidthPtr bandwidth = NULL, newBandwidth = NULL; + virNetDevBandwidthPtr bandwidth = NULL;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -7986,6 +7986,8 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, }
if (flags& VIR_DOMAIN_AFFECT_LIVE) { + virNetDevBandwidthPtr newBandwidth = NULL; + if (VIR_ALLOC(newBandwidth)< 0) { virReportOOMError(); goto cleanup; @@ -8053,7 +8055,6 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, ret = 0; cleanup: virNetDevBandwidthFree(bandwidth); - virNetDevBandwidthFree(newBandwidth); virCgroupFree(&group); if (vm) virDomainObjUnlock(vm); -- 1.7.4.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list ping.
On 01/04/2012 11:36 PM, Alex Jia wrote:
On 01/05/2012 09:13 AM, Hu Tao wrote:
On Tue, Jan 03, 2012 at 11:14:15AM +0800, Hu Tao wrote:
This is not a memory leak. See line 8029 and 8030 of qemu_driver.c.
To ensure this, I tested twice following these steps:
1. set bandwidth lively (--live) 2. query bandwidth (--live) 3. set bandwidth lively (--live)
The first time libvirtd crashed at step 2. The second time on step 2 I got strage data, and libvirtd crashed at step 3. Yeah, I can reproduce this and libvirtd crashed at step 3 for me.
In addition, valgrind can't find this memory leak, it's a negative branch, coverity complains it, line 7994 called allocation function "virAlloc" on "newBandwidth", and line 7999 variable "newBandwidth" is not freed or pointed-to in function "memset", lines 8007 and 8017 variable "newBandwidth" going out of scope leaks the storage it points to, because 'cleanup' label hasn't freed allocated 'newBandwidth' variable memory.
In fixing the memory leak on a negative path, we introduced a use-after-free on the positive path. Reverting things would re-introduce the memory leak on failure. The _correct_ fix, which I'm pushing now, is this: diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 82bab67..110c31b 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -8034,6 +8034,7 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, virNetDevBandwidthFree(net->bandwidth); net->bandwidth = newBandwidth; + newBandwidth = NULL; } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { if (!persistentNet->bandwidth) { -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Alex Jia -
Eric Blake -
Hu Tao