From 36e5f1686962797116f869c5f0fa41c28c454666 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 13 Feb 2017 12:55:40 +0000 Subject: [PATCH] dbus-hash: Fix memory leaks in internal hash table tests This includes fixing a memory leak in _dbus_hash_iter_lookup(), which is not one of the unit tests; but it is only ever called from the unit tests, so this is not a user-facing leak. Coverity IDs: 54730, 54740 Signed-off-by: Philip Withnall https://bugs.freedesktop.org/show_bug.cgi?id=99793 --- dbus/dbus-hash.c | 208 +++++++++++++++++++++++++++++-------------------------- 1 file changed, 111 insertions(+), 97 deletions(-) diff --git a/dbus/dbus-hash.c b/dbus/dbus-hash.c index ddc17fd..e30aded 100644 --- a/dbus/dbus-hash.c +++ b/dbus/dbus-hash.c @@ -724,8 +724,9 @@ _dbus_hash_iter_get_string_key (DBusHashIter *iter) * because create_if_not_found was #FALSE and the entry * did not exist. * - * If create_if_not_found is #TRUE and the entry is created, the hash - * table takes ownership of the key that's passed in. + * If create_if_not_found is #TRUE, the hash + * table takes ownership of the key that's passed in (either using it to create + * the entry, or freeing it immediately). * * For a hash table of type #DBUS_HASH_INT, cast the int * key to the key parameter using #_DBUS_INT_TO_POINTER(). @@ -869,6 +870,8 @@ find_generic_function (DBusHashTable *table, if (preallocated) _dbus_hash_table_free_preallocated_entry (table, preallocated); + if (create_if_not_found && table->free_key_function) + (* table->free_key_function) (key); return entry; } @@ -1555,6 +1558,19 @@ count_entries (DBusHashTable *table) return count; } +static inline void * +steal (void *ptr) +{ + /* @ptr is passed in as void* to avoid casting in the call */ + void **_ptr = (void **) ptr; + void *val; + + val = *_ptr; + *_ptr = NULL; + + return val; +} + /** * @ingroup DBusHashTableInternals * Unit test for DBusHashTable @@ -1571,6 +1587,8 @@ _dbus_hash_test (void) #define N_HASH_KEYS 5000 char **keys; dbus_bool_t ret = FALSE; + char *str_key = NULL; + char *str_value = NULL; keys = dbus_new (char *, N_HASH_KEYS); if (keys == NULL) @@ -1617,51 +1635,51 @@ _dbus_hash_test (void) i = 0; while (i < 3000) { - void *value; - char *key; + const void *out_value; - key = _dbus_strdup (keys[i]); - if (key == NULL) + str_key = _dbus_strdup (keys[i]); + if (str_key == NULL) goto out; - value = _dbus_strdup ("Value!"); - if (value == NULL) + str_value = _dbus_strdup ("Value!"); + if (str_value == NULL) goto out; - + if (!_dbus_hash_table_insert_string (table1, - key, value)) + steal (&str_key), + steal (&str_value))) goto out; - value = _dbus_strdup (keys[i]); - if (value == NULL) + str_value = _dbus_strdup (keys[i]); + if (str_value == NULL) goto out; - + if (!_dbus_hash_table_insert_int (table2, - i, value)) + i, steal (&str_value))) goto out; - value = _dbus_strdup (keys[i]); - if (value == NULL) + str_value = _dbus_strdup (keys[i]); + if (str_value == NULL) goto out; - + if (!_dbus_hash_table_insert_uintptr (table3, - i, value)) + i, steal (&str_value))) goto out; _dbus_assert (count_entries (table1) == i + 1); _dbus_assert (count_entries (table2) == i + 1); _dbus_assert (count_entries (table3) == i + 1); - value = _dbus_hash_table_lookup_string (table1, keys[i]); - _dbus_assert (value != NULL); - _dbus_assert (strcmp (value, "Value!") == 0); + out_value = _dbus_hash_table_lookup_string (table1, keys[i]); + _dbus_assert (out_value != NULL); + _dbus_assert (strcmp (out_value, "Value!") == 0); - value = _dbus_hash_table_lookup_int (table2, i); - _dbus_assert (value != NULL); - _dbus_assert (strcmp (value, keys[i]) == 0); + out_value = _dbus_hash_table_lookup_int (table2, i); + _dbus_assert (out_value != NULL); + _dbus_assert (strcmp (out_value, keys[i]) == 0); - value = _dbus_hash_table_lookup_uintptr (table3, i); - _dbus_assert (value != NULL); - _dbus_assert (strcmp (value, keys[i]) == 0); + out_value = _dbus_hash_table_lookup_uintptr (table3, i); + _dbus_assert (out_value != NULL); + _dbus_assert (strcmp (out_value, keys[i]) == 0); ++i; } @@ -1702,40 +1720,38 @@ _dbus_hash_test (void) dbus_free, dbus_free); if (table1 == NULL) goto out; - + table2 = _dbus_hash_table_new (DBUS_HASH_INT, NULL, dbus_free); if (table2 == NULL) goto out; - + i = 0; while (i < 5000) { - char *key; - void *value; - - key = _dbus_strdup (keys[i]); - if (key == NULL) + str_key = _dbus_strdup (keys[i]); + if (str_key == NULL) goto out; - value = _dbus_strdup ("Value!"); - if (value == NULL) + str_value = _dbus_strdup ("Value!"); + if (str_value == NULL) goto out; - + if (!_dbus_hash_table_insert_string (table1, - key, value)) + steal (&str_key), + steal (&str_value))) goto out; - value = _dbus_strdup (keys[i]); - if (value == NULL) + str_value = _dbus_strdup (keys[i]); + if (str_value == NULL) goto out; - + if (!_dbus_hash_table_insert_int (table2, - i, value)) + i, steal (&str_value))) goto out; - + _dbus_assert (count_entries (table1) == i + 1); _dbus_assert (count_entries (table2) == i + 1); - + ++i; } @@ -1743,22 +1759,22 @@ _dbus_hash_test (void) while (_dbus_hash_iter_next (&iter)) { const char *key; - void *value; + const void *value; key = _dbus_hash_iter_get_string_key (&iter); value = _dbus_hash_iter_get_value (&iter); _dbus_assert (_dbus_hash_table_lookup_string (table1, key) == value); - value = _dbus_strdup ("Different value!"); - if (value == NULL) + str_value = _dbus_strdup ("Different value!"); + if (str_value == NULL) goto out; - - _dbus_hash_iter_set_value (&iter, value); + value = str_value; + _dbus_hash_iter_set_value (&iter, steal (&str_value)); _dbus_assert (_dbus_hash_table_lookup_string (table1, key) == value); } - + _dbus_hash_iter_init (table1, &iter); while (_dbus_hash_iter_next (&iter)) { @@ -1771,19 +1787,19 @@ _dbus_hash_test (void) while (_dbus_hash_iter_next (&iter)) { int key; - void *value; + const void *value; key = _dbus_hash_iter_get_int_key (&iter); value = _dbus_hash_iter_get_value (&iter); _dbus_assert (_dbus_hash_table_lookup_int (table2, key) == value); - value = _dbus_strdup ("Different value!"); - if (value == NULL) + str_value = _dbus_strdup ("Different value!"); + if (str_value == NULL) goto out; - - _dbus_hash_iter_set_value (&iter, value); + value = str_value; + _dbus_hash_iter_set_value (&iter, steal (&str_value)); _dbus_assert (_dbus_hash_table_lookup_int (table2, key) == value); } @@ -1802,42 +1818,38 @@ _dbus_hash_test (void) i = 0; while (i < 1000) { - char *key; - void *value; - - key = _dbus_strdup (keys[i]); - if (key == NULL) + str_key = _dbus_strdup (keys[i]); + if (str_key == NULL) goto out; - value = _dbus_strdup ("Value!"); - if (value == NULL) + str_value = _dbus_strdup ("Value!"); + if (str_value == NULL) goto out; - + if (!_dbus_hash_table_insert_string (table1, - key, value)) + steal (&str_key), + steal (&str_value))) goto out; - + ++i; } --i; while (i >= 0) { - char *key; - void *value; - - key = _dbus_strdup (keys[i]); - if (key == NULL) + str_key = _dbus_strdup (keys[i]); + if (str_key == NULL) goto out; - value = _dbus_strdup ("Value!"); - if (value == NULL) + str_value = _dbus_strdup ("Value!"); + if (str_value == NULL) goto out; if (!_dbus_hash_table_remove_string (table1, keys[i])) goto out; - + if (!_dbus_hash_table_insert_string (table1, - key, value)) + steal (&str_key), + steal (&str_value))) goto out; if (!_dbus_hash_table_remove_string (table1, keys[i])) @@ -1860,63 +1872,62 @@ _dbus_hash_test (void) dbus_free, dbus_free); if (table1 == NULL) goto out; - + table2 = _dbus_hash_table_new (DBUS_HASH_INT, NULL, dbus_free); if (table2 == NULL) goto out; - + i = 0; while (i < 3000) { - void *value; - char *key; + const void *out_value; - key = _dbus_strdup (keys[i]); - if (key == NULL) + str_key = _dbus_strdup (keys[i]); + if (str_key == NULL) goto out; - value = _dbus_strdup ("Value!"); - if (value == NULL) + str_value = _dbus_strdup ("Value!"); + if (str_value == NULL) goto out; - + if (!_dbus_hash_iter_lookup (table1, - key, TRUE, &iter)) + steal (&str_key), TRUE, &iter)) goto out; _dbus_assert (_dbus_hash_iter_get_value (&iter) == NULL); - _dbus_hash_iter_set_value (&iter, value); + _dbus_hash_iter_set_value (&iter, steal (&str_value)); - value = _dbus_strdup (keys[i]); - if (value == NULL) + str_value = _dbus_strdup (keys[i]); + if (str_value == NULL) goto out; if (!_dbus_hash_iter_lookup (table2, _DBUS_INT_TO_POINTER (i), TRUE, &iter)) goto out; _dbus_assert (_dbus_hash_iter_get_value (&iter) == NULL); - _dbus_hash_iter_set_value (&iter, value); - + _dbus_hash_iter_set_value (&iter, steal (&str_value)); + _dbus_assert (count_entries (table1) == i + 1); _dbus_assert (count_entries (table2) == i + 1); if (!_dbus_hash_iter_lookup (table1, keys[i], FALSE, &iter)) goto out; - - value = _dbus_hash_iter_get_value (&iter); - _dbus_assert (value != NULL); - _dbus_assert (strcmp (value, "Value!") == 0); + + out_value = _dbus_hash_iter_get_value (&iter); + _dbus_assert (out_value != NULL); + _dbus_assert (strcmp (out_value, "Value!") == 0); /* Iterate just to be sure it works, though * it's a stupid thing to do */ while (_dbus_hash_iter_next (&iter)) ; - + if (!_dbus_hash_iter_lookup (table2, _DBUS_INT_TO_POINTER (i), FALSE, &iter)) goto out; - value = _dbus_hash_iter_get_value (&iter); - _dbus_assert (value != NULL); - _dbus_assert (strcmp (value, keys[i]) == 0); + out_value = _dbus_hash_iter_get_value (&iter); + _dbus_assert (out_value != NULL); + _dbus_assert (strcmp (out_value, keys[i]) == 0); /* Iterate just to be sure it works, though * it's a stupid thing to do @@ -1954,6 +1965,9 @@ _dbus_hash_test (void) dbus_free (keys[i]); dbus_free (keys); + + dbus_free (str_key); + dbus_free (str_value); return ret; } -- 2.9.3