>From 1192255df58026dc6dea6bc0ad7ee823c16a72ff Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ludovic=20Court=C3=A8s?=
Date: Sun, 22 Oct 2017 16:56:51 -0700
Subject: [PATCH 2/2] Keep weak hash table item count consistent.
Fixes a TOCTTOU kind of bug whereby we'd first count the number of items
deleted from the table, and later, *without* having the alloc lock, we'd
update the table's item count. The problem is that the item count could
have been updated in the meantime, hence the bug.
Fixes .
* libguile/hashtab.c (vacuum_weak_hash_table): Rename to...
(do_vacuum_weak_hash_table): ... this. Unmarshall the void* argument.
Replace 'fprintf' warning with an assertion.
(vacuum_weak_hash_table): New function. Call the above with
'GC_call_with_alloc_lock'.
(t_fixup_args): Add 'table' field; remove 'removed_items'.
(do_weak_bucket_fixup): Update TABLE's 'n_items' field.
(weak_bucket_assoc): Check 'SCM_HASHTABLE_N_ITEMS' instead of
'args.removed_items'.
---
libguile/hashtab.c | 68 +++++++++++++++++++++++++++---------------------------
1 file changed, 34 insertions(+), 34 deletions(-)
diff --git a/libguile/hashtab.c b/libguile/hashtab.c
index 86b9ca386..c06283339 100644
--- a/libguile/hashtab.c
+++ b/libguile/hashtab.c
@@ -96,7 +96,7 @@ static char *s_hashtable = "hashtable";
/* Remove nullified weak pairs from ALIST such that the result contains only
valid pairs. Set REMOVED_ITEMS to the number of pairs that have been
- deleted. */
+ deleted. Assumes the allocation lock is already taken. */
static SCM
scm_fixup_weak_alist (SCM alist, size_t *removed_items)
{
@@ -130,9 +130,10 @@ scm_fixup_weak_alist (SCM alist, size_t *removed_items)
return result;
}
-static void
-vacuum_weak_hash_table (SCM table)
+static void *
+do_vacuum_weak_hash_table (void *arg)
{
+ SCM table = SCM_PACK_POINTER (arg);
SCM buckets = SCM_HASHTABLE_VECTOR (table);
unsigned long k = SCM_SIMPLE_VECTOR_LENGTH (buckets);
size_t len = SCM_HASHTABLE_N_ITEMS (table);
@@ -142,44 +143,52 @@ vacuum_weak_hash_table (SCM table)
size_t removed;
SCM alist = SCM_SIMPLE_VECTOR_REF (buckets, k);
alist = scm_fixup_weak_alist (alist, &removed);
- if (removed <= len)
- len -= removed;
- else
- {
- /* The move to BDW-GC with Guile 2.0 introduced some bugs
- related to weak hash tables, threads, memory usage, and the
- alloc lock. We were unable to fix these issues
- satisfactorily in 2.0 but have addressed them via a rewrite
- in 2.2. If you see this message often, you probably want
- to upgrade to 2.2. */
- fprintf (stderr, "guile: warning: weak hash table corruption "
- "(https://bugs.gnu.org/19180)\n");
- len = 0;
- }
+
+ /* The alloc lock is taken, so we cannot get REMOVED > LEN. If we
+ do, that means we messed up while counting items. */
+ assert (removed <= len);
+
SCM_SIMPLE_VECTOR_SET (buckets, k, alist);
}
SCM_SET_HASHTABLE_N_ITEMS (table, len);
+
+ return table;
+}
+
+/* Remove deleted weak pairs from the buckets of TABLE, and update
+ TABLE's item count accordingly. */
+static void
+vacuum_weak_hash_table (SCM table)
+{
+ /* Take the alloc lock so we have a consistent view of the live
+ elements in TABLE. Failing to do that, we could be miscounting the
+ number of elements. */
+ GC_call_with_alloc_lock (do_vacuum_weak_hash_table,
+ SCM_PACK (table));
}
+
/* Packed arguments for `do_weak_bucket_fixup'. */
struct t_fixup_args
{
+ SCM table;
SCM bucket;
SCM *bucket_copy;
- size_t removed_items;
};
static void *
do_weak_bucket_fixup (void *data)
{
- struct t_fixup_args *args;
SCM pair, *copy;
+ size_t len, removed_items;
+ struct t_fixup_args *args = (struct t_fixup_args *) data;
- args = (struct t_fixup_args *) data;
+ args->bucket = scm_fixup_weak_alist (args->bucket, &removed_items);
- args->bucket = scm_fixup_weak_alist (args->bucket, &args->removed_items);
+ len = SCM_HASHTABLE_N_ITEMS (args->table);
+ SCM_SET_HASHTABLE_N_ITEMS (args->table, len - removed_items);
for (pair = args->bucket, copy = args->bucket_copy;
scm_is_pair (pair);
@@ -214,6 +223,7 @@ weak_bucket_assoc (SCM table, SCM buckets, size_t bucket_index,
and values in BUCKET. */
strong_refs = alloca (scm_ilength (bucket) * 2 * sizeof (SCM));
+ args.table = table;
args.bucket = bucket;
args.bucket_copy = strong_refs;
@@ -239,19 +249,9 @@ weak_bucket_assoc (SCM table, SCM buckets, size_t bucket_index,
scm_remember_upto_here_1 (strong_refs);
- if (args.removed_items > 0)
- {
- /* Update TABLE's item count and optionally trigger a rehash. */
- size_t remaining;
-
- assert (SCM_HASHTABLE_N_ITEMS (table) >= args.removed_items);
-
- remaining = SCM_HASHTABLE_N_ITEMS (table) - args.removed_items;
- SCM_SET_HASHTABLE_N_ITEMS (table, remaining);
-
- if (remaining < SCM_HASHTABLE_LOWER (table))
- scm_i_rehash (table, hash_fn, closure, "weak_bucket_assoc");
- }
+ if (SCM_HASHTABLE_N_ITEMS (table) < SCM_HASHTABLE_LOWER (table))
+ /* Trigger a rehash. */
+ scm_i_rehash (table, hash_fn, closure, "weak_bucket_assoc");
return result;
}
--
2.14.2