>From cc3d1bd7416552d33fa555241d16109b39a2ccd6 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 f00da1fd9..fb9ee6fdf 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