On 01/06/2012 03:11 PM, Osier Yang wrote:
On 2012年01月06日 13:14, ajia@xxxxxxxxxx wrote:
From: Alex Jia<ajia@xxxxxxxxxx>
Detected by valgrind. Leaks introduced in commit 973af236.
* src/network/bridge_driver.c: fix memory leaks on failure and
successful path.
* How to reproduce?
% cd tests&& valgrind -v --leak-check=full ./networkxml2argvtest
* Actual result:
==2226== 3 bytes in 1 blocks are definitely lost in loss record 1 of 24
==2226== at 0x4A05FDE: malloc (vg_replace_malloc.c:236)
==2226== by 0x39CF0FEDE7: __vasprintf_chk (in /lib64/libc-2.12.so)
==2226== by 0x41DFF7: virVasprintf (stdio2.h:199)
==2226== by 0x41E0B7: virAsprintf (util.c:1695)
==2226== by 0x41A2D9: networkBuildDhcpDaemonCommandLine
(bridge_driver.c:545)
==2226== by 0x4145C8: testCompareXMLToArgvHelper
(networkxml2argvtest.c:47)
==2226== by 0x4156A1: virtTestRun (testutils.c:141)
==2226== by 0x414332: mymain (networkxml2argvtest.c:123)
==2226== by 0x414D97: virtTestMain (testutils.c:696)
==2226== by 0x39CF01ECDC: (below main) (in /lib64/libc-2.12.so)
==2226==
==2226== 3 bytes in 1 blocks are definitely lost in loss record 2 of 24
==2226== at 0x4A05FDE: malloc (vg_replace_malloc.c:236)
==2226== by 0x39CF0FEDE7: __vasprintf_chk (in /lib64/libc-2.12.so)
==2226== by 0x41DFF7: virVasprintf (stdio2.h:199)
==2226== by 0x41E0B7: virAsprintf (util.c:1695)
==2226== by 0x41A307: networkBuildDhcpDaemonCommandLine
(bridge_driver.c:551)
==2226== by 0x4145C8: testCompareXMLToArgvHelper
(networkxml2argvtest.c:47)
==2226== by 0x4156A1: virtTestRun (testutils.c:141)
==2226== by 0x414332: mymain (networkxml2argvtest.c:123)
==2226== by 0x414D97: virtTestMain (testutils.c:696)
==2226== by 0x39CF01ECDC: (below main) (in /lib64/libc-2.12.so)
==2226==
==2226== 5 bytes in 1 blocks are definitely lost in loss record 4 of 24
==2226== at 0x4A05FDE: malloc (vg_replace_malloc.c:236)
==2226== by 0x39CF0FEDE7: __vasprintf_chk (in /lib64/libc-2.12.so)
==2226== by 0x41DFF7: virVasprintf (stdio2.h:199)
==2226== by 0x41E0B7: virAsprintf (util.c:1695)
==2226== by 0x41A2AB: networkBuildDhcpDaemonCommandLine
(bridge_driver.c:539)
==2226== by 0x4145C8: testCompareXMLToArgvHelper
(networkxml2argvtest.c:47)
==2226== by 0x4156A1: virtTestRun (testutils.c:141)
==2226== by 0x414332: mymain (networkxml2argvtest.c:123)
==2226== by 0x414D97: virtTestMain (testutils.c:696)
==2226== by 0x39CF01ECDC: (below main) (in /lib64/libc-2.12.so)
==2226==
==2226== LEAK SUMMARY:
==2226== definitely lost: 11 bytes in 3 blocks
Signed-off-by: Alex Jia<ajia@xxxxxxxxxx>
---
src/network/bridge_driver.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 9afada7..63a28a6 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -544,12 +544,15 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
if (dns->srvrecords[i].priority) {
if (virAsprintf(&recordPriority, "%d",
dns->srvrecords[i].priority)< 0) {
virReportOOMError();
+ VIR_FREE(recordPort);
goto cleanup;
}
}
if (dns->srvrecords[i].weight) {
if (virAsprintf(&recordWeight, "%d",
dns->srvrecords[i].weight)< 0) {
virReportOOMError();
+ VIR_FREE(recordPort);
+ VIR_FREE(recordPriority);
goto cleanup;
}
}
@@ -567,6 +570,9 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
}
virCommandAddArgPair(cmd, "--srv-host", record);
+ VIR_FREE(recordPort);
+ VIR_FREE(recordPriority);
+ VIR_FREE(recordWeight);
VIR_FREE(record);
}
}
These fixes are right, but you might also want to free() "recordPort",
"recordPriority", and "recordWeight" before "goto cleanup" in following
branch:
Yeah, indeed.
if (virAsprintf(&record, "%s.%s.%s,%s,%s,%s,%s",
dns->srvrecords[i].service,
dns->srvrecords[i].protocol,
dns->srvrecords[i].domain ?
dns->srvrecords[i].domain : "",
dns->srvrecords[i].target ?
dns->srvrecords[i].target : "",
recordPort ?
recordPort : "",
recordPriority ?
recordPriority : "",
recordWeight ?
recordWeight : "") < 0) {
And personally I'd use "goto", too much duplicate VIR_FREE use.
Yeah, I agree, but "recordPort", "recordPriority", and "recordWeight"
are declared and defined
in loop body, and they are re-initialized to NULL again on next time
loop, I guess it's author's
design, so I haven't defined a global "recordPort", "recordPriority",
and "recordWeight" variables
then use 'goto' to cleanup allocated memory on label 'cleanup'.
Thanks,
Alex
Regards,
Osier
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list