Address PR comments

Signed-off-by: Uri Yagelnik <[email protected]>

Author: uriyage

src/hashtable.c

@@ -46,6 +46,7 @@
  */
 #include "hashtable.h"
 #include "serverassert.h"
+#include "server.h"
 #include "zmalloc.h"
 #include "mt19937-64.h"
 #include "monotonic.h"
@@ -148,8 +149,6 @@ void hashtableSetResizePolicy(hashtableResizePolicy policy) {
 #define ENTRIES_PER_BUCKET 7
 #define BUCKET_BITS_TYPE uint8_t
 #define BITS_NEEDED_TO_STORE_POS_WITHIN_BUCKET 3
-/* Iterator table value indicating iteration is complete */
-#define HASHTABLE_ITER_END_TABLE 2
 
 /* Selecting the number of buckets.
  *
@@ -1936,6 +1935,11 @@ size_t hashtableScanDefrag(hashtable *ht, size_t cursor, hashtableScanFunction f
 
 /* --- Iterator --- */
 
+#define HASHTABLE_ITER_UNINITIALIZED -1
+#define HASHTABLE_ITER_PRIMARY_TABLE 0
+#define HASHTABLE_ITER_REHASH_TABLE 1
+#define HASHTABLE_ITER_FINISHED 2
+
 /* Initialize an iterator for a hashtable.
  *
  * The 'flags' argument can be used to tweak the behaviour. It's a bitwise-or
@@ -1978,11 +1982,12 @@ size_t hashtableScanDefrag(hashtable *ht, size_t cursor, hashtableScanFunction f
  * Call hashtableNext to fetch each entry. You must call hashtableResetIterator
  * when you are done with the iterator.
  */
+
 void hashtableInitIterator(hashtableIterator *iterator, hashtable *ht, uint8_t flags) {
     iter *iter;
     iter = iteratorFromOpaque(iterator);
     iter->hashtable = ht;
-    iter->table = 0;
+    iter->table = HASHTABLE_ITER_PRIMARY_TABLE;
     iter->index = -1;
     iter->flags = flags;
 }
@@ -1997,7 +2002,7 @@ void hashtableReinitIterator(hashtableIterator *iterator, hashtable *ht) {
 /* Resets a stack-allocated iterator. */
 void hashtableResetIterator(hashtableIterator *iterator) {
     iter *iter = iteratorFromOpaque(iterator);
-    if (!(iter->index == -1 && iter->table == 0)) {
+    if (!(iter->index == -1 && iter->table == HASHTABLE_ITER_PRIMARY_TABLE)) {
         if (isSafe(iter)) {
             hashtableResumeRehashing(iter->hashtable);
             assert(iter->hashtable->pause_rehash >= 0);
@@ -2028,13 +2033,19 @@ void hashtableReleaseIterator(hashtableIterator *iterator) {
 bool hashtableNext(hashtableIterator *iterator, void **elemptr) {
     iter *iter = iteratorFromOpaque(iterator);
 
-    assert(iter->table <= HASHTABLE_ITER_END_TABLE);
-    if (iter->table == HASHTABLE_ITER_END_TABLE) {
+    if (iter->table == HASHTABLE_ITER_FINISHED) {
         return false;
     }
 
+    /* Check for unexpected iterator states that indicate bugs */
+    debugServerAssert(iter->table < HASHTABLE_ITER_FINISHED &&
+                      (iter->table != HASHTABLE_ITER_REHASH_TABLE || hashtableIsRehashing(iter->hashtable)) &&
+                      (iter->index < 0 || !iter->hashtable->tables[iter->table] ||
+                       (size_t)iter->index < numBuckets(iter->hashtable->bucket_exp[iter->table])));
+
+
     while (1) {
-        if (iter->index == -1 && iter->table == 0) {
+        if (iter->index == -1 && iter->table == HASHTABLE_ITER_PRIMARY_TABLE) {
             /* It's the first call to next. */
             if (isSafe(iter)) {
                 hashtablePauseRehashing(iter->hashtable);
@@ -2079,9 +2090,9 @@ bool hashtableNext(hashtableIterator *iterator, void **elemptr) {
                 iter->pos_in_bucket = 0;
                 iter->index++;
                 if ((size_t)iter->index >= numBuckets(iter->hashtable->bucket_exp[iter->table])) {
-                    if (hashtableIsRehashing(iter->hashtable) && iter->table == 0) {
+                    if (hashtableIsRehashing(iter->hashtable) && iter->table == HASHTABLE_ITER_PRIMARY_TABLE) {
                         iter->index = 0;
-                        iter->table++;
+                        iter->table = HASHTABLE_ITER_REHASH_TABLE;
                     } else {
                         /* Done. */
                         break;
@@ -2110,7 +2121,7 @@ bool hashtableNext(hashtableIterator *iterator, void **elemptr) {
         }
         return true;
     }
-    iter->table = HASHTABLE_ITER_END_TABLE;
+    iter->table = HASHTABLE_ITER_FINISHED;
     return false;
 }
 

src/unit/test_files.h

@@ -41,6 +41,7 @@ int test_compact_bucket_chain(int argc, char **argv, int flags);
 int test_random_entry(int argc, char **argv, int flags);
 int test_random_entry_with_long_chain(int argc, char **argv, int flags);
 int test_random_entry_sparse_table(int argc, char **argv, int flags);
+int test_iterator_bounds_check(int argc, char **argv, int flags);
 int test_intsetValueEncodings(int argc, char **argv, int flags);
 int test_intsetBasicAdding(int argc, char **argv, int flags);
 int test_intsetLargeNumberRandomAdd(int argc, char **argv, int flags);
@@ -258,7 +259,7 @@ unitTest __test_crc64combine_c[] = {{"test_crc64combine", test_crc64combine}, {N
 unitTest __test_dict_c[] = {{"test_dictCreate", test_dictCreate}, {"test_dictAdd16Keys", test_dictAdd16Keys}, {"test_dictDisableResize", test_dictDisableResize}, {"test_dictAddOneKeyTriggerResize", test_dictAddOneKeyTriggerResize}, {"test_dictDeleteKeys", test_dictDeleteKeys}, {"test_dictDeleteOneKeyTriggerResize", test_dictDeleteOneKeyTriggerResize}, {"test_dictEmptyDirAdd128Keys", test_dictEmptyDirAdd128Keys}, {"test_dictDisableResizeReduceTo3", test_dictDisableResizeReduceTo3}, {"test_dictDeleteOneKeyTriggerResizeAgain", test_dictDeleteOneKeyTriggerResizeAgain}, {"test_dictBenchmark", test_dictBenchmark}, {NULL, NULL}};
 unitTest __test_endianconv_c[] = {{"test_endianconv", test_endianconv}, {NULL, NULL}};
 unitTest __test_entry_c[] = {{"test_entryCreate", test_entryCreate}, {"test_entryUpdate", test_entryUpdate}, {"test_entryHasexpiry_entrySetExpiry", test_entryHasexpiry_entrySetExpiry}, {"test_entryIsExpired", test_entryIsExpired}, {"test_entryMemUsage_entrySetExpiry_entrySetValue", test_entryMemUsage_entrySetExpiry_entrySetValue}, {NULL, NULL}};
-unitTest __test_hashtable_c[] = {{"test_cursor", test_cursor}, {"test_set_hash_function_seed", test_set_hash_function_seed}, {"test_add_find_delete", test_add_find_delete}, {"test_add_find_delete_avoid_resize", test_add_find_delete_avoid_resize}, {"test_instant_rehashing", test_instant_rehashing}, {"test_bucket_chain_length", test_bucket_chain_length}, {"test_two_phase_insert_and_pop", test_two_phase_insert_and_pop}, {"test_replace_reallocated_entry", test_replace_reallocated_entry}, {"test_incremental_find", test_incremental_find}, {"test_scan", test_scan}, {"test_iterator", test_iterator}, {"test_safe_iterator", test_safe_iterator}, {"test_compact_bucket_chain", test_compact_bucket_chain}, {"test_random_entry", test_random_entry}, {"test_random_entry_with_long_chain", test_random_entry_with_long_chain}, {"test_random_entry_sparse_table", test_random_entry_sparse_table}, {NULL, NULL}};
+unitTest __test_hashtable_c[] = {{"test_cursor", test_cursor}, {"test_set_hash_function_seed", test_set_hash_function_seed}, {"test_add_find_delete", test_add_find_delete}, {"test_add_find_delete_avoid_resize", test_add_find_delete_avoid_resize}, {"test_instant_rehashing", test_instant_rehashing}, {"test_bucket_chain_length", test_bucket_chain_length}, {"test_two_phase_insert_and_pop", test_two_phase_insert_and_pop}, {"test_replace_reallocated_entry", test_replace_reallocated_entry}, {"test_incremental_find", test_incremental_find}, {"test_scan", test_scan}, {"test_iterator", test_iterator}, {"test_safe_iterator", test_safe_iterator}, {"test_compact_bucket_chain", test_compact_bucket_chain}, {"test_random_entry", test_random_entry}, {"test_random_entry_with_long_chain", test_random_entry_with_long_chain}, {"test_random_entry_sparse_table", test_random_entry_sparse_table}, {"test_iterator_bounds_check", test_iterator_bounds_check}, {NULL, NULL}};
 unitTest __test_intset_c[] = {{"test_intsetValueEncodings", test_intsetValueEncodings}, {"test_intsetBasicAdding", test_intsetBasicAdding}, {"test_intsetLargeNumberRandomAdd", test_intsetLargeNumberRandomAdd}, {"test_intsetUpgradeFromint16Toint32", test_intsetUpgradeFromint16Toint32}, {"test_intsetUpgradeFromint16Toint64", test_intsetUpgradeFromint16Toint64}, {"test_intsetUpgradeFromint32Toint64", test_intsetUpgradeFromint32Toint64}, {"test_intsetStressLookups", test_intsetStressLookups}, {"test_intsetStressAddDelete", test_intsetStressAddDelete}, {NULL, NULL}};
 unitTest __test_kvstore_c[] = {{"test_kvstoreAdd16Keys", test_kvstoreAdd16Keys}, {"test_kvstoreIteratorRemoveAllKeysNoDeleteEmptyHashtable", test_kvstoreIteratorRemoveAllKeysNoDeleteEmptyHashtable}, {"test_kvstoreIteratorRemoveAllKeysDeleteEmptyHashtable", test_kvstoreIteratorRemoveAllKeysDeleteEmptyHashtable}, {"test_kvstoreHashtableIteratorRemoveAllKeysNoDeleteEmptyHashtable", test_kvstoreHashtableIteratorRemoveAllKeysNoDeleteEmptyHashtable}, {"test_kvstoreHashtableIteratorRemoveAllKeysDeleteEmptyHashtable", test_kvstoreHashtableIteratorRemoveAllKeysDeleteEmptyHashtable}, {"test_kvstoreHashtableExpand", test_kvstoreHashtableExpand}, {NULL, NULL}};
 unitTest __test_listpack_c[] = {{"test_listpackCreateIntList", test_listpackCreateIntList}, {"test_listpackCreateList", test_listpackCreateList}, {"test_listpackLpPrepend", test_listpackLpPrepend}, {"test_listpackLpPrependInteger", test_listpackLpPrependInteger}, {"test_listpackGetELementAtIndex", test_listpackGetELementAtIndex}, {"test_listpackPop", test_listpackPop}, {"test_listpackGetELementAtIndex2", test_listpackGetELementAtIndex2}, {"test_listpackIterate0toEnd", test_listpackIterate0toEnd}, {"test_listpackIterate1toEnd", test_listpackIterate1toEnd}, {"test_listpackIterate2toEnd", test_listpackIterate2toEnd}, {"test_listpackIterateBackToFront", test_listpackIterateBackToFront}, {"test_listpackIterateBackToFrontWithDelete", test_listpackIterateBackToFrontWithDelete}, {"test_listpackDeleteWhenNumIsMinusOne", test_listpackDeleteWhenNumIsMinusOne}, {"test_listpackDeleteWithNegativeIndex", test_listpackDeleteWithNegativeIndex}, {"test_listpackDeleteInclusiveRange0_0", test_listpackDeleteInclusiveRange0_0}, {"test_listpackDeleteInclusiveRange0_1", test_listpackDeleteInclusiveRange0_1}, {"test_listpackDeleteInclusiveRange1_2", test_listpackDeleteInclusiveRange1_2}, {"test_listpackDeleteWitStartIndexOutOfRange", test_listpackDeleteWitStartIndexOutOfRange}, {"test_listpackDeleteWitNumOverflow", test_listpackDeleteWitNumOverflow}, {"test_listpackBatchDelete", test_listpackBatchDelete}, {"test_listpackDeleteFooWhileIterating", test_listpackDeleteFooWhileIterating}, {"test_listpackReplaceWithSameSize", test_listpackReplaceWithSameSize}, {"test_listpackReplaceWithDifferentSize", test_listpackReplaceWithDifferentSize}, {"test_listpackRegressionGt255Bytes", test_listpackRegressionGt255Bytes}, {"test_listpackCreateLongListAndCheckIndices", test_listpackCreateLongListAndCheckIndices}, {"test_listpackCompareStrsWithLpEntries", test_listpackCompareStrsWithLpEntries}, {"test_listpackLpMergeEmptyLps", test_listpackLpMergeEmptyLps}, {"test_listpackLpMergeLp1Larger", test_listpackLpMergeLp1Larger}, {"test_listpackLpMergeLp2Larger", test_listpackLpMergeLp2Larger}, {"test_listpackLpNextRandom", test_listpackLpNextRandom}, {"test_listpackLpNextRandomCC", test_listpackLpNextRandomCC}, {"test_listpackRandomPairWithOneElement", test_listpackRandomPairWithOneElement}, {"test_listpackRandomPairWithManyElements", test_listpackRandomPairWithManyElements}, {"test_listpackRandomPairsWithOneElement", test_listpackRandomPairsWithOneElement}, {"test_listpackRandomPairsWithManyElements", test_listpackRandomPairsWithManyElements}, {"test_listpackRandomPairsUniqueWithOneElement", test_listpackRandomPairsUniqueWithOneElement}, {"test_listpackRandomPairsUniqueWithManyElements", test_listpackRandomPairsUniqueWithManyElements}, {"test_listpackPushVariousEncodings", test_listpackPushVariousEncodings}, {"test_listpackLpFind", test_listpackLpFind}, {"test_listpackLpValidateIntegrity", test_listpackLpValidateIntegrity}, {"test_listpackNumberOfElementsExceedsLP_HDR_NUMELE_UNKNOWN", test_listpackNumberOfElementsExceedsLP_HDR_NUMELE_UNKNOWN}, {"test_listpackStressWithRandom", test_listpackStressWithRandom}, {"test_listpackSTressWithVariableSize", test_listpackSTressWithVariableSize}, {"test_listpackBenchmarkInit", test_listpackBenchmarkInit}, {"test_listpackBenchmarkLpAppend", test_listpackBenchmarkLpAppend}, {"test_listpackBenchmarkLpFindString", test_listpackBenchmarkLpFindString}, {"test_listpackBenchmarkLpFindNumber", test_listpackBenchmarkLpFindNumber}, {"test_listpackBenchmarkLpSeek", test_listpackBenchmarkLpSeek}, {"test_listpackBenchmarkLpValidateIntegrity", test_listpackBenchmarkLpValidateIntegrity}, {"test_listpackBenchmarkLpCompareWithString", test_listpackBenchmarkLpCompareWithString}, {"test_listpackBenchmarkLpCompareWithNumber", test_listpackBenchmarkLpCompareWithNumber}, {"test_listpackBenchmarkFree", test_listpackBenchmarkFree}, {NULL, NULL}};

src/unit/test_hashtable.c

@@ -945,3 +945,32 @@ int test_random_entry_sparse_table(int argc, char **argv, int flags) {
     zfree(values);
     return 0;
 }
+
+int test_iterator_bounds_check(int argc, char **argv, int flags) {
+    UNUSED(argc);
+    UNUSED(argv);
+    UNUSED(flags);
+
+    hashtableType type = {0};
+    hashtable *ht = hashtableCreate(&type);
+
+    for (size_t j = 0; j < 100; j++) {
+        TEST_ASSERT(hashtableAdd(ht, (void *)j));
+    }
+
+    hashtableIterator iter;
+    void *entry;
+    hashtableInitIterator(&iter, ht, 0);
+
+    size_t count = 0;
+    while (hashtableNext(&iter, &entry)) count++;
+    TEST_ASSERT(count == 100);
+
+    /* Calling next again should return false */
+    TEST_ASSERT(!hashtableNext(&iter, &entry));
+    TEST_ASSERT(!hashtableNext(&iter, &entry));
+
+    hashtableResetIterator(&iter);
+    hashtableRelease(ht);
+    return 0;
+}