Re: [Qemu-devel] [PATCH v2 11/11] chardev: FDChardev::max_size be unsigned

[Paolo Bonzini <pbonzini@redhat.com>]

On 12/10/2018 02:22, Philippe Mathieu-Daudé wrote:
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  chardev/char-fd.c         | 2 +-
>  include/chardev/char-fd.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/chardev/char-fd.c b/chardev/char-fd.c
> index bb426fa4b1..900da2f935 100644
> --- a/chardev/char-fd.c
> +++ b/chardev/char-fd.c
> @@ -43,7 +43,7 @@ static gboolean fd_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
>  {
>      Chardev *chr = CHARDEV(opaque);
>      FDChardev *s = FD_CHARDEV(opaque);
> -    int len;
> +    size_t len;
>      uint8_t buf[CHR_READ_BUF_LEN];
>      ssize_t ret;
>  
> diff --git a/include/chardev/char-fd.h b/include/chardev/char-fd.h
> index e7c2b176f9..36c6b89cee 100644
> --- a/include/chardev/char-fd.h
> +++ b/include/chardev/char-fd.h
> @@ -31,7 +31,7 @@ typedef struct FDChardev {
>      Chardev parent;
>  
>      QIOChannel *ioc_in, *ioc_out;
> -    int max_size;
> +    size_t max_size;
>  } FDChardev;
>  
>  #define TYPE_CHARDEV_FD "chardev-fd"
> 

This shouldn't be just for max_size, it should be for all variables that
are set in the *_read_poll functions (those that you touch in patch 3).

These variables are than used very little, basically only in a

    len = MAX(s->max_size, sizeof(buf))

statement, so this switch is safe.  However, the order of the patches
should be first 4, then this one (the assertion shows that the switch to
unsigned is safe), then 5-6-9-10, then 7-8.  If you convert
implementations before users, the users could in principle overflow
"int" when passing an arguments or storing its value.

All this of course should be documented in commit messages, which are a
bit... scant in this series. :)  I'm usually okay with very short commit
messages when the changes are spread across many commits (in that case,
I usually document what all the repetitive changes are in the patches
before and/or after those changes), but in this case you are leaving out
completely the "why" for the changes, and that's not really a good idea.

Finally, can you please include a patch to adjust the assertions in the
USB smartcard code, as mentioned in my original reply to Prasad?

Thanks,

Paolo