Guarantee that the key is freed when replaced

 * When setting a key in a map that already exists, then free
   the old key and replace with the new one.
 * Fix related bug where key was not properly allocated
 * Add tests for this

https://bugs.freedesktop.org/show_bug.cgi?id=59087

3 files changed, 102 insertions, 35 deletions

diff --git a/p11-kit/hashmap.c b/p11-kit/hashmap.c
index 1c4aff1..d420221 100644
--- a/p11-kit/hashmap.c
+++ b/p11-kit/hashmap.c
@@ -157,11 +157,16 @@ _p11_hash_set (hashmap *map,
 	bucketp = lookup_or_create_bucket (map, key, 1);
 	if(bucketp && *bucketp) {
 
+		/* Destroy the previous key */
+		if ((*bucketp)->key && (*bucketp)->key != key && map->key_destroy_func)
+			map->key_destroy_func ((*bucketp)->key);
+
 		/* Destroy the previous value */
-		if ((*bucketp)->value && map->value_destroy_func)
+		if ((*bucketp)->value && (*bucketp)->value != val && map->value_destroy_func)
 			map->value_destroy_func ((*bucketp)->value);
 
 		/* replace entry */
+		(*bucketp)->key = key;
 		(*bucketp)->value = val;
 
 		/* check that the collision rate isn't too high */
diff --git a/p11-kit/modules.c b/p11-kit/modules.c
index a4ffc43..c097c4b 100644
--- a/p11-kit/modules.c
+++ b/p11-kit/modules.c
@@ -390,6 +390,7 @@ take_config_and_load_module_unlocked (char **name, hashmap **config)
 	Module *mod, *prev;
 	const char *module_filename;
 	char *path;
+	char *key;
 	CK_RV rv;
 
 	assert (name);
@@ -409,8 +410,11 @@ take_config_and_load_module_unlocked (char **name, hashmap **config)
 	path = expand_module_path (module_filename);
 	return_val_if_fail (path != NULL, CKR_HOST_MEMORY);
 
+	key = strdup ("module");
+	return_val_if_fail (key != NULL, CKR_HOST_MEMORY);
+
 	/* The hash map will take ownership of the variable */
-	if (!_p11_hash_set (*config, "module", path))
+	if (!_p11_hash_set (*config, key, path))
 		return_val_if_reached (CKR_HOST_MEMORY);
 
 	mod = alloc_module_unlocked ();
diff --git a/tests/hash-test.c b/tests/hash-test.c
index 530c67c..cf77094 100644
--- a/tests/hash-test.c
+++ b/tests/hash-test.c
@@ -35,6 +35,7 @@
 #include "config.h"
 #include "CuTest.h"
 
+#include <assert.h>
 #include <stdlib.h>
 #include <stdio.h>
 #include <string.h>
@@ -57,15 +58,40 @@ test_free_null (CuTest *tc)
 	_p11_hash_free (NULL);
 }
 
+typedef struct {
+	int value;
+	int freed;
+} Key;
+
+static unsigned int
+key_hash (const void *ptr)
+{
+	const Key *k = ptr;
+	assert (!k->freed);
+	return _p11_hash_intptr_hash (&k->value);
+}
+
+static int
+key_equal (const void *one,
+           const void *two)
+{
+	const Key *k1 = one;
+	const Key *k2 = two;
+	assert (!k1->freed);
+	assert (!k2->freed);
+	return _p11_hash_intptr_equal (&k1->value, &k2->value);
+}
+
 static void
-destroy_key (void *data)
+key_destroy (void *data)
 {
-	int *key = data;
-	*key = 1;
+	Key *k = data;
+	assert (!k->freed);
+	k->freed = 1;
 }
 
 static void
-destroy_value (void *data)
+value_destroy (void *data)
 {
 	int *value = data;
 	*value = 2;
@@ -75,16 +101,16 @@ static void
 test_free_destroys (CuTest *tc)
 {
 	hashmap *map;
-	int key = 0;
+	Key key = { 8, 0 };
 	int value = 0;
 
-	map = _p11_hash_create (_p11_hash_direct_hash, _p11_hash_direct_equal, destroy_key, destroy_value);
+	map = _p11_hash_create (key_hash, key_equal, key_destroy, value_destroy);
 	CuAssertPtrNotNull (tc, map);
 	if (!_p11_hash_set (map, &key, &value))
 		CuFail (tc, "should not be reached");
 	_p11_hash_free (map);
 
-	CuAssertIntEquals (tc, 1, key);
+	CuAssertIntEquals (tc, 1, key.freed);
 	CuAssertIntEquals (tc, 2, value);
 }
 
@@ -186,36 +212,36 @@ static void
 test_remove_destroys (CuTest *tc)
 {
 	hashmap *map;
-	int key = 0;
+	Key key = { 8, 0 };
 	int value = 0;
 	int ret;
 
-	map = _p11_hash_create (_p11_hash_direct_hash, _p11_hash_direct_equal, destroy_key, destroy_value);
+	map = _p11_hash_create (key_hash, key_equal, key_destroy, value_destroy);
 	CuAssertPtrNotNull (tc, map);
 	if (!_p11_hash_set (map, &key, &value))
 		CuFail (tc, "should not be reached");
 
 	ret = _p11_hash_remove (map, &key);
 	CuAssertIntEquals (tc, ret, 1);
-	CuAssertIntEquals (tc, 1, key);
+	CuAssertIntEquals (tc, 1, key.freed);
 	CuAssertIntEquals (tc, 2, value);
 
 	/* should not be destroyed again */
-	key = 0;
+	key.freed = 0;
 	value = 0;
 
 	ret = _p11_hash_remove (map, &key);
 	CuAssertIntEquals (tc, ret, 0);
-	CuAssertIntEquals (tc, 0, key);
+	CuAssertIntEquals (tc, 0, key.freed);
 	CuAssertIntEquals (tc, 0, value);
 
 	/* should not be destroyed again */
-	key = 0;
+	key.freed = 0;
 	value = 0;
 
 	_p11_hash_free (map);
 
-	CuAssertIntEquals (tc, 0, key);
+	CuAssertIntEquals (tc, 0, key.freed);
 	CuAssertIntEquals (tc, 0, value);
 }
 
@@ -223,31 +249,63 @@ static void
 test_set_destroys (CuTest *tc)
 {
 	hashmap *map;
-	int key = 0;
-	int value = 0;
-	int value2 = 0;
+	Key key = { 8, 0 };
+	Key key2 = { 8, 0 };
+	int value, value2;
 	int ret;
 
-	map = _p11_hash_create (_p11_hash_direct_hash, _p11_hash_direct_equal, destroy_key, destroy_value);
+	map = _p11_hash_create (key_hash, key_equal, key_destroy, value_destroy);
 	CuAssertPtrNotNull (tc, map);
 	if (!_p11_hash_set (map, &key, &value))
 		CuFail (tc, "should not be reached");
 
-	ret = _p11_hash_set (map, &key, &value2);
+	key.freed = key2.freed = value = value2 = 0;
+
+	/* Setting same key and value, should not be destroyed */
+	ret = _p11_hash_set (map, &key, &value);
 	CuAssertIntEquals (tc, ret, 1);
-	CuAssertIntEquals (tc, 0, key);
-	CuAssertIntEquals (tc, 2, value);
+	CuAssertIntEquals (tc, 0, key.freed);
+	CuAssertIntEquals (tc, 0, key2.freed);
+	CuAssertIntEquals (tc, 0, value);
 	CuAssertIntEquals (tc, 0, value2);
 
-	key = 0;
-	value = 0;
-	value2 = 0;
+	key.freed = key2.freed = value = value2 = 0;
 
-	_p11_hash_free (map);
+	/* Setting a new key same value, key should be destroyed */
+	ret = _p11_hash_set (map, &key2, &value);
+	CuAssertIntEquals (tc, ret, 1);
+	CuAssertIntEquals (tc, 1, key.freed);
+	CuAssertIntEquals (tc, 0, key2.freed);
+	CuAssertIntEquals (tc, 0, value);
+	CuAssertIntEquals (tc, 0, value2);
+
+	key.freed = key2.freed = value = value2 = 0;
+
+	/* Setting same key, new value, value should be destroyed */
+	ret = _p11_hash_set (map, &key2, &value2);
+	CuAssertIntEquals (tc, ret, 1);
+	CuAssertIntEquals (tc, 0, key.freed);
+	CuAssertIntEquals (tc, 0, key2.freed);
+	CuAssertIntEquals (tc, 2, value);
+	CuAssertIntEquals (tc, 0, value2);
 
-	CuAssertIntEquals (tc, 1, key);
+	key.freed = key2.freed = value = value2 = 0;
+
+	/* Setting new key new value, both should be destroyed */
+	ret = _p11_hash_set (map, &key, &value);
+	CuAssertIntEquals (tc, ret, 1);
+	CuAssertIntEquals (tc, 0, key.freed);
+	CuAssertIntEquals (tc, 1, key2.freed);
 	CuAssertIntEquals (tc, 0, value);
 	CuAssertIntEquals (tc, 2, value2);
+
+	key.freed = key2.freed = value = value2 = 0;
+
+	_p11_hash_free (map);
+	CuAssertIntEquals (tc, 1, key.freed);
+	CuAssertIntEquals (tc, 2, value);
+	CuAssertIntEquals (tc, 0, key2.freed);
+	CuAssertIntEquals (tc, 0, value2);
 }
 
 
@@ -255,33 +313,33 @@ static void
 test_clear_destroys (CuTest *tc)
 {
 	hashmap *map;
-	int key = 0;
+	Key key = { 18, 0 };
 	int value = 0;
 
-	map = _p11_hash_create (_p11_hash_direct_hash, _p11_hash_direct_equal, destroy_key, destroy_value);
+	map = _p11_hash_create (key_hash, key_equal, key_destroy, value_destroy);
 	CuAssertPtrNotNull (tc, map);
 	if (!_p11_hash_set (map, &key, &value))
 		CuFail (tc, "should not be reached");
 
 	_p11_hash_clear (map);
-	CuAssertIntEquals (tc, 1, key);
+	CuAssertIntEquals (tc, 1, key.freed);
 	CuAssertIntEquals (tc, 2, value);
 
 	/* should not be destroyed again */
-	key = 0;
+	key.freed = 0;
 	value = 0;
 
 	_p11_hash_clear (map);
-	CuAssertIntEquals (tc, 0, key);
+	CuAssertIntEquals (tc, 0, key.freed);
 	CuAssertIntEquals (tc, 0, value);
 
 	/* should not be destroyed again */
-	key = 0;
+	key.freed = 0;
 	value = 0;
 
 	_p11_hash_free (map);
 
-	CuAssertIntEquals (tc, 0, key);
+	CuAssertIntEquals (tc, 0, key.freed);
 	CuAssertIntEquals (tc, 0, value);
 }