[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] block/vhdx: check error return
From: |
Jeff Cody |
Subject: |
Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] block/vhdx: check error return of bdrv_getlength() |
Date: |
Mon, 7 Aug 2017 08:16:49 -0400 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
On Mon, Aug 07, 2017 at 12:46:30PM +0200, Kevin Wolf wrote:
> Am 07.08.2017 um 05:08 hat Jeff Cody geschrieben:
> > Calls to bdrv_getlength() were not checking for error. In vhdx.c, this
> > can lead to truncating an image file, so it is a definite bug. In
> > vhdx-log.c, the path for improper behavior is less clear, but it is best
> > to check in any case.
> >
> > Reported-by: Markus Armbruster <address@hidden>
> > Signed-off-by: Jeff Cody <address@hidden>
> > ---
> > block/vhdx-log.c | 20 ++++++++++++++++----
> > block/vhdx.c | 9 ++++++++-
> > 2 files changed, 24 insertions(+), 5 deletions(-)
> >
> > diff --git a/block/vhdx-log.c b/block/vhdx-log.c
> > index 01278f3..fd4e7af 100644
> > --- a/block/vhdx-log.c
> > +++ b/block/vhdx-log.c
> > @@ -491,6 +491,7 @@ static int vhdx_log_flush(BlockDriverState *bs,
> > BDRVVHDXState *s,
> > uint32_t cnt, sectors_read;
> > uint64_t new_file_size;
> > void *data = NULL;
> > + int64_t file_length;
> > VHDXLogDescEntries *desc_entries = NULL;
> > VHDXLogEntryHeader hdr_tmp = { 0 };
> >
> > @@ -510,10 +511,15 @@ static int vhdx_log_flush(BlockDriverState *bs,
> > BDRVVHDXState *s,
> > if (ret < 0) {
> > goto exit;
> > }
> > + file_length = bdrv_getlength(bs->file->bs);
> > + if (file_length < 0) {
> > + ret = file_length;
> > + goto exit;
> > + }
> > /* if the log shows a FlushedFileOffset larger than our current
> > file
> > * size, then that means the file has been truncated / corrupted,
> > and
> > * we must refused to open it / use it */
> > - if (hdr_tmp.flushed_file_offset > bdrv_getlength(bs->file->bs)) {
> > + if (hdr_tmp.flushed_file_offset > file_length) {
> > ret = -EINVAL;
> > goto exit;
> > }
> > @@ -543,7 +549,7 @@ static int vhdx_log_flush(BlockDriverState *bs,
> > BDRVVHDXState *s,
> > goto exit;
> > }
> > }
> > - if (bdrv_getlength(bs->file->bs) <
> > desc_entries->hdr.last_file_offset) {
> > + if (file_length < desc_entries->hdr.last_file_offset) {
> > new_file_size = desc_entries->hdr.last_file_offset;
> > if (new_file_size % (1024*1024)) {
> > /* round up to nearest 1MB boundary */
>
> The vhdx_log_flush() part looks good, but it made me notice a
> bdrv_flush() in the same function where the return value isn't checked.
> I'm almost sure that this is a bug.
>
I'll add 2 more simple patches to the series: one for bdrv_flush(), and one
for the unchecked bdrv_truncate() as well.
> > @@ -851,6 +857,7 @@ static int vhdx_log_write(BlockDriverState *bs,
> > BDRVVHDXState *s,
> > uint32_t partial_sectors = 0;
> > uint32_t bytes_written = 0;
> > uint64_t file_offset;
> > + int64_t file_length;
> > VHDXHeader *header;
> > VHDXLogEntryHeader new_hdr;
> > VHDXLogDescriptor *new_desc = NULL;
> > @@ -913,10 +920,15 @@ static int vhdx_log_write(BlockDriverState *bs,
> > BDRVVHDXState *s,
> > .sequence_number = s->log.sequence,
> > .descriptor_count = sectors,
> > .reserved = 0,
> > - .flushed_file_offset = bdrv_getlength(bs->file->bs),
> > - .last_file_offset = bdrv_getlength(bs->file->bs),
> > };
> >
> > + file_length = bdrv_getlength(bs->file->bs);
> > + if (file_length < 0) {
> > + ret = file_length;
> > + goto exit;
> > + }
> > + new_hdr.flushed_file_offset = file_length;
> > + new_hdr.last_file_offset = file_length;
> > new_hdr.log_guid = header->log_guid;
>
> If you move the bdrv_getlength() above the initialisation of new_hdr,
> you could keep these fields in the designated initialiser, which should
> be better for readability.
>
> I also don't know why .log_guid isn't part if it, could be moved, too.
>
> > desc_sectors = vhdx_compute_desc_sectors(new_hdr.descriptor_count);
> > diff --git a/block/vhdx.c b/block/vhdx.c
> > index a9cecd2..6a14999 100644
> > --- a/block/vhdx.c
> > +++ b/block/vhdx.c
> > @@ -1166,7 +1166,14 @@ exit:
> > static int vhdx_allocate_block(BlockDriverState *bs, BDRVVHDXState *s,
> > uint64_t *new_offset)
> > {
> > - *new_offset = bdrv_getlength(bs->file->bs);
> > + int64_t current_len;
> > + current_len = bdrv_getlength(bs->file->bs);
> > +
> > + if (current_len < 0) {
> > + return current_len;
> > + }
>
> Don't you want the empty line to be between declaration and code rather
> than assignment and check?
>
> > + *new_offset = current_len;
> >
> > /* per the spec, the address for a block is in units of 1MB */
> > *new_offset = ROUND_UP(*new_offset, 1024 * 1024);
>
> So the code looks correct, but we could make it a little nicer in a v2.
>
Yep, thanks. I'll address all those cleanups you mentioned in v2.