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

[Eric Blake]

On Fri, May 19, 2023 at 04:42:11PM +0200, Hanna Czenczek wrote:
> 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,

Adding context:

long long lresult;
...
lresult = strtoull(nptr, &ep, 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)

Yes.  Not only will I take that as a hint that I should improve my
commit message, but you have pointed out a hole in my unit testing and
which is not fixed here; namely, strings like "-0xffffffffffff0000"
are accepted when they should be rejected.

I'm thinking the commit message should be something along the lines of
the following:

Note that strtoull takes two input ranges that overlap into a single
output range: [-0xffffffffffffffff, -1] which maps to [1,
0xffffffffffffffff], and [0, 0xffffffffffffffff] maps to itself.  The
output value alone does not tell us whether the input was in the
desired range [-0xffffffff, 0xffffffff], or in one of the other two
ranges that also map to the desired range [-0xffffffffffffffff,
-0xffffffff00000001] and [0xffffffff00000001, 0xffffffffffffffff], so
we need to do some additional filtering.  Merely checking whether
strlen(input) is longer than strlen("-4294967295") is not going to
work, because ("000000000000000000000000000") is longer but in range;
but we can check for the presence or absence of '-' in the input being
inconsistent with the resulting sign when the number is cast as a
signed long long.

v3 of this series will come soon, once you finish finding anything
else in v2 I need to fix.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org