Re: [PATCH 11/11] cutils: Improve qemu_strtosz handling of fractions

[Hanna Czenczek]

On 08.05.23 22:03, Eric Blake wrote:
> We have several limitations and bugs worth fixing; they are
> inter-related enough that it is not worth splitting this patch into
> smaller pieces:
>
> * ".5k" should work to specify 512, just as "0.5k" does
> * "1.9999k" and "1." + "9"*50 + "k" should both produce the same
>    result of 2048 after rounding
> * "1." + "0"*350 + "1B" should not be treated the same as "1.0B";
>    underflow in the fraction should not be lost
> * "7.99e99" and "7.99e999" look similar, but our code was doing a
>    read-out-of-bounds on the latter because it was not expecting ERANGE
>    due to overflow. While we document that scientific notation is not
>    supported, and the previous patch actually fixed
>    qemu_strtod_finite() to no longer return ERANGE overflows, it is
>    easier to pre-filter than to try and determine after the fact if
>    strtod() consumed more than we wanted.  Note that this is a
>    low-level semantic change (when endptr is not NULL, we can now
>    successfully parse with a scale of 'E' and then report trailing
>    junk, instead of failing outright with EINVAL); but an earlier
>    commit already argued that this is not a high-level semantic change
>    since the only caller passing in a non-NULL endptr also checks that
>    the tail is whitespace-only.
>
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1629
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   tests/unit/test-cutils.c | 51 +++++++++++------------
>   util/cutils.c            | 89 ++++++++++++++++++++++++++++------------
>   2 files changed, 88 insertions(+), 52 deletions(-)

[...]

> diff --git a/util/cutils.c b/util/cutils.c
> index 0e056a27a44..d1dfbc69d16 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c

[...]

> @@ -246,27 +244,66 @@ static int do_strtosz(const char *nptr, const char **end,
>               retval = -EINVAL;
>               goto out;
>           }
> -    } else if (*endptr == '.') {
> +    } else if (*endptr == '.' || (endptr == nptr && strchr(nptr, '.'))) {

What case is there where we have a fraction but *endptr != '.'?

>           /*
>            * Input looks like a fraction.  Make sure even 1.k works
> -         * without fractional digits.  If we see an exponent, treat
> -         * the entire input as invalid instead.
> +         * without fractional digits.  strtod tries to treat 'e' as an
> +         * exponent, but we want to treat it as a scaling suffix;
> +         * doing this requires modifying a copy of the fraction.
>            */
> -        double fraction;
> +        double fraction = 0.0;
>
> -        f = endptr;
> -        retval = qemu_strtod_finite(f, &endptr, &fraction);
> -        if (retval) {
> +        if (retval == 0 && *endptr == '.' && !isdigit(endptr[1])) {
> +            /* If we got here, we parsed at least one digit already. */
>               endptr++;
> -        } else if (memchr(f, 'e', endptr - f) || memchr(f, 'E', endptr - f)) {
> -            endptr = nptr;
> -            retval = -EINVAL;
> -            goto out;
>           } else {
> -            /* Extract into a 64-bit fixed-point fraction. */
> +            char *e;
> +            const char *tail;
> +            g_autofree char *copy = g_strdup(endptr);
> +
> +            e = strchr(copy, 'e');
> +            if (e) {
> +                *e = '\0';
> +            }
> +            e = strchr(copy, 'E');
> +            if (e) {
> +                *e = '\0';
> +            }
> +            /*
> +             * If this is a floating point, we are guaranteed that '.'
> +             * appears before any possible digits in copy.  If it is
> +             * not a floating point, strtod will fail.  Either way,
> +             * there is now no exponent in copy, so if it parses, we
> +             * know 0.0 <= abs(result) <= 1.0 (after rounding), and
> +             * ERANGE is only possible on underflow which is okay.
> +             */
> +            retval = qemu_strtod_finite(copy, &tail, &fraction);
> +            endptr += tail - copy;
> +        }
> +
> +        /* Extract into a 64-bit fixed-point fraction. */
> +        if (fraction == 1.0) {
> +            if (val == UINT64_MAX) {
> +                retval = -ERANGE;
> +                goto out;
> +            }
> +            val++;
> +        } else if (retval == -ERANGE) {
> +            /* See comments above about underflow */
> +            valf = 1;

It doesn’t really matter because even an EiB is just 2^60, and so 1 EiB 
* 2^-64 (the resolution of our fractional part) is still less than 1, but:

DBL_MIN * 0x1p64 is 2^-(1022-64) == 2^-958, i.e. much less than 1, so 
I’d set valf to 0 here.

(If you put “.00000000000000000001” into this, there won’t be an 
underflow, but the value is so small that valf ends up 0.  But if you 
put `.$(yes 0 | head -n 307 | tr -d '\n')1` into this, there will be an 
underflow, setting valf to 1, even though the value is smaller.)

Hanna

> +            retval = 0;
> +        } else {
>               valf = (uint64_t)(fraction * 0x1p64);
>           }
>       }
> +    if (retval) {
> +        goto out;
> +    }
> +    if (memchr(nptr, '-', endptr - nptr) != NULL) {
> +        endptr = nptr;
> +        retval = -EINVAL;
> +        goto out;
> +    }
>       c = *endptr;
>       mul = suffix_mul(c, unit);
>       if (mul > 0) {