11:24 p.m.
From: Alex Jia <ajia(a)redhat.com>
Detected by valgrind. Leaks introduced in commit 973af236.
* src/network/bridge_driver.c: fix memory leaks on failure and successful path.
* How to reproduce?
% make -C tests check TESTS=networkxml2argvtest
% 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(a)redhat.com>
---
src/network/bridge_driver.c | 15 +++++++++------
1 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 5d0d528..c9a8ccf 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -458,7 +458,10 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
{
int r, ret = -1;
int nbleases = 0;
- int ii;
+ int ii, i, j;
+ char *recordPort = NULL;
+ char *recordPriority = NULL;
+ char *recordWeight = NULL;
virNetworkIpDefPtr tmpipdef;
/*
@@ -513,7 +516,6 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
if (network->def->dns != NULL) {
virNetworkDNSDefPtr dns = network->def->dns;
- int i;
for (i = 0; i < dns->ntxtrecords; i++) {
char *record = NULL;
@@ -530,10 +532,6 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
for (i = 0; i < dns->nsrvrecords; i++) {
char *record = NULL;
- char *recordPort = NULL;
- char *recordPriority = NULL;
- char *recordWeight = NULL;
-
if (dns->srvrecords[i].service && dns->srvrecords[i].protocol)
{
if (dns->srvrecords[i].port) {
if (virAsprintf(&recordPort, "%d",
dns->srvrecords[i].port) < 0) {
@@ -671,6 +669,11 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
ret = 0;
cleanup:
+ for (j = 0; j < i; j++) {
+ VIR_FREE(recordPort);
+ VIR_FREE(recordWeight);
+ VIR_FREE(recordPriority);
+ }
return ret;
}
--
1.7.1
5:32 a.m.
On 29.01.2012 06:24, ajia(a)redhat.com wrote:
From: Alex Jia <ajia(a)redhat.com>
Detected by valgrind. Leaks introduced in commit 973af236.
* src/network/bridge_driver.c: fix memory leaks on failure and successful path.
* How to reproduce?
% make -C tests check TESTS=networkxml2argvtest
% 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(a)redhat.com>
---
src/network/bridge_driver.c | 15 +++++++++------
1 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 5d0d528..c9a8ccf 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -458,7 +458,10 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
{
int r, ret = -1;
int nbleases = 0;
- int ii;
+ int ii, i, j;
+ char *recordPort = NULL;
+ char *recordPriority = NULL;
+ char *recordWeight = NULL;
virNetworkIpDefPtr tmpipdef;
/*
@@ -513,7 +516,6 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
if (network->def->dns != NULL) {
virNetworkDNSDefPtr dns = network->def->dns;
- int i;
for (i = 0; i < dns->ntxtrecords; i++) {
char *record = NULL;
@@ -530,10 +532,6 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
for (i = 0; i < dns->nsrvrecords; i++) {
char *record = NULL;
- char *recordPort = NULL;
- char *recordPriority = NULL;
- char *recordWeight = NULL;
-
if (dns->srvrecords[i].service && dns->srvrecords[i].protocol)
{
if (dns->srvrecords[i].port) {
if (virAsprintf(&recordPort, "%d",
dns->srvrecords[i].port) < 0) {
@@ -671,6 +669,11 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
ret = 0;
cleanup:
+ for (j = 0; j < i; j++) {
+ VIR_FREE(recordPort);
+ VIR_FREE(recordWeight);
+ VIR_FREE(recordPriority);
+ }
return ret;
}
I think we need v4; You want to free memory in the very same loop where
it is allocated. Otherwise we still leak memory from previous loop
iterations. More detailed, virAsprintf() takes given pointer and
overwrites it. So if pointer was referring to an allocated memory, we
lost this single reference and thus leak.
Michal
9:11 a.m.
Hi Michal,
Thanks for your comment, the following v1 patch should be a right way?
https://www.redhat.com/archives/libvir-list/2012-January/msg00209.html
Regards,
Alex
----- Original Message -----
From: "Michal Privoznik" <mprivozn(a)redhat.com>
To: ajia(a)redhat.com
Cc: libvir-list(a)redhat.com
Sent: Monday, January 30, 2012 7:32:59 PM
Subject: Re: [libvirt] [PATCHv3] network: Avoid memory leaks on networkBuildDnsmasqArgv
On 29.01.2012 06:24, ajia(a)redhat.com wrote:
From: Alex Jia <ajia(a)redhat.com>
Detected by valgrind. Leaks introduced in commit 973af236.
* src/network/bridge_driver.c: fix memory leaks on failure and successful path.
* How to reproduce?
% make -C tests check TESTS=networkxml2argvtest
% 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(a)redhat.com>
---
src/network/bridge_driver.c | 15 +++++++++------
1 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 5d0d528..c9a8ccf 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -458,7 +458,10 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
{
int r, ret = -1;
int nbleases = 0;
- int ii;
+ int ii, i, j;
+ char *recordPort = NULL;
+ char *recordPriority = NULL;
+ char *recordWeight = NULL;
virNetworkIpDefPtr tmpipdef;
/*
@@ -513,7 +516,6 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
if (network->def->dns != NULL) {
virNetworkDNSDefPtr dns = network->def->dns;
- int i;
for (i = 0; i < dns->ntxtrecords; i++) {
char *record = NULL;
@@ -530,10 +532,6 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
for (i = 0; i < dns->nsrvrecords; i++) {
char *record = NULL;
- char *recordPort = NULL;
- char *recordPriority = NULL;
- char *recordWeight = NULL;
-
if (dns->srvrecords[i].service && dns->srvrecords[i].protocol)
{
if (dns->srvrecords[i].port) {
if (virAsprintf(&recordPort, "%d",
dns->srvrecords[i].port) < 0) {
@@ -671,6 +669,11 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
ret = 0;
cleanup:
+ for (j = 0; j < i; j++) {
+ VIR_FREE(recordPort);
+ VIR_FREE(recordWeight);
+ VIR_FREE(recordPriority);
+ }
return ret;
}
I think we need v4; You want to free memory in the very same loop where
it is allocated. Otherwise we still leak memory from previous loop
iterations. More detailed, virAsprintf() takes given pointer and
overwrites it. So if pointer was referring to an allocated memory, we
lost this single reference and thus leak.
Michal
3:34 p.m.
[reordering - top-posting is hard to follow]
On 01/30/2012 08:11 AM, Alex Jia wrote:
> @@ -530,10 +532,6 @@ networkBuildDnsmasqArgv(virNetworkObjPtr
network,
>
> for (i = 0; i < dns->nsrvrecords; i++) {
> char *record = NULL;
> - char *recordPort = NULL;
> - char *recordPriority = NULL;
> - char *recordWeight = NULL;
> -
> if (dns->srvrecords[i].service &&
dns->srvrecords[i].protocol) {
> if (dns->srvrecords[i].port) {
> if (virAsprintf(&recordPort, "%d",
dns->srvrecords[i].port) < 0) {
> @@ -671,6 +669,11 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
I think we need v4; You want to free memory in the very same loop where
it is allocated. Otherwise we still leak memory from previous loop
iterations. More detailed, virAsprintf() takes given pointer and
overwrites it. So if pointer was referring to an allocated memory, we
lost this single reference and thus leak.
Michal
Hi Michal,
Thanks for your comment, the following v1 patch should be a right way?
https://www.redhat.com/archives/libvir-list/2012-January/msg00209.html
v1 was incomplete, but was at least attempting to free things in the for
loop where they were allocated.
What I'd suggest is:
declare variables outside the loop,
start the for loop by freeing any state from previous iterations,
and also free all state in the cleanup label
at which point, you _don't_ have to follow the v1 approach of trying to
free variables before every goto or break (and missing some). Something
like:
char *record = NULL;
for (i = 0; i < dns->nsrrvrecords; i++) {
/* free previous iteration */
VIR_FREE(record);
...
record = ...
if (error)
goto cleanup;
}
cleanup:
VIR_FREE(record);
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:15 a.m.
On 01/31/2012 05:34 AM, Eric Blake wrote:
[reordering - top-posting is hard to follow]
On 01/30/2012 08:11 AM, Alex Jia wrote:
>> @@ -530,10 +532,6 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
>>
>> for (i = 0; i< dns->nsrvrecords; i++) {
>> char *record = NULL;
>> - char *recordPort = NULL;
>> - char *recordPriority = NULL;
>> - char *recordWeight = NULL;
>> -
>> if (dns->srvrecords[i].service&&
dns->srvrecords[i].protocol) {
>> if (dns->srvrecords[i].port) {
>> if (virAsprintf(&recordPort, "%d",
dns->srvrecords[i].port)< 0) {
>> @@ -671,6 +669,11 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
> I think we need v4; You want to free memory in the very same loop where
> it is allocated. Otherwise we still leak memory from previous loop
> iterations. More detailed, virAsprintf() takes given pointer and
> overwrites it. So if pointer was referring to an allocated memory, we
> lost this single reference and thus leak.
>
> Michal
> Hi Michal,
> Thanks for your comment, the following v1 patch should be a right way?
> https://www.redhat.com/archives/libvir-list/2012-January/msg00209.html
v1 was incomplete, but was at least attempting to free things in the for
loop where they were allocated.
What I'd suggest is:
declare variables outside the loop,
start the for loop by freeing any state from previous iterations,
and also free all state in the cleanup label
at which point, you _don't_ have to follow the v1 approach of trying to
free variables before every goto or break (and missing some). Something
like:
char *record = NULL;
for (i = 0; i< dns->nsrrvrecords; i++) {
/* free previous iteration */
VIR_FREE(record);
Hi Eric, thanks for your nice comment, the 'record'
variable is
allocated memory in previous for loop,
but we free it in here, the issue is 'dns->ntxtrecords' =
'dns->nsrrvrecord' ? if not, is it also okay?
Thanks & Regards,
Alex
...
record = ...
if (error)
goto cleanup;
}
cleanup:
VIR_FREE(record);
6:23 p.m.
On 01/31/2012 12:15 AM, Alex Jia wrote:
> What I'd suggest is:
>
> declare variables outside the loop,
> start the for loop by freeing any state from previous iterations,
> and also free all state in the cleanup label
>
> at which point, you _don't_ have to follow the v1 approach of trying to
> free variables before every goto or break (and missing some). Something
> like:
>
> char *record = NULL;
> for (i = 0; i< dns->nsrrvrecords; i++) {
> /* free previous iteration */
> VIR_FREE(record);
Hi Eric, thanks for your nice comment, the 'record' variable is
allocated memory in previous for loop,
but we free it in here, the issue is 'dns->ntxtrecords' =
'dns->nsrrvrecord' ? if not, is it also okay?
That's why you _also_ free things in the cleanup label.
If every iteration of the loop frees the previous iteration, then the
first iteration of the loop is a no-op, and the cleanup code frees the
last iteration, no matter whether you get to the cleanup code via a goto
or whether the last iteration exited normally.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4677
days inactive
4680
days old
5 comments
4 participants
participants (4)
-
ajia＠redhat.com -
Alex Jia
-
Eric Blake -
Michal Privoznik