qdev: Validate hex properties

strtoull() might return '-1' to signify an overflow.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 hw/core/qdev-properties.c | 9 +++++++++
 1 file changed, 9 insertions(+)

On 26 November 2013 15:05, Hannes Reinecke <hare@suse.de> wrote:
> strtoull() might return '-1' to signify an overflow.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  hw/core/qdev-properties.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index dc8ae69..5761295 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -202,6 +202,9 @@ static int parse_hex8(DeviceState *dev, Property *prop, const char *str)
>      if ((*end != '\0') || (end == str)) {
>          return -EINVAL;
>      }
> +    if (*ptr == (uint8_t)-1) {
> +        return -ERANGE;
> +    }

This doesn't look right -- *ptr might legitimately be anything
from 0x00 to 0xff; -1 can be a valid strtoul() return as well as
the error indication.

The right way to check for overflow is to clear errno before the call
to strtoul(), and then check whether it's non-zero after the call;
this is the approach recommended by POSIX.
http://pubs.opengroup.org/onlinepubs/9699919799/functions/strtoul.html

For the hex8 version where we're putting the result in a smaller
sized integer than the type returned by strtoul(), we should
put the result into an unsigned long, check it's in 0..255, and
then stick it in the uint8_t.

thanks
-- PMM

On 11/26/2013 08:05 AM, Hannes Reinecke wrote:
> strtoull() might return '-1' to signify an overflow.

Yes, but -1 (aka ULLONG_MAX) is also a valid return when there is not
overflow, so testing for -1 is NOT a reliable indicator on whether
overflow occurred.  Also, you mention strtoull() here...

> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  hw/core/qdev-properties.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index dc8ae69..5761295 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -202,6 +202,9 @@ static int parse_hex8(DeviceState *dev, Property *prop, const char *str)
>      if ((*end != '\0') || (end == str)) {

...but this was calling strtoul().

>          return -EINVAL;
>      }
> +    if (*ptr == (uint8_t)-1) {
> +        return -ERANGE;
> +    }

NAK.  This is NOT how you check for overflow with strtoull.  Instead,
you should use:

errno = 0;
*ptr = strtoul(str, &end, 16);
if (errno) {
    return -errno;
}
if (!*end || end == str) {
    return -EINVAL;
}
return 0;

>  
>      return 0;
>  }
> @@ -333,6 +336,9 @@ static int parse_hex32(DeviceState *dev, Property *prop, const char *str)
>      if ((*end != '\0') || (end == str)) {
>          return -EINVAL;
>      }
> +    if (*ptr == (uint32_t)-1) {
> +        return -ERANGE;
> +    }

NAK.  And this function is even MORE broken.  On a 32-bit platform,
assigning the results of strtol() into a uint32_t may result in silent
truncation.  If you are trying to detect overflow, you MUST assign the
results into a long, then check that the long does not overflow
uint32_t, rather than checking (too late) whether the uint32_t has
already been overflowed.