qcow2: Fix fail path in realloc_refcount_block()

If qcow2_alloc_clusters() fails, new_offset and ret will both be
negative after the fail label, thus passing the first if condition and
subsequently resulting in a call of qcow2_free_clusters() with an
invalid (negative) offset parameter. Fix this by introducing a new label
"fail_free_cluster" which is only invoked if new_offset is indeed
pointing to a newly allocated cluster that should be cleaned up by
freeing it.

While we're at it, clean up the whole fail path. qcow2_cache_put()
should (and actually can) never fail, hence the return value can safely
be ignored (aside from asserting that it indeed did not fail).

Furthermore, there is no reason to give QCOW2_DISCARD_ALWAYS to
qcow2_free_clusters(), a mere QCOW2_DISCARD_OTHER will suffice.

Ultimately, rename the "fail" label to "done", as it is invoked both on
failure and success.

Suggested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>

1 files changed, 18 insertions, 16 deletions

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3f2ed08466..4a2df5fb99 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1399,14 +1399,14 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
         fprintf(stderr, "Could not allocate new cluster: %s\n",
                 strerror(-new_offset));
         ret = new_offset;
-        goto fail;
+        goto done;
     }
 
     /* fetch current refcount block content */
     ret = qcow2_cache_get(bs, s->refcount_block_cache, offset, &refcount_block);
     if (ret < 0) {
         fprintf(stderr, "Could not fetch refcount block: %s\n", strerror(-ret));
-        goto fail;
+        goto fail_free_cluster;
     }
 
     /* new block has not yet been entered into refcount table, therefore it is
@@ -1417,8 +1417,7 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
                 "check failed: %s\n", strerror(-ret));
         /* the image will be marked corrupt, so don't even attempt on freeing
          * the cluster */
-        new_offset = 0;
-        goto fail;
+        goto done;
     }
 
     /* write to new block */
@@ -1426,7 +1425,7 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
             s->cluster_sectors);
     if (ret < 0) {
         fprintf(stderr, "Could not write refcount block: %s\n", strerror(-ret));
-        goto fail;
+        goto fail_free_cluster;
     }
 
     /* update refcount table */
@@ -1436,24 +1435,27 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
     if (ret < 0) {
         fprintf(stderr, "Could not update refcount table: %s\n",
                 strerror(-ret));
-        goto fail;
+        goto fail_free_cluster;
     }
 
-fail:
-    if (new_offset && (ret < 0)) {
-        qcow2_free_clusters(bs, new_offset, s->cluster_size,
-                QCOW2_DISCARD_ALWAYS);
-    }
+    goto done;
+
+fail_free_cluster:
+    qcow2_free_clusters(bs, new_offset, s->cluster_size, QCOW2_DISCARD_OTHER);
+
+done:
     if (refcount_block) {
-        if (ret < 0) {
-            qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
-        } else {
-            ret = qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
-        }
+        /* This should never fail, as it would only do so if the given refcount
+         * block cannot be found in the cache. As this is impossible as long as
+         * there are no bugs, assert the success. */
+        int tmp = qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
+        assert(tmp == 0);
     }
+
     if (ret < 0) {
         return ret;
     }
+
     return new_offset;
 }