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 <ebl...@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
