Re: [libvirt] [PATCHv3] network: Avoid memory leaks on networkBuildDnsmasqArgv
[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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)