Re: [PATCH 10/13] util: hash: Reimplement virHashTable using GHashTable
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(a)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.