[libvirt] [PATCH] Fix memory leak in virNWFilterDefParseXML()

[Daniel P. Berrange]

On Fri, Nov 29, 2013 at 08:31:52PM +0530, Nehal J Wani wrote:
> While running nwfilterxml2xmltest, it was found that valgrind pointed out the
> following error...
> 
> ==7466== 16 bytes in 1 blocks are definitely lost in loss record 26 of 90
> ==7466==    at 0x4A06B6F: calloc (vg_replace_malloc.c:593)
>     ==7466==    by 0x4C651AD: virAlloc (viralloc.c:142)
>     ==7466==    by 0x4D0450D: virNWFilterDefParseNode (nwfilter_conf.c:2575)
>     ==7466==    by 0x4D05D84: virNWFilterDefParse (nwfilter_conf.c:2647)
>     ==7466==    by 0x401FDE: testCompareXMLToXMLHelper (nwfilterxml2xmltest.c:39)
>     ==7466==    by 0x402DE1: virtTestRun (testutils.c:138)
>     ==7466==    by 0x4018E9: mymain (nwfilterxml2xmltest.c:111)
>     ==7466==    by 0x403482: virtTestMain (testutils.c:593)
>     ==7466==    by 0x341F421A04: (below main) (libc-start.c:225)
> 
> ...21 times, which are related to 21 tests in nwfilterxml2xmltest.c which sent
> EXPECT_WARN = false. There were two scenarios in virNWFilterDefParseXML(),
> when the variable 'entry' was malloc'ed, but not freed.
> 
> ---
>  src/conf/nwfilter_conf.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
> index 793cb0e..ee26a62 100644
> --- a/src/conf/nwfilter_conf.c
> +++ b/src/conf/nwfilter_conf.c
> @@ -2576,11 +2576,15 @@ virNWFilterDefParseXML(xmlXPathContextPtr ctxt) {
>                  goto cleanup;
>  
>              if (xmlStrEqual(curr->name, BAD_CAST "rule")) {
> -                if (!(entry->rule = virNWFilterRuleParse(curr)))
> +                if (!(entry->rule = virNWFilterRuleParse(curr))) {
> +                    VIR_FREE(entry);
>                      goto cleanup;
> +                }
>              } else if (xmlStrEqual(curr->name, BAD_CAST "filterref")) {
> -                if (!(entry->include = virNWFilterIncludeParse(curr)))
> +                if (!(entry->include = virNWFilterIncludeParse(curr))) {
> +                    VIR_FREE(entry);
>                      goto cleanup;
> +                }
>              }

I think it is preferrable to use virNWFilterEntryFree() here.

>              if (entry->rule || entry->include) {

Just following this line there is another call to VIR_FREE(entry)
which leaks entry->rule and entry->include. This should be made
to call virNWFilterEntryFree too


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|