2010/1/5 Jim Meyering <jim@meyering.net>:
Matthias Bolte wrote:
2009/12/16 Jim Meyering <jim@meyering.net>:
Similar to others,
From 5dcb4d95a351cb6fa32ebaafb96756275e1079e2 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 16 Dec 2009 13:56:57 +0100 Subject: [PATCH] vbox_tmpl.c: avoid NULL deref upon vboxDomainCreateXML failure
* src/vbox/vbox_tmpl.c (vboxDomainCreateXML): Don't call vboxDomainUndefine on a NULL "dom". --- src/vbox/vbox_tmpl.c | 16 +++++----------- 1 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index d6b681c..ba70053 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -990,8 +990,6 @@ cleanup:
static virDomainPtr vboxDomainCreateXML(virConnectPtr conn, const char *xml, unsigned int flags ATTRIBUTE_UNUSED) { - virDomainPtr dom = NULL; - /* VirtualBox currently doesn't have support for running * virtual machines without actually defining them and thus * for time being just define new machine and start it. @@ -1000,17 +998,13 @@ static virDomainPtr vboxDomainCreateXML(virConnectPtr conn, const char *xml, * change this behaviour to the expected one. */
- dom = vboxDomainDefineXML(conn, xml); - if (dom) { - if (vboxDomainCreate(dom) < 0) - goto cleanup; - } else { - goto cleanup; - } + virDomainPtr dom = vboxDomainDefineXML(conn, xml); + if (dom == NULL) + return NULL;
- return dom; + if (0 < vboxDomainCreate(dom)) + return dom;
This check is wrong. You meant to check for !(vboxDomainCreate(dom) < 0) but resolved the ! incorrectly.
I reversed the condition, but left off the "=".
Either change it to
if (vboxDomainCreate(dom) >= 0) return dom;
or keep the negative check
if (vboxDomainCreate(dom) < 0) { vboxDomainUndefine(dom); return NULL; }
return dom;
I prefer the second version.
So do I. Here's the adjusted patch.
From 7e8355ea7c2e50995b139322de574ea4abf24fe3 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 16 Dec 2009 13:56:57 +0100 Subject: [PATCH] vbox_tmpl.c: avoid NULL deref upon vboxDomainCreateXML failure
* src/vbox/vbox_tmpl.c (vboxDomainCreateXML): Don't call vboxDomainUndefine on a NULL "dom". --- src/vbox/vbox_tmpl.c | 19 +++++++------------ 1 files changed, 7 insertions(+), 12 deletions(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 5a1d8dc..5889f32 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -990,8 +990,6 @@ cleanup:
static virDomainPtr vboxDomainCreateXML(virConnectPtr conn, const char *xml, unsigned int flags ATTRIBUTE_UNUSED) { - virDomainPtr dom = NULL; - /* VirtualBox currently doesn't have support for running * virtual machines without actually defining them and thus * for time being just define new machine and start it. @@ -1000,19 +998,16 @@ static virDomainPtr vboxDomainCreateXML(virConnectPtr conn, const char *xml, * change this behaviour to the expected one. */
- dom = vboxDomainDefineXML(conn, xml); - if (dom) { - if (vboxDomainCreate(dom) < 0) - goto cleanup; - } else { - goto cleanup; + virDomainPtr dom = vboxDomainDefineXML(conn, xml); + if (dom == NULL) + return NULL; + + if (vboxDomainCreate(dom) < 0) { + vboxDomainUndefine(dom); + return NULL; }
return dom; - -cleanup: - vboxDomainUndefine(dom); - return NULL; }
static virDomainPtr vboxDomainLookupByID(virConnectPtr conn, int id) { -- 1.6.6.387.g2649b1
ACK. Matthias