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

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

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