Re: [Qemu-devel] [PATCH 2/6] qcow2: simplify qcow2_cache_put() and qcow2_cache_entry_mark_dirty()

[Alberto Garcia]

On Wed 06 May 2015 05:00:50 PM CEST, Stefan Hajnoczi wrote:

>> >>  int qcow2_cache_put(BlockDriverState *bs, Qcow2Cache *c, void **table)
>> >>  {
>> >> -    int i;
>> >> +    int i = (*table - c->table_array) / c->table_size;
>> >>  
>> >> -    for (i = 0; i < c->size; i++) {
>> >> -        if (table_addr(c, i) == *table) {
>> >> -            goto found;
>> >> -        }
>> >> +    if (c->entries[i].offset == 0) {
>> >> +        return -ENOENT;
>> >>      }
>> >> -    return -ENOENT;
>> >>  
>> >> -found:
>> >>      c->entries[i].ref--;
>> >>      *table = NULL;
>> >>  
>> >
>> > When is this function called with a bogus table pointer?
>> 
>> I also could not image any such scenario, but I decided to be
>> conservative and keep the error handling code. I'll double check all
>> places where it's used and remove the relevant code.
>
> The reason why I'd think this is worth looking into is:
>
> The new qcow2_cache_put() code indexes outside the array bounds if
>table is a bogus value.  The old code would return -ENOENT.  So I am a
>little nervous about the change, although I suspect the function is
>never called with invalid inputs at all.

I checked again all callers of qcow2_cache_put() and I didn't notice
anything unusual.

In some cases it even goes after qcow2_cache_entry_mark_dirty(), and
that one directly aborts if the table pointer is bogus.

Berto