Similar to others, >From 5dcb4d95a351cb6fa32ebaafb96756275e1079e2 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com&gt; 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;

-cleanup:     vboxDomainUndefine(dom);     return NULL;  } -- 1.6.6.rc2.275.g51e2d

Matthias Bolte wrote: > 2009/12/16 Jim Meyering <jim(a)meyering.net&gt;: >> Similar to others, >> >> >From 5dcb4d95a351cb6fa32ebaafb96756275e1079e2 Mon Sep 17 00:00:00 2001 >> From: Jim Meyering <meyering(a)redhat.com&gt; >> 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(a)redhat.com&gt; 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

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.

From e47e126afe21b55a3f94a4cde3d9e0b4616de9cb Mon Sep 17 00:00:00 2001