VHDX cleanup

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 <[email protected]>

Signed-off-by Jeff Cody <[email protected]>

---
 block/vhdx-log.c | 20 ++++++++++++++++----
 block/vhdx.c     |  9 ++++++++-
 2 files changed, 24 insertions(+), 5 deletions(-)

block/vhdx-log.c +7 -1

index 01278f3..fd4e7af 100644
@@ -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;
         }

block/vhdx-log.c +1 -2

@@ -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),

block/vhdx-log.c +0 -0

     desc_sectors = vhdx_compute_desc_sectors(new_hdr.descriptor_count);

block/vhdx.c +0 -0

index a9cecd2..6a14999 100644

Am 07.08.2017 um 05:08 hat Jeff Cody geschrieben:

Quoted 46 lines

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 <[email protected]>

Signed-off-by Jeff Cody <[email protected]>

---
 block/vhdx-log.c | 20 ++++++++++++++++----
 block/vhdx.c     |  9 ++++++++-
 2 files changed, 24 insertions(+), 5 deletions(-)

block/vhdx-log.c +7 -2

index 01278f3..fd4e7af 100644
@@ -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) {

block/vhdx-log.c static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s, · line 555

Quoted 4 lines

block/vhdx-log.c +1 -0

+        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.

Quoted 14 lines

block/vhdx-log.c +1 -2

@@ -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),

block/vhdx-log.c static int vhdx_log_write(BlockDriverState *bs, BDRVVHDXState *s, · line 932

Quoted 10 lines

block/vhdx-log.c +7 -0

               };
 
+    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.

Quoted 6 lines

block/vhdx-log.c +0 -0

     desc_sectors = vhdx_compute_desc_sectors(new_hdr.descriptor_count);

block/vhdx.c +0 -0

index a9cecd2..6a14999 100644

block/vhdx.c exit: · line 1174

Quoted 10 lines

block/vhdx.c +6 -1

@@ -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?

block/vhdx.c exit: · line 1179

Quoted 4 lines

block/vhdx.c +1 -0

+    *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.

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 <[email protected]>

Signed-off-by Jeff Cody <[email protected]>

---
 block/vhdx-log.c | 20 ++++++++++++++++----
 block/vhdx.c     |  9 ++++++++-
 2 files changed, 24 insertions(+), 5 deletions(-)

VHDX uses uint64_t types for most offsets, following the VHDX spec. However, bdrv_truncate() takes an int64_t value for the truncating offset. Check for overflow before calling bdrv_truncate().

N.B.: For a compliant image this is not an issue, as the maximum VHDX image size is defined per the spec to be 64TB.

Signed-off-by Jeff Cody <[email protected]>

VHDX uses uint64_t types for most offsets, following the VHDX spec. However, bdrv_truncate() takes an int64_t value for the truncating offset. Check for overflow before calling bdrv_truncate().

N.B.: For a compliant image this is not an issue, as the maximum VHDX image size is defined per the spec to be 64TB.

Signed-off-by Jeff Cody <[email protected]>

---
 block/vhdx-log.c | 4 ++++
 block/vhdx.c     | 3 +++
 2 files changed, 7 insertions(+)

block/vhdx-log.c +0 -0

index fd4e7af..3b74e5d 100644

Quoted 13 lines · Jeff Cody

VHDX uses uint64_t types for most offsets, following the VHDX spec. However, bdrv_truncate() takes an int64_t value for the truncating offset. Check for overflow before calling bdrv_truncate().

N.B.: For a compliant image this is not an issue, as the maximum VHDX image size is defined per the spec to be 64TB.

Signed-off-by Jeff Cody <[email protected]>

---
 block/vhdx-log.c | 4 ++++
 block/vhdx.c     | 3 +++
 2 files changed, 7 insertions(+)

block/vhdx-log.c static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s, · line 556

Quoted 8 lines

block/vhdx-log.c +0 -0

index fd4e7af..3b74e5d 100644
@@ -554,6 +554,10 @@ static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s,
             if (new_file_size % (1024*1024)) {
                 /* round up to nearest 1MB boundary */
                 new_file_size = ((new_file_size >> 20) + 1) << 20;

Since you're touching here, can you fix this to use QEMU_ALIGN_UP instead?

block/vhdx-log.c static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s, · line 561

Quoted 5 lines

block/vhdx-log.c +4 -0

+                if (new_file_size > INT64_MAX) {
+                    ret = -EINVAL;
+                    goto exit;
+                }
                 bdrv_truncate(bs->file, new_file_size, PREALLOC_MODE_OFF, NULL);

Reviewed-by Eric Blake <[email protected]>

Signature · 3 lines

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Two VHDX items cleaned up:

1. Check for error when calling bdrv_getlength() [Markus]

2. Check for overflow in offset prior to calling bdrv_truncate().