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@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