Attachment 72612 Details for Bug 59087 – I decided to put together a patch. Does the attached patch fix the leak for you?

[patch] I decided to put together a patch. Does the attached patch fix the leak for you?

Guarantee-that-the-key-is-freed-when-replaced.patch (text/plain), 8.14 KB, created by Stef Walter on 2013-01-07 08:31:31 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Stef Walter

Created: 2013-01-07 08:31:31 UTC

Size: 8.14 KB

patch

obsolete

>From e2b5bba185c96bf4ecddfe22d34ace02706122b4 Mon Sep 17 00:00:00 2001
>From: Stef Walter <stefw@gnome.org>
>Date: Mon, 7 Jan 2013 09:20:25 +0100
>Subject: [PATCH] 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
>---
> p11-kit/hashmap.c |   7 ++-
> p11-kit/modules.c |   6 ++-
> tests/hash-test.c | 124 +++++++++++++++++++++++++++++++++++++++---------------
> 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);
> }
> 
>-- 
>1.8.1

Actions: View | Splinter Review

Attachments on bug 59087: 72612

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.