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

[Alex Jia]

On 01/20/2012 04:43 AM, Eric Blake wrote:
> On 01/18/2012 03:04 AM, 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?
>> % make -C tests check TESTS=networkxml2argvtest
>> % cd tests&&  valgrind -v --leak-check=full ./networkxml2argvtest
>>
>>   src/network/bridge_driver.c |    9 ++++++---
>>   1 files changed, 6 insertions(+), 3 deletions(-)
> Needs a v3.
>
>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>> index 5d0d528..5bd5a50 100644
>> --- a/src/network/bridge_driver.c
>> +++ b/src/network/bridge_driver.c
>> @@ -459,6 +459,9 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
>>       int r, ret = -1;
>>       int nbleases = 0;
>>       int ii;
>> +    char *recordPort = NULL;
>> +    char *recordPriority = NULL;
>> +    char *recordWeight = NULL;
>>       virNetworkIpDefPtr tmpipdef;
> Moving the declaration here, and a cleanup at the end, will only free
> _one_ instance of allocation.
>
>>
>>       /*
>> @@ -530,9 +533,6 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
>>
>>           for (i = 0; i<  dns->nsrvrecords; i++) {
>>               char *record = NULL;
>> -            char *recordPort = NULL;
>> -            char *recordPriority = NULL;
>> -            char *recordWeight = NULL;
> But these values were allocated in a for loop, so if there is more than
> one nsrvrecords, then you are leaking on each iteration of the loop.
>
>>
>>               if (dns->srvrecords[i].service&&  dns->srvrecords[i].protocol) {
>>                   if (dns->srvrecords[i].port) {
>> @@ -671,6 +671,9 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
>>
>>       ret = 0;
>>   cleanup:
>> +    VIR_FREE(recordPort);
>> +    VIR_FREE(recordWeight);
>> +    VIR_FREE(recordPriority);
> You need to also copy these three lines into the end of the for loop.
>
Yeah, these values were allocated in a for loop, so also should free 
them in a loop, thanks for
your comment, and will commit a v3 now.

Regards,
Alex