[Qemu-devel] [PATCH 0/3] qcow2: Small error path fixes for snapshot writing

[Qemu-devel] [PATCH 0/3] qcow2: Small error path fixes for snapshot writing

[Max Reitz]

Errors in qcow2_write_snapshots should always go down the error path. If
this path is taken, the newly allocated snapshot table clusters are
abandoned and should thus be freed.

Furthermore, we should safeguard against a possible future increase of
QEMU's maximum snapshot name/ID length.

Max Reitz (3):
  qcow2: Always use error path on writing snapshots
  qcow2: Free allocated snapshot table on error
  qcow2: Assert against snapshot name/ID overflow

 block/qcow2-snapshot.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/3] qcow2: Always use error path on writing snapshots

[Max Reitz]

qcow2_write_snapshots does contain a fail label and there is no reason
not to use it on some errors; therefore, we should always jump there on
error.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-snapshot.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 5e8a779..3337974 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -182,11 +182,12 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
     snapshots_offset = qcow2_alloc_clusters(bs, snapshots_size);
     offset = snapshots_offset;
     if (offset < 0) {
-        return offset;
+        ret = offset;
+        goto fail;
     }
     ret = bdrv_flush(bs);
     if (ret < 0) {
-        return ret;
+        goto fail;
     }
 
     /* The snapshot list position has not yet been updated, so these clusters
@@ -194,7 +195,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
     ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT, offset,
                                         snapshots_size);
     if (ret < 0) {
-        return ret;
+        goto fail;
     }
 
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/3] qcow2: Free allocated snapshot table on error

[Max Reitz]

If an error occurs during qcow2_write_snapshots, the newly allocated
snapshot table clusters are leaked and should thus be freed.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-snapshot.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 3337974..f6f3e64 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -279,6 +279,10 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
     return 0;
 
 fail:
+    if (snapshots_offset > 0) {
+        qcow2_free_clusters(bs, snapshots_offset, snapshots_size,
+                            QCOW2_DISCARD_ALWAYS);
+    }
     return ret;
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/3] qcow2: Assert against snapshot name/ID overflow

[Max Reitz]

qcow2_write_snapshots relies on the length of every snapshot ID and name
fitting into an unsigned 16 bit integer. This is currently ensured by
QEMU through generally only allowing 128 byte IDs and 256 byte names.
However, if this should change in the future, the length written to the
image file should not be silently truncated (though the name itself
would be written completely).

Since this is currently not an issue but might require attention due to
internal QEMU changes in the future, an assert ensuring sanity is enough
for now.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-snapshot.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index f6f3e64..812dab2 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -221,6 +221,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
 
         id_str_size = strlen(sn->id_str);
         name_size = strlen(sn->name);
+        assert(id_str_size <= UINT16_MAX && name_size <= UINT16_MAX);
         h.id_str_size = cpu_to_be16(id_str_size);
         h.name_size = cpu_to_be16(name_size);
         offset = align_offset(offset, 8);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/3] qcow2: Small error path fixes for snapshot writing

[Kevin Wolf]

Am 09.10.2013 um 10:51 hat Max Reitz geschrieben:
> Errors in qcow2_write_snapshots should always go down the error path. If
> this path is taken, the newly allocated snapshot table clusters are
> abandoned and should thus be freed.
> 
> Furthermore, we should safeguard against a possible future increase of
> QEMU's maximum snapshot name/ID length.

Thanks, applied all to the block branch.

Kevin