On Tue, Oct 13, 2020 at 12:40:43AM +0000, Elijah Newren via GitGitGadget wrote:
> Previously, once map->table had been freed, any calls to hashmap_put(),
> hashmap_get(), or hashmap_remove() would cause a NULL pointer
> dereference (since hashmap_free_() also zeros the memory; without that
> zeroing, calling these functions would cause a use-after-free problem).
>
> Modify these functions to check for a NULL table and automatically
> allocate as needed.
Unsurprisingly, I like this direction. The code looks correct to me,
though I think you could reduce duplication slightly by checking
map->table in find_entry_ptr(). That covers both hashmap_get() and
hashmap_remove(). But I'm happy either way.
> I also thought about creating a HASHMAP_INIT macro to allow initializing
> hashmaps on the stack without calling hashmap_init(), but virtually all
> uses of hashmap specify a usecase-specific equals_function which defeats
> the utility of such a macro.
This part I disagree with. If we did:
#define HASHMAP_INIT(fn, data) = { .cmpfn = cmpfn, cmpfn_data = data }
then many callers could avoid handling the lazy-init themselves. E.g.:
attr.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/attr.c b/attr.c
index a826b2ef1f..55a2783f1b 100644
--- a/attr.c
+++ b/attr.c
@@ -57,7 +57,9 @@ static inline void hashmap_unlock(struct attr_hashmap *map)
* is a singleton object which is shared between threads.
* Access to this dictionary must be surrounded with a mutex.
*/
-static struct attr_hashmap g_attr_hashmap;
+static struct attr_hashmap g_attr_hashmap = {
+ HASHMAP_INIT(attr_hash_entry_cmp, NULL)
+};
/* The container for objects stored in "struct attr_hashmap" */
struct attr_hash_entry {
@@ -80,12 +82,6 @@ static int attr_hash_entry_cmp(const void *unused_cmp_data,
return (a->keylen != b->keylen) || strncmp(a->key, b->key, a->keylen);
}
-/* Initialize an 'attr_hashmap' object */
-static void attr_hashmap_init(struct attr_hashmap *map)
-{
- hashmap_init(&map->map, attr_hash_entry_cmp, NULL, 0);
-}
-
/*
* Retrieve the 'value' stored in a hashmap given the provided 'key'.
* If there is no matching entry, return NULL.
@@ -96,9 +92,6 @@ static void *attr_hashmap_get(struct attr_hashmap *map,
struct attr_hash_entry k;
struct attr_hash_entry *e;
- if (!map->map.tablesize)
- attr_hashmap_init(map);
-
hashmap_entry_init(&k.ent, memhash(key, keylen));
k.key = key;
k.keylen = keylen;
@@ -114,9 +107,6 @@ static void attr_hashmap_add(struct attr_hashmap *map,
{
struct attr_hash_entry *e;
- if (!map->map.tablesize)
- attr_hashmap_init(map);
-
e = xmalloc(sizeof(struct attr_hash_entry));
hashmap_entry_init(&e->ent, memhash(key, keylen));
e->key = key;
