[Libvir] PATCH Fix a couple of mem leaks
I ran 'make valgrind' for the first time in too long and found a hanful of memory leaks in code exercised by the test suite. The attached patch fixes them all. I'm sure there are more lurking though - the test suite is mainly focused on exercising & validating correctness of parsing routines - its not really tried doing any validation of failure scenaarios. So I fear there's probably a number of error code paths where we don't cleanup properly, and/or don't reject invalid data. Its an area which should be addressed by someone with some copious free time ;-) Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Mon, Jul 30, 2007 at 10:14:25PM +0100, Daniel P. Berrange wrote:
I ran 'make valgrind' for the first time in too long and found a hanful of memory leaks in code exercised by the test suite. The attached patch fixes them all.
BTW, someone might have the answer for a question about valgrind. No matter what args I try, it consistently exits with a status code of '0' even if there are memory leaks $ valgrind --leak-check=full --error-exitcode=1 ./qemuxml2xmltest ==14631== Memcheck, a memory error detector. ==14631== Copyright (C) 2002-2006, and GNU GPL'd, by Julian Seward et al. ==14631== Using LibVEX rev 1658, a library for dynamic binary translation. ==14631== Copyright (C) 2004-2006, and GNU GPL'd, by OpenWorks LLP. ==14631== Using valgrind-3.2.1, a dynamic binary instrumentation framework. ==14631== Copyright (C) 2000-2006, and GNU GPL'd, by Julian Seward et al. ==14631== For more details, rerun with: -v ==14631== QEMU XML-2-ARGV minimal ... OK QEMU XML-2-ARGV Boot CDROM ... OK QEMU XML-2-ARGV Boot Network ... OK QEMU XML-2-ARGV Boot Floppy ... OK QEMU XML-2-ARGV Clock UTC ... OK QEMU XML-2-ARGV Clock Localtime ... OK QEMU XML-2-ARGV Disk CDROM ... OK QEMU XML-2-ARGV Disk Floppy ... OK QEMU XML-2-ARGV Disk Many ... OK QEMU XML-2-ARGV Graphics VNC ... OK QEMU XML-2-ARGV Graphics SDL ... OK QEMU XML-2-ARGV Input USB Mouse ... OK QEMU XML-2-ARGV Input USB Tablet ... OK QEMU XML-2-ARGV Misc ACPI ... OK QEMU XML-2-ARGV Misc No Reboot ... OK QEMU XML-2-ARGV Net User ... OK ==14631== ==14631== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 24 from 1) ==14631== malloc/free: in use at exit: 370,538 bytes in 64 blocks. ==14631== malloc/free: 9,731 allocs, 9,667 frees, 1,236,729 bytes allocated. ==14631== For counts of detected errors, rerun with: -v ==14631== searching for pointers to 64 not-freed blocks. ==14631== checked 314,304 bytes. ==14631== ==14631== 369,984 (265,344 direct, 104,640 indirect) bytes in 16 blocks are definitely lost in loss record 10 of 10 ==14631== at 0x400473F: calloc (vg_replace_malloc.c:279) ==14631== by 0x80657C3: qemudParseVMDef (qemu_conf.c:912) ==14631== by 0x804CF51: testCompareXMLToXMLHelper (qemuxml2xmltest.c:28) ==14631== by 0x804D990: virtTestRun (testutils.c:69) ==14631== by 0x804C975: main (qemuxml2xmltest.c:78) ==14631== ==14631== LEAK SUMMARY: ==14631== definitely lost: 265,344 bytes in 16 blocks. ==14631== indirectly lost: 104,640 bytes in 28 blocks. ==14631== possibly lost: 0 bytes in 0 blocks. ==14631== still reachable: 554 bytes in 20 blocks. ==14631== suppressed: 0 bytes in 0 blocks. ==14631== Reachable blocks (those to which a pointer was found) are not shown. ==14631== To see them, rerun with: --show-reachable=yes $ echo $? 0 Notice how there are 16 chunks of memory which are definitely lost, but it still reports 0 errors in its 'ERROR SUMMARY' section, and exits as if it were successful. Any one know how to make 'do the right thing' if it finds leaks & return a non-zero status ? Makes it very hard to put into the makefile for automated error detection[1]. Dan. [1] I really don't want to parse its 'LEAK SUMMARY" output. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Mon, Jul 30, 2007 at 10:19:17PM +0100, Daniel P. Berrange wrote:
On Mon, Jul 30, 2007 at 10:14:25PM +0100, Daniel P. Berrange wrote:
I ran 'make valgrind' for the first time in too long and found a hanful of memory leaks in code exercised by the test suite. The attached patch fixes them all.
BTW, someone might have the answer for a question about valgrind. No matter what args I try, it consistently exits with a status code of '0' even if there are memory leaks
--error-exitcode=1 --error-exitcode=<number> [default: 0] Specifies an alternative exit code to return if Valgrind reported any errors in the run. When set to the default value (zero), the return value from Valgrind will always be the return value of the process being simulated. When set to a nonzero value, that value is returned instead, if Valgrind detects any errors. This is useful for using Valgrind as part of an automated test suite, since it makes it easy to detect test cases for which Valgrind has reported errors, just by inspecting return codes.
$ valgrind --leak-check=full --error-exitcode=1 ./qemuxml2xmltest
Hum, then that sounds like a bug ! I would report it, I'm afraid though that they may consider this a non-runtime error, and then not apply the error exit code path.
==14631== Memcheck, a memory error detector. ==14631== Copyright (C) 2002-2006, and GNU GPL'd, by Julian Seward et al. ==14631== Using LibVEX rev 1658, a library for dynamic binary translation. ==14631== Copyright (C) 2004-2006, and GNU GPL'd, by OpenWorks LLP. ==14631== Using valgrind-3.2.1, a dynamic binary instrumentation framework. ==14631== Copyright (C) 2000-2006, and GNU GPL'd, by Julian Seward et al. ==14631== For more details, rerun with: -v ==14631==
Also add --quiet to avoid the extra verbosity in the regression tests (that's my only feature to valgrind so I kind of like it :-) [...]
Notice how there are 16 chunks of memory which are definitely lost, but it still reports 0 errors in its 'ERROR SUMMARY' section, and exits as if it were successful. Any one know how to make 'do the right thing' if it finds leaks & return a non-zero status ? Makes it very hard to put into the makefile for automated error detection[1].
Dan.
[1] I really don't want to parse its 'LEAK SUMMARY" output.
What I did with 'make valgrind' in libxml2 is use the --quiet option, output the error to a file and test for a zero size. No parsing, still relatively simple at the shell level. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Mon, Jul 30, 2007 at 10:14:25PM +0100, Daniel P. Berrange wrote:
I ran 'make valgrind' for the first time in too long and found a hanful of memory leaks in code exercised by the test suite. The attached patch fixes them all.
I'm sure there are more lurking though - the test suite is mainly focused on exercising & validating correctness of parsing routines - its not really tried doing any validation of failure scenaarios. So I fear there's probably a number of error code paths where we don't cleanup properly, and/or don't reject invalid data. Its an area which should be addressed by someone with some copious free time ;-)
Oops, sure looks fine to me !
--- src/xml.c 18 Jul 2007 21:08:22 -0000 1.85 +++ src/xml.c 30 Jul 2007 19:47:12 -0000 @@ -70,8 +70,11 @@ virXPathString(const char *xpath, xmlXPa } obj = xmlXPathEval(BAD_CAST xpath, ctxt); if ((obj == NULL) || (obj->type != XPATH_STRING) || - (obj->stringval == NULL) || (obj->stringval[0] == 0)) + (obj->stringval == NULL) || (obj->stringval[0] == 0)) { + if (obj) + xmlXPathFreeObject(obj); return(NULL); + } ret = strdup((char *) obj->stringval); xmlXPathFreeObject(obj); if (ret == NULL) {
And that one is mine, dohhh :-( +1 Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Daniel P. Berrange wrote:
I ran 'make valgrind' for the first time in too long and found a hanful of memory leaks in code exercised by the test suite. The attached patch fixes them all.
+1
I'm sure there are more lurking though - the test suite is mainly focused on exercising & validating correctness of parsing routines - its not really tried doing any validation of failure scenaarios. So I fear there's probably a number of error code paths where we don't cleanup properly, and/or don't reject invalid data. Its an area which should be addressed by someone with some copious free time ;-)
Yeah, earlier this month I actually got as far as importing[0] the whole of libvirt into CIL[1]/Blast[2], which could analyse these sorts of bugs without either testing or spending too much time looking at code by hand. (Think: Coverity, but free software). Then work intervened. Given a week to make the tools work we could have regression tests which would analyse the code and find these problems automatically. Rich. [0] http://et.redhat.com/~rjones/cil-analysis-of-libvirt/ [1] http://hal.cs.berkeley.edu/cil/ [2] http://mtc.epfl.ch/software-tools/blast/ -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Richard W.M. Jones