Re: [Qemu-devel] [PATCH v2 2/7] block/qcow2-refcount: avoid eating RAM

[Vladimir Sementsov-Ogievskiy]

On 10/08/2018 06:31 PM, Max Reitz wrote:
> On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote:
>> qcow2_inc_refcounts_imrt() (through realloc_refcount_array()) can eat
>> an unpredictable amount of memory on corrupted table entries, which are
>> referencing regions far beyond the end of file.
>>
>> Prevent this, by skipping such regions from further processing.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>   block/qcow2-refcount.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index 615847eb09..566c19fbfa 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -1499,12 +1499,26 @@ int qcow2_inc_refcounts_imrt(BlockDriverState *bs, 
>> BdrvCheckResult *res,
>>   {
>>       BDRVQcow2State *s = bs->opaque;
>>       uint64_t start, last, cluster_offset, k, refcount;
>> +    int64_t file_len;
>>       int ret;
>>   
>>       if (size <= 0) {
>>           return 0;
>>       }
>>   
>> +    file_len = bdrv_getlength(bs->file->bs);
>> +    if (file_len < 0) {
>> +        return file_len;
>> +    }
> 
> Doesn't this slow things down?  Can we not cache the length somewhere
> and update it whenever the image is modified?


hmm. bdrv_getlength is used everywhere in Qemu, and I don't think it is 
good idea to improve it locally for these series. If we can improve it 
somehow with a cache or something like this, it should be done for all 
users and therefore it is outside of these series..

> 
>> +
>> +    if (offset + size - file_len > s->cluster_size) {
>> +        fprintf(stderr, "ERROR: counting reference for region exceeding the 
>> "
>> +                "end of the file by more than one cluster: offset 0x%" 
>> PRIx64
>> +                " size 0x%" PRIx64 "\n", offset, size);
> 
> Why is one cluster OK?  Is there a specific case you're trying to catch
> here?

raw file under qcow2 may be not aligned in real size to qcow2 cluster, 
as I understand, it's normal for the last cluster to be semi-allocated

> 
> Max
> 
>> +        res->corruptions++;
>> +        return 0;
>> +    }
>> +
>>       start = start_of_cluster(s, offset);
>>       last = start_of_cluster(s, offset + size - 1);
>>       for(cluster_offset = start; cluster_offset <= last;
>>
> 
>