From 517bcf9987b103a4116a77993482ef78c0264b30 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 | 216 ++++++++++++++++++++++++++++++------------------------- 1 file changed, 118 insertions(+), 98 deletions(-) diff --git a/dbus/dbus-hash.c b/dbus/dbus-hash.c index ddc17fd..ba5e7ca 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(). @@ -754,7 +755,15 @@ _dbus_hash_iter_lookup (DBusHashTable *table, if (entry == NULL) return FALSE; - + + if (create_if_not_found) + { + if (table->free_key_function && entry->key != key) + (* table->free_key_function) (entry->key); + + entry->key = key; + } + real->table = table; real->bucket = bucket; real->entry = entry; @@ -1555,6 +1564,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 +1593,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 +1641,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 +1726,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 +1765,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 +1793,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 +1824,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 +1878,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 +1971,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