Re: [Qemu-devel] [PATCH v7 10/13] qcow2: Rebuild refcount structure during check

[Kevin Wolf]

Am 22.10.2014 um 10:08 hat Max Reitz geschrieben:
> The previous commit introduced the "rebuild" variable to qcow2's
> implementation of the image consistency check. Now make use of this by
> adding a function which creates a completely new refcount structure
> based solely on the in-memory information gathered before.
> 
> The old refcount structure will be leaked, however. This leak will be
> dealt with in a follow-up commit.
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  block/qcow2-refcount.c | 310 
> ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 304 insertions(+), 6 deletions(-)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 101d7bb..8ca2324 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1640,6 +1640,284 @@ static void compare_refcounts(BlockDriverState *bs, 
> BdrvCheckResult *res,
>  }
>  
>  /*
> + * Allocates clusters using an in-memory refcount table (IMRT) in contrast to
> + * the on-disk refcount structures.
> + *
> + * On input, *first_free_cluster tells where to start looking, and need not
> + * actually be a free cluster; the returned offset will not be before that
> + * cluster.  On output, *first_free_cluster points to the first gap found, 
> even
> + * if that gap was too small to be used as the returned offset.
> + *
> + * Note that *first_free_cluster is a cluster index whereas the return value 
> is
> + * an offset.
> + */
> +static int64_t alloc_clusters_imrt(BlockDriverState *bs,
> +                                   int cluster_count,
> +                                   uint16_t **refcount_table,
> +                                   int64_t *imrt_nb_clusters,
> +                                   int64_t *first_free_cluster)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    int64_t cluster = *first_free_cluster, i;
> +    bool first_gap = true;
> +    int contiguous_free_clusters;
> +
> +    /* Starting at *first_free_cluster, find a range of at least 
> cluster_count
> +     * continuously free clusters */
> +    for (contiguous_free_clusters = 0;
> +         cluster < *imrt_nb_clusters &&
> +         contiguous_free_clusters < cluster_count;
> +         cluster++)
> +    {
> +        if (!(*refcount_table)[cluster]) {
> +            contiguous_free_clusters++;
> +            if (first_gap) {
> +                /* If this is the first free cluster found, update
> +                 * *first_free_cluster accordingly */
> +                *first_free_cluster = cluster;
> +                first_gap = false;
> +            }
> +        } else if (contiguous_free_clusters) {
> +            contiguous_free_clusters = 0;
> +        }
> +    }
> +
> +    /* If contiguous_free_clusters is greater than zero, it contains the 
> number
> +     * of continuously free clusters until the current cluster; the first 
> free
> +     * cluster in the current "gap" is therefore
> +     * cluster - contiguous_free_clusters */
> +
> +    /* If no such range could be found, grow the in-memory refcount table
> +     * accordingly to append free clusters at the end of the image */
> +    if (contiguous_free_clusters < cluster_count) {
> +        int64_t old_imrt_nb_clusters = *imrt_nb_clusters;
> +        uint16_t *new_refcount_table;
> +
> +        /* contiguous_free_clusters clusters are already empty at the image 
> end;
> +         * we need cluster_count clusters; therefore, we have to allocate
> +         * cluster_count - contiguous_free_clusters new clusters at the end 
> of
> +         * the image (which is the current value of cluster; note that 
> cluster
> +         * may exceed old_imrt_nb_clusters if *first_free_cluster pointed 
> beyond
> +         * the image end) */
> +        *imrt_nb_clusters = cluster + cluster_count - 
> contiguous_free_clusters;
> +        new_refcount_table = g_try_realloc(*refcount_table,
> +                                           *imrt_nb_clusters *
> +                                           sizeof(**refcount_table));
> +        if (!new_refcount_table) {
> +            return -ENOMEM;

Now the old refcount table is preserved, but nb_clusters has already
been updated. Smells like the next thing will be a buffer overflow,
doesn't it?

> +        }
> +        *refcount_table = new_refcount_table;
> +
> +        memset(*refcount_table + old_imrt_nb_clusters, 0,
> +               (*imrt_nb_clusters - old_imrt_nb_clusters) *
> +               sizeof(**refcount_table));
> +    }

Kevin