[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 00/25] chardev: Convert qemu_chr_write() to t
Daniel P . Berrangé
Re: [Qemu-devel] [PATCH v3 00/25] chardev: Convert qemu_chr_write() to take a size_t argument
Wed, 20 Feb 2019 11:30:16 +0000
On Wed, Feb 20, 2019 at 11:53:42AM +0100, Marc-André Lureau wrote:
> 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
> 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
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.
|: 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 :|
- [Qemu-devel] [PATCH v3 18/25] s390x/3270: Let insert_IAC_escape_char() use size_t, (continued)
- [Qemu-devel] [PATCH v3 18/25] s390x/3270: Let insert_IAC_escape_char() use size_t, Philippe Mathieu-Daudé, 2019/02/19
- [Qemu-devel] [PATCH v3 21/25] s390x/sclp: Use size_t in process_mdb(), Philippe Mathieu-Daudé, 2019/02/19
- [Qemu-devel] [PATCH v3 22/25] s390x/sclp: Let write_console_data() take a size_t, Philippe Mathieu-Daudé, 2019/02/19
- [Qemu-devel] [PATCH v3 24/25] chardev: Let qemu_chr_fe_write[_all] use size_t type argument, Philippe Mathieu-Daudé, 2019/02/19
- Re: [Qemu-devel] [PATCH v3 00/25] chardev: Convert qemu_chr_write() to take a size_t argument, Marc-André Lureau, 2019/02/20