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.

diff --git a/lib/isc/hashmap.c b/lib/isc/hashmap.c
index 3316bac6c348cdf9cea5698fade7101b5fed484a..a8e635a96953340fab01f717d580a5644a75e212 100644 (file)
--- 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 084b84b964dc1f5a6a32cbd57cccaa8b56f819c5..754478f968ff058744799eb9b1950174cbd59ecd 100644 (file)
--- 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]));
 }