Re: [Qemu-devel] [PATCH v2 1/3] qemu-io: fix cvtnum lval types

[Kevin Wolf <kwolf@redhat.com>]

Am 27.10.2015 um 00:45 hat John Snow geschrieben:
> cvtnum() returns int64_t: we should not be storing this
> result inside of an int.
> 
> In a few cases, we need an extra sprinkling of error handling
> where we expect to pass this number on towards a function that
> expects something smaller than int64_t.
> 
> Reported-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  qemu-io-cmds.c | 31 +++++++++++++++++--------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 6e5d1e4..704db89 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -642,10 +642,11 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>      int c, cnt;
>      char *buf;
>      int64_t offset;
> -    int count;
> +    int64_t count;
>      /* Some compilers get confused and warn if this is not initialized.  */
>      int total = 0;
> -    int pattern = 0, pattern_offset = 0, pattern_count = 0;
> +    int pattern = 0;
> +    int64_t pattern_offset = 0, pattern_count = 0;
>  
>      while ((c = getopt(argc, argv, "bCl:pP:qs:v")) != -1) {
>          switch (c) {
> @@ -734,7 +735,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>              return 0;
>          }
>          if (count & 0x1ff) {
> -            printf("count %d is not sector aligned\n",
> +            printf("count %"PRId64" is not sector aligned\n",
>                     count);
>              return 0;
>          }
> @@ -762,7 +763,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>          memset(cmp_buf, pattern, pattern_count);
>          if (memcmp(buf + pattern_offset, cmp_buf, pattern_count)) {
>              printf("Pattern verification failed at offset %"
> -                   PRId64 ", %d bytes\n",
> +                   PRId64 ", %"PRId64" bytes\n",
>                     offset + pattern_offset, pattern_count);
>          }
>          g_free(cmp_buf);

read_f calls a few helper function which only take an int for count:

do_pread(), do_load_vmstate(), do_read() actually perform the request.
These should probably take int64_t as well (and if we want to be really
careful to avoid wraparounds, check limits individually).

qemu_io_alloc() takes size_t, so will wrap around on 32 bit hosts.
Should take int64_t and check against SIZE_MAX.

dump_buffer() also only takes an int, but I hope nobody tries to dump
more than 2 GB...

print_report() should probably be fixed to take int64_t.

And for total to make sense, it probably needs to be converted to
int64_t as well.

> @@ -957,7 +958,7 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
>      int c, cnt;
>      char *buf = NULL;
>      int64_t offset;
> -    int count;
> +    int64_t count;
>      /* Some compilers get confused and warn if this is not initialized.  */
>      int total = 0;
>      int pattern = 0xcd;
> @@ -1029,7 +1030,7 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
>          }
>  
>          if (count & 0x1ff) {
> -            printf("count %d is not sector aligned\n",
> +            printf("count %"PRId64" is not sector aligned\n",
>                     count);
>              return 0;
>          }

For writes, the helper functions to perform the request are different,
but they also only take int: do_pwrite(), do_save_vmstate(),
do_co_write_zeroes(), do_write_compressed(), do_write().

The rest should be fixed when you fix the helpers for read.

> @@ -1777,8 +1778,7 @@ static int discard_f(BlockBackend *blk, int argc, char **argv)
>      struct timeval t1, t2;
>      int Cflag = 0, qflag = 0;
>      int c, ret;
> -    int64_t offset;
> -    int count;
> +    int64_t offset, count;
>  
>      while ((c = getopt(argc, argv, "Cq")) != -1) {
>          switch (c) {

Here, blk_discard() is called directly without a helper function. A
check that the number of sectors fits in an int is missing.

> @@ -1833,11 +1833,10 @@ out:
>  static int alloc_f(BlockBackend *blk, int argc, char **argv)
>  {
>      BlockDriverState *bs = blk_bs(blk);
> -    int64_t offset, sector_num;
> -    int nb_sectors, remaining;
> +    int64_t offset, sector_num, nb_sectors, remaining;
>      char s1[64];
> -    int num, sum_alloc;
> -    int ret;
> +    int num, ret;
> +    int64_t sum_alloc;
>  
>      offset = cvtnum(argv[1]);
>      if (offset < 0) {
> @@ -1881,7 +1880,7 @@ static int alloc_f(BlockBackend *blk, int argc, char **argv)
>  
>      cvtstr(offset, s1, sizeof(s1));
>  
> -    printf("%d/%d sectors allocated at offset %s\n",
> +    printf("%"PRId64"/%"PRId64" sectors allocated at offset %s\n",
>             sum_alloc, nb_sectors, s1);
>      return 0;
>  }

remaining is passed to bdrv_is_allocated() without checking against
INT_MAX first.

Kevin