[libvirt] [PATCH] virsh: Plug memory leak on cmdUndefine
From: Alex Jia <ajia@redhat.com> Detected by valgrind. Leak is introduced in commit 3bb6bcf. Free 'vol' memory before allocating memory, the codes will miss one time free when 'vol_i = nvolumes' in for loop, so plug memory leak. * tools/virsh.c: fix memory leak on cmdUndefine. * How to reproduce? % dd if=/dev/null of=/var/lib/libvirt/images/foo bs=1 count=1 seek=10M % virsh define foo.xml (disk source file points to '/var/lib/libvirt/images/foo') % virsh vol-clone foo foo-clone default (the original guest name is 'foo') % virsh pool-refresh default % virsh vol-list default (make sure 'foo-clone' volume exists) % virsh define foo-clone.xml (disk source file points to '/var/lib/libvirt/images/foo-clone') % valgrind -v --leak-check=full virsh undefine foo-clone --remove-all-storage * Actual results: 1. virsh output Domain foo-clone has been undefined Volume '/var/lib/libvirt/images/foo-clone' removed. error: Failed to disconnect from the hypervisor, 1 leaked reference(s) 2. valgrind result ==6515== 92 (40 direct, 52 indirect) bytes in 1 blocks are definitely lost in loss record 46 of 69 ==6515== at 0x4A04A28: calloc (vg_replace_malloc.c:467) ==6515== by 0x4C89B71: virAlloc (memory.c:101) ==6515== by 0x4CFCACE: virGetStorageVol (datatypes.c:724) ==6515== by 0x4D4A8E0: remoteStorageVolLookupByPath (remote_driver.c:4664) ==6515== by 0x4D07153: virStorageVolLookupByPath (libvirt.c:12508) ==6515== by 0x4270E6: cmdUndefine (virsh.c:2828) ==6515== by 0x4151B6: vshCommandRun (virsh.c:17693) ==6515== by 0x4264D3: main (virsh.c:19270) ==6515== ==6515== LEAK SUMMARY: ==6515== definitely lost: 40 bytes in 1 blocks RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=786674 Signed-off-by: Alex Jia <ajia@redhat.com> --- tools/virsh.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index c8fd448..73c2192 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2787,10 +2787,6 @@ out: ctxt->node = vol_nodes[vol_i]; VIR_FREE(target); VIR_FREE(source); - if (vol) { - virStorageVolFree(vol); - vol = NULL; - } /* get volume source and target paths */ if (!(target = virXPathString("string(./target/@dev)", ctxt))) { @@ -2852,6 +2848,10 @@ out: vol_del_failed = true; } vshPrint(ctl, _("Volume '%s' removed.\n"), volume_tok?volume_tok:source); + + /* cleanup */ + virStorageVolFree(vol); + vol = NULL; } /* print volumes specified by user that were not found in domain definition */ -- 1.7.1
On 02/02/2012 07:25 AM, ajia@redhat.com wrote:
From: Alex Jia<ajia@redhat.com>
--- tools/virsh.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index c8fd448..73c2192 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2787,10 +2787,6 @@ out: ctxt->node = vol_nodes[vol_i]; VIR_FREE(target); VIR_FREE(source); - if (vol) { - virStorageVolFree(vol); - vol = NULL; - }
Actually, this code is needed, as error paths in the loop are handled gracefuly with a 'continue;' so we need to free the volume on such path;
/* get volume source and target paths */ if (!(target = virXPathString("string(./target/@dev)", ctxt))) { @@ -2852,6 +2848,10 @@ out: vol_del_failed = true; } vshPrint(ctl, _("Volume '%s' removed.\n"), volume_tok?volume_tok:source); + + /* cleanup */ + virStorageVolFree(vol); + vol = NULL; }
Yeah, I actualy forgot to clean up the volume after the loop. I modified your patch to correct this in the final cleanup: diff --git a/tools/virsh.c b/tools/virsh.c index c8fd448..af78102 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2875,6 +2875,8 @@ cleanup: VIR_FREE(volume_tokens); VIR_FREE(def); VIR_FREE(vol_nodes); + if (vol) + virStorageVolFree(vol); xmlFreeDoc(doc); xmlXPathFreeContext(ctxt); virDomainFree(dom); and pushed. Thanks for catching this bug. Peter
participants (2)
-
ajia＠redhat.com -
Peter Krempa