Re: [libvirt] [PATCH] vbox_tmpl.c: avoid NULL deref upon vboxDomainCreateXML failure

[Jim Meyering <jim@xxxxxxxxxxxx>]

Matthias Bolte wrote:
> 2009/12/16 Jim Meyering <jim@xxxxxxxxxxxx>:
>> Similar to others,
>>
>> >From 5dcb4d95a351cb6fa32ebaafb96756275e1079e2 Mon Sep 17 00:00:00 2001
>> From: Jim Meyering <meyering@xxxxxxxxxx>
>> 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@xxxxxxxxxx>
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

--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list