From 64f76bb1734c85986284b7f01eea76d9e1f1fdbf Mon Sep 17 00:00:00 2001 From: Allison Karlitskaya Date: Wed, 15 May 2019 16:17:04 +0200 Subject: [PATCH 1/5] ghash: fix bug introduced by valgrind fix g_hash_table_new_full() had an invocation of g_hash_table_realloc_key_or_value_array() with the @is_big argument incorrectly hardcoded to FALSE, even though later in the function the values of have_big_keys and have_big_values would be set conditionally. This never caused problems before because on 64bit platforms, this would result in the allocation of a guint-sized array (which would be fine, as have_big_keys and have_big_values would always start out as false) and on 32bit platforms, this function ignored the value and always allocated a gpointer-sized array. Since merge request GNOME/glib!845 we have the possibility for have_big_keys and have_big_values to start out as TRUE on 64bit platforms. We need to make sure we pass the argument through correctly. --- glib/ghash.c | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/glib/ghash.c b/glib/ghash.c index c58e8c585..71b9fd862 100644 --- a/glib/ghash.c +++ b/glib/ghash.c @@ -1017,22 +1017,6 @@ g_hash_table_new_full (GHashFunc hash_func, GHashTable *hash_table; gboolean small; - 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; - hash_table->hash_func = hash_func ? hash_func : g_direct_hash; - hash_table->key_equal_func = key_equal_func; -#ifndef G_DISABLE_ASSERT - hash_table->version = 0; -#endif - hash_table->key_destroy_func = key_destroy_func; - hash_table->value_destroy_func = value_destroy_func; - 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); - /* 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. @@ -1048,8 +1032,23 @@ g_hash_table_new_full (GHashFunc hash_func, # endif #endif - hash_table->have_big_keys = !small; - hash_table->have_big_values = !small; + 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; + hash_table->hash_func = hash_func ? hash_func : g_direct_hash; + hash_table->key_equal_func = key_equal_func; +#ifndef G_DISABLE_ASSERT + hash_table->version = 0; +#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); return hash_table; } From c6137e974dcf8440e84ac4530f528abf028d29d5 Mon Sep 17 00:00:00 2001 From: Allison Karlitskaya Date: Mon, 20 May 2019 15:17:11 +0200 Subject: [PATCH 2/5] ghash: Improve internal documentation The changes introduced by 18745ff674896c931379d097b18d74678044668e made the comment at the top of g_hash_table_remove_all_nodes() no longer correct. Fix that inaccuracy and add more documentation all-around. --- glib/ghash.c | 42 +++++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 9 deletions(-) diff --git a/glib/ghash.c b/glib/ghash.c index 71b9fd862..6a79e59bc 100644 --- a/glib/ghash.c +++ b/glib/ghash.c @@ -560,11 +560,18 @@ g_hash_table_remove_node (GHashTable *hash_table, * @hash_table: our #GHashTable * @notify: %TRUE if the destroy notify handlers are to be called * - * Removes all nodes from the table. Since this may be a precursor to - * freeing the table entirely, no resize is performed. + * Removes all nodes from the table. * * If @notify is %TRUE then the destroy notify functions are called * for the key and value of the hash node. + * + * Since this may be a precursor to freeing the table entirely, we'd + * ideally perform no resize, and we can indeed avoid that in some + * cases. However: in the case that we'll be making callbacks to user + * code (via destroy notifies) we need to consider that the user code + * might call back into the table again. In this case, we setup a new + * set of arrays so that any callers will see an empty (but valid) + * table. */ static void g_hash_table_remove_all_nodes (GHashTable *hash_table, @@ -586,6 +593,7 @@ g_hash_table_remove_all_nodes (GHashTable *hash_table, hash_table->nnodes = 0; hash_table->noccupied = 0; + /* Easy case: no callbacks, so we just zero out the arrays */ if (!notify || (hash_table->key_destroy_func == NULL && hash_table->value_destroy_func == NULL)) @@ -606,32 +614,48 @@ g_hash_table_remove_all_nodes (GHashTable *hash_table, return; } - /* Keep the old storage space around to iterate over it. */ + /* Hard case: we need to do user callbacks. There are two + * possibilities here: + * + * 1) there are no outstanding references on the table and therefore + * nobody should be calling into it again (destroying == true) + * + * 2) there are outstanding references, and there may be future + * calls into the table, either after we return, or from the destroy + * notifies that we're about to do (destroying == false) + * + * We handle both cases by taking the current state of the table into + * local variables and replacing it with something else: in the "no + * outstanding references" cases we replace it with a bunch of + * null/zero values so that any access to the table will fail. In the + * "may receive future calls" case, we reinitialise the struct to + * appear like a newly-created empty table. + * + * In both cases, we take over the references for the current state, + * freeing them below. + */ old_size = hash_table->size; old_keys = hash_table->keys; old_values = hash_table->values; old_hashes = hash_table->hashes; - /* Now create a new storage space; If the table is destroyed we can use the - * shortcut of not creating a new storage. This saves the allocation at the - * cost of not allowing any recursive access. - * However, the application doesn't own any reference anymore, so access - * is not allowed. If accesses are done, then either an assert or crash - * *will* happen. */ g_hash_table_set_shift (hash_table, HASH_TABLE_MIN_SHIFT); if (!destruction) + /* Any accesses will see an empty table */ { 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); } else + /* Will cause a quick crash on any attempted access */ { hash_table->keys = NULL; hash_table->values = NULL; hash_table->hashes = NULL; } + /* Now do the actual destroy notifies */ for (i = 0; i < old_size; i++) { if (HASH_IS_REAL (old_hashes[i])) From bef8b10c9648894cd4d38ea9961a76c48447a8c3 Mon Sep 17 00:00:00 2001 From: Allison Karlitskaya Date: Mon, 20 May 2019 16:36:31 +0200 Subject: [PATCH 3/5] ghash: do less work when destroying the table We were calling g_hash_table_set_shift() to reinitialise the hash table even in the case of destroying it. Only do that for the non-destruction case, and fill the relevant fields with zeros for the destruction case. This has a nice side effect of causing more certain crashes in case of invalid reuse of the table after (or during) destruction. --- glib/ghash.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/glib/ghash.c b/glib/ghash.c index 6a79e59bc..bb04fdbc0 100644 --- a/glib/ghash.c +++ b/glib/ghash.c @@ -639,10 +639,10 @@ g_hash_table_remove_all_nodes (GHashTable *hash_table, old_values = hash_table->values; old_hashes = hash_table->hashes; - g_hash_table_set_shift (hash_table, HASH_TABLE_MIN_SHIFT); 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); @@ -650,6 +650,7 @@ g_hash_table_remove_all_nodes (GHashTable *hash_table, else /* Will cause a quick crash on any attempted access */ { + hash_table->size = hash_table->mod = hash_table->mask = 0; hash_table->keys = NULL; hash_table->values = NULL; hash_table->hashes = NULL; From 2b13c3f7974e3933dd12e39b07706188cff71a4e Mon Sep 17 00:00:00 2001 From: Allison Karlitskaya Date: Mon, 20 May 2019 16:39:35 +0200 Subject: [PATCH 4/5] ghash: Be more explicit about memory in g_hash_table_destroy_all_nodes() Make it clear that there is a reference transfer going on here, rather than relying on the fields being overwritten on each branch of the conditional below. --- glib/ghash.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/glib/ghash.c b/glib/ghash.c index bb04fdbc0..82c3365f6 100644 --- a/glib/ghash.c +++ b/glib/ghash.c @@ -635,9 +635,9 @@ g_hash_table_remove_all_nodes (GHashTable *hash_table, * freeing them below. */ old_size = hash_table->size; - old_keys = hash_table->keys; - old_values = hash_table->values; - old_hashes = hash_table->hashes; + 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 */ @@ -649,12 +649,7 @@ g_hash_table_remove_all_nodes (GHashTable *hash_table, } else /* Will cause a quick crash on any attempted access */ - { - hash_table->size = hash_table->mod = hash_table->mask = 0; - hash_table->keys = NULL; - hash_table->values = NULL; - hash_table->hashes = NULL; - } + hash_table->size = hash_table->mod = hash_table->mask = 0; /* Now do the actual destroy notifies */ for (i = 0; i < old_size; i++) From 3bed8a13bc558ced8a518b3610fd5619a7bb75df Mon Sep 17 00:00:00 2001 From: Allison Karlitskaya Date: Mon, 20 May 2019 16:41:51 +0200 Subject: [PATCH 5/5] 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 82c3365f6..c8c488266 100644 --- a/glib/ghash.c +++ b/glib/ghash.c @@ -555,6 +555,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 @@ -585,6 +621,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) @@ -635,18 +673,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; @@ -656,13 +691,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); @@ -672,9 +707,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); @@ -1035,25 +1067,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; @@ -1064,11 +1079,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; }