From 75344fa4c5d6a3ebe2dec2d87cbea8524f5d0f42 Mon Sep 17 00:00:00 2001
From: Fam Zheng <famz@redhat.com>
Date: Tue, 20 Jan 2015 11:28:46 +0800
Subject: [PATCH 01/12] virtio-blk: Pass req to virtio_blk_handle_scsi_req

In preparation for calling blk_aio_ioctl. Also make the function static
as no other files need it.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/block/virtio-blk.c          | 9 +++++----
 include/hw/virtio/virtio-blk.h | 3 ---
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index b19b102b42..60cb1d88ed 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -127,12 +127,13 @@ static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock *s)
     return req;
 }
 
-int virtio_blk_handle_scsi_req(VirtIOBlock *blk,
-                               VirtQueueElement *elem)
+static int virtio_blk_handle_scsi_req(VirtIOBlockReq *req)
 {
     int status = VIRTIO_BLK_S_OK;
     struct virtio_scsi_inhdr *scsi = NULL;
-    VirtIODevice *vdev = VIRTIO_DEVICE(blk);
+    VirtIODevice *vdev = VIRTIO_DEVICE(req->dev);
+    VirtQueueElement *elem = &req->elem;
+    VirtIOBlock *blk = req->dev;
 
 #ifdef __linux__
     int i;
@@ -252,7 +253,7 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
 {
     int status;
 
-    status = virtio_blk_handle_scsi_req(req->dev, &req->elem);
+    status = virtio_blk_handle_scsi_req(req);
     virtio_blk_req_complete(req, status);
     virtio_blk_free_request(req);
 }
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 3979dc41af..4652b70b5d 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -153,9 +153,6 @@ VirtIOBlockReq *virtio_blk_alloc_request(VirtIOBlock *s);
 
 void virtio_blk_free_request(VirtIOBlockReq *req);
 
-int virtio_blk_handle_scsi_req(VirtIOBlock *blk,
-                               VirtQueueElement *elem);
-
 void virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb);
 
 void virtio_submit_multiwrite(BlockBackend *blk, MultiReqBuffer *mrb);

From 1dc936aa84b300940b2797c391cc3ca519bc78ce Mon Sep 17 00:00:00 2001
From: Fam Zheng <famz@redhat.com>
Date: Tue, 20 Jan 2015 11:28:47 +0800
Subject: [PATCH 02/12] virtio-blk: Use blk_aio_ioctl

Use the asynchronous interface of ioctl. This will not make the VM
unresponsive if the ioctl takes a long time.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/block/virtio-blk.c | 125 ++++++++++++++++++++++++++----------------
 1 file changed, 79 insertions(+), 46 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 60cb1d88ed..4032fcae27 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -115,6 +115,56 @@ static void virtio_blk_flush_complete(void *opaque, int ret)
     virtio_blk_free_request(req);
 }
 
+#ifdef __linux__
+
+typedef struct {
+    VirtIOBlockReq *req;
+    struct sg_io_hdr hdr;
+} VirtIOBlockIoctlReq;
+
+static void virtio_blk_ioctl_complete(void *opaque, int status)
+{
+    VirtIOBlockIoctlReq *ioctl_req = opaque;
+    VirtIOBlockReq *req = ioctl_req->req;
+    VirtIODevice *vdev = VIRTIO_DEVICE(req->dev);
+    struct virtio_scsi_inhdr *scsi;
+    struct sg_io_hdr *hdr;
+
+    scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base;
+
+    if (status) {
+        status = VIRTIO_BLK_S_UNSUPP;
+        virtio_stl_p(vdev, &scsi->errors, 255);
+        goto out;
+    }
+
+    hdr = &ioctl_req->hdr;
+    /*
+     * From SCSI-Generic-HOWTO: "Some lower level drivers (e.g. ide-scsi)
+     * clear the masked_status field [hence status gets cleared too, see
+     * block/scsi_ioctl.c] even when a CHECK_CONDITION or COMMAND_TERMINATED
+     * status has occurred.  However they do set DRIVER_SENSE in driver_status
+     * field. Also a (sb_len_wr > 0) indicates there is a sense buffer.
+     */
+    if (hdr->status == 0 && hdr->sb_len_wr > 0) {
+        hdr->status = CHECK_CONDITION;
+    }
+
+    virtio_stl_p(vdev, &scsi->errors,
+                 hdr->status | (hdr->msg_status << 8) |
+                 (hdr->host_status << 16) | (hdr->driver_status << 24));
+    virtio_stl_p(vdev, &scsi->residual, hdr->resid);
+    virtio_stl_p(vdev, &scsi->sense_len, hdr->sb_len_wr);
+    virtio_stl_p(vdev, &scsi->data_len, hdr->dxfer_len);
+
+out:
+    virtio_blk_req_complete(req, status);
+    virtio_blk_free_request(req);
+    g_free(ioctl_req);
+}
+
+#endif
+
 static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock *s)
 {
     VirtIOBlockReq *req = virtio_blk_alloc_request(s);
@@ -137,7 +187,7 @@ static int virtio_blk_handle_scsi_req(VirtIOBlockReq *req)
 
 #ifdef __linux__
     int i;
-    struct sg_io_hdr hdr;
+    VirtIOBlockIoctlReq *ioctl_req;
 #endif
 
     /*
@@ -172,71 +222,52 @@ static int virtio_blk_handle_scsi_req(VirtIOBlockReq *req)
     }
 
 #ifdef __linux__
-    memset(&hdr, 0, sizeof(struct sg_io_hdr));
-    hdr.interface_id = 'S';
-    hdr.cmd_len = elem->out_sg[1].iov_len;
-    hdr.cmdp = elem->out_sg[1].iov_base;
-    hdr.dxfer_len = 0;
+    ioctl_req = g_new0(VirtIOBlockIoctlReq, 1);
+    ioctl_req->req = req;
+    ioctl_req->hdr.interface_id = 'S';
+    ioctl_req->hdr.cmd_len = elem->out_sg[1].iov_len;
+    ioctl_req->hdr.cmdp = elem->out_sg[1].iov_base;
+    ioctl_req->hdr.dxfer_len = 0;
 
     if (elem->out_num > 2) {
         /*
          * If there are more than the minimally required 2 output segments
          * there is write payload starting from the third iovec.
          */
-        hdr.dxfer_direction = SG_DXFER_TO_DEV;
-        hdr.iovec_count = elem->out_num - 2;
+        ioctl_req->hdr.dxfer_direction = SG_DXFER_TO_DEV;
+        ioctl_req->hdr.iovec_count = elem->out_num - 2;
 
-        for (i = 0; i < hdr.iovec_count; i++)
-            hdr.dxfer_len += elem->out_sg[i + 2].iov_len;
+        for (i = 0; i < ioctl_req->hdr.iovec_count; i++) {
+            ioctl_req->hdr.dxfer_len += elem->out_sg[i + 2].iov_len;
+        }
 
-        hdr.dxferp = elem->out_sg + 2;
+        ioctl_req->hdr.dxferp = elem->out_sg + 2;
 
     } else if (elem->in_num > 3) {
         /*
          * If we have more than 3 input segments the guest wants to actually
          * read data.
          */
-        hdr.dxfer_direction = SG_DXFER_FROM_DEV;
-        hdr.iovec_count = elem->in_num - 3;
-        for (i = 0; i < hdr.iovec_count; i++)
-            hdr.dxfer_len += elem->in_sg[i].iov_len;
+        ioctl_req->hdr.dxfer_direction = SG_DXFER_FROM_DEV;
+        ioctl_req->hdr.iovec_count = elem->in_num - 3;
+        for (i = 0; i < ioctl_req->hdr.iovec_count; i++) {
+            ioctl_req->hdr.dxfer_len += elem->in_sg[i].iov_len;
+        }
 
-        hdr.dxferp = elem->in_sg;
+        ioctl_req->hdr.dxferp = elem->in_sg;
     } else {
         /*
          * Some SCSI commands don't actually transfer any data.
          */
-        hdr.dxfer_direction = SG_DXFER_NONE;
+        ioctl_req->hdr.dxfer_direction = SG_DXFER_NONE;
     }
 
-    hdr.sbp = elem->in_sg[elem->in_num - 3].iov_base;
-    hdr.mx_sb_len = elem->in_sg[elem->in_num - 3].iov_len;
+    ioctl_req->hdr.sbp = elem->in_sg[elem->in_num - 3].iov_base;
+    ioctl_req->hdr.mx_sb_len = elem->in_sg[elem->in_num - 3].iov_len;
 
-    status = blk_ioctl(blk->blk, SG_IO, &hdr);
-    if (status) {
-        status = VIRTIO_BLK_S_UNSUPP;
-        goto fail;
-    }
-
-    /*
-     * From SCSI-Generic-HOWTO: "Some lower level drivers (e.g. ide-scsi)
-     * clear the masked_status field [hence status gets cleared too, see
-     * block/scsi_ioctl.c] even when a CHECK_CONDITION or COMMAND_TERMINATED
-     * status has occurred.  However they do set DRIVER_SENSE in driver_status
-     * field. Also a (sb_len_wr > 0) indicates there is a sense buffer.
-     */
-    if (hdr.status == 0 && hdr.sb_len_wr > 0) {
-        hdr.status = CHECK_CONDITION;
-    }
-
-    virtio_stl_p(vdev, &scsi->errors,
-                 hdr.status | (hdr.msg_status << 8) |
-                 (hdr.host_status << 16) | (hdr.driver_status << 24));
-    virtio_stl_p(vdev, &scsi->residual, hdr.resid);
-    virtio_stl_p(vdev, &scsi->sense_len, hdr.sb_len_wr);
-    virtio_stl_p(vdev, &scsi->data_len, hdr.dxfer_len);
-
-    return status;
+    blk_aio_ioctl(blk->blk, SG_IO, &ioctl_req->hdr,
+                  virtio_blk_ioctl_complete, ioctl_req);
+    return -EINPROGRESS;
 #else
     abort();
 #endif
@@ -254,8 +285,10 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
     int status;
 
     status = virtio_blk_handle_scsi_req(req);
-    virtio_blk_req_complete(req, status);
-    virtio_blk_free_request(req);
+    if (status != -EINPROGRESS) {
+        virtio_blk_req_complete(req, status);
+        virtio_blk_free_request(req);
+    }
 }
 
 void virtio_submit_multiwrite(BlockBackend *blk, MultiReqBuffer *mrb)

From 8dd93d9339505376f6ce6737ead871ff6d7e676f Mon Sep 17 00:00:00 2001
From: Max Reitz <mreitz@redhat.com>
Date: Mon, 19 Jan 2015 15:49:03 -0500
Subject: [PATCH 03/12] qcow2: Add two more unalignment checks

This adds checks for unaligned L2 table offsets and unaligned data
cluster offsets (actually the preallocated offsets for zero clusters) to
the zero cluster expansion function.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-cluster.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 1fea5142d0..183177d518 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1651,6 +1651,14 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
             continue;
         }
 
+        if (offset_into_cluster(s, l2_offset)) {
+            qcow2_signal_corruption(bs, true, -1, -1, "L2 table offset %#"
+                                    PRIx64 " unaligned (L1 index: %#x)",
+                                    l2_offset, i);
+            ret = -EIO;
+            goto fail;
+        }
+
         if (is_active_l1) {
             /* get active L2 tables from cache */
             ret = qcow2_cache_get(bs, s->l2_table_cache, l2_offset,
@@ -1709,6 +1717,19 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
                 }
             }
 
+            if (offset_into_cluster(s, offset)) {
+                qcow2_signal_corruption(bs, true, -1, -1, "Data cluster offset "
+                                        "%#" PRIx64 " unaligned (L2 offset: %#"
+                                        PRIx64 ", L2 index: %#x)", offset,
+                                        l2_offset, j);
+                if (!preallocated) {
+                    qcow2_free_clusters(bs, offset, s->cluster_size,
+                                        QCOW2_DISCARD_ALWAYS);
+                }
+                ret = -EIO;
+                goto fail;
+            }
+
             ret = qcow2_pre_write_overlap_check(bs, 0, offset, s->cluster_size);
             if (ret < 0) {
                 if (!preallocated) {

From f30136b35a271109b89353b9a5349cf32e6aeda7 Mon Sep 17 00:00:00 2001
From: Max Reitz <mreitz@redhat.com>
Date: Mon, 19 Jan 2015 15:49:04 -0500
Subject: [PATCH 04/12] iotests: Add tests for more corruption cases

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/060     | 15 +++++++++++++++
 tests/qemu-iotests/060.out | 13 +++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index 73863bf1f6..c81319c169 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -186,6 +186,12 @@ $QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
 poke_file "$TEST_IMG" "$l1_offset" "\x80\x00\x00\x00\x00\x04\x2a\x00"
 $QEMU_IO -c "read 0 64k" "$TEST_IMG" | _filter_qemu_io
 
+# Test how well zero cluster expansion can cope with this
+_make_test_img 64M
+$QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
+poke_file "$TEST_IMG" "$l1_offset" "\x80\x00\x00\x00\x00\x04\x2a\x00"
+$QEMU_IMG amend -o compat=0.10 "$TEST_IMG"
+
 echo
 echo "=== Testing unaligned L2 entry ==="
 echo
@@ -194,6 +200,15 @@ $QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
 poke_file "$TEST_IMG" "$l2_offset" "\x80\x00\x00\x00\x00\x05\x2a\x00"
 $QEMU_IO -c "read 0 64k" "$TEST_IMG" | _filter_qemu_io
 
+echo
+echo "=== Testing unaligned pre-allocated zero cluster ==="
+echo
+_make_test_img 64M
+$QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
+poke_file "$TEST_IMG" "$l2_offset" "\x80\x00\x00\x00\x00\x05\x2a\x01"
+# zero cluster expansion
+$QEMU_IMG amend -o compat=0.10 "$TEST_IMG"
+
 echo
 echo "=== Testing unaligned reftable entry ==="
 echo
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index 7d493bbe61..dc9f6b7570 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -123,6 +123,11 @@ wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 qcow2: Marking image as corrupt: L2 table offset 0x42a00 unaligned (L1 index: 0); further corruption events will be suppressed
 read failed: Input/output error
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+qcow2: Marking image as corrupt: L2 table offset 0x42a00 unaligned (L1 index: 0); further corruption events will be suppressed
+qemu-img: Error while amending options: Input/output error
 
 === Testing unaligned L2 entry ===
 
@@ -132,6 +137,14 @@ wrote 65536/65536 bytes at offset 0
 qcow2: Marking image as corrupt: Data cluster offset 0x52a00 unaligned (L2 offset: 0x40000, L2 index: 0); further corruption events will be suppressed
 read failed: Input/output error
 
+=== Testing unaligned pre-allocated zero cluster ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+qcow2: Marking image as corrupt: Data cluster offset 0x52a00 unaligned (L2 offset: 0x40000, L2 index: 0); further corruption events will be suppressed
+qemu-img: Error while amending options: Input/output error
+
 === Testing unaligned reftable entry ===
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864

From 395a22fae064df64d987d703cf70ae0f57306be8 Mon Sep 17 00:00:00 2001
From: Jeff Cody <jcody@redhat.com>
Date: Thu, 22 Jan 2015 08:03:25 -0500
Subject: [PATCH 05/12] block: vmdk - make ret variable usage clear

Keep the variable 'ret' something that is returned by the function it is
defined in.  For the return value of 'sscanf', use a more meaningful
variable name.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vmdk.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 52cb8888e5..dc6459cc00 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -785,6 +785,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
                               const char *desc_file_path, Error **errp)
 {
     int ret;
+    int matches;
     char access[11];
     char type[11];
     char fname[512];
@@ -796,6 +797,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
     BDRVVmdkState *s = bs->opaque;
     VmdkExtent *extent;
 
+
     while (*p) {
         /* parse extent line in one of below formats:
          *
@@ -805,23 +807,23 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
          * RW [size in sectors] VMFSSPARSE "file-name.vmdk"
          */
         flat_offset = -1;
-        ret = sscanf(p, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64,
-                access, &sectors, type, fname, &flat_offset);
-        if (ret < 4 || strcmp(access, "RW")) {
+        matches = sscanf(p, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64,
+                         access, &sectors, type, fname, &flat_offset);
+        if (matches < 4 || strcmp(access, "RW")) {
             goto next_line;
         } else if (!strcmp(type, "FLAT")) {
-            if (ret != 5 || flat_offset < 0) {
+            if (matches != 5 || flat_offset < 0) {
                 error_setg(errp, "Invalid extent lines: \n%s", p);
                 return -EINVAL;
             }
         } else if (!strcmp(type, "VMFS")) {
-            if (ret == 4) {
+            if (matches == 4) {
                 flat_offset = 0;
             } else {
                 error_setg(errp, "Invalid extent lines:\n%s", p);
                 return -EINVAL;
             }
-        } else if (ret != 4) {
+        } else if (matches != 4) {
             error_setg(errp, "Invalid extent lines:\n%s", p);
             return -EINVAL;
         }

From fe2065629a9c256f836770ca54449ae77b22d188 Mon Sep 17 00:00:00 2001
From: Jeff Cody <jcody@redhat.com>
Date: Thu, 22 Jan 2015 08:03:26 -0500
Subject: [PATCH 06/12] block: vmdk - move string allocations from stack to the
 heap

Functions 'vmdk_parse_extents' and 'vmdk_create' allocate several
PATH_MAX sized arrays on the stack.  Make these dynamically allocated.

Signed-off-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vmdk.c | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index dc6459cc00..7d079adc4a 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -792,12 +792,11 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
     const char *p = desc;
     int64_t sectors = 0;
     int64_t flat_offset;
-    char extent_path[PATH_MAX];
+    char *extent_path;
     BlockDriverState *extent_file;
     BDRVVmdkState *s = bs->opaque;
     VmdkExtent *extent;
 
-
     while (*p) {
         /* parse extent line in one of below formats:
          *
@@ -843,11 +842,13 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
             return -EINVAL;
         }
 
+        extent_path = g_malloc0(PATH_MAX);
         path_combine(extent_path, sizeof(extent_path),
                 desc_file_path, fname);
         extent_file = NULL;
         ret = bdrv_open(&extent_file, extent_path, NULL, NULL,
                         bs->open_flags | BDRV_O_PROTOCOL, NULL, errp);
+        g_free(extent_path);
         if (ret) {
             return ret;
         }
@@ -1797,10 +1798,15 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)
     int ret = 0;
     bool flat, split, compress;
     GString *ext_desc_lines;
-    char path[PATH_MAX], prefix[PATH_MAX], postfix[PATH_MAX];
+    char *path = g_malloc0(PATH_MAX);
+    char *prefix = g_malloc0(PATH_MAX);
+    char *postfix = g_malloc0(PATH_MAX);
+    char *desc_line = g_malloc0(BUF_SIZE);
+    char *ext_filename = g_malloc0(PATH_MAX);
+    char *desc_filename = g_malloc0(PATH_MAX);
     const int64_t split_size = 0x80000000;  /* VMDK has constant split size */
     const char *desc_extent_line;
-    char parent_desc_line[BUF_SIZE] = "";
+    char *parent_desc_line = g_malloc0(BUF_SIZE);
     uint32_t parent_cid = 0xffffffff;
     uint32_t number_heads = 16;
     bool zeroed_grain = false;
@@ -1916,33 +1922,27 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)
         }
         parent_cid = vmdk_read_cid(bs, 0);
         bdrv_unref(bs);
-        snprintf(parent_desc_line, sizeof(parent_desc_line),
+        snprintf(parent_desc_line, BUF_SIZE,
                 "parentFileNameHint=\"%s\"", backing_file);
     }
 
     /* Create extents */
     filesize = total_size;
     while (filesize > 0) {
-        char desc_line[BUF_SIZE];
-        char ext_filename[PATH_MAX];
-        char desc_filename[PATH_MAX];
         int64_t size = filesize;
 
         if (split && size > split_size) {
             size = split_size;
         }
         if (split) {
-            snprintf(desc_filename, sizeof(desc_filename), "%s-%c%03d%s",
+            snprintf(desc_filename, PATH_MAX, "%s-%c%03d%s",
                     prefix, flat ? 'f' : 's', ++idx, postfix);
         } else if (flat) {
-            snprintf(desc_filename, sizeof(desc_filename), "%s-flat%s",
-                    prefix, postfix);
+            snprintf(desc_filename, PATH_MAX, "%s-flat%s", prefix, postfix);
         } else {
-            snprintf(desc_filename, sizeof(desc_filename), "%s%s",
-                    prefix, postfix);
+            snprintf(desc_filename, PATH_MAX, "%s%s", prefix, postfix);
         }
-        snprintf(ext_filename, sizeof(ext_filename), "%s%s",
-                path, desc_filename);
+        snprintf(ext_filename, PATH_MAX, "%s%s", path, desc_filename);
 
         if (vmdk_create_extent(ext_filename, size,
                                flat, compress, zeroed_grain, opts, errp)) {
@@ -1952,7 +1952,7 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)
         filesize -= size;
 
         /* Format description line */
-        snprintf(desc_line, sizeof(desc_line),
+        snprintf(desc_line, BUF_SIZE,
                     desc_extent_line, size / BDRV_SECTOR_SIZE, desc_filename);
         g_string_append(ext_desc_lines, desc_line);
     }
@@ -2007,6 +2007,13 @@ exit:
     g_free(backing_file);
     g_free(fmt);
     g_free(desc);
+    g_free(path);
+    g_free(prefix);
+    g_free(postfix);
+    g_free(desc_line);
+    g_free(ext_filename);
+    g_free(desc_filename);
+    g_free(parent_desc_line);
     g_string_free(ext_desc_lines, true);
     return ret;
 }

From 564d64bdde8e0a6b732a56fa41670e73b19ee062 Mon Sep 17 00:00:00 2001
From: Jeff Cody <jcody@redhat.com>
Date: Thu, 22 Jan 2015 08:03:27 -0500
Subject: [PATCH 07/12] block: qapi - move string allocation from stack to the
 heap

Rather than declaring 'backing_filename2' on the stack in
bdrv_query_image_info(), dynamically allocate it on the heap.

Reviewed-by: John Snow <jsnow@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qapi.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index a6fd6f7ab2..dec9f60d10 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -175,7 +175,6 @@ void bdrv_query_image_info(BlockDriverState *bs,
 {
     int64_t size;
     const char *backing_filename;
-    char backing_filename2[1024];
     BlockDriverInfo bdi;
     int ret;
     Error *err = NULL;
@@ -211,13 +210,14 @@ void bdrv_query_image_info(BlockDriverState *bs,
 
     backing_filename = bs->backing_file;
     if (backing_filename[0] != '\0') {
+        char *backing_filename2 = g_malloc0(1024);
         info->backing_filename = g_strdup(backing_filename);
         info->has_backing_filename = true;
-        bdrv_get_full_backing_filename(bs, backing_filename2,
-                                       sizeof(backing_filename2), &err);
+        bdrv_get_full_backing_filename(bs, backing_filename2, 1024, &err);
         if (err) {
             error_propagate(errp, err);
             qapi_free_ImageInfo(info);
+            g_free(backing_filename2);
             return;
         }
 
@@ -231,6 +231,7 @@ void bdrv_query_image_info(BlockDriverState *bs,
             info->backing_filename_format = g_strdup(bs->backing_format);
             info->has_backing_filename_format = true;
         }
+        g_free(backing_filename2);
     }
 
     ret = bdrv_query_snapshot_info_list(bs, &info->snapshots, &err);

From a1a11d10abfff6638479b7510ee1df4f737d89d6 Mon Sep 17 00:00:00 2001
From: Jeff Cody <jcody@redhat.com>
Date: Thu, 22 Jan 2015 08:03:28 -0500
Subject: [PATCH 08/12] block: remove unused variable in bdrv_commit

As Stefan pointed out, the variable 'filename' in bdrv_commit is unused,
despite being maintained in previous patches.

With this patch, get rid of the variable for good.

Signed-off-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/block.c b/block.c
index cbe4a32a5a..d45e4ddf31 100644
--- a/block.c
+++ b/block.c
@@ -2207,7 +2207,6 @@ int bdrv_commit(BlockDriverState *bs)
     int n, ro, open_flags;
     int ret = 0;
     uint8_t *buf = NULL;
-    char filename[PATH_MAX];
 
     if (!drv)
         return -ENOMEDIUM;
@@ -2222,8 +2221,6 @@ int bdrv_commit(BlockDriverState *bs)
     }
 
     ro = bs->backing_hd->read_only;
-    /* Use pstrcpy (not strncpy): filename must be NUL-terminated. */
-    pstrcpy(filename, sizeof(filename), bs->backing_hd->filename);
     open_flags =  bs->backing_hd->open_flags;
 
     if (ro) {

From 1d33936ea847693a6d69f9049691a0341d6e0b9f Mon Sep 17 00:00:00 2001
From: Jeff Cody <jcody@redhat.com>
Date: Thu, 22 Jan 2015 08:03:29 -0500
Subject: [PATCH 09/12] block: mirror - change string allocation to 2-bytes

The backing_filename string in mirror_run() is only used to check
for a NULL string, so we don't need to allocate 1024 bytes (or, later,
PATH_MAX bytes), when we only need to copy the first 2 characters.

We technically only need 1 byte, as we are just checking for NULL, but
since backing_filename[] is populated by bdrv_get_backing_filename(), a
string size of 1 will always only return '\0';

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/mirror.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index 9019d1ba56..405616422b 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -378,7 +378,8 @@ static void coroutine_fn mirror_run(void *opaque)
     int64_t sector_num, end, sectors_per_chunk, length;
     uint64_t last_pause_ns;
     BlockDriverInfo bdi;
-    char backing_filename[1024];
+    char backing_filename[2]; /* we only need 2 characters because we are only
+                                 checking for a NULL string */
     int ret = 0;
     int n;
 

From 9a29e18f7dfd5a0e80d1c60fc856ebba18ddb738 Mon Sep 17 00:00:00 2001
From: Jeff Cody <jcody@redhat.com>
Date: Thu, 22 Jan 2015 08:03:30 -0500
Subject: [PATCH 10/12] block: update string sizes for
 filename,backing_file,exact_filename

The string field entries 'filename', 'backing_file', and
'exact_filename' in the BlockDriverState struct are defined as 1024
bytes.

However, many places that use these values accept a maximum of PATH_MAX
bytes, so we have a mixture of 1024 byte and PATH_MAX byte allocations.
This patch makes the BlockDriverStruct field string sizes match usage.

This patch also does a few fixes related to the size that needs to
happen now:

    * the block qapi driver is updated to use PATH_MAX bytes
    * the qcow and qcow2 drivers have an additional safety check
    * the block vvfat driver is updated to use PATH_MAX bytes
      for the size of backing_file, for systems where PATH_MAX is < 1024
      bytes.
    * qemu-img uses PATH_MAX rather than 1024.  These instances were not
      changed to be dynamically allocated, however, as the extra
      temporary 3K in stack usage for qemu-img does not seem worrisome.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qapi.c              | 4 ++--
 block/qcow.c              | 2 +-
 block/qcow2.c             | 3 ++-
 block/vvfat.c             | 4 ++--
 include/block/block_int.h | 8 ++++----
 qemu-img.c                | 4 ++--
 6 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index dec9f60d10..75c388e90b 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -210,10 +210,10 @@ void bdrv_query_image_info(BlockDriverState *bs,
 
     backing_filename = bs->backing_file;
     if (backing_filename[0] != '\0') {
-        char *backing_filename2 = g_malloc0(1024);
+        char *backing_filename2 = g_malloc0(PATH_MAX);
         info->backing_filename = g_strdup(backing_filename);
         info->has_backing_filename = true;
-        bdrv_get_full_backing_filename(bs, backing_filename2, 1024, &err);
+        bdrv_get_full_backing_filename(bs, backing_filename2, PATH_MAX, &err);
         if (err) {
             error_propagate(errp, err);
             qapi_free_ImageInfo(info);
diff --git a/block/qcow.c b/block/qcow.c
index ece22697a6..ccbe9e0d2c 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -215,7 +215,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
     /* read the backing file name */
     if (header.backing_file_offset != 0) {
         len = header.backing_file_size;
-        if (len > 1023) {
+        if (len > 1023 || len > sizeof(bs->backing_file)) {
             error_setg(errp, "Backing file name too long");
             ret = -EINVAL;
             goto fail;
diff --git a/block/qcow2.c b/block/qcow2.c
index e4e690a42b..dbaf016bc7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -868,7 +868,8 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     /* read the backing file name */
     if (header.backing_file_offset != 0) {
         len = header.backing_file_size;
-        if (len > MIN(1023, s->cluster_size - header.backing_file_offset)) {
+        if (len > MIN(1023, s->cluster_size - header.backing_file_offset) ||
+            len > sizeof(bs->backing_file)) {
             error_setg(errp, "Backing file name too long");
             ret = -EINVAL;
             goto fail;
diff --git a/block/vvfat.c b/block/vvfat.c
index e34a789699..a1a44f0ef5 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2909,8 +2909,8 @@ static int enable_write_target(BDRVVVFATState *s, Error **errp)
 
     array_init(&(s->commits), sizeof(commit_t));
 
-    s->qcow_filename = g_malloc(1024);
-    ret = get_tmp_filename(s->qcow_filename, 1024);
+    s->qcow_filename = g_malloc(PATH_MAX);
+    ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "can't create temporary file");
         goto err;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 06a21dd13d..e264be97b2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -339,13 +339,13 @@ struct BlockDriverState {
      * regarding this BDS's context */
     QLIST_HEAD(, BdrvAioNotifier) aio_notifiers;
 
-    char filename[1024];
-    char backing_file[1024]; /* if non zero, the image is a diff of
-                                this file image */
+    char filename[PATH_MAX];
+    char backing_file[PATH_MAX]; /* if non zero, the image is a diff of
+                                    this file image */
     char backing_format[16]; /* if non-zero and backing_file exists */
 
     QDict *full_open_options;
-    char exact_filename[1024];
+    char exact_filename[PATH_MAX];
 
     BlockDriverState *backing_hd;
     BlockDriverState *file;
diff --git a/qemu-img.c b/qemu-img.c
index 7876258fa9..4e9a7f5741 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2556,7 +2556,7 @@ static int img_rebase(int argc, char **argv)
 
     /* For safe rebasing we need to compare old and new backing file */
     if (!unsafe) {
-        char backing_name[1024];
+        char backing_name[PATH_MAX];
 
         blk_old_backing = blk_new_with_bs("old_backing", &error_abort);
         bs_old_backing = blk_bs(blk_old_backing);
@@ -2614,7 +2614,7 @@ static int img_rebase(int argc, char **argv)
         }
         old_backing_num_sectors = bdrv_nb_sectors(bs_old_backing);
         if (old_backing_num_sectors < 0) {
-            char backing_name[1024];
+            char backing_name[PATH_MAX];
 
             bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
             error_report("Could not get size of '%s': %s",

From cdf9634bdf76c8f59d9f7a04df30fa26d8e93d96 Mon Sep 17 00:00:00 2001
From: Jeff Cody <jcody@redhat.com>
Date: Tue, 20 Jan 2015 16:01:43 -0500
Subject: [PATCH 11/12] block: vhdx - force FileOffsetMB field to '0' for
 certain block states

The v1.0.0 spec calls out PAYLOAD_BLOCK_ZERO FileOffsetMB field as being
'reserved'.  In practice, this means that Hyper-V will fail to read a
disk image with PAYLOAD_BLOCK_ZERO block states with a FileOffsetMB
value other than 0.

The other states that indicate a block that is not there
(PAYLOAD_BLOCK_UNDEFINED, PAYLOAD_BLOCK_NOT_PRESENT,
 PAYLOAD_BLOCK_UNMAPPED) have multiple options for what FileOffsetMB may
be set to, and '0' is explicitly called out as an option.

For all the above states, we will also just set the FileOffsetMB value
to 0.

Signed-off-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: a9fe92f53f07e6ab1693811e4312c0d1e958500b.1421787566.git.jcody@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/vhdx.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/block/vhdx.c b/block/vhdx.c
index 06f2b1a0cb..bb3ed45d5c 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1174,7 +1174,18 @@ static void vhdx_update_bat_table_entry(BlockDriverState *bs, BDRVVHDXState *s,
 {
     /* The BAT entry is a uint64, with 44 bits for the file offset in units of
      * 1MB, and 3 bits for the block state. */
-    s->bat[sinfo->bat_idx]  = sinfo->file_offset;
+    if ((state == PAYLOAD_BLOCK_ZERO)        ||
+        (state == PAYLOAD_BLOCK_UNDEFINED)   ||
+        (state == PAYLOAD_BLOCK_NOT_PRESENT) ||
+        (state == PAYLOAD_BLOCK_UNMAPPED)) {
+        s->bat[sinfo->bat_idx]  = 0;  /* For PAYLOAD_BLOCK_ZERO, the
+                                         FileOffsetMB field is denoted as
+                                         'reserved' in the v1.0 spec.  If it is
+                                         non-zero, MS Hyper-V will fail to read
+                                         the disk image */
+    } else {
+        s->bat[sinfo->bat_idx]  = sinfo->file_offset;
+    }
 
     s->bat[sinfo->bat_idx] |= state & VHDX_BAT_STATE_BIT_MASK;
 

From e35053b25aeeef5c67b38603e4ded526c2229371 Mon Sep 17 00:00:00 2001
From: Max Reitz <mreitz@redhat.com>
Date: Fri, 23 Jan 2015 09:59:45 -0500
Subject: [PATCH 12/12] iotests: Lower 064's memory usage

Test 064 reads a lot of data at once which currently results in qemu-io
having to allocate up to about 1 GB of memory (958 MB, to be exact).
This patch lowers that amount to 128 MB by making the test read smaller
chunks.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Message-id: 1422025185-25229-1-git-send-email-mreitz@redhat.com
---
 tests/qemu-iotests/064     | 19 +++++++++++++++++--
 tests/qemu-iotests/064.out | 34 ++++++++++++++++++++++++++++++----
 2 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/064 b/tests/qemu-iotests/064
index 1c74c31a1a..7564563abd 100755
--- a/tests/qemu-iotests/064
+++ b/tests/qemu-iotests/064
@@ -54,7 +54,15 @@ $QEMU_IO -r -c "read -pP 0x96 33M 33M" "$TEST_IMG" | _filter_qemu_io
 
 echo
 echo "=== Verify pattern 0x00, 66M - 1024M ==="
-$QEMU_IO -r -c "read -pP 0x00 66M 958M" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -r -c "read -pP 0x00 66M 62M" \
+            -c "read -pP 0x00 128M 128M" \
+            -c "read -pP 0x00 256M 128M" \
+            -c "read -pP 0x00 384M 128M" \
+            -c "read -pP 0x00 512M 128M" \
+            -c "read -pP 0x00 640M 128M" \
+            -c "read -pP 0x00 768M 128M" \
+            -c "read -pP 0x00 896M 128M" \
+            "$TEST_IMG" | _filter_qemu_io
 
 echo
 echo "=== Verify pattern write, 0xc3 99M-157M ==="
@@ -63,7 +71,14 @@ $QEMU_IO -c "write -pP 0xc3 99M 58M" "$TEST_IMG" | _filter_qemu_io
 $QEMU_IO -c "read -pP 0xa5 0 33M" "$TEST_IMG" | _filter_qemu_io
 $QEMU_IO -c "read -pP 0x96 33M 33M" "$TEST_IMG" | _filter_qemu_io
 $QEMU_IO -c "read -pP 0x00 66M 33M" "$TEST_IMG" | _filter_qemu_io
-$QEMU_IO -c "read -pP 0x00 157MM 867MM" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -pP 0x00 157M 99M" \
+         -c "read -pP 0x00 256M 128M" \
+         -c "read -pP 0x00 384M 128M" \
+         -c "read -pP 0x00 512M 128M" \
+         -c "read -pP 0x00 640M 128M" \
+         -c "read -pP 0x00 768M 128M" \
+         -c "read -pP 0x00 896M 128M" \
+         "$TEST_IMG" | _filter_qemu_io
 # now verify what we should have actually written
 $QEMU_IO -c "read -pP 0xc3 99M 58M" "$TEST_IMG" | _filter_qemu_io
 
diff --git a/tests/qemu-iotests/064.out b/tests/qemu-iotests/064.out
index 5346a4e630..1a5b9e2d7b 100644
--- a/tests/qemu-iotests/064.out
+++ b/tests/qemu-iotests/064.out
@@ -9,8 +9,22 @@ read 34603008/34603008 bytes at offset 34603008
 33 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 === Verify pattern 0x00, 66M - 1024M ===
-read 1004535808/1004535808 bytes at offset 69206016
-958 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65011712/65011712 bytes at offset 69206016
+62 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 134217728/134217728 bytes at offset 134217728
+128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 134217728/134217728 bytes at offset 268435456
+128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 134217728/134217728 bytes at offset 402653184
+128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 134217728/134217728 bytes at offset 536870912
+128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 134217728/134217728 bytes at offset 671088640
+128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 134217728/134217728 bytes at offset 805306368
+128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 134217728/134217728 bytes at offset 939524096
+128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 === Verify pattern write, 0xc3 99M-157M ===
 wrote 60817408/60817408 bytes at offset 103809024
@@ -21,8 +35,20 @@ read 34603008/34603008 bytes at offset 34603008
 33 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 34603008/34603008 bytes at offset 69206016
 33 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-read 909115392/909115392 bytes at offset 164626432
-867 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 103809024/103809024 bytes at offset 164626432
+99 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 134217728/134217728 bytes at offset 268435456
+128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 134217728/134217728 bytes at offset 402653184
+128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 134217728/134217728 bytes at offset 536870912
+128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 134217728/134217728 bytes at offset 671088640
+128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 134217728/134217728 bytes at offset 805306368
+128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 134217728/134217728 bytes at offset 939524096
+128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 60817408/60817408 bytes at offset 103809024
 58 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done