[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv3] network: Avoid memory leaks on networkBuildDnsmasqArgv



On 29.01.2012 06:24, ajia redhat com wrote:
> From: Alex Jia <ajia 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 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]