On Fri, May 19, 2023 at 04:27:10PM +0200, Hanna Czenczek wrote:
> On 12.05.23 04:10, Eric Blake wrote:
> > We have quite a few undertested and underdocumented integer parsing
> > corner cases. To ensure that any changes we make in the code are
> > intentional rather than accidental semantic changes, it is time to add
> > more unit tests of existing behavior.
> >
> > In particular, this demonstrates that parse_uint() and qemu_strtou64()
> > behave differently. For "-0", it's hard to argue why parse_uint needs
> > to reject it (it's not a negative integer), but the documentation sort
> > of mentions it; but it is intentional that all other negative values
> > are treated as ERANGE with value 0 (compared to qemu_strtou64()
> > treating "-2" as success and UINT64_MAX-1, for example).
> >
> > Also, when mixing overflow/underflow with a check for no trailing
> > junk, parse_uint_full favors ERANGE over EINVAL, while qemu_strto[iu]*
> > favor EINVAL. This behavior is outside the C standard, so we can pick
> > whatever we want, but it would be nice to be consistent.
> >
> > Note that C requires that "9223372036854775808" fail strtoll() with
> > ERANGE/INT64_MAX, but "-9223372036854775808" pass with INT64_MIN; we
> > weren't testing this. For strtol(), the behavior depends on whether
> > long is 32- or 64-bits (the cutoff point either being the same as
> > strtoll() or at "-2147483648"). Meanwhile, C is clear that
> > "-18446744073709551615" pass stroull() (but not strtoll) with value 1,
> > even though we want it to fail parse_uint(). And although
> > qemu_strtoui() has no C counterpart, it makes more sense if we design
> > it like 32-bit strtoul() (that is, where "-4294967296" be an alternate
> > acceptable spelling for "1". We aren't there yet, so some of the
> > tests added in this patch have FIXME comments.
> >
> > Signed-off-by: Eric Blake <ebl...@redhat.com>
> > ---
> > tests/unit/test-cutils.c | 799 ++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 738 insertions(+), 61 deletions(-)
> >
> > diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c
> > index 1eeaf21ae22..89c10f5307a 100644
> > --- a/tests/unit/test-cutils.c
> > +++ b/tests/unit/test-cutils.c
>
> [...]
>
> > @@ -717,34 +890,75 @@ static void test_qemu_strtoui_max(void)
> >
> > static void test_qemu_strtoui_overflow(void)
> > {
> > - char *str = g_strdup_printf("%lld", (long long)UINT_MAX + 1ll);
> > - char f = 'X';
> > - const char *endptr = &f;
> > - unsigned int res = 999;
> > + const char *str;
> > + const char *endptr;
> > + unsigned int res;
> > int err;
> >
> > + str = "4294967296"; /* UINT_MAX + 1ll */
> > + endptr = "somewhere";
> > + res = 999;
> > err = qemu_strtoui(str, &endptr, 0, &res);
> > + g_assert_cmpint(err, ==, -ERANGE);
> > + g_assert_cmpint(res, ==, UINT_MAX);
>
> Why cmpint and not cmpuint here? (I see you’re using cmpint instead of
> cmpuint in many strtou* test functions below, too.)
Probably a combination of copy-paste vs rebase patch re-ordering.
Yes, all test_*strtoui* tests should be using
g_assert_cmpuint(res...). Will fix.
>
> [...]
>
> > @@ -1325,31 +1697,67 @@ static void test_qemu_strtoul_max(void)
>
> [...]
>
> > static void test_qemu_strtoul_underflow(void)
> > {
> > - const char *str = "-99999999999999999999999999999999999999999999";
> > - char f = 'X';
> > - const char *endptr = &f;
> > - unsigned long res = 999;
> > + const char *str;
> > + const char *endptr;
> > + unsigned long res;
> > int err;
> >
> > + /* 1 less than -ULONG_MAX */
> > + str = ULONG_MAX == UINT_MAX ? "-4294967297" : "-18446744073709551617";
>
> Technically these are 2 less than -ULONG_MAX, not 1 less.
Indeed. Both constants should end in 6, not 7. Will fix.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
