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 diff --git a/lib/isc/hashmap.c b/lib/isc/hashmap.c index 3316bac6c3..1fa5460770 100644 --- a/lib/isc/hashmap.c +++ b/lib/isc/hashmap.c @@ -87,11 +87,13 @@ 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 { isc_hashmap_t *hashmap; size_t i; + size_t size; uint8_t hindex; hashmap_node_t *cur; }; @@ -305,11 +307,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 +331,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 @@ -343,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) { @@ -360,8 +371,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 +469,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; } @@ -495,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' */ @@ -592,6 +605,8 @@ isc_hashmap_iter_create(isc_hashmap_t *hashmap, isc_hashmap_iter_t **iterp) { .hindex = hashmap->hindex, }; + hashmap->iterators++; + *iterp = iter; } @@ -606,19 +621,22 @@ 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 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 +645,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 +658,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 +681,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])); }