[libvirt] [PATCH] network: Avoid memory leaks on networkBuildDnsmasqArgv

[Alex Jia]

On 01/06/2012 03:11 PM, Osier Yang wrote:
> On 2012年01月06日 13:14, ajia at redhat.com wrote:
>> From: Alex Jia<ajia at 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?
>> % 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 at redhat.com>
>> ---
>>   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