[libvirt] [PATCH] virconf: clarify type range checks for integers

[Andrea Bolognani]

On Thu, 2016-07-14 at 11:22 +0100, Daniel P. Berrange wrote:
> When virConf 'l' field is a 'signed long long', so whenever

s/When//

> the 'type' field is VIR_CONF_ULONG, we should explicitly cast
> 'l' to a 'unsigned long long' before doing range checks.
> 
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> ---
>  src/util/virconf.c | 81 +++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 49 insertions(+), 32 deletions(-)

This doesn't apply any longer after b7caf4fcd2ab, but rebasing
it is trivial.

> diff --git a/src/util/virconf.c b/src/util/virconf.c
> index 5085768..f82d114 100644
> --- a/src/util/virconf.c
> +++ b/src/util/virconf.c
> @@ -1208,21 +1208,28 @@ int virConfGetValueSizeT(virConfPtr conf,
>      if (!cval)
>          return 0;
>  
> -    if (cval->type != VIR_CONF_ULONG) {
> +    if (cval->type == VIR_CONF_LONG) {
> +        if (cval->l < 0 || cval->l > SIZE_MAX) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("%s: value for '%s' parameter must be in range 0:%zu"),
> +                           conf->filename, setting, SIZE_MAX);
> +            return -1;
> +        }
> +   } else if (cval->type == VIR_CONF_ULONG) {
> +        if (((unsigned long long)cval->l) > SIZE_MAX) {

I personally prefer

  (type) var

when doing casts, but I won't oppose it if you prefer to
keep it this way. Note, though, that the other style is
already used in the very same module...

> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("%s: value for '%s' parameter must be in range 0:%zu"),
> +                           conf->filename, setting, SIZE_MAX);
> +            return -1;
> +        }
> +    } else {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("%s: expected an unsigned integer for '%s' parameter"),
>                         conf->filename, setting);
>          return -1;
>      }
>  
> -    if (cval->l > SIZE_MAX || cval->l < 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("%s: value for '%s' parameter must be in range 0:%zu"),
> -                       conf->filename, setting, SIZE_MAX);
> -        return -1;
> -    }
> -
> -    *value = cval->l;
> +    *value = (size_t)cval->l;
>  
>      return 1;
>  }
> @@ -1256,22 +1263,28 @@ int virConfGetValueSSizeT(virConfPtr conf,
>      if (!cval)
>          return 0;
>  
> -    if (cval->type != VIR_CONF_LONG &&
> -        cval->type != VIR_CONF_ULONG) {
> +    if (cval->type == VIR_CONF_ULONG) {
> +        if (((unsigned long long)cval->l) > SSIZE_MAX) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("%s: value for '%s' parameter must be in range %zd:%zd"),
> +                           conf->filename, setting, -SSIZE_MAX - 1, SSIZE_MAX);

These need of course to be worked around same as b7caf4fcd2ab
not to break building on 32 bit.

> +            return -1;
> +        }
> +    } else if (cval->type == VIR_CONF_LONG) {
> +        if (cval->l < (-SSIZE_MAX - 1) || cval->l > SSIZE_MAX) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("%s: value for '%s' parameter must be in range %zd:%zd"),
> +                           conf->filename, setting, -SSIZE_MAX - 1, SSIZE_MAX);

Same here.

> +            return -1;
> +        }
> +    } else {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("%s: expected a signed integer for '%s' parameter"),
>                         conf->filename, setting);
>          return -1;
>      }
>  
> -    if (cval->l > SSIZE_MAX || cval->l < (-SSIZE_MAX - 1)) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("%s: value for '%s' parameter must be in range %zd:%zd"),
> -                       conf->filename, setting, -SSIZE_MAX - 1, SSIZE_MAX);
> -        return -1;
> -    }
> -
> -    *value = cval->l;
> +    *value = (ssize_t)cval->l;
>  
>      return 1;
>  }
> @@ -1305,22 +1318,20 @@ int virConfGetValueLLong(virConfPtr conf,
>      if (!cval)
>          return 0;
>  
> -    if (cval->type != VIR_CONF_LONG &&
> -        cval->type != VIR_CONF_ULONG) {
> +    if (cval->type == VIR_CONF_ULONG) {
> +        if (((unsigned long long)cval->l) > LLONG_MAX) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("%s: value for '%s' parameter must be in range 0:%lld"),
> +                           conf->filename, setting, LLONG_MAX);
> +            return -1;
> +        }
> +    } else if (cval->type != VIR_CONF_LONG) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("%s: expected a signed integer for '%s' parameter"),
>                         conf->filename, setting);
>          return -1;
>      }
>  
> -    if (cval->type == VIR_CONF_ULONG &&
> -        cval->l > LLONG_MAX) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("%s: value for '%s' parameter must be in range 0:%lld"),
> -                       conf->filename, setting, LLONG_MAX);
> -        return -1;
> -    }
> -
>      *value = cval->l;
>  
>      return 1;
> @@ -1354,15 +1365,21 @@ int virConfGetValueULLong(virConfPtr conf,
>      if (!cval)
>          return 0;
>  
> -    if (cval->type != VIR_CONF_LONG &&
> -        cval->type != VIR_CONF_ULONG) {
> +    if (cval->type == VIR_CONF_LONG) {
> +        if (cval->l < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("%s: value for '%s' parameter must be in range 0:%llu"),
> +                           conf->filename, setting, ULLONG_MAX);
> +            return -1;
> +        }
> +    } else if (cval->type != VIR_CONF_ULONG) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("%s: expected an unsigned integer for '%s' parameter"),
>                         conf->filename, setting);
>          return -1;
>      }
>  
> -    *value = cval->l;
> +    *value = (unsigned long long)cval->l;
>  
>      return 1;
>  }

ACK with the above taken care of.

-- 
Andrea Bolognani / Red Hat / Virtualization