[glib/th/hash-steal-extended-set: 2/2] ghash: fix g_hash_table_steal_extended() when requesting key and value of a set

[Thomas Haller <thaller src gnome org>]

commit ecdbfa48e4b3d8d1210e1efe67080972d597dec2
Author: Thomas Haller <thaller redhat com>
Date:   Tue Oct 18 20:51:06 2022 +0200

    ghash: fix g_hash_table_steal_extended() when requesting key and value of a set
    
    GHashTable optimizes for the "set" case, where key and value are the same.
    See g_hash_table_add().
    
    A user cannot see from outside, whether a GHashTable internally is a set
    and shares the keys and values array. Adding one key/value pair with
    differing key and value, will expand the GHashTable.
    
    In all other cases, the GHashTable API hides this implementation detail
    correctly. Except with g_hash_table_steal_extended(), when stealing both the
    key and the value.
    
    Fix that. This bug fix is obviously a change in behavior. In practice,
    it's unlikely that somebody would notice, because GHashTable contains
    opaque pointers and the user must know what the keys/values are and
    be aware of their ownership semantics when stealing them. That means,
    the change in behavior only affects instances that are internally a set,
    of what the user most likely is aware and fills the table with
    g_hash_table_add(). Such a user would not steal both the key and
    values at the same time. Even if they do, then previously stealing the
    value was pointless and would not give them what they wanted. It would
    not have meaningfully worked, and since nobody reported a bug about this
    yet, it's unlikely somebody noticed.
    
    The more problematic case when the user exhibits the bug is when the
    dictionary is unexpected a set internally. Imagine a mapping from numbers
    to numbers (e.g. a permutation). If "unexpectedly" the dictionary contains
    the identity permutation, steal-extended gives always NULL for the target
    number.
    
    The example is far fetched. In practice, it's unlikely that somebody is
    gonna notice either way. That is not an argument for fixing anything.
    The argument for fixing this, is that the bug breaks the illusion that
    the set is only an internal optimization. That is ugly and inconsistent.

 glib/ghash.c      | 18 ++++++++++++------
 glib/tests/hash.c |  6 ++++--
 2 files changed, 16 insertions(+), 8 deletions(-)
---
diff --git a/glib/ghash.c b/glib/ghash.c
index 366c751acc..35a423a658 100644
--- a/glib/ghash.c
+++ b/glib/ghash.c
@@ -1842,8 +1842,9 @@ g_hash_table_steal (GHashTable    *hash_table,
  * of @hash_table are %NULL-safe.
  *
  * The dictionary implementation optimizes for having all values identical to
- * their keys, for example by using g_hash_table_add(). When stealing both the
- * key and the value from such a dictionary, the value will be %NULL.
+ * their keys, for example by using g_hash_table_add(). Before 2.76, when stealing
+ * both the key and the value from such a dictionary, the value was %NULL.
+ * Now the returned value and the key will be the same.
  *
  * Returns: %TRUE if the key was found in the #GHashTable
  * Since: 2.58
@@ -1877,10 +1878,15 @@ g_hash_table_steal_extended (GHashTable    *hash_table,
   }
 
   if (stolen_value != NULL)
-  {
-    *stolen_value = g_hash_table_fetch_key_or_value (hash_table->values, node_index, 
hash_table->have_big_values);
-    g_hash_table_assign_key_or_value (hash_table->values, node_index, hash_table->have_big_values, NULL);
-  }
+    {
+      if (stolen_key && hash_table->keys == hash_table->values)
+        *stolen_value = *stolen_key;
+      else
+        {
+          *stolen_value = g_hash_table_fetch_key_or_value (hash_table->values, node_index, 
hash_table->have_big_values);
+          g_hash_table_assign_key_or_value (hash_table->values, node_index, hash_table->have_big_values, 
NULL);
+        }
+    }
 
   g_hash_table_remove_node (hash_table, node_index, FALSE);
   g_hash_table_maybe_resize (hash_table);
diff --git a/glib/tests/hash.c b/glib/tests/hash.c
index 121e14548b..bd21443584 100644
--- a/glib/tests/hash.c
+++ b/glib/tests/hash.c
@@ -1283,8 +1283,9 @@ test_steal_extended (void)
   g_assert_true (g_hash_table_steal_extended (hash, "a", (gpointer *) &stolen_key,
                                               (gpointer *) &stolen_value));
   g_assert_cmpstr (stolen_key, ==, "a");
-  g_assert_cmpstr (stolen_value, ==, NULL);
+  g_assert_cmpstr (stolen_value, ==, "a");
   g_clear_pointer (&stolen_key, g_free);
+  stolen_value = NULL;
 
   g_assert_true (g_hash_table_steal_extended (hash, "b", (gpointer *) &stolen_key,
                                               NULL));
@@ -1299,8 +1300,9 @@ test_steal_extended (void)
   g_assert_true (g_hash_table_steal_extended (hash, "d", (gpointer *) &stolen_key,
                                               (gpointer *) &stolen_value));
   g_assert_cmpstr (stolen_key, ==, "d");
-  g_assert_cmpstr (stolen_value, ==, NULL);
+  g_assert_cmpstr (stolen_value, ==, "d");
   g_clear_pointer (&stolen_key, g_free);
+  stolen_value = NULL;
 
   g_hash_table_replace (hash, g_strdup ("x"), NULL);