Re: [Qemu-devel] [PATCH v3 07/22] qcow2: Helper function for refcount modification

[Max Reitz <mreitz@redhat.com>]

On 2014-11-27 at 16:21, Stefan Hajnoczi wrote:
> On Thu, Nov 20, 2014 at 06:06:23PM +0100, Max Reitz wrote:
>> @@ -116,20 +137,24 @@ int64_t qcow2_get_refcount(BlockDriverState *bs, int64_t cluster_index)
>>       }
>>   
>>       ret = qcow2_cache_get(bs, s->refcount_block_cache, refcount_block_offset,
>> -        (void**) &refcount_block);
>> +                          &refcount_block);
>>       if (ret < 0) {
>>           return ret;
>>       }
>>   
>>       block_index = cluster_index & (s->refcount_block_size - 1);
>> -    refcount = be16_to_cpu(refcount_block[block_index]);
>> +    refcount = s->get_refcount(refcount_block, block_index);
>>   
>> -    ret = qcow2_cache_put(bs, s->refcount_block_cache,
>> -        (void**) &refcount_block);
>> +    ret = qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
>>       if (ret < 0) {
>>           return ret;
>>       }
>>   
>> +    if (refcount < 0) {
>> +        /* overflow */
>> +        return -ERANGE;
>> +    }
> I guess this is because QEMU uses int64_t but the maximum refcount size
> is 64 bits.
>
> If we refuse to open files with 64-bit refcounts, can we drop this
> check?

Well, I just reduced the maximum refcount order to 6... If we don't want 
to open images with 64 bit refcounts, it'd be easiest to completely 
disallow them in the qcow2 specification. But I don't see the need.

>> @@ -583,7 +608,12 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
>>           /* we can update the count and save it */
>>           block_index = cluster_index & (s->refcount_block_size - 1);
>>   
>> -        refcount = be16_to_cpu(refcount_block[block_index]);
>> +        refcount = s->get_refcount(refcount_block, block_index);
>> +        if (refcount < 0) {
>> +            ret = -ERANGE;
>> +            goto fail;
>> +        }
> Here again.
>
>> @@ -1206,11 +1236,14 @@ static int inc_refcounts(BlockDriverState *bs,
>>               }
>>           }
>>   
>> -        if (++(*refcount_table)[k] == 0) {
>> +        refcount = s->get_refcount(*refcount_table, k);
> Here the refcount < 0 check is missing.  That's why it would be simpler
> to eliminate the refcount < 0 case entirely.

It's not missing. This is part of the refcount check, as are all the 
following ("in other places"). The refcount check builds a refcount 
array in memory all by itself, so it knows for sure there are no 
overflows. The line which you omitted directly after this clips the 
refcount values against s->refcount_max which is INT64_MAX for 64-bit 
refcounts.

Therefore, no overflows possible in the refcount checking functions, 
because the refcount checking functions don't introduce the overflows 
themselves. The other overflow checks are only in place to reject faulty 
images provided from the outside.

>> @@ -1726,7 +1761,7 @@ static void compare_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>>               continue;
>>           }
>>   
>> -        refcount2 = refcount_table[i];
>> +        refcount2 = s->get_refcount(refcount_table, i);
> Missing here too and in other places.
>
>> +typedef uint64_t Qcow2GetRefcountFunc(const void *refcount_array,
>> +                                      uint64_t index);
> Should the return type be int64_t?

No. If it was, we'd have to check for errors every time we call it, but 
it cannot return errors (well, if we let it return uint64_t, it might 
return -ERANGE, but that's exactly what I don't want). Therefore, let it 
return uint64_t so we know this function cannot fail.

>> +typedef void Qcow2SetRefcountFunc(void *refcount_array,
>> +                                  uint64_t index, uint64_t value);
> Should value's type be int64_t?  Just because the type is unsigned
> doesn't make (uint64_t)-1ULL a valid value.

Actually, it does. It's just that the implementation provided here does 
not support it.

Since there is an assertion against that case in the 64-bit 
implementation for this function, I don't have a problem with using 
int64_t here, though. But that would break symmetry with 
Qcow2GetRefcountFunc(), and I do have a reason there not to return a 
signed value, as explained above.

Max