[PATCH 10/13] util: hash: Reimplement virHashTable using GHashTable

[Peter Krempa]

On Mon, Oct 26, 2020 at 16:08:34 +0000, Daniel Berrange wrote:
> On Mon, Oct 26, 2020 at 04:45:50PM +0100, Peter Krempa wrote:
> > Glib's hash table provides basically the same functionality as our hash
> > table.
> > 
> > In most cases the only thing that remains in the virHash* wrappers is
> > NULL-checks of '@table' argument as glib's hash functions don't tolerate
> > NULL.
> > 
> > In case of iterators, we adapt the existing API of iterators to glibs to
> > prevent having rewrite all callers at this point.
> > 
> > Signed-off-by: Peter Krempa <pkrempa at redhat.com>
> > ---
> >  src/libvirt_private.syms |   4 -
> >  src/util/meson.build     |   1 -
> >  src/util/virhash.c       | 416 ++++++++++-----------------------------
> >  src/util/virhash.h       |   4 +-
> >  src/util/virhashcode.c   | 125 ------------
> >  src/util/virhashcode.h   |  33 ----
> 
> Our hash code impl uses Murmurhash which makes some efforts to be
> robust against malicious inputs triggering collisons, notably using
> a random seed.
> 
> The new code uses  g_str_hash which is much weaker, and the API
> docs explicitly recommend against using it if the input can be from
> an untrusted user.

Yes, I've noticed that, but didn't consider it to be that much of a
problem as any untrusted input which is stored in a hash table (so that
the attacker can use crafted keys) must be in the first place
safeguarded against OOM condition by limiting the input count/size.

Apart from insertion we do have at least one place where lookup is based
on untrusted data so there is a possibility for bigger scope of impact.

Said that I don't really believe that the complexity vs impact ratio
makes it worth even for a completely static hashing function, but when
we can do better, we should.

> IOW, I don't think we should be removing our virhashcode impl. If
> anything we should upgrade our hash code impl to use SipHash which
> is used by perl, python, ruby, rust and more.

Okay, I'll leave virhashcode in. Regardless whether we have a "cool"
hashing function or not, devs need to be vigilant of use of any data
structure based on untrusted data though.