diff mbox series

[v2,08/19] cutils: Allow NULL endptr in parse_uint()

Message ID 20230512021033.1378730-9-eblake@redhat.com (mailing list archive)
State New, archived
Headers show
Series Fix qemu_strtosz() read-out-of-bounds | expand

Commit Message

Eric Blake May 12, 2023, 2:10 a.m. UTC
All the qemu_strto*() functions permit a NULL endptr, just like their
libc counterparts, leaving parse_uint() as the oddball that caused
SEGFAULT on NULL and required the user to call parse_uint_full()
instead.  Relax things for consistency, even though the testsuite is
the only impacted caller.  Add one more unit test to ensure even
parse_uint_full(NULL, 0, &value) works.  This also fixes our code to
uniformly favor EINVAL over ERANGE when both apply.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/unit/test-cutils.c | 18 ++++++++++++++++--
 util/cutils.c            | 34 ++++++++++++----------------------
 2 files changed, 28 insertions(+), 24 deletions(-)

Comments

Eric Blake May 12, 2023, 4:44 p.m. UTC | #1
On Thu, May 11, 2023 at 09:10:22PM -0500, Eric Blake wrote:
> 
> All the qemu_strto*() functions permit a NULL endptr, just like their
> libc counterparts, leaving parse_uint() as the oddball that caused
> SEGFAULT on NULL and required the user to call parse_uint_full()
> instead.  Relax things for consistency, even though the testsuite is
> the only impacted caller.  Add one more unit test to ensure even
> parse_uint_full(NULL, 0, &value) works.  This also fixes our code to
> uniformly favor EINVAL over ERANGE when both apply.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/unit/test-cutils.c | 18 ++++++++++++++++--
>  util/cutils.c            | 34 ++++++++++++----------------------
>  2 files changed, 28 insertions(+), 24 deletions(-)

> 
> @@ -788,28 +793,13 @@ out:
>   * @base: integer base, between 2 and 36 inclusive, or 0
>   * @value: Destination for parsed integer value
>   *
> - * Parse unsigned integer from entire string
> + * Parse unsigned integer from entire string, rejecting any trailing slop.
>   *
> - * Have the same behavior of parse_uint(), but with an additional
> - * check for additional data after the parsed number. If extra
> - * characters are present after a non-overflowing parsed number, the
> - * function will return -EINVAL, and *@v will be set to 0.
> + * Shorthand for parse_uint(s, NULL, base, value).

I'm just now noticing that I also had a nice side effect of removing
the reference to *@v when the parameter is actually named value.

>   */
>  int parse_uint_full(const char *s, int base, uint64_t *value)

I don't know if there is an easy way to test our doc comments for
mismatch from parameter names.
Hanna Czenczek May 19, 2023, 2:54 p.m. UTC | #2
On 12.05.23 04:10, Eric Blake wrote:
> All the qemu_strto*() functions permit a NULL endptr, just like their
> libc counterparts, leaving parse_uint() as the oddball that caused
> SEGFAULT on NULL and required the user to call parse_uint_full()
> instead.  Relax things for consistency, even though the testsuite is
> the only impacted caller.  Add one more unit test to ensure even
> parse_uint_full(NULL, 0, &value) works.  This also fixes our code to
> uniformly favor EINVAL over ERANGE when both apply.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   tests/unit/test-cutils.c | 18 ++++++++++++++++--
>   util/cutils.c            | 34 ++++++++++++----------------------
>   2 files changed, 28 insertions(+), 24 deletions(-)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
diff mbox series

Patch

diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c
index 0c7d07b3297..d3076c3fec1 100644
--- a/tests/unit/test-cutils.c
+++ b/tests/unit/test-cutils.c
@@ -260,14 +260,26 @@  static void test_parse_uint_full_correct(void)

 static void test_parse_uint_full_erange_junk(void)
 {
-    /* FIXME - inconsistent with qemu_strto* which favors EINVAL */
+    /* EINVAL has priority over ERANGE */
     uint64_t i = 999;
     const char *str = "-2junk";
     int r;

     r = parse_uint_full(str, 0, &i);

-    g_assert_cmpint(r, ==, -ERANGE /* FIXME -EINVAL */);
+    g_assert_cmpint(r, ==, -EINVAL);
+    g_assert_cmpuint(i, ==, 0);
+}
+
+static void test_parse_uint_full_null(void)
+{
+    uint64_t i = 999;
+    const char *str = NULL;
+    int r;
+
+    r = parse_uint_full(str, 0, &i);
+
+    g_assert_cmpint(r, ==, -EINVAL);
     g_assert_cmpuint(i, ==, 0);
 }

@@ -3207,6 +3219,8 @@  int main(int argc, char **argv)
                     test_parse_uint_full_correct);
     g_test_add_func("/cutils/parse_uint_full/erange_junk",
                     test_parse_uint_full_erange_junk);
+    g_test_add_func("/cutils/parse_uint_full/null",
+                    test_parse_uint_full_null);

     /* qemu_strtoi() tests */
     g_test_add_func("/cutils/qemu_strtoi/correct",
diff --git a/util/cutils.c b/util/cutils.c
index 8ab0cae5e75..e599924a0c4 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -715,8 +715,7 @@  const char *qemu_strchrnul(const char *s, int c)
  * parse_uint:
  *
  * @s: String to parse
- * @endptr: Destination for pointer to first character not consumed, must
- * not be %NULL
+ * @endptr: Destination for pointer to first character not consumed
  * @base: integer base, between 2 and 36 inclusive, or 0
  * @value: Destination for parsed integer value
  *
@@ -730,7 +729,8 @@  const char *qemu_strchrnul(const char *s, int c)
  *
  * Set *@endptr to point right beyond the parsed integer (even if the integer
  * overflows or is negative, all digits will be parsed and *@endptr will
- * point right beyond them).
+ * point right beyond them).  If @endptr is %NULL, any trailing character
+ * instead causes a result of -EINVAL with *@value of 0.
  *
  * If the integer is negative, set *@value to 0, and return -ERANGE.
  * (If you want to allow negative numbers that wrap around within
@@ -777,7 +777,12 @@  int parse_uint(const char *s, const char **endptr, int base, uint64_t *value)

 out:
     *value = val;
-    *endptr = endp;
+    if (endptr) {
+        *endptr = endp;
+    } else if (s && *endp) {
+        r = -EINVAL;
+        *value = 0;
+    }
     return r;
 }

@@ -788,28 +793,13 @@  out:
  * @base: integer base, between 2 and 36 inclusive, or 0
  * @value: Destination for parsed integer value
  *
- * Parse unsigned integer from entire string
+ * Parse unsigned integer from entire string, rejecting any trailing slop.
  *
- * Have the same behavior of parse_uint(), but with an additional
- * check for additional data after the parsed number. If extra
- * characters are present after a non-overflowing parsed number, the
- * function will return -EINVAL, and *@v will be set to 0.
+ * Shorthand for parse_uint(s, NULL, base, value).
  */
 int parse_uint_full(const char *s, int base, uint64_t *value)
 {
-    const char *endp;
-    int r;
-
-    r = parse_uint(s, &endp, base, value);
-    if (r < 0) {
-        return r;
-    }
-    if (*endp) {
-        *value = 0;
-        return -EINVAL;
-    }
-
-    return 0;
+    return parse_uint(s, NULL, base, value);
 }

 int qemu_parse_fd(const char *param)