[v2,18/19] cutils: Improve qemu_strtod* error paths

Commit Message

Eric Blake May 12, 2023, 2:10 a.m. UTC

Previous patches changed all integral qemu_strto*() error paths to
guarantee that *value is never left uninitialized.  Do likewise for
qemu_strtod.  Also, tighten qemu_strtod_finite() to never return a
non-finite value (prior to this patch, we were rejecting "inf" with
-EINVAL and unspecified result 0.0, but failing "9e999" with -ERANGE
and HUGE_VAL - which is infinite on IEEE machines - despite our
function claiming to recognize only finite values).

Auditing callers, we have no external callers of qemu_strtod, and
among the callers of qemu_strtod_finite:

- qapi/qobject-input-visitor.c:qobject_input_type_number_keyval() and
  qapi/string-input-visitor.c:parse_type_number() which reject all
  errors (does not matter what we store)

- utils/cutils.c:do_strtosz() incorrectly assumes that *endptr points
  to '.' on all failures (that is, it is not distinguishing between
  EINVAL and ERANGE; and therefore still does the WRONG THING for
  "9.9e999".  The change here does not entirely fix that (a later
  patch will tackle this more systematically), but at least the value
  of endptr is now less likely to be out of bounds on overflow

- our testsuite, which we can update to match what we document

Signed-off-by: Eric Blake <[e-mail]>
Reviewed-by: Hanna Czenczek <[e-mail]>

---

v2: no change to cutils.c; commit message tweaks and testsuite rebase
were minor enough to keep R-b
---
 tests/unit/test-cutils.c | 63 +++++++++++++++++++++++-----------------
 util/cutils.c            | 32 +++++++++++---------
 2 files changed, 55 insertions(+), 40 deletions(-)

Comments

Eric Blake May 18, 2023, 1:47 p.m. UTC | #1

On Thu, May 11, 2023 at 09:10:32PM -0500, Eric Blake wrote:
> 
> Previous patches changed all integral qemu_strto*() error paths to
> guarantee that *value is never left uninitialized.  Do likewise for
> qemu_strtod.  Also, tighten qemu_strtod_finite() to never return a
> non-finite value (prior to this patch, we were rejecting "inf" with
> -EINVAL and unspecified result 0.0, but failing "9e999" with -ERANGE
> and HUGE_VAL - which is infinite on IEEE machines - despite our
> function claiming to recognize only finite values).
> 
> Auditing callers, we have no external callers of qemu_strtod, and
> among the callers of qemu_strtod_finite:
> 
> - qapi/qobject-input-visitor.c:qobject_input_type_number_keyval() and
>   qapi/string-input-visitor.c:parse_type_number() which reject all
>   errors (does not matter what we store)
> 
> - utils/cutils.c:do_strtosz() incorrectly assumes that *endptr points
>   to '.' on all failures (that is, it is not distinguishing between
>   EINVAL and ERANGE; and therefore still does the WRONG THING for
>   "9.9e999".  The change here does not entirely fix that (a later
>   patch will tackle this more systematically), but at least the value
>   of endptr is now less likely to be out of bounds on overflow
> 
> - our testsuite, which we can update to match what we document
> 
> Signed-off-by: Eric Blake <[e-mail]>
> Reviewed-by: Hanna Czenczek <[e-mail]>
> 
> ---

cc'ing qemu-stable for comments, since this patch fixes a sanitizer
issue flagged in https://gitlab.com/qemu-project/qemu/-/issues/1629.
While we decided the read-out-of-bounds in qemu_strtosz() is probably
not CVE-worthy (if you have control over the command line or QMP, you
can probably do much worse than cause a size parser to segfault), it
does raise the question of whether this patch is useful for
qemu-stable.  What's more, taking this patch in isolation without all
the prerequisite patches is probably sufficient to prevent the
read-out-of-bounds, but still leaves qemu_strtosz() with some odd
corner cases; while taking the whole series is a much bigger
committment but easier to analyze as a unit given that a lot of thte
series is testsuite unit test additions.  But either way, we'd need
reviews on this series if anyone thinks it warrants backports to
stable releases.

Patch

diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c
index b8ad4d7fbac..8f2dd335f13 100644
--- a/tests/unit/test-cutils.c
+++ b/tests/unit/test-cutils.c
@@ -2747,7 +2747,8 @@  static void test_qemu_strtod_einval(void)
     res = 999;
     err = qemu_strtod(str, &endptr, &res);
     g_assert_cmpint(err, ==, -EINVAL);
-    g_assert_cmpfloat(res, ==, 999.0);
+    g_assert_cmpfloat(res, ==, 0.0);
+    g_assert_false(signbit(res));
     g_assert_null(endptr);

     /* not recognizable */
@@ -2979,7 +2980,8 @@  static void test_qemu_strtod_finite_einval(void)
     res = 999;
     err = qemu_strtod_finite(str, &endptr, &res);
     g_assert_cmpint(err, ==, -EINVAL);
-    g_assert_cmpfloat(res, ==, 999.0);
+    g_assert_cmpfloat(res, ==, 0.0);
+    g_assert_false(signbit(res));
     g_assert_true(endptr == str);

     /* NULL */
@@ -2988,7 +2990,8 @@  static void test_qemu_strtod_finite_einval(void)
     res = 999;
     err = qemu_strtod_finite(str, &endptr, &res);
     g_assert_cmpint(err, ==, -EINVAL);
-    g_assert_cmpfloat(res, ==, 999.0);
+    g_assert_cmpfloat(res, ==, 0.0);
+    g_assert_false(signbit(res));
     g_assert_null(endptr);

     /* not recognizable */
@@ -2997,7 +3000,8 @@  static void test_qemu_strtod_finite_einval(void)
     res = 999;
     err = qemu_strtod_finite(str, &endptr, &res);
     g_assert_cmpint(err, ==, -EINVAL);
-    g_assert_cmpfloat(res, ==, 999.0);
+    g_assert_cmpfloat(res, ==, 0.0);
+    g_assert_false(signbit(res));
     g_assert_true(endptr == str);
 }

@@ -3008,24 +3012,26 @@  static void test_qemu_strtod_finite_erange(void)
     int err;
     double res;

-    /* overflow */
+    /* overflow turns into EINVAL */
     str = "9e999";
     endptr = "somewhere";
     res = 999;
     err = qemu_strtod_finite(str, &endptr, &res);
-    g_assert_cmpint(err, ==, -ERANGE);
-    g_assert_cmpfloat(res, ==, HUGE_VAL);
-    g_assert_true(endptr == str + 5);
+    g_assert_cmpint(err, ==, -EINVAL);
+    g_assert_cmpfloat(res, ==, 0.0);
+    g_assert_false(signbit(res));
+    g_assert_true(endptr == str);

     str = "-9e+999";
     endptr = "somewhere";
     res = 999;
     err = qemu_strtod_finite(str, &endptr, &res);
-    g_assert_cmpint(err, ==, -ERANGE);
-    g_assert_cmpfloat(res, ==, -HUGE_VAL);
-    g_assert_true(endptr == str + 7);
+    g_assert_cmpint(err, ==, -EINVAL);
+    g_assert_cmpfloat(res, ==, 0.0);
+    g_assert_false(signbit(res));
+    g_assert_true(endptr == str);

-    /* underflow */
+    /* underflow is still possible */
     str = "-9e-999";
     endptr = "somewhere";
     res = 999;
@@ -3050,7 +3056,8 @@  static void test_qemu_strtod_finite_nonfinite(void)
     res = 999;
     err = qemu_strtod_finite(str, &endptr, &res);
     g_assert_cmpint(err, ==, -EINVAL);
-    g_assert_cmpfloat(res, ==, 999.0);
+    g_assert_cmpfloat(res, ==, 0.0);
+    g_assert_false(signbit(res));
     g_assert_true(endptr == str);

     str = "-infinity";
@@ -3058,7 +3065,8 @@  static void test_qemu_strtod_finite_nonfinite(void)
     res = 999;
     err = qemu_strtod_finite(str, &endptr, &res);
     g_assert_cmpint(err, ==, -EINVAL);
-    g_assert_cmpfloat(res, ==, 999.0);
+    g_assert_cmpfloat(res, ==, 0.0);
+    g_assert_false(signbit(res));
     g_assert_true(endptr == str);

     /* not a number */
@@ -3067,7 +3075,8 @@  static void test_qemu_strtod_finite_nonfinite(void)
     res = 999;
     err = qemu_strtod_finite(str, &endptr, &res);
     g_assert_cmpint(err, ==, -EINVAL);
-    g_assert_cmpfloat(res, ==, 999.0);
+    g_assert_cmpfloat(res, ==, 0.0);
+    g_assert_false(signbit(res));
     g_assert_true(endptr == str);
 }

@@ -3091,7 +3100,8 @@  static void test_qemu_strtod_finite_trailing(void)
     res = 999;
     err = qemu_strtod_finite(str, NULL, &res);
     g_assert_cmpint(err, ==, -EINVAL);
-    g_assert_cmpfloat(res, ==, 999.0);
+    g_assert_cmpfloat(res, ==, 1.0);
+    g_assert_false(signbit(res));

     /* trailing e is not an exponent */
     str = ".5e";
@@ -3106,7 +3116,7 @@  static void test_qemu_strtod_finite_trailing(void)
     res = 999;
     err = qemu_strtod_finite(str, NULL, &res);
     g_assert_cmpint(err, ==, -EINVAL);
-    g_assert_cmpfloat(res, ==, 999.0);
+    g_assert_cmpfloat(res, ==, 0.5);

     /* trailing ( not part of long NaN */
     str = "nan(";
@@ -3114,14 +3124,16 @@  static void test_qemu_strtod_finite_trailing(void)
     res = 999;
     err = qemu_strtod_finite(str, &endptr, &res);
     g_assert_cmpint(err, ==, -EINVAL);
-    g_assert_cmpfloat(res, ==, 999.0);
+    g_assert_cmpfloat(res, ==, 0.0);
+    g_assert_false(signbit(res));
     g_assert_true(endptr == str);

     endptr = "somewhere";
     res = 999;
     err = qemu_strtod_finite(str, NULL, &res);
     g_assert_cmpint(err, ==, -EINVAL);
-    g_assert_cmpfloat(res, ==, 999.0);
+    g_assert_cmpfloat(res, ==, 0.0);
+    g_assert_false(signbit(res));
 }

 static void test_qemu_strtod_finite_erange_junk(void)
@@ -3146,7 +3158,8 @@  static void test_qemu_strtod_finite_erange_junk(void)
     res = 999;
     err = qemu_strtod_finite(str, NULL, &res);
     g_assert_cmpint(err, ==, -EINVAL);
-    g_assert_cmpfloat(res, ==, 999.0);
+    g_assert_cmpfloat(res, ==, 0.0);
+    g_assert_false(signbit(res));
 }

 typedef int (*qemu_strtosz_fn)(const char *, const char **, uint64_t *);
@@ -3384,13 +3397,9 @@  static void test_qemu_strtosz_trailing(void)
                     0 /* FIXME EiB * 1.5 */, 0 /* FIXME 4 */,
                     -EINVAL, 0);

-    /*
-     * FIXME overflow in fraction is so buggy it can read beyond bounds
-     * if we don't stuff extra \0 in our literal
-     */
-    do_strtosz_full("1.5E999\0\0" /* FIXME 1.5E999" */, qemu_strtosz,
-                    0, 1 /* FIXME EiB * 1.5 */, 8 /* FIXME 4 */,
-                    0 /* FIXME -EINVAL */, 1 /* FIXME 0 */);
+    /* FIXME overflow in fraction is still buggy */
+    do_strtosz_full("1.5E999", qemu_strtosz, 0, 1 /* FIXME EiB * 1.5 */,
+                    2 /* FIXME 4 */, -EINVAL, 0);
 }

 static void test_qemu_strtosz_erange(void)
diff --git a/util/cutils.c b/util/cutils.c
index 550abbe5c06..25c95b0933e 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -653,12 +653,13 @@  int qemu_strtou64(const char *nptr, const char **endptr, int base,
  *
  * @nptr may be null, and no conversion is performed then.
  *
- * If no conversion is performed, store @nptr in *@endptr and return
- * -EINVAL.
+ * If no conversion is performed, store @nptr in *@endptr, +0.0 in
+ * @result, and return -EINVAL.
  *
  * If @endptr is null, and the string isn't fully converted, return
- * -EINVAL. This is the case when the pointer that would be stored in
- * a non-null @endptr points to a character other than '\0'.
+ * -EINVAL with @result set to the parsed value.  This is the case
+ * when the pointer that would be stored in a non-null @endptr points
+ * to a character other than '\0'.
  *
  * If the conversion overflows, store +/-HUGE_VAL in @result, depending
  * on the sign, and return -ERANGE.
@@ -673,6 +674,7 @@  int qemu_strtod(const char *nptr, const char **endptr, double *result)
     char *ep;

     if (!nptr) {
+        *result = 0.0;
         if (endptr) {
             *endptr = nptr;
         }
@@ -687,24 +689,28 @@  int qemu_strtod(const char *nptr, const char **endptr, double *result)
 /**
  * Convert string @nptr to a finite double.
  *
- * Works like qemu_strtod(), except that "NaN" and "inf" are rejected
- * with -EINVAL and no conversion is performed.
+ * Works like qemu_strtod(), except that "NaN", "inf", and strings
+ * that cause ERANGE overflow errors are rejected with -EINVAL as if
+ * no conversion is performed, storing 0.0 into @result regardless of
+ * any sign.  -ERANGE failures for underflow still preserve the parsed
+ * sign.
  */
 int qemu_strtod_finite(const char *nptr, const char **endptr, double *result)
 {
-    double tmp;
+    const char *tmp;
     int ret;

-    ret = qemu_strtod(nptr, endptr, &tmp);
-    if (!ret && !isfinite(tmp)) {
+    ret = qemu_strtod(nptr, &tmp, result);
+    if (!isfinite(*result)) {
         if (endptr) {
             *endptr = nptr;
         }
+        *result = 0.0;
+        ret = -EINVAL;
+    } else if (endptr) {
+        *endptr = tmp;
+    } else if (*tmp) {
         ret = -EINVAL;
-    }
-
-    if (ret != -EINVAL) {
-        *result = tmp;
     }
     return ret;
 }