Message ID | 1445903114-22566-2-git-send-email-jsnow@redhat.com |
---|---|
State | New |
Headers | show |
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
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); @@ -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; } @@ -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) { @@ -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; } @@ -2191,10 +2190,14 @@ static const cmdinfo_t sigraise_cmd = { static int sigraise_f(BlockBackend *blk, int argc, char **argv) { - int sig = cvtnum(argv[1]); + int64_t sig = cvtnum(argv[1]); if (sig < 0) { printf("non-numeric signal number argument -- %s\n", argv[1]); return 0; + } else if (sig > NSIG) { + printf("signal argument '%s' is too large to be a valid signal\n", + argv[1]); + return 0; } /* Using raise() to kill this process does not necessarily flush all open