Re: [Qemu-devel] [PATCH for-2.11 4/5] qcow2: Add bounds check to get_refblock_offset()

[Alberto Garcia]

On Tue 14 Nov 2017 04:27:56 PM CET, Max Reitz wrote:
>>> +static int64_t get_refblock_offset(BlockDriverState *bs, uint64_t offset)
>>> +{
>>> +    BDRVQcow2State *s = bs->opaque;
>>> +    uint32_t index = offset_to_reftable_index(s, offset);
>>> +    int64_t covering_refblock_offset = 0;
>>> +
>>> +    if (index < s->refcount_table_size) {
>>> +        covering_refblock_offset = s->refcount_table[index] & 
>>> REFT_OFFSET_MASK;
>>> +    }
>>> +    if (!covering_refblock_offset) {
>>> +        qcow2_signal_corruption(bs, true, -1, -1, "Refblock at %#" PRIx64 
>>> " is "
>>> +                                "not covered by the refcount structures",
>>> +                                offset);
>>> +        return -EIO;
>>> +    }
>>> +
>>> +    return covering_refblock_offset;
>>> +}
>> 
>> Isn't it simpler to do something like this instead?
>> 
>>    if (index >= s->refcount_table_size) {
>>        qcow2_signal_corruption(...);
>>        return -EIO;
>>    }
>>    return s->refcount_table[index] & REFT_OFFSET_MASK;
>
> But that doesn't cover the case were s->refcount_table[index] &
> REFT_OFFSET_MASK is 0.

Ah, you're right. That would be detected by the qcow2_cache_get() call
though, but it's fine to do the check here as well.

Reviewed-by: Alberto Garcia <address@hidden>

Berto