| Message ID | 20230601220305.2130121-8-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/21] iotests: Fix test 104 under NBD | expand |
02.06.2023 01:02, Eric Blake пишет: > 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 (see the unit test for > examples). The comment for that function even described what we have > to do for a correct parse, but didn't implement it correctly: since > strtoull checks for overflow against the wrong values and then > negates, we have to temporarily undo negation before checking for > overflow against our desired value. > > 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. > > Whether we parse C2x binary strings like "0b1000" is currently up to > what libc does; our unit tests intentionally don't cover that at the > moment, though. > > 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> > CC: qemu-stable@nongnu.org Trying to pick this one up for -stable. The implementation changes are good. But the testsuite changes are.. difficult. The thing is that testsuite changes (here and in the other -stable patch) applies on top of previous changes in there (in the same series), which, in turn, requires other previous code changes in the implementation to succeed. For example, this patch changes test_qemu_strtoui_overflow() which was introduced in previous patch "Test more integer corner cases" from this series and further modified in "Test integral qemu_strto* value on failures" one. Picking them result in testsuite failing due to missing previous code changes. I tried to drop just the testsuite changes, but the result is that the testsuite fails with fixed code :) It's quite fun situation actually, like it fails no matter what you do, one way or another. I'll try to find the most stright-forward way from here. Good stuff. Thanks, /mjt
On Sat, Jun 03, 2023 at 11:17:27AM +0300, Michael Tokarev wrote: > 02.06.2023 01:02, Eric Blake пишет: > > 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 (see the unit test for > > examples). The comment for that function even described what we have > > to do for a correct parse, but didn't implement it correctly: since > > strtoull checks for overflow against the wrong values and then > > negates, we have to temporarily undo negation before checking for > > overflow against our desired value. > > > > 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. > > > > Whether we parse C2x binary strings like "0b1000" is currently up to > > what libc does; our unit tests intentionally don't cover that at the > > moment, though. > > > > 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> > > CC: qemu-stable@nongnu.org > > Trying to pick this one up for -stable. The implementation changes are good. > But the testsuite changes are.. difficult. The thing is that testsuite changes > (here and in the other -stable patch) applies on top of previous changes in > there (in the same series), which, in turn, requires other previous code changes > in the implementation to succeed. Yeah, I did wonder if it is worth even trying to get this for stable. The series is inter-tangled enough that it feels like an all-or-nothing approach may be easiest - but all means 19 patches and hundreds of lines of testsuite additions. My other argument when first posting this to qemu-stable was to just declare that these have been broken long enough that it is not a recent regression, and users are unlikely to be supplying command-line or QMP strings to tickle these bugs, so not backporting may be okay. > > For example, this patch changes test_qemu_strtoui_overflow() which was introduced > in previous patch "Test more integer corner cases" from this series and further > modified in "Test integral qemu_strto* value on failures" one. Picking them result > in testsuite failing due to missing previous code changes. > > I tried to drop just the testsuite changes, but the result is that the testsuite > fails with fixed code :) > > It's quite fun situation actually, like it fails no matter what you do, one way > or another. > > I'll try to find the most stright-forward way from here. Good stuff. Let me know how I can help in deciding what, if any, is worth backporting.
diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c index 011123a2111..ce71900cb73 100644 --- a/tests/unit/test-cutils.c +++ b/tests/unit/test-cutils.c @@ -909,7 +909,7 @@ static void test_qemu_strtoui_hex(void) static void test_qemu_strtoui_wrap(void) { - /* FIXME - wraparound should be consistent with 32-bit strtoul */ + /* wraparound is consistent with 32-bit strtoul */ const char *str = "-4294967295"; /* 1 mod 2^32 */ char f = 'X'; const char *endptr = &f; @@ -918,8 +918,8 @@ static void test_qemu_strtoui_wrap(void) err = qemu_strtoui(str, &endptr, 0, &res); - g_assert_cmpint(err, ==, -ERANGE /* FIXME 0 */); - g_assert_cmphex(res, ==, UINT_MAX /* FIXME 1 */); + g_assert_cmpint(err, ==, 0); + g_assert_cmphex(res, ==, 1); g_assert_true(endptr == str + strlen(str)); } @@ -978,13 +978,12 @@ static void test_qemu_strtoui_overflow(void) g_assert_cmpuint(res, ==, UINT_MAX); g_assert_true(endptr == str + strlen(str)); - /* FIXME - overflow should be consistent with 32-bit strtoul */ str = "0xfffffffffffffffe"; /* ULLONG_MAX - 1 (not UINT_MAX - 1) */ endptr = "somewhere"; res = 999; err = qemu_strtoui(str, &endptr, 0, &res); - g_assert_cmpint(err, ==, 0 /* FIXME -ERANGE */); - g_assert_cmpuint(res, ==, UINT_MAX - 1 /* FIXME UINT_MAX */); + g_assert_cmpint(err, ==, -ERANGE); + g_assert_cmpuint(res, ==, UINT_MAX); g_assert_true(endptr == str + strlen(str)); str = "0x10000000000000000"; /* 65 bits, 32-bit sign bit clear */ @@ -1019,21 +1018,20 @@ static void test_qemu_strtoui_underflow(void) g_assert_cmpuint(res, ==, UINT_MAX); g_assert_true(endptr == str + strlen(str)); - /* FIXME - overflow should be consistent with 32-bit strtoul */ str = "-18446744073709551615"; /* -UINT64_MAX (not -(-1)) */ endptr = "somewhere"; res = 999; err = qemu_strtoui(str, &endptr, 0, &res); - g_assert_cmpint(err, ==, 0 /* FIXME -ERANGE */); - g_assert_cmpuint(res, ==, 1 /* FIXME UINT_MAX */); + g_assert_cmpint(err, ==, -ERANGE); + g_assert_cmpuint(res, ==, UINT_MAX); g_assert_true(endptr == str + strlen(str)); str = "-0xffffffff00000002"; endptr = "somewhere"; res = 999; err = qemu_strtoui(str, &endptr, 0, &res); - g_assert_cmpint(err, ==, 0 /* FIXME -ERANGE */); - g_assert_cmpuint(res, ==, UINT_MAX - 1 /* FIXME UINT_MAX */); + g_assert_cmpint(err, ==, -ERANGE); + g_assert_cmpuint(res, ==, UINT_MAX); g_assert_true(endptr == str + strlen(str)); str = "-0x10000000000000000"; /* 65 bits, 32-bit sign bit clear */ diff --git a/util/cutils.c b/util/cutils.c index 5887e744140..9b6ce9179c4 100644 --- a/util/cutils.c +++ b/util/cutils.c @@ -391,6 +391,9 @@ static int check_strtox_error(const char *nptr, char *ep, * and return -ERANGE. * * Else store the converted value in @result, and return zero. + * + * This matches the behavior of strtol() on 32-bit platforms, even on + * platforms where long is 64-bits. */ int qemu_strtoi(const char *nptr, const char **endptr, int base, int *result) @@ -443,13 +446,15 @@ int qemu_strtoi(const char *nptr, const char **endptr, int base, * * Note that a number with a leading minus sign gets converted without * the minus sign, checked for overflow (see above), then negated (in - * @result's type). This is exactly how strtoul() works. + * @result's type). This matches the behavior of strtoul() on 32-bit + * platforms, even on platforms where long is 64-bits. */ int qemu_strtoui(const char *nptr, const char **endptr, int base, unsigned int *result) { char *ep; - long long lresult; + unsigned long long lresult; + bool neg; assert((unsigned) base <= 36 && base != 1); if (!nptr) { @@ -466,14 +471,22 @@ int qemu_strtoui(const char *nptr, const char **endptr, int base, if (errno == ERANGE) { *result = -1; } else { + /* + * Note that platforms with 32-bit strtoul only accept input + * in the range [-4294967295, 4294967295]; but we used 64-bit + * strtoull which wraps -18446744073709551615 to 1 instead of + * declaring overflow. So we must check if '-' was parsed, + * and if so, undo the negation before doing our bounds check. + */ + neg = memchr(nptr, '-', ep - nptr) != NULL; + if (neg) { + lresult = -lresult; + } if (lresult > UINT_MAX) { *result = UINT_MAX; errno = ERANGE; - } else if (lresult < INT_MIN) { - *result = UINT_MAX; - errno = ERANGE; } else { - *result = lresult; + *result = neg ? -lresult : lresult; } } return check_strtox_error(nptr, ep, endptr, lresult == 0, errno);
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 (see the unit test for examples). The comment for that function even described what we have to do for a correct parse, but didn't implement it correctly: since strtoull checks for overflow against the wrong values and then negates, we have to temporarily undo negation before checking for overflow against our desired value. 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. Whether we parse C2x binary strings like "0b1000" is currently up to what libc does; our unit tests intentionally don't cover that at the moment, though. 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> CC: qemu-stable@nongnu.org Message-Id: <20230522190441.64278-6-eblake@redhat.com> Reviewed-by: Hanna Czenczek <hreitz@redhat.com> --- tests/unit/test-cutils.c | 20 +++++++++----------- util/cutils.c | 25 +++++++++++++++++++------ 2 files changed, 28 insertions(+), 17 deletions(-)