fix: dev: Fix the assertion failure in the isc_hashmap iterator

When the round robin hashing reorders the map entries on deletion, we
were adjusting the iterator table size only when the reordering was
happening at the internal table boundary.  The iterator table size had
to be reduced by one to prevent seeing the entry that resized on
position [0] twice because it migrated to [iter->size - 1] position.

However, the same thing could happen when the same entry migrates a
second time from [iter->size - 1] to [iter->size - 2] position (and so
on) because the check that we are manipulating the entry just in the [0]
position was insufficient.  Instead of checking the position [pos == 0],
we now check that the [pos % iter->size == 0], thus ignoring all the
entries that might have moved back to the end of the internal table.

Closes #4838

Merge branch '4838-fix-assertion-failure-in-hashmap-deletion-iterator' into 'main'

Closes #4838

See merge request isc-projects/bind9!9292

lib/isc/hashmap.c

@@ -310,7 +310,8 @@ isc_hashmap_find(const isc_hashmap_t *hashmap, const uint32_t hashval,
 
 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 hashval, uint32_t psl, const uint8_t idx,
+		    size_t size) {
 	uint32_t pos;
 	uint32_t hash;
 	bool last = false;
@@ -318,7 +319,7 @@ hashmap_delete_node(isc_hashmap_t *hashmap, hashmap_node_t *entry,
 	hashmap->count--;
 
 	hash = isc_hash_bits32(hashval, hashmap->tables[idx].hashbits);
-	pos = hash + psl;
+	pos = (hash + psl) & hashmap->tables[idx].hashmask;
 
 	while (true) {
 		hashmap_node_t *node = NULL;
@@ -332,7 +333,7 @@ hashmap_delete_node(isc_hashmap_t *hashmap, hashmap_node_t *entry,
 			break;
 		}
 
-		if (pos == 0) {
+		if ((pos % size) == 0) {
 			last = true;
 		}
 
@@ -373,7 +374,7 @@ hashmap_rehash_one(isc_hashmap_t *hashmap) {
 	node = oldtable[hashmap->hiter];
 
 	(void)hashmap_delete_node(hashmap, &oldtable[hashmap->hiter],
-				  node.hashval, node.psl, oldidx);
+				  node.hashval, node.psl, oldidx, UINT32_MAX);
 
 	isc_result_t result = hashmap_add(hashmap, node.hashval, NULL, node.key,
 					  node.value, NULL, hashmap->hindex);
@@ -470,7 +471,8 @@ 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);
-		(void)hashmap_delete_node(hashmap, node, hashval, psl, idx);
+		(void)hashmap_delete_node(hashmap, node, hashval, psl, idx,
+					  UINT32_MAX);
 		result = ISC_R_SUCCESS;
 	}
 
@@ -644,7 +646,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->i = hashmap->hiter;
 		iter->size = hashmap->tables[iter->hindex].size;
 		return (isc__hashmap_iter_next(iter));
 	}
@@ -682,7 +684,7 @@ isc_hashmap_iter_delcurrent_next(isc_hashmap_iter_t *iter) {
 		&iter->hashmap->tables[iter->hindex].table[iter->i];
 
 	if (hashmap_delete_node(iter->hashmap, node, node->hashval, node->psl,
-				iter->hindex))
+				iter->hindex, iter->size))
 	{
 		/*
 		 * We have seen the new last element so reduce the size

tests/isc/Makefile.am

@@ -87,6 +87,10 @@ dnsstream_utils_test_SOURCES =	\
 	dnsstream_utils_test.c \
 	dnsstream_utils_test_data.h
 
+hashmap_test_SOURCES =	\
+	hashmap_test.c	\
+	hashmap_nodes.h
+
 hmac_test_CPPFLAGS =	\
 	$(AM_CPPFLAGS)	\
 	$(OPENSSL_CFLAGS)

tests/isc/hashmap_test.c

@@ -216,8 +216,10 @@ test_hashmap_full(uint8_t init_bits, uintptr_t count) {
 	isc_mem_cput(mctx, upper_nodes, count, sizeof(nodes[0]));
 }
 
+#include "hashmap_nodes.h"
+
 static void
-test_hashmap_iterator(void) {
+test_hashmap_iterator(bool random_data) {
 	isc_hashmap_t *hashmap = NULL;
 	isc_result_t result;
 	isc_hashmap_iter_t *iter = NULL;
@@ -231,11 +233,17 @@ test_hashmap_iterator(void) {
 	isc_hashmap_create(mctx, HASHMAP_MIN_BITS, &hashmap);
 	assert_non_null(hashmap);
 
-	for (size_t i = 0; i < count; i++) {
-		/* short keys */
-		snprintf((char *)nodes[i].key, 16, "%u", (unsigned int)i);
-		strlcat((char *)nodes[i].key, " key of a raw hashmap!!", 16);
-		nodes[i].hashval = isc_hash32(nodes[i].key, 16, true);
+	if (random_data) {
+		for (size_t i = 0; i < count; i++) {
+			/* short keys */
+			snprintf((char *)nodes[i].key, 16, "%u",
+				 (unsigned int)i);
+			strlcat((char *)nodes[i].key, " key of a raw hashmap!!",
+				16);
+			nodes[i].hashval = isc_hash32(nodes[i].key, 16, true);
+		}
+	} else {
+		memmove(nodes, test_nodes, sizeof(test_nodes));
 	}
 
 	for (size_t i = 0; i < count; i++) {
@@ -387,7 +395,12 @@ ISC_RUN_TEST_IMPL(isc_hashmap_8_20000) {
 /* test hashmap iterator */
 
 ISC_RUN_TEST_IMPL(isc_hashmap_iterator) {
-	test_hashmap_iterator();
+	test_hashmap_iterator(true);
+	return;
+}
+
+ISC_RUN_TEST_IMPL(isc_hashmap_iterator_static) {
+	test_hashmap_iterator(false);
 	return;
 }
 
@@ -500,6 +513,7 @@ ISC_TEST_ENTRY(isc_hashmap_24_200000)
 ISC_TEST_ENTRY(isc_hashmap_1_48000)
 ISC_TEST_ENTRY(isc_hashmap_8_20000)
 ISC_TEST_ENTRY(isc_hashmap_iterator)
+ISC_TEST_ENTRY(isc_hashmap_iterator_static)
 ISC_TEST_LIST_END
 
 ISC_TEST_MAIN