From 92a0d65a518c037f1ed4eb2dc6d0946900914983 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Tue, 19 Sep 2023 11:42:03 +1000 Subject: [PATCH 1/3] Fix hashmap iteration When isc_hashmap_iter_delcurrent_next calls hashmap_delete_node nodes from the front of the table could be added to the end of the table resulting in them being returned twice. Detect when this is happening and prevent those nodes being returned twice buy reducing the effective size of the table by one each time it happens. --- lib/isc/hashmap.c | 33 +++++++++++++++++++++++++-------- tests/isc/hashmap_test.c | 32 ++++++++++++++++++-------------- 2 files changed, 43 insertions(+), 22 deletions(-) diff --git a/lib/isc/hashmap.c b/lib/isc/hashmap.c index 3316bac6c3..a8e635a969 100644 --- a/lib/isc/hashmap.c +++ b/lib/isc/hashmap.c @@ -92,6 +92,7 @@ struct isc_hashmap { struct isc_hashmap_iter { isc_hashmap_t *hashmap; size_t i; + size_t size; uint8_t hindex; hashmap_node_t *cur; }; @@ -305,11 +306,12 @@ isc_hashmap_find(const isc_hashmap_t *hashmap, const uint32_t hashval, return (ISC_R_SUCCESS); } -static void +static bool hashmap_delete_node(isc_hashmap_t *hashmap, hashmap_node_t *entry, uint32_t hashval, uint32_t psl, const uint8_t idx) { uint32_t pos; uint32_t hash; + bool last = false; hashmap->count--; @@ -328,12 +330,17 @@ hashmap_delete_node(isc_hashmap_t *hashmap, hashmap_node_t *entry, break; } + if (pos == 0) { + last = true; + } + node->psl--; *entry = *node; entry = &hashmap->tables[idx].table[pos]; } *entry = (hashmap_node_t){ 0 }; + return (last); } static void @@ -360,8 +367,8 @@ hashmap_rehash_one(isc_hashmap_t *hashmap) { /* Move the first non-empty node from old table to new table */ node = oldtable[hashmap->hiter]; - hashmap_delete_node(hashmap, &oldtable[hashmap->hiter], node.hashval, - node.psl, oldidx); + (void)hashmap_delete_node(hashmap, &oldtable[hashmap->hiter], + node.hashval, node.psl, oldidx); isc_result_t result = hashmap_add(hashmap, node.hashval, NULL, node.key, node.value, NULL, hashmap->hindex); @@ -458,7 +465,7 @@ isc_hashmap_delete(isc_hashmap_t *hashmap, const uint32_t hashval, node = hashmap_find(hashmap, hashval, match, key, &psl, &idx); if (node != NULL) { INSIST(node->key != NULL); - hashmap_delete_node(hashmap, node, hashval, psl, idx); + (void)hashmap_delete_node(hashmap, node, hashval, psl, idx); result = ISC_R_SUCCESS; } @@ -612,13 +619,13 @@ static isc_result_t isc__hashmap_iter_next(isc_hashmap_iter_t *iter) { isc_hashmap_t *hashmap = iter->hashmap; - while (iter->i < hashmap->tables[iter->hindex].size && + while (iter->i < iter->size && hashmap->tables[iter->hindex].table[iter->i].key == NULL) { iter->i++; } - if (iter->i < hashmap->tables[iter->hindex].size) { + if (iter->i < iter->size) { iter->cur = &hashmap->tables[iter->hindex].table[iter->i]; return (ISC_R_SUCCESS); @@ -627,6 +634,7 @@ isc__hashmap_iter_next(isc_hashmap_iter_t *iter) { if (try_nexttable(hashmap, iter->hindex)) { iter->hindex = hashmap_nexttable(iter->hindex); iter->i = 0; + iter->size = hashmap->tables[iter->hindex].size; return (isc__hashmap_iter_next(iter)); } @@ -639,6 +647,7 @@ isc_hashmap_iter_first(isc_hashmap_iter_t *iter) { iter->hindex = iter->hashmap->hindex; iter->i = 0; + iter->size = iter->hashmap->tables[iter->hashmap->hindex].size; return (isc__hashmap_iter_next(iter)); } @@ -661,8 +670,16 @@ isc_hashmap_iter_delcurrent_next(isc_hashmap_iter_t *iter) { hashmap_node_t *node = &iter->hashmap->tables[iter->hindex].table[iter->i]; - hashmap_delete_node(iter->hashmap, node, node->hashval, node->psl, - iter->hindex); + if (hashmap_delete_node(iter->hashmap, node, node->hashval, node->psl, + iter->hindex)) + { + /* + * We have seen the new last element so reduce the size + * so we don't iterate over it twice. + */ + INSIST(iter->size != 0); + iter->size--; + } return (isc__hashmap_iter_next(iter)); } diff --git a/tests/isc/hashmap_test.c b/tests/isc/hashmap_test.c index 084b84b964..754478f968 100644 --- a/tests/isc/hashmap_test.c +++ b/tests/isc/hashmap_test.c @@ -222,10 +222,11 @@ test_hashmap_iterator(void) { isc_result_t result; isc_hashmap_iter_t *iter = NULL; size_t count = 7600; - uint32_t walked; test_node_t *nodes; + bool *seen; nodes = isc_mem_cget(mctx, count, sizeof(nodes[0])); + seen = isc_mem_cget(mctx, count, sizeof(seen[0])); isc_hashmap_create(mctx, HASHMAP_MIN_BITS, &hashmap); assert_non_null(hashmap); @@ -248,7 +249,7 @@ test_hashmap_iterator(void) { /* We want to iterate while rehashing is in progress */ assert_true(rehashing_in_progress(hashmap)); - walked = 0; + memset(seen, 0, count * sizeof(seen[0])); isc_hashmap_iter_create(hashmap, &iter); for (result = isc_hashmap_iter_first(iter); result == ISC_R_SUCCESS; @@ -269,13 +270,16 @@ test_hashmap_iterator(void) { assert_memory_equal(key, tkey, 16); - walked++; + assert_false(seen[i]); + seen[i] = true; } - assert_int_equal(walked, count); assert_int_equal(result, ISC_R_NOMORE); + for (size_t i = 0; i < count; i++) { + assert_true(seen[i]); + } /* erase odd */ - walked = 0; + memset(seen, 0, count * sizeof(seen[0])); result = isc_hashmap_iter_first(iter); while (result == ISC_R_SUCCESS) { char key[16] = { 0 }; @@ -296,13 +300,17 @@ test_hashmap_iterator(void) { } else { result = isc_hashmap_iter_next(iter); } - walked++; + + assert_false(seen[i]); + seen[i] = true; } assert_int_equal(result, ISC_R_NOMORE); - assert_int_equal(walked, count); + for (size_t i = 0; i < count; i++) { + assert_true(seen[i]); + } /* erase even */ - walked = 0; + memset(seen, 0, count * sizeof(seen[0])); result = isc_hashmap_iter_first(iter); while (result == ISC_R_SUCCESS) { char key[16] = { 0 }; @@ -323,20 +331,15 @@ test_hashmap_iterator(void) { } else { result = isc_hashmap_iter_next(iter); } - walked++; } assert_int_equal(result, ISC_R_NOMORE); - assert_int_equal(walked, count / 2); - walked = 0; for (result = isc_hashmap_iter_first(iter); result == ISC_R_SUCCESS; result = isc_hashmap_iter_next(iter)) { - walked++; + assert_true(false); } - assert_int_equal(result, ISC_R_NOMORE); - assert_int_equal(walked, 0); /* Iterator doesn't progress rehashing */ assert_true(rehashing_in_progress(hashmap)); @@ -347,6 +350,7 @@ test_hashmap_iterator(void) { isc_hashmap_destroy(&hashmap); assert_null(hashmap); + isc_mem_cput(mctx, seen, count, sizeof(seen[0])); isc_mem_cput(mctx, nodes, count, sizeof(nodes[0])); } From 45fb84076dff6144e00e47baa3bbeeec79e7c466 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Sat, 16 Sep 2023 08:32:54 +0200 Subject: [PATCH 2/3] Add assertion failure when adding to hashmap when iterating When iterating the table, we can't add new nodes to the hashmap because we can't assure that we are not adding the new node before the iterator. This also applies to rehashing - which might be triggered by both isc_hashmap_add() and isc_hashmap_delete(), but not isc_hashmap_iter_delcurrent_next(). --- lib/isc/hashmap.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lib/isc/hashmap.c b/lib/isc/hashmap.c index a8e635a969..1fa5460770 100644 --- a/lib/isc/hashmap.c +++ b/lib/isc/hashmap.c @@ -87,6 +87,7 @@ struct isc_hashmap { isc_mem_t *mctx; size_t count; hashmap_table_t tables[HASHMAP_NUM_TABLES]; + uint_fast32_t iterators; }; struct isc_hashmap_iter { @@ -350,6 +351,9 @@ hashmap_rehash_one(isc_hashmap_t *hashmap) { hashmap_node_t *oldtable = hashmap->tables[oldidx].table; hashmap_node_t node; + /* Don't rehash when iterating */ + INSIST(hashmap->iterators == 0); + /* Find first non-empty node */ while (hashmap->hiter < oldsize && oldtable[hashmap->hiter].key == NULL) { @@ -502,6 +506,8 @@ hashmap_add(isc_hashmap_t *hashmap, const uint32_t hashval, hashmap_node_t *current = NULL; uint32_t pos; + INSIST(hashmap->iterators == 0); + hash = isc_hash_bits32(hashval, hashmap->tables[idx].hashbits); /* Initialize the node to be store to 'node' */ @@ -599,6 +605,8 @@ isc_hashmap_iter_create(isc_hashmap_t *hashmap, isc_hashmap_iter_t **iterp) { .hindex = hashmap->hindex, }; + hashmap->iterators++; + *iterp = iter; } @@ -613,6 +621,9 @@ isc_hashmap_iter_destroy(isc_hashmap_iter_t **iterp) { *iterp = NULL; hashmap = iter->hashmap; isc_mem_put(hashmap->mctx, iter, sizeof(*iter)); + + INSIST(hashmap->iterators > 0); + hashmap->iterators--; } static isc_result_t From f467dbc182696eb2f29a8354cfff3cea5621130e Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Tue, 19 Sep 2023 11:49:47 +1000 Subject: [PATCH 3/3] Add CHANGES note for [GL #3422] --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES b/CHANGES index 98b8314ab2..9497883e3f 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +6251. [bug] Interating a hashmap could return the same element + twice. [GL #3422] + 6250. [bug] The wrong covered value was being set by dns_ncache_current for RRSIG records in the returned rdataset structure. This resulted in TYPE0 being