Am 20.10.2014 um 16:35 hat Max Reitz geschrieben:
> If the qcow2 check function detects a refcount block located beyond the
> image end, grow the image appropriately. This cannot break anything and
> is the logical fix for such a case.
>
> Signed-off-by: Max Reitz <mre...@redhat.com>
> Reviewed-by: Eric Blake <ebl...@redhat.com>
> ---
> block/qcow2-refcount.c | 62
> ++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 58 insertions(+), 4 deletions(-)
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 55a539f..0225769 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1544,7 +1544,8 @@ static int check_refblocks(BlockDriverState *bs,
> BdrvCheckResult *res,
> int64_t *nb_clusters)
> {
> BDRVQcowState *s = bs->opaque;
> - int64_t i;
> + int64_t i, size;
> + int ret;
>
> for(i = 0; i < s->refcount_table_size; i++) {
> uint64_t offset, cluster;
> @@ -1560,9 +1561,62 @@ static int check_refblocks(BlockDriverState *bs,
> BdrvCheckResult *res,
> }
>
> if (cluster >= *nb_clusters) {
> - fprintf(stderr, "ERROR refcount block %" PRId64
> - " is outside image\n", i);
> - res->corruptions++;
> + fprintf(stderr, "%s refcount block %" PRId64 " is outside
> image\n",
> + fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
> +
> + if (fix & BDRV_FIX_ERRORS) {
> + int64_t old_nb_clusters = *nb_clusters;
> +
> + if (offset + s->cluster_size < offset ||
> + offset + s->cluster_size > INT64_MAX)
I _think_ this code is correct because offset is unsigned, but it would
be easier and not rely on overflow semantics like this:
if (offset > INT64_MAX - s->cluster_size)
> + {
> + ret = -EINVAL;
> + goto resize_fail;
> + }
> +
> + ret = bdrv_truncate(bs->file, offset + s->cluster_size);
> + if (ret < 0) {
> + goto resize_fail;
> + }
> + size = bdrv_getlength(bs->file);
> + if (size < 0) {
> + ret = size;
> + goto resize_fail;
> + }
> +
> + *nb_clusters = size_to_clusters(s, size);
> + assert(*nb_clusters >= old_nb_clusters);
> +
> + *refcount_table = g_try_realloc(*refcount_table,
> + *nb_clusters * sizeof(uint16_t));
> + if (!*refcount_table) {
> + res->check_errors++;
> + return -ENOMEM;
> + }
> +
> + memset(*refcount_table + old_nb_clusters, 0,
> + (*nb_clusters - old_nb_clusters) * sizeof(uint16_t));
Considering the comments you got in previous comments,
sizeof(**refcount_table) might make it more obvious what's going on, and
would also be more robust against later changes of the type.
> + if (cluster >= *nb_clusters) {
> + ret = -EINVAL;
> + goto resize_fail;
> + }
> +
> + res->corruptions_fixed++;
> + inc_refcounts(bs, res, *refcount_table, *nb_clusters,
> + offset, s->cluster_size);
> + /* No need to check whether the refcount is now greater than
> 1:
> + * This area was just allocated and zeroed, so it can only be
> + * exactly 1 after inc_refcounts() */
> + continue;
> +
> +resize_fail:
> + res->corruptions++;
> + fprintf(stderr, "ERROR could not resize image: %s\n",
> + strerror(-ret));
> + } else {
> + res->corruptions++;
> + }
> continue;
> }
Reviewed-by: Kevin Wolf <kw...@redhat.com>
