[libvirt] [PATCH] PHYP: Checking for NULL values when building new guest

[Matthias Bolte]

2010/8/20 Eduardo Otubo <otubo at linux.vnet.ibm.com>:
> When creating a new gust, the function phypBuildLpar() was not
> checking for NULL values, making the driver to have a segmentation
> fault.
> ---
>  src/phyp/phyp_driver.c |   19 +++++++++++++++++++
>  1 files changed, 19 insertions(+), 0 deletions(-)
>
> diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
> index 251111d..999870e 100644
> --- a/src/phyp/phyp_driver.c
> +++ b/src/phyp/phyp_driver.c
> @@ -3701,6 +3701,25 @@ phypBuildLpar(virConnectPtr conn, virDomainDefPtr def)
>     int exit_status = 0;
>     virBuffer buf = VIR_BUFFER_INITIALIZER;
>
> +    if (!def->name) {
> +        VIR_ERROR0(_("Field \"<name>\" on the domain XML file missing."));
> +        goto err;

def->name cannot be NULL, virDomainDefParseXML fails when there is no
name given in the domain XML

> +    } else if (!def->memory) {
> +        VIR_ERROR0(_
> +                   ("Field \"<memory>\" on the domain XML file missing."));
> +        goto err;
> +    } else if (!def->maxmem) {
> +        VIR_ERROR0(_
> +                   ("Field \"<currentMemory>\" on the domain XML file missing."));
> +        goto err;

memory and maxmem might be 0 when the user sets the to 0 in the domain
XML. IMHO comparing > 0 is cleaner here.

Also the error message is misleading here. The elements aren't
missing, but the user might have specified a value that is invalid for
this driver.

> +    } else if (!def->vcpus) {
> +        VIR_ERROR0(_("Field \"<vcpu>\" on the domain XML file missing."));
> +        goto err;

vcpus can only be 0 when the user explicitly specified it that way,
the vcpu element isn't missing in that case. If the element is really
missing then vcpus defaults to 1.

> +    } else if (!def->disks[0]->src) {
> +        VIR_ERROR0(_("Field \"<disk>\" on the domain XML file missing."));
> +        goto err;
> +    }

This can segfault because you dereference the first disk element
without even checking if there are any disks.

Once again the error message is misleading.

Also you should use PHYP_ERROR instead of VIR_ERROR here, because
VIR_ERROR will just output the error to the log and doesn't raise an
error at the libvirt API level, but this is required when a driver
functions fails.

In general all other occurrences of  VIR_ERROR in this driver should
be replaced by PHYP_ERROR for the same reason.

There are also several instances of VIR_WARN followed by goto err that
should be PHYP_ERRORs too.

Matthias