Re: [PATCH v2 03/10] hashmap: allow re-use after hashmap_free()

From

Jeff King <peff@peff.net>

Date

Oct 30, 2020, 13:35 UTC

Message-ID

<20201030133518.GC3277724@coredump.intra.peff.net>

In-Reply-To

<a686d0758a2652c91363dcf68da7726093d00c23.1602549650.git.gitgitgadget@gmail.com>

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.

> 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;