Attachment 129553 Details for Bug 99793 – dbus-hash: Fix memory leaks in internal hash table tests

[patch] dbus-hash: Fix memory leaks in internal hash table tests

dbus-hash-Fix-memory-leaks-in-internal-hash-table-.patch (text/plain), 12.10 KB, created by Philip Withnall on 2017-02-13 13:04:00 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Philip Withnall

Created: 2017-02-13 13:04:00 UTC

Size: 12.10 KB

patch

obsolete

>From 36e5f1686962797116f869c5f0fa41c28c454666 Mon Sep 17 00:00:00 2001
>From: Philip Withnall <withnall@endlessm.com>
>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 <withnall@endlessm.com>
>
>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

Actions: View | Splinter Review

Attachments on bug 99793: 129553 | 129665

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.