mirrors/qemu
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

qcow2: Fix fail path in realloc_refcount_block()

Browse Source 
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>
This commit is contained in:
Max Reitz 2014-03-17 23:04:52 +01:00 committed by Kevin Wolf
parent 8a15b813e6
commit a134d90f50
1 changed files with 18 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

34
block/qcow2-refcount.c
View File

@ -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;
}