[libvirt] [PATCH] Fix memory leak in virNWFilterDefParseXML()
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; + } } if (entry->rule || entry->include) { -- 1.8.1.4
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 :|
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
Would it be better to remove all the VIR_FREE(entry) and just put one virNWFilterEntryFree(entry) below the label cleanup ? -- Nehal J Wani
On Fri, Nov 29, 2013 at 08:40:31PM +0530, Nehal J Wani wrote:
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
Would it be better to remove all the VIR_FREE(entry) and just put one virNWFilterEntryFree(entry) below the label cleanup ?
Yes, just make sure it is always initialized sanely. 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 :|
On Fri, Nov 29, 2013 at 8:54 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Fri, Nov 29, 2013 at 08:40:31PM +0530, Nehal J Wani wrote:
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
Would it be better to remove all the VIR_FREE(entry) and just put one virNWFilterEntryFree(entry) below the label cleanup ?
Yes, just make sure it is always initialized sanely.
Just realized.. variable entry is malloc'd each time in a loop, so it cannot be freed inside cleanup. Sending patch v2 -- Nehal J Wani
participants (2)
-
Daniel P. Berrange -
Nehal J Wani