From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41563) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtvQ0-0007K7-O2 for qemu-devel@nongnu.org; Wed, 04 Nov 2015 05:35:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZtvPz-0004Zn-MR for qemu-devel@nongnu.org; Wed, 04 Nov 2015 05:35:52 -0500 Date: Wed, 4 Nov 2015 11:35:44 +0100 From: Kevin Wolf Message-ID: <20151104103544.GA4026@noname.redhat.com> References: <1446596262-15328-1-git-send-email-jsnow@redhat.com> <1446596262-15328-2-git-send-email-jsnow@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1446596262-15328-2-git-send-email-jsnow@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 1/3] qemu-io: fix cvtnum lval types List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com Am 04.11.2015 um 01:17 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 > Signed-off-by: John Snow > --- > qemu-io-cmds.c | 88 +++++++++++++++++++++++++++++++++++----------------------- > 1 file changed, 53 insertions(+), 35 deletions(-) > v3: > - pulled a lot of loose yarn, now missing my sweater > (Updated patch 1 even further, reported-by Kevin) I'm afraid you'll have to start using up another sweater. > diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c > index 6e5d1e4..f04c1db 100644 > --- a/qemu-io-cmds.c > +++ b/qemu-io-cmds.c > @@ -294,7 +294,7 @@ static void qemu_io_free(void *p) > qemu_vfree(p); > } > > -static void dump_buffer(const void *buffer, int64_t offset, int len) > +static void dump_buffer(const void *buffer, int64_t offset, int64_t len) > { > int i, j; > const uint8_t *p; One more line of context: for (i = 0, p = buffer; i < len; i += 16) { For len > INT_MAX, this is an endless loop. The same way, i + j a few lines below can wrap around. > @@ -393,8 +393,8 @@ fail: > return buf; > } > > -static int do_read(BlockBackend *blk, char *buf, int64_t offset, int count, > - int *total) > +static int do_read(BlockBackend *blk, char *buf, int64_t offset, int64_t count, > + int64_t *total) > { > int ret; Again, one more line of context: ret = blk_read(blk, offset >> 9, (uint8_t *)buf, count >> 9); count is silently truncated if it's larger than INT_MAX << 9. I think we should return an error (ERANGE? EINVAL? EFBIG?) instead. Same for do_write, do_pread, do_pwrite, co_write_zeroes_entry, do_write_compressed, do_load_vmstate, do_save_vmstate. Kevin