| Message ID | 85fe374c783a6d659bc86aa7f15d5f442b9d7a45.1502075213.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <armbru@redhat.com> > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > 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. > @@ -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. Kevin
On 08/06/2017 10:08 PM, Jeff Cody wrote: > 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 <armbru@redhat.com> > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block/vhdx-log.c | 20 ++++++++++++++++---- > block/vhdx.c | 9 ++++++++- > 2 files changed, 24 insertions(+), 5 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
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 <armbru@redhat.com> > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > --- > > 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.
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 */ @@ -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; 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; + } + + *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);
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 <armbru@redhat.com> Signed-off-by: Jeff Cody <jcody@redhat.com> --- block/vhdx-log.c | 20 ++++++++++++++++---- block/vhdx.c | 9 ++++++++- 2 files changed, 24 insertions(+), 5 deletions(-)