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

[Matthias Bolte <matthias.bolte@xxxxxxxxxxxxxx>]

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.

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.

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.

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

Matthias

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