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>

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3f2ed084661ea8142a81329d3b5425c6f1915b1f..4a2df5fb9940aaf12486728d51726e6e1059f097 100644 (file)
--- 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;
 }