Re: [PATCH 06/11] test-cutils: Add more coverage to qemu_strtosz

[Eric Blake]

On Tue, May 09, 2023 at 05:15:04PM +0200, Hanna Czenczek wrote:
> On 08.05.23 22:03, 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>
> > ---
> >   tests/unit/test-cutils.c | 226 +++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 215 insertions(+), 11 deletions(-)
> > 
> > diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c
> > index afae2ee5331..9fa6fb042e8 100644
> > --- a/tests/unit/test-cutils.c
> > +++ b/tests/unit/test-cutils.c
> 
> [...]
> 
> > @@ -2875,6 +3056,20 @@ static void test_qemu_strtosz_trailing(void)
> >       err = qemu_strtosz(str, NULL, &res);
> >       g_assert_cmpint(err, ==, -EINVAL);
> >       g_assert_cmphex(res, ==, 0xbaadf00d);
> > +
> > +    /* FIXME overflow in fraction is buggy */
> > +    str = "1.5E999";
> > +    endptr = NULL;
> > +    res = 0xbaadf00d;
> > +    err = qemu_strtosz(str, &endptr, &res);
> > +    g_assert_cmpint(err, ==, 0);
> > +    g_assert_cmpuint(res, ==, EiB /* FIXME EiB * 1.5 */);
> > +    g_assert(endptr == str + 9 /* FIXME + 4 */);
> > +
> > +    res = 0xbaadf00d;
> > +    err = qemu_strtosz(str, NULL, &res);
> > +    g_assert_cmpint(err, ==, -EINVAL);
> > +    g_assert_cmphex(res, ==, 0xbaadf00d);
> 
> Got it now!
> 
> Our problem is that `endptr` is beyond the end of the string, precisely as
> gcc complains.  The data there is undefined, and depending on the value in
> the g_assert_cmpuint() (which is converted to strings for the potential
> error message) it sometimes is "endptr == str + 9" (the one in the
> g_assert()) and sometimes isn’t.
> 
> If it is "endptr == str + 9", then the 'e' is taken as a suffix, which makes
> `res == EiB`, and `endptr == "ndptr == str + 9"`.
> 
> If it isn’t, well, it might be anything, so there often is no valid suffix,
> making `res == 1`.
> 
> So the solution is to set `str = "1.5E999\0\0"`, so we don’t get out of
> bounds and know exactly what will be parsed.  Then, at str[8] there is no
> valid suffix (it’s \0), so `res == 1` and `endptr == str + 8`.  This will
> then lead to the qemu_strtosz(str, NULL, &res) below succeed, because, well,
> it’s a valid number.  I suppose it failed on your end because the
> out-of-bounds `str[9]` value was not '\0'.
> 
> That was a fun debugging session.

Wow, yeah, that's a mess.  The very test proving that we have a
read-out-of-bounds bug is super-sensitive to what is at that location.
Your hack of passing in extra \0 is awesome; I'll fold that in whether
we need a v2 for other reasons, or in-place if we can take the rest of
this series as-is.  It all goes away at the end of the series,
anyways, once the out-of-bounds read is fixed.

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