----- Original Message ----- | From: "Martin Kletzander" <mkletzan(a)redhat.com&gt; | To: "shyu" <shyu(a)redhat.com&gt; | Cc: libvir-list(a)redhat.com | Sent: Monday, February 24, 2014 4:48:01 PM | Subject: Re: [libvirt] [PATCH] Fix memory leak when lookup pool list with invalid type option | | On Mon, Feb 24, 2014 at 02:12:34PM +0800, shyu wrote: | > There will be memory leak when lookup pool list with invalid type option | > https://bugzilla.redhat.com/show_bug.cgi?id=1069068 | > | > ==23060== Memcheck, a memory error detector | > ==23060== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al. | > ==23060== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info | > ==23060== Command: virsh pool-list --type invalid | > ==23060== | > error: Invalid pool type | > | > ==23060== | > ==23060== HEAP SUMMARY: | > ==23060== in use at exit: 138,383 bytes in 1,559 blocks | > ==23060== total heap usage: 4,199 allocs, 2,640 frees, 21,372,544 bytes | > allocated | > ==23060== | > ==23060== LEAK SUMMARY: | > ==23060== definitely lost: 8 bytes in 1 blocks | > ==23060== indirectly lost: 0 bytes in 0 blocks | > ==23060== possibly lost: 0 bytes in 0 blocks | > ==23060== still reachable: 138,375 bytes in 1,558 blocks | > ==23060== suppressed: 0 bytes in 0 blocks | > ==23060== Rerun with --leak-check=full to see details of leaked memory | > ==23060== | > ==23060== For counts of detected and suppressed errors, rerun with: -v | > ==23060== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 8 from 6) | > | | This info is useless since there is no information about the leak. | You should've included this kind of info instead: | | ==2472== 5 bytes in 1 blocks are definitely lost in loss record 5 of 84 | ==2472== at 0x4A069EE: malloc (vg_replace_malloc.c:270) | ==2472== by 0x3B88281171: strdup (strdup.c:43) | ==2472== by 0x40EB40: _vshStrdup (virsh.c:129) | ==2472== by 0x40FC68: vshStringToArray (virsh.c:182) | ==2472== by 0x42A9F9: cmdPoolList (virsh-pool.c:857) | ==2472== by 0x40D240: vshCommandRun (virsh.c:1652) | ==2472== by 0x410B4A: main (virsh.c:3093) | | > Signed-off-by: shyu <shyu(a)redhat.com&gt; | > --- | > tools/virsh-pool.c | 1 + | > 1 files changed, 1 insertions(+), 0 deletions(-) | > | > diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c | > index 642b078..db4d3ae 100644 | > --- a/tools/virsh-pool.c | > +++ b/tools/virsh-pool.c | > @@ -859,6 +859,7 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd | > ATTRIBUTE_UNUSED) | > for (i = 0; i < npoolTypes; i++) { | > if ((poolType = virStoragePoolTypeFromString(poolTypes[i])) < | > 0) { | > vshError(ctl, "%s", _("Invalid pool type")); | > + VIR_FREE(*poolTypes); | | This does not properly free all the poolTypes in case this doesn't | fails with i > 0. | | > VIR_FREE(poolTypes); | | And judging by the code, this is not upstream at all, right? I guess | this is not applicable upstream for half a year already, at least. | Please submit patches that apply on top of current master when cloning | from git://libvirt.org/libvirt.git . | Hi Martin, I think I made an mistake. Since I found this bug with 0.10.2-29.el6_5.4 so I make this patch according v0.10.2-maint branch. I knew it was already be fixed on upstream. I thought there should be fixed on v0.10.2-maint. Now I know that. Thanks for your kind remind. | Thanks for your effort, but I believe this is already fixed within | commit d64af6ce as I already commented in the Bug you referenced. | | Martin | -- Regards shyu