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

From: Eric Blake <eblake@redhat.com>
To: qemu-devel@nongnu.org
Cc: hreitz@redhat.com, qemu-block@nongnu.org, qemu-stable@nongnu.org
Subject: [PATCH v3 05/19] cutils: Fix wraparound parsing in qemu_strtoui
Date: Mon, 22 May 2023 14:04:27 -0500	[thread overview]
Message-ID: <20230522190441.64278-6-eblake@redhat.com> (raw)
In-Reply-To: <20230522190441.64278-1-eblake@redhat.com>

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
---

v3: rewrite with better comments in code and commit message [Hanna];
fix additional problems with wraparound, cc qemu-stable although this
has been long-standing enough to be low priority, R-b dropped
---
 tests/unit/test-cutils.c | 20 +++++++++-----------
 util/cutils.c            | 25 +++++++++++++++++++------
 2 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c
index 7d0ab4ca4c2..249fc1fa7e7 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);
-- 
2.40.1


