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

[Eric Blake]

[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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120130/a8a36a1b/attachment-0001.sig>