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>
Bug: https://bugs.freedesktop.org/show_bug.cgi?id=99793
Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>

diff --git a/dbus/dbus-hash.c b/dbus/dbus-hash.c
index ddc17fd908b6c279229a4f8ebb0ca28e356e3e15..e30aded176fb410e160a8b3c8a4bddf923d6aedf 100644 (file)
--- 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;
 }