Re: [PULL 07/21] cutils: Fix wraparound parsing in qemu_strtoui

From: Eric Blake <eblake@redhat.com>
To: Michael Tokarev <mjt@tls.msk.ru>
Cc: qemu-devel@nongnu.org, qemu-stable@nongnu.org,
	 Hanna Czenczek <hreitz@redhat.com>
Subject: Re: [PULL 07/21] cutils: Fix wraparound parsing in qemu_strtoui
Date: Mon, 5 Jun 2023 08:32:55 -0500	[thread overview]
Message-ID: <j322ihz6lx6zpjy7mun4mzfwnpih7seqjnpffm5dhebyur45ur@p2aqn2znt6wq> (raw)
In-Reply-To: <62d84222-7da7-d036-7ef1-37b88de69572@tls.msk.ru>

On Sat, Jun 03, 2023 at 11:17:27AM +0300, Michael Tokarev wrote:
> 02.06.2023 01:02, Eric Blake пишет:
> > While we were matching 32-bit strtol in qemu_strtoi, our use of a
> > 64-bit parse was leaking through for some inaccurate answers in
> > qemu_strtoui in comparison to a 32-bit strtoul (see the unit test for
> > examples).  The comment for that function even described what we have
> > to do for a correct parse, but didn't implement it correctly: since
> > strtoull checks for overflow against the wrong values and then
> > negates, we have to temporarily undo negation before checking for
> > overflow against our desired value.
> > 
> > Our int wrappers would be a lot easier to write if libc had a
> > guaranteed 32-bit parser even on platforms with 64-bit long.
> > 
> > Whether we parse C2x binary strings like "0b1000" is currently up to
> > what libc does; our unit tests intentionally don't cover that at the
> > moment, though.
> > 
> > Fixes: 473a2a331e ("cutils: add qemu_strtoi & qemu_strtoui parsers for int/unsigned int types", v2.12.0)
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > CC: qemu-stable@nongnu.org
> 
> Trying to pick this one up for -stable. The implementation changes are good.
> But the testsuite changes are.. difficult.  The thing is that testsuite changes
> (here and in the other -stable patch) applies on top of previous changes in
> there (in the same series), which, in turn, requires other previous code changes
> in the implementation to succeed.

Yeah, I did wonder if it is worth even trying to get this for stable.
The series is inter-tangled enough that it feels like an
all-or-nothing approach may be easiest - but all means 19 patches and
hundreds of lines of testsuite additions.  My other argument when
first posting this to qemu-stable was to just declare that these have
been broken long enough that it is not a recent regression, and users
are unlikely to be supplying command-line or QMP strings to tickle
these bugs, so not backporting may be okay.

> 
> For example, this patch changes test_qemu_strtoui_overflow() which was introduced
> in previous patch "Test more integer corner cases" from this series and further
> modified in "Test integral qemu_strto* value on failures" one.  Picking them result
> in testsuite failing due to missing previous code changes.
> 
> I tried to drop just the testsuite changes, but the result is that the testsuite
> fails with fixed code :)
> 
> It's quite fun situation actually, like it fails no matter what you do, one way
> or another.
> 
> I'll try to find the most stright-forward way from here.  Good stuff.

Let me know how I can help in deciding what, if any, is worth backporting.

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