Merge branch '4322-hashmap-iterator-can-iterate-twice-the-same-item-if-deleting-items-via-delcurrent_next' into 'main'

Resolve "hashmap iterator can iterate twice the same item if deleting items via delcurrent_next"

Closes #4322

See merge request isc-projects/bind9!8309

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

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));
 }

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]));
 }