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

2010/1/5 Jim Meyering <jim@meyering.net>:

Matthias Bolte wrote: ...

...

The original code also contains a domain pointer reference leak. vboxDomainUndefine does not unref the domain pointer, so there is a virUnrefDomain call missing in the error case. So this could be changed to

    if (vboxDomainCreate(dom) < 0) {         vboxDomainUndefine(dom);         virUnrefDomain(dom);         return NULL;     }

    return dom;

But I think this is worth a separate commit.

Indeed. Here's the proposed patch:

From e47e126afe21b55a3f94a4cde3d9e0b4616de9cb Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 5 Jan 2010 17:45:46 +0100 Subject: [PATCH] vbox_tmpl.c: don't leak a domain pointer upon failure to create

* src/vbox/vbox_tmpl.c (vboxDomainCreateXML): "Unref" the domain upon failure.  Patch by Mattias Bolte.

I would like my name to be spelled correctly with 'tth' :)

---  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 5889f32..07696c0 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -1004,6 +1004,7 @@ static virDomainPtr vboxDomainCreateXML(virConnectPtr conn, const char *xml,

    if (vboxDomainCreate(dom) < 0) {         vboxDomainUndefine(dom); +        virUnrefDomain(dom);         return NULL;     }

-- 1.6.6.387.g2649b1

ACK, when my name is spelled correct. Matthias