From 6c2d8670459c156287e3df0364882b79b54b539a Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Fri, 2 May 2014 16:58:56 -0500 Subject: [PATCH 24/26] qcow2: Fix fail path in realloc_refcount_block() RH-Author: Max Reitz Message-id: <1399049936-13496-25-git-send-email-mreitz@redhat.com> Patchwork-id: 58670 O-Subject: [RHEL-6.6 qemu-kvm PATCH v3 24/24] qcow2: Fix fail path in realloc_refcount_block() Bugzilla: 1004420 RH-Acked-by: Laszlo Ersek RH-Acked-by: Kevin Wolf RH-Acked-by: Stefan Hajnoczi BZ: 1004420 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 Signed-off-by: Max Reitz Reviewed-by: Laszlo Ersek Signed-off-by: Kevin Wolf (cherry picked from commit a134d90f50806597c5da4fd191352fe62d40f71a) Conflicts: block/qcow2-refcount.c qcow2_free_clusters() does not have a "type" parameter downstream. The downstream-only comment modification in realloc_refcount_block() (backport of afa50193cde574528a130a25544fd6f3aa8da069) results in a contextual conflict. Signed-off-by: Max Reitz --- block/qcow2-refcount.c | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) Signed-off-by: Jeff E. Nelson --- block/qcow2-refcount.c | 33 ++++++++++++++++++--------------- 1 files changed, 18 insertions(+), 15 deletions(-) diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index 873baee..1d35695 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -1230,14 +1230,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 @@ -1248,8 +1248,7 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index, "check failed: %s\n", strerror(-ret)); /* the image is corrupt and no write accesses should occur beyond this * point, so don't even attempt on freeing the cluster */ - new_offset = 0; - goto fail; + goto done; } /* write to new block */ @@ -1257,7 +1256,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 */ @@ -1267,23 +1266,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); - } + goto done; + +fail_free_cluster: + qcow2_free_clusters(bs, new_offset, s->cluster_size); + +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; } -- 1.7.1