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

[Matthias Bolte]

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