Re: [Qemu-devel] [PATCH v6 06/11] qcow2: Fix refcount blocks beyond image end

[Max Reitz <mreitz@redhat.com>]

On 2014-10-20 at 18:44, Kevin Wolf wrote:
> 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 <mreitz@redhat.com>
>> Reviewed-by: Eric Blake <eblake@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)

Okay.

>> +                {
>> +                    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.

I highly doubt I'll change the type so that sizeof(**refcount_table) 
works. If variable refcounts are implemented, I'll probably make 
*refcount_table just a void * and then access its elements through a 
function; sadly, qemu's make doesn't error out against sizeof(void) 
(which would be -Wpointer-arith or just -pedantic), so just using 
sizeof(**refcount_table) so that everything throws an error once it's a 
void ** won't work.

I guess I'll change it in a v7 because sizeof(**refcount_table) is 
easier to grep for than sizeof(uint16_t).

>> +                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 <kwolf@redhat.com>

Thanks!

Max