From 92a0d65a518c037f1ed4eb2dc6d0946900914983 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Tue, 19 Sep 2023 11:42:03 +1000 Subject: [PATCH] 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])); }