Re: [libvirt] [PATCHv3] network: Avoid memory leaks on networkBuildDnsmasqArgv
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);