[v2] Fix qemu_strtosz() read-out-of-bounds

[PATCH v2 14/19] test-cutils: Add more coverage to qemu_strtosz11; rgb:1e1e/1e1e/1e1e

Posted by Eric Blake 2 years, 9 months ago

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>

---

v2: even more tests added, pad a string to avoid out-of-bounds
randomness [Hanna]
---
 tests/unit/test-cutils.c | 147 +++++++++++++++++++++++++++++++++++----
 1 file changed, 135 insertions(+), 12 deletions(-)

diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c
index 1936c7b5795..7800caf9b0e 100644
--- a/tests/unit/test-cutils.c
+++ b/tests/unit/test-cutils.c
@@ -3162,7 +3162,12 @@ static void do_strtosz_full(const char *str, qemu_strtosz_fn fn,
     ret = fn(str, &endptr, &val);
     g_assert_cmpint(ret, ==, exp_ptr_ret);
     g_assert_cmpuint(val, ==, exp_ptr_val);
-    g_assert_true(endptr == str + exp_ptr_offset);
+    if (str) {
+        g_assert_true(endptr == str + exp_ptr_offset);
+    } else {
+        g_assert_cmpint(exp_ptr_offset, ==, 0);
+        g_assert_null(endptr);
+    }

     val = 0xbaadf00d;
     ret = fn(str, NULL, &val);
@@ -3198,8 +3203,8 @@ static void test_qemu_strtosz_simple(void)
     /* Leading 0 gives decimal results, not octal */
     do_strtosz("08", 0, 8, 2);

-    /* Leading space is ignored */
-    do_strtosz(" 12345", 0, 12345, 6);
+    /* Leading space and + are ignored */
+    do_strtosz(" +12345", 0, 12345, 7);

     /* 2^53-1 */
     do_strtosz("9007199254740991", 0, 0x1fffffffffffffULL, 16);
@@ -3226,17 +3231,27 @@ static void test_qemu_strtosz_hex(void)

     do_strtosz("0xab", 0, 171, 4);

-    do_strtosz("0xae", 0, 174, 4);
+    do_strtosz(" +0xae", 0, 174, 6);
 }

 static void test_qemu_strtosz_units(void)
 {
-    /* default is M */
+    /* default scale depends on function */
+    do_strtosz("1", 0, 1, 1);
     do_strtosz_MiB("1", 0, MiB, 1);
+    do_strtosz_metric("1", 0, 1, 1);

+    /* Explicit byte suffix works for all functions */
     do_strtosz("1B", 0, 1, 2);
+    do_strtosz_MiB("1B", 0, 1, 2);
+    do_strtosz_metric("1B", 0, 1, 2);

+    /* Expose the scale */
     do_strtosz("1K", 0, KiB, 2);
+    do_strtosz_MiB("1K", 0, KiB, 2);
+    do_strtosz_metric("1K", 0, 1000, 2);
+
+    /* Other suffixes, see also test_qemu_strtosz_metric */
     do_strtosz("1M", 0, MiB, 2);
     do_strtosz("1G", 0, GiB, 2);
     do_strtosz("1T", 0, TiB, 2);
@@ -3248,14 +3263,37 @@ static void test_qemu_strtosz_float(void)
 {
     do_strtosz("0.5E", 0, EiB / 2, 4);

+    /* Implied M suffix okay */
+    do_strtosz_MiB("0.5", 0, MiB / 2, 3);
+
     /* For convenience, a fraction of 0 is tolerated even on bytes */
     do_strtosz("1.0B", 0, 1, 4);

-    /* An empty fraction is tolerated */
+    /* An empty fraction tail is tolerated */
     do_strtosz("1.k", 0, 1024, 3);

+    /* FIXME An empty fraction head should be tolerated */
+    do_strtosz(" .5k", -EINVAL /* FIXME 0 */, 0xbaadf00d /* FIXME 512 */,
+               0 /* FIXME 4 */);
+
     /* For convenience, we permit values that are not byte-exact */
     do_strtosz("12.345M", 0, (uint64_t) (12.345 * MiB + 0.5), 7);
+
+    /* FIXME Fraction tail should round correctly */
+    do_strtosz("1.9999k", 0, 2048, 7);
+    do_strtosz("1.9999999999999999999999999999999999999999999999999999k", 0,
+               1024 /* FIXME 2048 */, 55);
+
+    /* FIXME ERANGE underflow in the fraction tail should not matter for 'k' */
+    do_strtosz("1."
+               "00000000000000000000000000000000000000000000000000"
+               "00000000000000000000000000000000000000000000000000"
+               "00000000000000000000000000000000000000000000000000"
+               "00000000000000000000000000000000000000000000000000"
+               "00000000000000000000000000000000000000000000000000"
+               "00000000000000000000000000000000000000000000000000"
+               "00000000000000000000000000000000000000000000000000"
+               "1k", 0, 1 /* FIXME 1024 */, 354);
 }

 static void test_qemu_strtosz_invalid(void)
@@ -3265,57 +3303,142 @@ static void test_qemu_strtosz_invalid(void)
     /* Must parse at least one digit */
     do_strtosz("", -EINVAL, 0xbaadf00d, 0);
     do_strtosz(" \t ", -EINVAL, 0xbaadf00d, 0);
-    do_strtosz("crap", -EINVAL, 0xbaadf00d, 0);
+    do_strtosz(".", -EINVAL, 0xbaadf00d, 0);
+    do_strtosz(" .", -EINVAL, 0xbaadf00d, 0);
+    do_strtosz(" .k", -EINVAL, 0xbaadf00d, 0);
     do_strtosz("inf", -EINVAL, 0xbaadf00d, 0);
     do_strtosz("NaN", -EINVAL, 0xbaadf00d, 0);

+    /* Lone suffix is not okay */
+    do_strtosz("k", -EINVAL, 0xbaadf00d, 0);
+    do_strtosz(" M", -EINVAL, 0xbaadf00d, 0);
+
     /* Fractional values require scale larger than bytes */
     do_strtosz("1.1B", -EINVAL, 0xbaadf00d, 0);
     do_strtosz("1.1", -EINVAL, 0xbaadf00d, 0);

+    /* FIXME underflow in the fraction tail should matter for 'B' */
+    do_strtosz("1.00001B", -EINVAL, 0xbaadf00d, 0);
+    do_strtosz("1.00000000000000000001B", 0 /* FIXME -EINVAL */,
+               1 /* FIXME 0xbaadf00d */, 23 /* FIXME 0 */);
+    do_strtosz("1."
+               "00000000000000000000000000000000000000000000000000"
+               "00000000000000000000000000000000000000000000000000"
+               "00000000000000000000000000000000000000000000000000"
+               "00000000000000000000000000000000000000000000000000"
+               "00000000000000000000000000000000000000000000000000"
+               "00000000000000000000000000000000000000000000000000"
+               "00000000000000000000000000000000000000000000000000"
+               "1B", 0 /* FIXME -EINVAL */, 1 /* FIXME 0xbaadf00d */,
+               354 /* FIXME 0 */);
+
     /* No hex fractions */
     do_strtosz("0x1.8k", -EINVAL, 0xbaadf00d, 0);
+    do_strtosz("0x1.k", -EINVAL, 0xbaadf00d, 0);

-    /* No suffixes */
+    /* No hex suffixes */
     do_strtosz("0x18M", -EINVAL, 0xbaadf00d, 0);
+    do_strtosz("0x1p1", -EINVAL, 0xbaadf00d, 0);

-    /* No negative values */
-    do_strtosz("-0", -EINVAL, 0xbaadf00d, 0);
-    do_strtosz("-1", -EINVAL, 0xbaadf00d, 0);
+    /* decimal in place of scaling suffix */
+    do_strtosz("1.1.k", -EINVAL, 0xbaadf00d, 0);
+    do_strtosz("1.1.", -EINVAL, 0xbaadf00d, 0);
 }

 static void test_qemu_strtosz_trailing(void)
 {
+    /* Trailing whitespace */
+    do_strtosz_full("1k ", qemu_strtosz, 0, 1024, 2, -EINVAL, 0xbaadf00d);
+
+    /* Unknown suffix overrides even implied scale*/
+    do_strtosz_full("123xxx", qemu_strtosz, 0, 123, 3, -EINVAL, 0xbaadf00d);
+
+    /* Implied scale allows partial parse */
     do_strtosz_full("123xxx", qemu_strtosz_MiB, 0, 123 * MiB, 3,
                     -EINVAL, 0xbaadf00d);
+    do_strtosz_full("1.5.k", qemu_strtosz_MiB, 0, 1.5 * MiB, 3,
+                    -EINVAL, 0xbaadf00d);

+    /* Junk after one-byte suffix */
     do_strtosz_full("1kiB", qemu_strtosz, 0, 1024, 2, -EINVAL, 0xbaadf00d);
+
+    /* Incomplete hex is an unknown suffix */
     do_strtosz_full("0x", qemu_strtosz, 0, 0, 1, -EINVAL, 0xbaadf00d);
+
+    /* Hex literals use only one leading zero */
+    do_strtosz_full("00x1", qemu_strtosz, 0, 0, 2, -EINVAL, 0xbaadf00d);
+
+    /* No support for binary literals; 'b' is valid suffix */
+    do_strtosz_full("0b1000", qemu_strtosz, 0, 0, 2, -EINVAL, 0xbaadf00d);
+
+    /* Junk after decimal */
     do_strtosz_full("0.NaN", qemu_strtosz, 0, 0, 2, -EINVAL, 0xbaadf00d);
+
+    /* Although negatives are invalid, '-' may be in trailing junk */
     do_strtosz_full("123-45", qemu_strtosz, 0, 123, 3, -EINVAL, 0xbaadf00d);
+    do_strtosz_full(" 123 - 45", qemu_strtosz, 0, 123, 4, -EINVAL, 0xbaadf00d);

     /* FIXME should stop parse after 'e'. No floating point exponents */
     do_strtosz_full("1.5e1k", qemu_strtosz, -EINVAL /* FIXME 0 */,
                     0xbaadf00d /* FIXME EiB * 1.5 */, 0 /* FIXME 4 */,
                     -EINVAL, 0xbaadf00d);
-
     do_strtosz_full("1.5E+0k", qemu_strtosz, -EINVAL /* FIXME 0 */,
                     0xbaadf00d /* FIXME EiB * 1.5 */, 0 /* FIXME 4 */,
                     -EINVAL, 0xbaadf00d);
+
+    /*
+     * 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 0xbaadf00d */);
 }

 static void test_qemu_strtosz_erange(void)
 {
+    /* FIXME negative values fit better as ERANGE */
+    do_strtosz(" -0", -EINVAL /* FIXME -ERANGE */, 0xbaadf00d, 0 /* FIXME 3 */);
+    do_strtosz("-1", -EINVAL /* FIXME -ERANGE */, 0xbaadf00d, 0 /* FIXME 2 */);
+    do_strtosz_full("-2M", qemu_strtosz, -EINVAL /* FIXME -ERANGE */,
+                    0xbaadf00d, 0 /* FIXME 2 */, -EINVAL, 0xbaadf00d);
+    do_strtosz(" -.0", -EINVAL /* FIXME -ERANGE */, 0xbaadf00d,
+               0 /* FIXME 4 */);
+    do_strtosz_full("-.1k", qemu_strtosz, -EINVAL /* FIXME -ERANGE */,
+                    0xbaadf00d, 0 /* FIXME 3 */, -EINVAL, 0xbaadf00d);
+    do_strtosz_full(" -."
+                    "00000000000000000000000000000000000000000000000000"
+                    "00000000000000000000000000000000000000000000000000"
+                    "00000000000000000000000000000000000000000000000000"
+                    "00000000000000000000000000000000000000000000000000"
+                    "00000000000000000000000000000000000000000000000000"
+                    "00000000000000000000000000000000000000000000000000"
+                    "00000000000000000000000000000000000000000000000000"
+                    "1M", qemu_strtosz, -EINVAL /* FIXME -ERANGE */,
+                    0xbaadf00d, 0 /* FIXME 354 */, -EINVAL, 0xbaadf00d);
+
     /* 2^64; see strtosz_simple for 2^64-1 */
     do_strtosz("18446744073709551616", -ERANGE, 0xbaadf00d, 20);

     do_strtosz("20E", -ERANGE, 0xbaadf00d, 3);
+
+    /* FIXME Fraction tail can cause ERANGE overflow */
+    do_strtosz("15.9999999999999999999999999999999999999999999999999999E",
+               0 /* FIXME -ERANGE */, 15ULL * EiB /* FIXME 0xbaadf00d */, 56);
+
+    /* EINVAL has priority over ERANGE */
+    do_strtosz_full("100000Pjunk", qemu_strtosz, -ERANGE, 0xbaadf00d, 7,
+                    -EINVAL, 0xbaadf00d);
 }

 static void test_qemu_strtosz_metric(void)
 {
     do_strtosz_metric("12345k", 0, 12345000, 6);
     do_strtosz_metric("12.345M", 0, 12345000, 7);
+
+    /* Fraction is affected by floating-point rounding */
+    /* This would be 0xfffffffffffffbff with infinite precision */
+    do_strtosz_metric("18.446744073709550591E", 0, 0xfffffffffffffc0cULL, 22);
 }

 static void test_freq_to_str(void)
-- 
2.40.1

Re: [PATCH v2 14/19] test-cutils: Add more coverage to qemu_strtosz11;rgb:1e1e/1e1e/1e1e

Posted by Hanna Czenczek 2 years, 8 months ago

On 12.05.23 04:10, 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>
>
> ---
>
> v2: even more tests added, pad a string to avoid out-of-bounds
> randomness [Hanna]
> ---
>   tests/unit/test-cutils.c | 147 +++++++++++++++++++++++++++++++++++----
>   1 file changed, 135 insertions(+), 12 deletions(-)

The subject line appears as if it contained an ANSI escape sequence.

> diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c
> index 1936c7b5795..7800caf9b0e 100644
> --- a/tests/unit/test-cutils.c
> +++ b/tests/unit/test-cutils.c
> @@ -3162,7 +3162,12 @@ static void do_strtosz_full(const char *str, qemu_strtosz_fn fn,
>       ret = fn(str, &endptr, &val);
>       g_assert_cmpint(ret, ==, exp_ptr_ret);
>       g_assert_cmpuint(val, ==, exp_ptr_val);
> -    g_assert_true(endptr == str + exp_ptr_offset);
> +    if (str) {
> +        g_assert_true(endptr == str + exp_ptr_offset);
> +    } else {
> +        g_assert_cmpint(exp_ptr_offset, ==, 0);
> +        g_assert_null(endptr);
> +    }

This patch adds no new cases that call do_strtosz*() with a NULL str – 
did you intent for this to go into patch 12?

Regardless (with the subject fixed, though):

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

Re: [PATCH v2 14/19] test-cutils: Add more coverage to qemu_strtosz11;rgb:1e1e/1e1e/1e1e

Posted by Eric Blake 2 years, 8 months ago

On Fri, May 19, 2023 at 05:26:12PM +0200, Hanna Czenczek wrote:
> On 12.05.23 04:10, 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>
> > 
> > ---
> > 
> > v2: even more tests added, pad a string to avoid out-of-bounds
> > randomness [Hanna]
> > ---
> >   tests/unit/test-cutils.c | 147 +++++++++++++++++++++++++++++++++++----
> >   1 file changed, 135 insertions(+), 12 deletions(-)
> 
> The subject line appears as if it contained an ANSI escape sequence.

Yep, and I even flagged that in reply to the cover letter.

> 
> > diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c
> > index 1936c7b5795..7800caf9b0e 100644
> > --- a/tests/unit/test-cutils.c
> > +++ b/tests/unit/test-cutils.c
> > @@ -3162,7 +3162,12 @@ static void do_strtosz_full(const char *str, qemu_strtosz_fn fn,
> >       ret = fn(str, &endptr, &val);
> >       g_assert_cmpint(ret, ==, exp_ptr_ret);
> >       g_assert_cmpuint(val, ==, exp_ptr_val);
> > -    g_assert_true(endptr == str + exp_ptr_offset);
> > +    if (str) {
> > +        g_assert_true(endptr == str + exp_ptr_offset);
> > +    } else {
> > +        g_assert_cmpint(exp_ptr_offset, ==, 0);
> > +        g_assert_null(endptr);
> > +    }
> 
> This patch adds no new cases that call do_strtosz*() with a NULL str – did
> you intent for this to go into patch 12?

Oh, indeed - it was patch 12 that added do_strtosz(NULL, -EINVAL,
0xbaadf00d, 0); it's a shame the compiler doesn't complain about 'NULL
+ 0' as being an odd expression.  Yes, I'll hoist this hunk to 12 for
v3...

> 
> Regardless (with the subject fixed, though):
> 
> Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

...while keeping your R-b.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org