Re: [PATCH v2 05/19] cutils: Fix wraparound parsing in qemu_strtoui

[Hanna Czenczek]

On 12.05.23 04:10, Eric Blake wrote:
> While we were matching 32-bit strtol in qemu_strtoi, our use of a
> 64-bit parse was leaking through for some inaccurate answers in
> qemu_strtoui in comparison to a 32-bit strtoul.  Fix those, and update
> the testsuite now that our bounds checks are correct.
>
> Our int wrappers would be a lot easier to write if libc had a
> guaranteed 32-bit parser even on platforms with 64-bit long.
>
> Fixes: 473a2a331e ("cutils: add qemu_strtoi & qemu_strtoui parsers for int/unsigned 
> int types", v2.12.0)
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   tests/unit/test-cutils.c | 11 +++++------
>   util/cutils.c            | 14 ++++++++++----
>   2 files changed, 15 insertions(+), 10 deletions(-)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

> diff --git a/util/cutils.c b/util/cutils.c
> index 5887e744140..997ddcd09e5 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -466,10 +466,16 @@ int qemu_strtoui(const char *nptr, const char **endptr, 
> int base,
>       if (errno == ERANGE) {
>           *result = -1;
>       } else {
> -        if (lresult > UINT_MAX) {
> -            *result = UINT_MAX;
> -            errno = ERANGE;
> -        } else if (lresult < INT_MIN) {
> +        /*
> +         * Note that platforms with 32-bit strtoul accept input in the
> +         * range [-4294967295, 4294967295]; but we used 64-bit
> +         * strtoull which wraps -18446744073709551615 to 1.  Reject
> +         * positive values that contain '-', and wrap all valid
> +         * negative values.
> +         */
> +        if (lresult > UINT_MAX ||
> +            lresult < -(long long)UINT_MAX ||
> +            (lresult > 0 && memchr(nptr, '-', ep - nptr))) {
>               *result = UINT_MAX;
>               errno = ERANGE;
>           } else {

Just a question whether I guessed correctly, because there’s no comment 
on the matter: We store the (supposedly unsigned) result of strtoull() 
in a signed long long because e.g. -1 is mapped to ULLONG_MAX, so the 
valid unsigned ranges would be [0, UINT_MAX] \cup [ULLONG_MAX - UINT_MAX 
+ 1, ULLONG_MAX], which is more cumbersome to check than the [-UINT_MAX, 
UINT_MAX] range?  (And we’d need to exclude strings with - in them if 
ullresult > UINT_MAX rather than > 0, probably)

Hanna