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 <ebl...@redhat.com>
> > ---
> > tests/unit/test-cutils.c | 11 +++++------
> > util/cutils.c | 14 ++++++++++----
> > 2 files changed, 15 insertions(+), 10 deletions(-)
>
> Reviewed-by: Hanna Czenczek <hre...@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
