Re: [qemu-s390x] [Qemu-devel] [PATCH v3 00/25] chardev: Convert qemu_chr_write() to take a size_t argument

[Daniel P . Berrangé]

On Wed, Feb 20, 2019 at 11:53:42AM +0100, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Feb 20, 2019 at 2:02 AM Philippe Mathieu-Daudé
> <address@hidden> wrote:
> >
> > Hi,
> >
> > This series convert the chardev::qemu_chr_write() to take unsigned
> > length argument. To do so I went through all caller and checked if
> > there are no negative value possible.
> 
> 
> Changing signedness is problematic and can easily introduce bugs that
> are easy to miss during review.
> 
> I agree with Cornelia about idiomatic use of int. Changing "int" for
> "size_t" isn't systematically a clear win.
> 
> Even Google C++ style recommends to avoid unsigned types "(except for
> representing bitfields or modular arithmetic). Do not use an unsigned
> type merely to assert that a variable is non-negative."
> https://google.github.io/styleguide/cppguide.html#Integer_Types - see 
> rationale
> 
> Since Paolo you suggested the change, could you give some convincing
> arguments that it's worth taking the plunge?

The chardev write/read methods will end up calling libc read/write
methods, whose parameters are "size_t count".

Thus if there is QEMU code that could currently (mistakenly) pass a
negative value for length to qemu_chr_write, unless something stops
it, this is going to be cast to a size_t when we finally call read/
write on the FD, leading to a large positive value & array out of
bounds read/write. 

IOW we already have inconsistent use of signed vs unsigned in our code
which has potential to cause bugs. Converting chardev to use size_t
we get rid fo the mismatch with the underlying libc APIs we call,
which ultimately eliminates an area of risk longer term. There is a
chance it could uncover some pre-existing dormant bugs, but provided
we do due diligence to check callers I think its a win to be consistent
with libc APIs in size_t usage for read/write.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|