We have to free def here, rather than after "cleanup:", because "cleanup:" is on the successful-return path. >From 2c7800c3560f03fd77e4a458cb0e557165faed3f Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com&gt; Date: Mon, 15 Feb 2010 17:54:15 +0100 Subject: [PATCH] vbox_tmpl.c: avoid leak on OOM error path * src/vbox/vbox_tmpl.c (vboxDomainDumpXML): Free def upon virAsprintf failure. ---  src/vbox/vbox_tmpl.c |    1 +  1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 68dffd2..1fb8d56 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -6059,14 +6059,15 @@ static char *vboxNetworkDumpXML(virNetworkPtr network, int flags ATTRIBUTE_UNUSE     if (VIR_ALLOC(def) < 0) {         virReportOOMError();         goto cleanup;     }     if (virAsprintf(&networkNameUtf8, "HostInterfaceNetworking-%s", network->name) < 0) {         virReportOOMError(); +        VIR_FREE(def);         goto cleanup;     }     PRUnichar *networkInterfaceNameUtf16    = NULL;     IHostNetworkInterface *networkInterface = NULL;     VBOX_UTF8_TO_UTF16(network->name, &networkInterfaceNameUtf16); -- 1.7.0.181.g41533

Matthias Bolte wrote: ... > If you look closely at what the function does with the > virNetworkDefPtr def, you see that it fills it with information and > calls virNetworkDefFormat in the end. So def is never returned from > the function and leaks in the successful-return path. I suggest this > patch instead: > > diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c > index 68dffd2..d1a701e 100644 > --- a/src/vbox/vbox_tmpl.c > +++ b/src/vbox/vbox_tmpl.c > @@ -6183,6 +6183,7 @@ static char *vboxNetworkDumpXML(virNetworkPtr > network, int flags ATTRIBUTE_UNUSE >      ret = virNetworkDefFormat(def); > >  cleanup: > +    virNetworkDefFree(def); >      VIR_FREE(networkNameUtf8); >      return ret; >  } Good point.  Definite improvement. Here you go: From 1dc101a80c3af855c72e8dbc64fbeb91614c49b4 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com&gt; Date: Mon, 15 Feb 2010 17:54:15 +0100 Subject: [PATCH] vbox_tmpl.c: avoid an unconditional leak * src/vbox/vbox_tmpl.c (vboxDomainDumpXML): Free def. Improved by Matthias Bolte. ---  src/vbox/vbox_tmpl.c |    1 +  1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 68dffd2..751fe52 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -6183,6 +6183,7 @@ static char *vboxNetworkDumpXML(virNetworkPtr network, int flags ATTRIBUTE_UNUSE     ret = virNetworkDefFormat(def);  cleanup: +    VIR_FREE(def);

    VIR_FREE(networkNameUtf8);     return ret;  } -- 1.7.0.181.g41533

Matthias Bolte wrote: ... >>  cleanup: >> +    VIR_FREE(def); > > No, a simple VIR_FREE isn't enough here. At this point def can > contains allocated parts. It's necessary to call > virNetworkDefFree(def) here. virNetworkDefFree takes care of freeing > all parts of def and def itself. Well, that's obviously what I meant ;-) Need to re-tune this AI that's mangling my patches. From 2e7a52d3832952f6757d8f9ddfc92f497d10c2a9 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com&gt; Date: Mon, 15 Feb 2010 17:54:15 +0100 Subject: [PATCH] vbox_tmpl.c: avoid an unconditional leak * src/vbox/vbox_tmpl.c (vboxDomainDumpXML): Free def. Improved by Matthias Bolte. ---  src/vbox/vbox_tmpl.c |    1 +  1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 68dffd2..d1a701e 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -6183,6 +6183,7 @@ static char *vboxNetworkDumpXML(virNetworkPtr network, int flags ATTRIBUTE_UNUSE     ret = virNetworkDefFormat(def);  cleanup: +    virNetworkDefFree(def);     VIR_FREE(networkNameUtf8);     return ret;  } -- 1.7.0.181.g41533