[libvirt] [PATCH 6/7] conf: check the return value of virXPathNodeSet

[Osier Yang]

On 2012年11月28日 21:34, Ján Tomko wrote:
> In a few places, the return value could get passed to VIR_ALLOC_N without
> being checked, resulting in a request to allocate a lot of memory if the
> return value was negative.

Isn't it expected to fail? calloc fails that on Linux, but not sure what
the behaviour on other platform.

> ---
>   src/conf/domain_conf.c  |    8 ++++++--
>   src/conf/storage_conf.c |    4 +++-
>   2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 2ca608f..814859a 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3258,7 +3258,9 @@ virSecurityLabelDefsParseXML(virDomainDefPtr def,
>       saved_node = ctxt->node;
>
>       /* Allocate a security labels based on XML */
> -    if ((n = virXPathNodeSet("./seclabel", ctxt,&list)) == 0)
> +    if ((n = virXPathNodeSet("./seclabel", ctxt,&list))<  0)
> +        goto error;
> +    if (n == 0)
>           return 0;

but anyway, this is good fix, even calloc will fails, the error just
points to the wrong direction.

>
>       if (VIR_ALLOC_N(def->seclabels, n)<  0) {
> @@ -3345,7 +3347,9 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn,
>       virSecurityLabelDefPtr vmDef = NULL;
>       char *model, *relabel, *label;
>
> -    if ((n = virXPathNodeSet("./seclabel", ctxt,&list)) == 0)
> +    if ((n = virXPathNodeSet("./seclabel", ctxt,&list))<  0)
> +        goto error;
> +    if (n == 0)
>           return 0;
>
>       if (VIR_ALLOC_N(seclabels, n)<  0) {
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 5a2cf1b..feeba17 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -510,7 +510,9 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
>           VIR_FREE(format);
>       }
>
> -    source->nhost = virXPathNodeSet("./host", ctxt,&nodeset);
> +    if ((i = virXPathNodeSet("./host", ctxt,&nodeset))<  0)
> +        goto cleanup;
> +    source->nhost = i;

I'd like have a new var like "n", instead of reusing "i", which
is used for loop index in convention.

>
>       if (source->nhost) {
>           if (VIR_ALLOC_N(source->hosts, source->nhost)<  0) {

ACK. I will use "n" when pushing.

Osier