[Mesa-dev] [PATCH 1/5] util/hash_table: Don't hash the deleted and freed keys

[Caio Marcelo de Oliveira Filho]

On Mon, Aug 05, 2019 at 10:02:43AM -0700, Alyssa Rosenzweig wrote:
> To be clear, Panfrost relies on stuffing (bogus) magic numbers into u64
> keys and u64 values. Is this going to break on us? That is, when you
> mention this is a bug, who's buggy here? (Panfrost or hash_table_u64 or
> both?)

There's a bug in _mesa_hash_table_u64_clear(), it assumes it can use
the u64 directly as key, but that's not always the case.  For
`sizeof(void *) != 8` we stuff the u64 key into a struct.  Note we use
different hash functions in those cases.

The fix in the original patch "also works", but would make a special
case like "when delete_function callback is called, there are cases
where the hash in hash_entry is invalid".  The right fix is just doing
in _clear() something similar than what hash_table_u64_search() do.

When using the u64 hash table, you can use the entire u64 value space
-- there shouldn't be "bad" or "bogus" keys from the caller
perspective.  So I think Panfrost should be fine.


	Caio




> On Mon, Aug 05, 2019 at 09:10:00AM -0700, Caio Marcelo de Oliveira Filho wrote:
> > On Mon, Aug 05, 2019 at 05:18:32PM +0200, Tomeu Vizoso wrote:
> > > Some hash functions (eg. key_u64_hash) will attempt to dereference the
> > > key, causing an invalid access when passed DELETED_KEY_VALUE (0x1) or
> > > FREED_KEY_VALUE (0x0).
> > > 
> > > The entry.hash field isn't needed by the delete_function, so just stop
> > > populating it.
> > 
> > delete_function is a callback, so it is up to the caller whether it
> > makes use it or not.  We also would have to add a comment somewhere
> > about this exception.
> > 
> > Seems to me this is a bug of not calling hash correctly when
> > `sizeof(void *) != 8`.  To fix we should do like in
> > hash_table_u64_search() and create a temporary local struct
> > hash_key_u64 in that case.
> > 
> > 
> > 	Caio