Be more careful when calling destroy notifies

If we are, we can allow modification of the hash table
from destroy notifies.

https://bugzilla.gnome.org/show_bug.cgi?id=650459

glib/ghash.c

@@ -419,11 +419,11 @@ g_hash_table_remove_node (GHashTable   *hash_table,
                           int           i,
                           gboolean      notify)
 {
-  if (notify && hash_table->key_destroy_func)
+  gpointer key;
-    hash_table->key_destroy_func (hash_table->keys[i]);
+  gpointer value;
 
-  if (notify && hash_table->value_destroy_func)
+  key = hash_table->keys[i];
-    hash_table->value_destroy_func (hash_table->values[i]);
+  value = hash_table->values[i];
 
   /* Erect tombstone */
   hash_table->hashes[i] = 1;
@@ -433,6 +433,13 @@ g_hash_table_remove_node (GHashTable   *hash_table,
   hash_table->values[i] = NULL;
 
   hash_table->nnodes--;
+
+  if (notify && hash_table->key_destroy_func)
+    hash_table->key_destroy_func (key);
+
+  if (notify && hash_table->value_destroy_func)
+    hash_table->value_destroy_func (value);
+
 }
 
 /*
@@ -451,33 +458,41 @@ g_hash_table_remove_all_nodes (GHashTable *hash_table,
                                gboolean    notify)
 {
   int i;
+  gpointer key;
+  gpointer value;
 
-  if (notify &&
+  hash_table->nnodes = 0;
-      (hash_table->key_destroy_func != NULL ||
+  hash_table->noccupied = 0;
-       hash_table->value_destroy_func != NULL))
-    {
-      for (i = 0; i < hash_table->size; i++)
-        {
-          if (HASH_IS_REAL (hash_table->hashes[i]))
-            {
-              if (hash_table->key_destroy_func != NULL)
-                hash_table->key_destroy_func (hash_table->keys[i]);
 
-              if (hash_table->value_destroy_func != NULL)
+  if (!notify ||
-                hash_table->value_destroy_func (hash_table->values[i]);
+      (hash_table->key_destroy_func == NULL &&
-            }
+       hash_table->value_destroy_func == NULL))
-        }
+    {
-    }
-
-  /* We need to set node->key_hash = 0 for all nodes - might as well be GC
-   * friendly and clear everything
-   */
       memset (hash_table->hashes, 0, hash_table->size * sizeof (guint));
       memset (hash_table->keys, 0, hash_table->size * sizeof (gpointer));
       memset (hash_table->values, 0, hash_table->size * sizeof (gpointer));
 
-  hash_table->nnodes = 0;
+      return;
-  hash_table->noccupied = 0;
+    }
+
+  for (i = 0; i < hash_table->size; i++)
+    {
+      if (HASH_IS_REAL (hash_table->hashes[i]))
+        {
+          key = hash_table->keys[i];
+          value = hash_table->values[i];
+
+          hash_table->hashes[i] = 0;
+          hash_table->keys[i] = NULL;
+          hash_table->values[i] = NULL;
+
+          if (hash_table->key_destroy_func != NULL)
+            hash_table->key_destroy_func (key);
+
+          if (hash_table->value_destroy_func != NULL)
+            hash_table->value_destroy_func (value);
+        }
+    }
 }
 
 /*
@@ -969,6 +984,8 @@ g_hash_table_insert_internal (GHashTable *hash_table,
   guint node_index;
   guint key_hash;
   guint old_hash;
+  gpointer old_key;
+  gpointer old_value;
 
   g_return_if_fail (hash_table != NULL);
   g_return_if_fail (hash_table->ref_count > 0);
@@ -979,24 +996,13 @@ g_hash_table_insert_internal (GHashTable *hash_table,
   node_index = g_hash_table_lookup_node (hash_table, key, &key_hash);
 
   old_hash = hash_table->hashes[node_index];
+  old_key = hash_table->keys[node_index];
+  old_value = hash_table->values[node_index];
 
   if (HASH_IS_REAL (old_hash))
     {
-      if (hash_table->value_destroy_func)
-        hash_table->value_destroy_func (hash_table->values[node_index]);
-
       if (keep_new_key)
-        {
-          if (hash_table->key_destroy_func)
-            hash_table->key_destroy_func (hash_table->keys[node_index]);
         hash_table->keys[node_index] = key;
-        }
-      else
-        {
-          if (hash_table->key_destroy_func)
-            hash_table->key_destroy_func (key);
-        }
-
       hash_table->values[node_index] = value;
     }
   else
@@ -1018,6 +1024,14 @@ g_hash_table_insert_internal (GHashTable *hash_table,
       hash_table->version++;
 #endif
     }
+
+  if (HASH_IS_REAL (old_hash))
+    {
+      if (hash_table->key_destroy_func)
+        hash_table->key_destroy_func (keep_new_key ? old_key : key);
+      if (hash_table->value_destroy_func)
+        hash_table->value_destroy_func (old_value);
+    }
 }
 
 /**

glib/tests/hash.c

@@ -825,6 +825,34 @@ set_ref_hash_test (void)
   key_unref (key2);
 }
 
+GHashTable *h;
+
+static void
+value_destroy_insert (gpointer value)
+{
+  g_hash_table_remove_all (h);
+}
+
+static void
+test_destroy_modify (void)
+{
+  g_test_bug ("650459");
+
+  h = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, value_destroy_insert);
+
+  g_hash_table_insert (h, g_strdup ("a"), g_strdup ("b"));
+  g_hash_table_insert (h, g_strdup ("c"), g_strdup ("d"));
+  g_hash_table_insert (h, g_strdup ("e"), g_strdup ("f"));
+  g_hash_table_insert (h, g_strdup ("g"), g_strdup ("h"));
+  g_hash_table_insert (h, g_strdup ("h"), g_strdup ("k"));
+  g_hash_table_insert (h, g_strdup ("a"), g_strdup ("c"));
+
+  g_hash_table_remove (h, "c");
+  g_hash_table_remove (h, "e");
+
+  g_hash_table_unref (h);
+}
+
 int
 main (int argc, char *argv[])
 {
@@ -846,6 +874,7 @@ main (int argc, char *argv[])
 
   /* tests for individual bugs */
   g_test_add_func ("/hash/lookup-null-key", test_lookup_null_key);
+  g_test_add_func ("/hash/destroy-modify", test_destroy_modify);
 
   return g_test_run ();