[glib/wip/desrt/more-ghash-valgrind: 13/13] ghash: fix small array handling in g_hash_table_remove_all_nodes()

[Allison Karlitskaya <desrt src gnome org>]

commit 115033338b7e55a4aab01df7113c2ea21301d644
Author: Allison Karlitskaya <allison karlitskaya redhat com>
Date:   Mon May 20 16:41:51 2019 +0200

    ghash: fix small array handling in g_hash_table_remove_all_nodes()
    
    Factor out the code for setting up the hash table size, mask and mod,
    detecting valgrind and allocating the arrays for hashes, keys, and
    values.
    
    Make use of this new function from g_hash_table_remove_all_nodes().
    
    The handling of have_big_keys and have_big_values was never correct in
    this function because it reallocated the array without changing the
    flags in the struct.  Any calls in to the hashtable from destroy
    notifies would find the table in an inconsistent state.
    
    Many thanks to Thomas Haller who is essentially responsible for all the
    real work in this patch: both discovering and identifying the original
    problem, as well as finding the solution to it.

 glib/ghash.c | 82 ++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 47 insertions(+), 35 deletions(-)
---
diff --git a/glib/ghash.c b/glib/ghash.c
index b21d00ff9..c7b7139a3 100644
--- a/glib/ghash.c
+++ b/glib/ghash.c
@@ -557,6 +557,42 @@ g_hash_table_remove_node (GHashTable   *hash_table,
 
 }
 
+/*
+ * g_hash_table_setup_storage:
+ * @hash_table: our #GHashTable
+ *
+ * Initialise the hash table size, mask, mod, and arrays.
+ */
+static void
+g_hash_table_setup_storage (GHashTable *hash_table)
+{
+  gboolean small;
+
+  /* We want to use small arrays only if:
+   *   - we are running on a system where that makes sense (64 bit); and
+   *   - we are not running under valgrind.
+   */
+  small = FALSE;
+
+#ifdef USE_SMALL_ARRAYS
+  small = TRUE;
+
+# ifdef ENABLE_VALGRIND
+  if (RUNNING_ON_VALGRIND)
+    small = FALSE;
+# endif
+#endif
+
+  g_hash_table_set_shift (hash_table, HASH_TABLE_MIN_SHIFT);
+
+  hash_table->have_big_keys = !small;
+  hash_table->have_big_values = !small;
+
+  hash_table->keys   = g_hash_table_realloc_key_or_value_array (NULL, hash_table->size, 
hash_table->have_big_keys);
+  hash_table->values = hash_table->keys;
+  hash_table->hashes = g_new0 (guint, hash_table->size);
+}
+
 /*
  * g_hash_table_remove_all_nodes:
  * @hash_table: our #GHashTable
@@ -587,6 +623,8 @@ g_hash_table_remove_all_nodes (GHashTable *hash_table,
   gpointer *old_keys;
   gpointer *old_values;
   guint    *old_hashes;
+  gboolean  old_have_big_keys;
+  gboolean  old_have_big_values;
 
   /* If the hash table is already empty, there is nothing to be done. */
   if (hash_table->nnodes == 0)
@@ -637,18 +675,15 @@ g_hash_table_remove_all_nodes (GHashTable *hash_table,
    * freeing them below.
    */
   old_size = hash_table->size;
+  old_have_big_keys = hash_table->have_big_keys;
+  old_have_big_values = hash_table->have_big_values;
   old_keys   = g_steal_pointer (&hash_table->keys);
   old_values = g_steal_pointer (&hash_table->values);
   old_hashes = g_steal_pointer (&hash_table->hashes);
 
   if (!destruction)
     /* Any accesses will see an empty table */
-    {
-      g_hash_table_set_shift (hash_table, HASH_TABLE_MIN_SHIFT);
-      hash_table->keys   = g_hash_table_realloc_key_or_value_array (NULL, hash_table->size, FALSE);
-      hash_table->values = hash_table->keys;
-      hash_table->hashes = g_new0 (guint, hash_table->size);
-    }
+    g_hash_table_setup_storage (hash_table);
   else
     /* Will cause a quick crash on any attempted access */
     hash_table->size = hash_table->mod = hash_table->mask = 0;
@@ -658,13 +693,13 @@ g_hash_table_remove_all_nodes (GHashTable *hash_table,
     {
       if (HASH_IS_REAL (old_hashes[i]))
         {
-          key = g_hash_table_fetch_key_or_value (old_keys, i, hash_table->have_big_keys);
-          value = g_hash_table_fetch_key_or_value (old_values, i, hash_table->have_big_values);
+          key = g_hash_table_fetch_key_or_value (old_keys, i, old_have_big_keys);
+          value = g_hash_table_fetch_key_or_value (old_values, i, old_have_big_values);
 
           old_hashes[i] = UNUSED_HASH_VALUE;
 
-          g_hash_table_assign_key_or_value (old_keys, i, hash_table->have_big_keys, NULL);
-          g_hash_table_assign_key_or_value (old_values, i, hash_table->have_big_values, NULL);
+          g_hash_table_assign_key_or_value (old_keys, i, old_have_big_keys, NULL);
+          g_hash_table_assign_key_or_value (old_values, i, old_have_big_values, NULL);
 
           if (hash_table->key_destroy_func != NULL)
             hash_table->key_destroy_func (key);
@@ -674,9 +709,6 @@ g_hash_table_remove_all_nodes (GHashTable *hash_table,
         }
     }
 
-  hash_table->have_big_keys = FALSE;
-  hash_table->have_big_values = FALSE;
-
   /* Destroy old storage space. */
   if (old_keys != old_values)
     g_free (old_values);
@@ -1037,25 +1069,8 @@ g_hash_table_new_full (GHashFunc      hash_func,
                        GDestroyNotify value_destroy_func)
 {
   GHashTable *hash_table;
-  gboolean small;
-
-  /* We want to use small arrays only if:
-   *   - we are running on a system where that makes sense (64 bit); and
-   *   - we are not running under valgrind.
-   */
-  small = FALSE;
-
-#ifdef USE_SMALL_ARRAYS
-  small = TRUE;
-
-# ifdef ENABLE_VALGRIND
-  if (RUNNING_ON_VALGRIND)
-    small = FALSE;
-# endif
-#endif
 
   hash_table = g_slice_new (GHashTable);
-  g_hash_table_set_shift (hash_table, HASH_TABLE_MIN_SHIFT);
   g_atomic_ref_count_init (&hash_table->ref_count);
   hash_table->nnodes             = 0;
   hash_table->noccupied          = 0;
@@ -1066,11 +1081,8 @@ g_hash_table_new_full (GHashFunc      hash_func,
 #endif
   hash_table->key_destroy_func   = key_destroy_func;
   hash_table->value_destroy_func = value_destroy_func;
-  hash_table->have_big_keys      = !small;
-  hash_table->have_big_values    = !small;
-  hash_table->keys               = g_hash_table_realloc_key_or_value_array (NULL, hash_table->size, 
hash_table->have_big_keys);
-  hash_table->values             = hash_table->keys;
-  hash_table->hashes             = g_new0 (guint, hash_table->size);
+
+  g_hash_table_setup_storage (hash_table);
 
   return hash_table;
 }