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