[libvirt] [PATCH] vbox_tmpl.c: avoid leak on OOM error path
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@redhat.com> 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
2010/2/15 Jim Meyering <jim@meyering.net>:
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@redhat.com> 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
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; } Matthias
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@redhat.com> 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
2010/2/15 Jim Meyering <jim@meyering.net>:
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@redhat.com> 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);
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.
VIR_FREE(networkNameUtf8); return ret; } -- 1.7.0.181.g41533
Matthias
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@redhat.com> 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
2010/2/15 Jim Meyering <jim@meyering.net>:
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@redhat.com> 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
ACK Matthias
participants (2)
-
Jim Meyering -
Matthias Bolte