| Message ID | 20230508200343.791450-7-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix qemu_strtosz() read-out-of-bounds | expand |
On 08.05.23 22:03, Eric Blake wrote: > Add some more strings that the user might send our way. In > particular, some of these additions include FIXME comments showing > where our parser doesn't quite behave the way we want. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > tests/unit/test-cutils.c | 226 +++++++++++++++++++++++++++++++++++++-- > 1 file changed, 215 insertions(+), 11 deletions(-) I wonder: The plan is to have "1.5e+1k" be parsed as "1.5e" + endptr == "+1k"; but "0x1p1" is not parsed at all (could be "0x1" + "p1"). Is that fully intentional? (Similarly, "1.1.k" is also not parsed at all, but the problem there is not just two decimal points, but also that "1.1" would be an invalid size in itself, so it really shouldn’t be parsed at all.) I don’t think it matters to users, really, but I still wonder. > diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c > index afae2ee5331..9fa6fb042e8 100644 > --- a/tests/unit/test-cutils.c > +++ b/tests/unit/test-cutils.c [...] > @@ -2875,6 +3056,20 @@ static void test_qemu_strtosz_trailing(void) > err = qemu_strtosz(str, NULL, &res); > g_assert_cmpint(err, ==, -EINVAL); > g_assert_cmphex(res, ==, 0xbaadf00d); > + > + /* FIXME overflow in fraction is buggy */ > + str = "1.5E999"; > + endptr = NULL; > + res = 0xbaadf00d; > + err = qemu_strtosz(str, &endptr, &res); > + g_assert_cmpint(err, ==, 0); > + g_assert_cmpuint(res, ==, EiB /* FIXME EiB * 1.5 */); > + g_assert(endptr == str + 9 /* FIXME + 4 */); This is “correct” (i.e. it’s the value we’ll get right now, which is the wrong one), but gcc complains that the array index is out of bounds (well...), which breaks the build. Hanna > + > + res = 0xbaadf00d; > + err = qemu_strtosz(str, NULL, &res); > + g_assert_cmpint(err, ==, -EINVAL); > + g_assert_cmphex(res, ==, 0xbaadf00d); > }
On 09.05.23 14:31, Hanna Czenczek wrote: > On 08.05.23 22:03, Eric Blake wrote: >> Add some more strings that the user might send our way. In >> particular, some of these additions include FIXME comments showing >> where our parser doesn't quite behave the way we want. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> tests/unit/test-cutils.c | 226 +++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 215 insertions(+), 11 deletions(-) > > I wonder: The plan is to have "1.5e+1k" be parsed as "1.5e" + endptr > == "+1k"; but "0x1p1" is not parsed at all (could be "0x1" + "p1"). Is > that fully intentional? > > (Similarly, "1.1.k" is also not parsed at all, but the problem there > is not just two decimal points, but also that "1.1" would be an > invalid size in itself, so it really shouldn’t be parsed at all.) > > I don’t think it matters to users, really, but I still wonder. > >> diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c >> index afae2ee5331..9fa6fb042e8 100644 >> --- a/tests/unit/test-cutils.c >> +++ b/tests/unit/test-cutils.c > > [...] > >> @@ -2875,6 +3056,20 @@ static void test_qemu_strtosz_trailing(void) >> err = qemu_strtosz(str, NULL, &res); >> g_assert_cmpint(err, ==, -EINVAL); >> g_assert_cmphex(res, ==, 0xbaadf00d); >> + >> + /* FIXME overflow in fraction is buggy */ >> + str = "1.5E999"; >> + endptr = NULL; >> + res = 0xbaadf00d; >> + err = qemu_strtosz(str, &endptr, &res); >> + g_assert_cmpint(err, ==, 0); >> + g_assert_cmpuint(res, ==, EiB /* FIXME EiB * 1.5 */); So… I have no idea what happens here but this always fails with “assertion failed (res == EiB): (1 == 1152921504606846976)”. But when I replace the EiB by 1, it suddenly fails with “assertion failed (res == 1): (1152921504606846976 == 1)” instead. Replacing the EiB by anything but 1 also tells me that res is 1. Now, here’s the kicker. I put an `fprintf(stderr, "res == %" PRIu64 "\n", res);` before this g_assert_cmpuint() (changed to (res, ==, 1))… And it passes. Sometimes I really want to change professions. (Of note is that changing the g_assert() below into a g_assert_true() also has g_assert_cmpuint(res, ==, 1) pass.) >> + g_assert(endptr == str + 9 /* FIXME + 4 */); > > This is “correct” (i.e. it’s the value we’ll get right now, which is > the wrong one), but gcc complains that the array index is out of > bounds (well...), which breaks the build. Oh, it also isn’t correct, I think it needs to be str + 8. As a bonus, the compiler doesn’t complain then (for some reason…? it still seems out of bounds). (Otherwise, to get around the complaint, I used g_assert_cmphex((uintptr_t)endptr, ==, (uintptr_t)str + 8). Which is another thing, patch 1 explained to me that we shouldn’t use g_assert() :)) Hanna
On 08.05.23 22:03, Eric Blake wrote: > Add some more strings that the user might send our way. In > particular, some of these additions include FIXME comments showing > where our parser doesn't quite behave the way we want. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > tests/unit/test-cutils.c | 226 +++++++++++++++++++++++++++++++++++++-- > 1 file changed, 215 insertions(+), 11 deletions(-) > > diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c > index afae2ee5331..9fa6fb042e8 100644 > --- a/tests/unit/test-cutils.c > +++ b/tests/unit/test-cutils.c [...] > @@ -2875,6 +3056,20 @@ static void test_qemu_strtosz_trailing(void) > err = qemu_strtosz(str, NULL, &res); > g_assert_cmpint(err, ==, -EINVAL); > g_assert_cmphex(res, ==, 0xbaadf00d); > + > + /* FIXME overflow in fraction is buggy */ > + str = "1.5E999"; > + endptr = NULL; > + res = 0xbaadf00d; > + err = qemu_strtosz(str, &endptr, &res); > + g_assert_cmpint(err, ==, 0); > + g_assert_cmpuint(res, ==, EiB /* FIXME EiB * 1.5 */); > + g_assert(endptr == str + 9 /* FIXME + 4 */); > + > + res = 0xbaadf00d; > + err = qemu_strtosz(str, NULL, &res); > + g_assert_cmpint(err, ==, -EINVAL); > + g_assert_cmphex(res, ==, 0xbaadf00d); Got it now! Our problem is that `endptr` is beyond the end of the string, precisely as gcc complains. The data there is undefined, and depending on the value in the g_assert_cmpuint() (which is converted to strings for the potential error message) it sometimes is "endptr == str + 9" (the one in the g_assert()) and sometimes isn’t. If it is "endptr == str + 9", then the 'e' is taken as a suffix, which makes `res == EiB`, and `endptr == "ndptr == str + 9"`. If it isn’t, well, it might be anything, so there often is no valid suffix, making `res == 1`. So the solution is to set `str = "1.5E999\0\0"`, so we don’t get out of bounds and know exactly what will be parsed. Then, at str[8] there is no valid suffix (it’s \0), so `res == 1` and `endptr == str + 8`. This will then lead to the qemu_strtosz(str, NULL, &res) below succeed, because, well, it’s a valid number. I suppose it failed on your end because the out-of-bounds `str[9]` value was not '\0'. That was a fun debugging session. Hanna
On Tue, May 09, 2023 at 05:15:04PM +0200, Hanna Czenczek wrote: > On 08.05.23 22:03, Eric Blake wrote: > > Add some more strings that the user might send our way. In > > particular, some of these additions include FIXME comments showing > > where our parser doesn't quite behave the way we want. > > > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > > tests/unit/test-cutils.c | 226 +++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 215 insertions(+), 11 deletions(-) > > > > diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c > > index afae2ee5331..9fa6fb042e8 100644 > > --- a/tests/unit/test-cutils.c > > +++ b/tests/unit/test-cutils.c > > [...] > > > @@ -2875,6 +3056,20 @@ static void test_qemu_strtosz_trailing(void) > > err = qemu_strtosz(str, NULL, &res); > > g_assert_cmpint(err, ==, -EINVAL); > > g_assert_cmphex(res, ==, 0xbaadf00d); > > + > > + /* FIXME overflow in fraction is buggy */ > > + str = "1.5E999"; > > + endptr = NULL; > > + res = 0xbaadf00d; > > + err = qemu_strtosz(str, &endptr, &res); > > + g_assert_cmpint(err, ==, 0); > > + g_assert_cmpuint(res, ==, EiB /* FIXME EiB * 1.5 */); > > + g_assert(endptr == str + 9 /* FIXME + 4 */); > > + > > + res = 0xbaadf00d; > > + err = qemu_strtosz(str, NULL, &res); > > + g_assert_cmpint(err, ==, -EINVAL); > > + g_assert_cmphex(res, ==, 0xbaadf00d); > > Got it now! > > Our problem is that `endptr` is beyond the end of the string, precisely as > gcc complains. The data there is undefined, and depending on the value in > the g_assert_cmpuint() (which is converted to strings for the potential > error message) it sometimes is "endptr == str + 9" (the one in the > g_assert()) and sometimes isn’t. > > If it is "endptr == str + 9", then the 'e' is taken as a suffix, which makes > `res == EiB`, and `endptr == "ndptr == str + 9"`. > > If it isn’t, well, it might be anything, so there often is no valid suffix, > making `res == 1`. > > So the solution is to set `str = "1.5E999\0\0"`, so we don’t get out of > bounds and know exactly what will be parsed. Then, at str[8] there is no > valid suffix (it’s \0), so `res == 1` and `endptr == str + 8`. This will > then lead to the qemu_strtosz(str, NULL, &res) below succeed, because, well, > it’s a valid number. I suppose it failed on your end because the > out-of-bounds `str[9]` value was not '\0'. > > That was a fun debugging session. Wow, yeah, that's a mess. The very test proving that we have a read-out-of-bounds bug is super-sensitive to what is at that location. Your hack of passing in extra \0 is awesome; I'll fold that in whether we need a v2 for other reasons, or in-place if we can take the rest of this series as-is. It all goes away at the end of the series, anyways, once the out-of-bounds read is fixed.
On Tue, May 09, 2023 at 02:31:08PM +0200, Hanna Czenczek wrote: > On 08.05.23 22:03, Eric Blake wrote: > > Add some more strings that the user might send our way. In > > particular, some of these additions include FIXME comments showing > > where our parser doesn't quite behave the way we want. > > > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > > tests/unit/test-cutils.c | 226 +++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 215 insertions(+), 11 deletions(-) > > I wonder: The plan is to have "1.5e+1k" be parsed as "1.5e" + endptr == > "+1k"; but "0x1p1" is not parsed at all (could be "0x1" + "p1"). Is that > fully intentional? Pre-series: "1.5e+1k" is parsed as "1" ".5e+1" "k", no slop Post-series: "1.5e+1k" is parsed as "1" ".5" "e", slop "+1k" If you call qemu_strtosz(,NULL,), both versions still give EINVAL error (pre-patch: we detected an exponent in the middle portion of the parse, which is not allowed; post-series: we detect slop, which is not allowed). If you call qemu_strtosz(,&endptr), the pre-patch version fails with EINVAL but the new code succeeds. But of all the callers, the only one that passed non-NULL endptr did it's own check that *slop is only whitespace ("+1k" fails that check), so the end result is that at the high level, we reject "1.5e+1k" from the user, but the rejection is now at a different point in the call stack. The reason I made the change is that it was less code to guarantee that the internal qemu_strtod_finite() is passed a string that cannot possibly contain an exponent, than to blindly call qemu_strtod_finite() on an arbitrary suffix and then check after the fact on whether an exponent was actually consumed as part of that parse. For "0x1p1", the parse is "0x1" plus slop "p1" (unchanged by the series). This is rejected with EINVAL (the moment you have hex, we insist on no suffix or slop, regardless of endptr being NULL or not). I could have changed it similar to how I changed "1.5e+1k" to allow the parse at the qemu_strtosz layer and then detect the slop at the caller, but that was more lines of code and I didn't feel like it was worth it. Either way, the unit tests accurately cover the difference in parse behavior, and I tried to make the documentation (by patch 11/11) be consistent as well. But since this is why we have a review process, I'm open to feedback if you think I need to tweak which parse styles we should be favoring. > > (Similarly, "1.1.k" is also not parsed at all, but the problem there is not > just two decimal points, but also that "1.1" would be an invalid size in > itself, so it really shouldn’t be parsed at all.) "1.1k" is a valid (if unusual) size. This particular test addition did not happen to improve coverage for my final code, but given that qemu_strtod_finite(".1.k") would stop parsing after the ".1" and still be pointing at '.', and our read-out-of-bounds was caused by assuming that qemu_strtod_finite() failures leave *endptr == '.' (which was invalidated for ".9e999" failing with ERANGE), it didn't hurt to add the coverage. > > I don’t think it matters to users, really, but I still wonder. If any of our callers had actually done something like: "size %llx followed by junk %s" with the parsed value and the junk string, then it would matter. But since all of the callers either passed in NULL endptr (and got EINVAL before even knowing how much of the string was parsed) or insisted that the junk be whitespace only, and did not print details about the parsed value before reporting such errors, you're right that I don't see this as mattering to end users. But it took me quite a bit of audit time to convince myself of that. > > @@ -2875,6 +3056,20 @@ static void test_qemu_strtosz_trailing(void) > > err = qemu_strtosz(str, NULL, &res); > > g_assert_cmpint(err, ==, -EINVAL); > > g_assert_cmphex(res, ==, 0xbaadf00d); > > + > > + /* FIXME overflow in fraction is buggy */ > > + str = "1.5E999"; > > + endptr = NULL; > > + res = 0xbaadf00d; > > + err = qemu_strtosz(str, &endptr, &res); > > + g_assert_cmpint(err, ==, 0); > > + g_assert_cmpuint(res, ==, EiB /* FIXME EiB * 1.5 */); > > + g_assert(endptr == str + 9 /* FIXME + 4 */); > > This is “correct” (i.e. it’s the value we’ll get right now, which is the > wrong one), but gcc complains that the array index is out of bounds > (well...), which breaks the build. Huh - wonder what build settings you are using that I wasn't for seeing that warning - but it makes total sense that 'str + 9' would trigger a build failure if we don't pad str with enough '\0' to keep things in bounds.
On Mon, May 08, 2023 at 03:03:38PM -0500, Eric Blake wrote: > Add some more strings that the user might send our way. In > particular, some of these additions include FIXME comments showing > where our parser doesn't quite behave the way we want. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > tests/unit/test-cutils.c | 226 +++++++++++++++++++++++++++++++++++++-- > 1 file changed, 215 insertions(+), 11 deletions(-) > > @@ -2704,13 +2749,30 @@ static void test_qemu_strtosz_invalid(void) > > str = " \t "; > endptr = NULL; > + res = 0xbaadf00d; > err = qemu_strtosz(str, &endptr, &res); > g_assert_cmpint(err, ==, -EINVAL); > g_assert_cmphex(res, ==, 0xbaadf00d); > g_assert_true(endptr == str); > > + str = "."; > + endptr = NULL; > + res = 0xbaadf00d; > + err = qemu_strtosz(str, &endptr, &res); > + g_assert_cmpint(err, ==, -EINVAL); > + g_assert_cmphex(res, ==, 0xbaadf00d); > + g_assert(endptr == str); Rebase botch. I should be using g_assert_true() here in line with earlier in the series. I think I cleaned it up later in the series, but shouldn't be churning on it that badly. Looks like I get to send a v2 to fix this and other things; I'll wait another day for other reviews first. (That's what I get for rearranging patches after the fact for a nicer presentation order...)
diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c index afae2ee5331..9fa6fb042e8 100644 --- a/tests/unit/test-cutils.c +++ b/tests/unit/test-cutils.c @@ -2478,14 +2478,14 @@ static void test_qemu_strtosz_simple(void) g_assert_cmpuint(res, ==, 8); g_assert_true(endptr == str + 2); - /* Leading space is ignored */ - str = " 12345"; + /* Leading space and + are ignored */ + str = " +12345"; endptr = str; res = 0xbaadf00d; err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, 0); g_assert_cmpuint(res, ==, 12345); - g_assert_true(endptr == str + 6); + g_assert_true(endptr == str + 7); res = 0xbaadf00d; err = qemu_strtosz(str, NULL, &res); @@ -2564,13 +2564,13 @@ static void test_qemu_strtosz_hex(void) g_assert_cmpuint(res, ==, 171); g_assert_true(endptr == str + 4); - str = "0xae"; + str = " +0xae"; endptr = str; res = 0xbaadf00d; err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, 0); g_assert_cmpuint(res, ==, 174); - g_assert_true(endptr == str + 4); + g_assert_true(endptr == str + 6); } static void test_qemu_strtosz_units(void) @@ -2669,14 +2669,23 @@ static void test_qemu_strtosz_float(void) g_assert_cmpuint(res, ==, 1); g_assert_true(endptr == str + 4); - /* An empty fraction is tolerated */ - str = "1.k"; + /* An empty fraction tail is tolerated */ + str = " 1.k"; endptr = str; res = 0xbaadf00d; err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, 0); g_assert_cmpuint(res, ==, 1024); - g_assert_true(endptr == str + 3); + g_assert_true(endptr == str + 4); + + /* FIXME An empty fraction head should be tolerated */ + str = " .5k"; + endptr = str; + res = 0xbaadf00d; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, -EINVAL /* FIXME 0 */); + g_assert_cmpuint(res, ==, 0xbaadf00d /* FIXME 512 */); + g_assert_true(endptr == str /* FIXME + 4 */); /* For convenience, we permit values that are not byte-exact */ str = "12.345M"; @@ -2686,6 +2695,32 @@ static void test_qemu_strtosz_float(void) g_assert_cmpint(err, ==, 0); g_assert_cmpuint(res, ==, (uint64_t) (12.345 * MiB + 0.5)); g_assert_true(endptr == str + 7); + + /* FIXME Fraction tail should round correctly */ + str = "1.9999999999999999999999999999999999999999999999999999k"; + endptr = str; + res = 0xbaadf00d; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert_cmpint(res, ==, 1024 /* FIXME 2048 */); + g_assert_true(endptr == str + 55); + + /* FIXME ERANGE underflow in the fraction tail should not matter for 'k' */ + str = "1." + "00000000000000000000000000000000000000000000000000" + "00000000000000000000000000000000000000000000000000" + "00000000000000000000000000000000000000000000000000" + "00000000000000000000000000000000000000000000000000" + "00000000000000000000000000000000000000000000000000" + "00000000000000000000000000000000000000000000000000" + "00000000000000000000000000000000000000000000000000" + "1k"; + endptr = str; + res = 0xbaadf00d; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert_cmpuint(res, ==, 1 /* FIXME 1024 */); + g_assert_true(endptr == str + 354); } static void test_qemu_strtosz_invalid(void) @@ -2693,10 +2728,20 @@ static void test_qemu_strtosz_invalid(void) const char *str; const char *endptr; int err; - uint64_t res = 0xbaadf00d; + uint64_t res; + + /* Must parse at least one digit */ + str = NULL; + endptr = "somewhere"; + res = 0xbaadf00d; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, -EINVAL); + g_assert_cmphex(res, ==, 0xbaadf00d); + g_assert_null(endptr); str = ""; endptr = NULL; + res = 0xbaadf00d; err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, -EINVAL); g_assert_cmphex(res, ==, 0xbaadf00d); @@ -2704,13 +2749,30 @@ static void test_qemu_strtosz_invalid(void) str = " \t "; endptr = NULL; + res = 0xbaadf00d; err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, -EINVAL); g_assert_cmphex(res, ==, 0xbaadf00d); g_assert_true(endptr == str); + str = "."; + endptr = NULL; + res = 0xbaadf00d; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, -EINVAL); + g_assert_cmphex(res, ==, 0xbaadf00d); + g_assert(endptr == str); + + str = " ."; + endptr = NULL; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, -EINVAL); + g_assert_cmphex(res, ==, 0xbaadf00d); + g_assert(endptr == str); + str = "crap"; endptr = NULL; + res = 0xbaadf00d; err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, -EINVAL); g_assert_cmphex(res, ==, 0xbaadf00d); @@ -2718,6 +2780,7 @@ static void test_qemu_strtosz_invalid(void) str = "inf"; endptr = NULL; + res = 0xbaadf00d; err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, -EINVAL); g_assert_cmphex(res, ==, 0xbaadf00d); @@ -2725,6 +2788,7 @@ static void test_qemu_strtosz_invalid(void) str = "NaN"; endptr = NULL; + res = 0xbaadf00d; err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, -EINVAL); g_assert_cmphex(res, ==, 0xbaadf00d); @@ -2733,6 +2797,7 @@ static void test_qemu_strtosz_invalid(void) /* Fractional values require scale larger than bytes */ str = "1.1B"; endptr = NULL; + res = 0xbaadf00d; err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, -EINVAL); g_assert_cmphex(res, ==, 0xbaadf00d); @@ -2740,14 +2805,42 @@ static void test_qemu_strtosz_invalid(void) str = "1.1"; endptr = NULL; + res = 0xbaadf00d; err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, -EINVAL); g_assert_cmphex(res, ==, 0xbaadf00d); g_assert_true(endptr == str); + /* FIXME Fraction tail can cause ERANGE overflow */ + str = "15.9999999999999999999999999999999999999999999999999999E"; + endptr = str; + res = 0xbaadf00d; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0 /* FIXME -ERANGE */); + g_assert_cmpuint(res, ==, 15ULL * EiB /* FIXME 0xbaadf00d */); + g_assert_true(endptr == str + 56 /* FIXME str */); + + /* FIXME ERANGE underflow in the fraction tail should matter for 'B' */ + str = "1." + "00000000000000000000000000000000000000000000000000" + "00000000000000000000000000000000000000000000000000" + "00000000000000000000000000000000000000000000000000" + "00000000000000000000000000000000000000000000000000" + "00000000000000000000000000000000000000000000000000" + "00000000000000000000000000000000000000000000000000" + "00000000000000000000000000000000000000000000000000" + "1B"; + endptr = str; + res = 0xbaadf00d; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0 /* FIXME -EINVAL */); + g_assert_cmpuint(res, ==, 1 /* FIXME 0xbaadf00d */); + g_assert_true(endptr == str + 354 /* FIXME str */); + /* No hex fractions */ str = "0x1.8k"; endptr = NULL; + res = 0xbaadf00d; err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, -EINVAL); g_assert_cmphex(res, ==, 0xbaadf00d); @@ -2756,14 +2849,24 @@ static void test_qemu_strtosz_invalid(void) /* No suffixes */ str = "0x18M"; endptr = NULL; + res = 0xbaadf00d; err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, -EINVAL); g_assert_cmphex(res, ==, 0xbaadf00d); g_assert_true(endptr == str); + str = "0x1p1"; + endptr = NULL; + res = 0xbaadf00d; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, -EINVAL); + g_assert_cmphex(res, ==, 0xbaadf00d); + g_assert(endptr == str); + /* No negative values */ - str = "-0"; + str = " -0"; endptr = NULL; + res = 0xbaadf00d; err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, -EINVAL); g_assert_cmphex(res, ==, 0xbaadf00d); @@ -2771,10 +2874,28 @@ static void test_qemu_strtosz_invalid(void) str = "-1"; endptr = NULL; + res = 0xbaadf00d; err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, -EINVAL); g_assert_cmphex(res, ==, 0xbaadf00d); g_assert_true(endptr == str); + + str = "-.0k"; + endptr = NULL; + res = 0xbaadf00d; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, -EINVAL); + g_assert_cmphex(res, ==, 0xbaadf00d); + g_assert_true(endptr == str); + + /* Too many decimals */ + str = "1.1.k"; + endptr = NULL; + res = 0xbaadf00d; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, -EINVAL); + g_assert_cmphex(res, ==, 0xbaadf00d); + g_assert(endptr == str); } static void test_qemu_strtosz_trailing(void) @@ -2784,6 +2905,21 @@ static void test_qemu_strtosz_trailing(void) int err; uint64_t res; + /* Trailing whitespace */ + str = "1k "; + endptr = NULL; + res = 0xbaadf00d; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert_cmpuint(res, ==, 1024); + g_assert(endptr == str + 2); + + res = 0xbaadf00d; + err = qemu_strtosz(str, NULL, &res); + g_assert_cmpint(err, ==, -EINVAL); + g_assert_cmphex(res, ==, 0xbaadf00d); + + /* Unknown suffix */ str = "123xxx"; endptr = NULL; res = 0xbaadf00d; @@ -2797,6 +2933,7 @@ static void test_qemu_strtosz_trailing(void) g_assert_cmpint(err, ==, -EINVAL); g_assert_cmphex(res, ==, 0xbaadf00d); + /* Junk after one-byte suffix */ str = "1kiB"; endptr = NULL; res = 0xbaadf00d; @@ -2810,6 +2947,7 @@ static void test_qemu_strtosz_trailing(void) g_assert_cmpint(err, ==, -EINVAL); g_assert_cmphex(res, ==, 0xbaadf00d); + /* Incomplete hex is an unknown suffix */ str = "0x"; endptr = NULL; res = 0xbaadf00d; @@ -2823,6 +2961,7 @@ static void test_qemu_strtosz_trailing(void) g_assert_cmpint(err, ==, -EINVAL); g_assert_cmphex(res, ==, 0xbaadf00d); + /* Junk after decimal */ str = "0.NaN"; endptr = NULL; res = 0xbaadf00d; @@ -2836,6 +2975,35 @@ static void test_qemu_strtosz_trailing(void) g_assert_cmpint(err, ==, -EINVAL); g_assert_cmphex(res, ==, 0xbaadf00d); + /* No support for binary literals; 'b' is valid suffix */ + str = "0b1000"; + endptr = NULL; + res = 0xbaadf00d; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert_cmpuint(res, ==, 0); + g_assert(endptr == str + 2); + + res = 0xbaadf00d; + err = qemu_strtosz(str, NULL, &res); + g_assert_cmpint(err, ==, -EINVAL); + g_assert_cmphex(res, ==, 0xbaadf00d); + + /* Hex literals use only one leading zero */ + str = "00x1"; + endptr = NULL; + res = 0xbaadf00d; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert_cmpuint(res, ==, 0); + g_assert(endptr == str + 2); + + res = 0xbaadf00d; + err = qemu_strtosz(str, NULL, &res); + g_assert_cmpint(err, ==, -EINVAL); + g_assert_cmphex(res, ==, 0xbaadf00d); + + /* Although negatives are invalid, '-' may be in trailing junk */ str = "123-45"; endptr = NULL; res = 0xbaadf00d; @@ -2849,6 +3017,19 @@ static void test_qemu_strtosz_trailing(void) g_assert_cmpint(err, ==, -EINVAL); g_assert_cmphex(res, ==, 0xbaadf00d); + str = " 123 - 45"; + endptr = NULL; + res = 0xbaadf00d; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert_cmpuint(res, ==, 123); + g_assert(endptr == str + 4); + + res = 0xbaadf00d; + err = qemu_strtosz(str, NULL, &res); + g_assert_cmpint(err, ==, -EINVAL); + g_assert_cmphex(res, ==, 0xbaadf00d); + /* FIXME should stop parse after 'e'. No floating point exponents */ str = "1.5e1k"; endptr = NULL; @@ -2861,7 +3042,7 @@ static void test_qemu_strtosz_trailing(void) res = 0xbaadf00d; err = qemu_strtosz(str, NULL, &res); g_assert_cmpint(err, ==, -EINVAL); - g_assert_cmpint(res, ==, 0xbaadf00d); + g_assert_cmphex(res, ==, 0xbaadf00d); str = "1.5E+0k"; endptr = NULL; @@ -2875,6 +3056,20 @@ static void test_qemu_strtosz_trailing(void) err = qemu_strtosz(str, NULL, &res); g_assert_cmpint(err, ==, -EINVAL); g_assert_cmphex(res, ==, 0xbaadf00d); + + /* FIXME overflow in fraction is buggy */ + str = "1.5E999"; + endptr = NULL; + res = 0xbaadf00d; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert_cmpuint(res, ==, EiB /* FIXME EiB * 1.5 */); + g_assert(endptr == str + 9 /* FIXME + 4 */); + + res = 0xbaadf00d; + err = qemu_strtosz(str, NULL, &res); + g_assert_cmpint(err, ==, -EINVAL); + g_assert_cmphex(res, ==, 0xbaadf00d); } static void test_qemu_strtosz_erange(void) @@ -2921,6 +3116,15 @@ static void test_qemu_strtosz_metric(void) g_assert_cmpint(err, ==, 0); g_assert_cmpuint(res, ==, 12345000); g_assert_true(endptr == str + 7); + + /* Fraction is affected by floating-point rounding */ + str = "18.446744073709550591E"; /* resembles 0xfffffffffffffbff */ + endptr = str; + res = 0xbaadf00d; + err = qemu_strtosz_metric(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert_cmpuint(res, ==, 0xfffffffffffffc0c); + g_assert_true(endptr == str + 22); } static void test_freq_to_str(void)
Add some more strings that the user might send our way. In particular, some of these additions include FIXME comments showing where our parser doesn't quite behave the way we want. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/unit/test-cutils.c | 226 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 215 insertions(+), 11 deletions(-)