Add safe iterator tracking in hashtable to prevents invalid access on hashtable delete by rainsupreme · Pull Request #2807 · valkey-io/valkey

rainsupreme · 2025-11-05T21:09:58Z

This makes it safe to delete hashtable while a safe iterator is iterating it. This currently isn't possible, but this improvement is required for fork-less replication #1754 which @JimB123 is actively working on.

We discussed these issues in #2611 which guards against a different but related issue: calling hashtableNext again after it has already returned false.

I implemented a singly linked list that hashtable uses to track its current safe iterators. It is used to invalidate all associated safe iterators when the hashtable is released. A singly linked list is acceptable because the list length is always very small - typically zero and no more than a handful.

Also, renames the internal functions:

hashtableReinitIterator -> hashtableRetargetIterator
hashtableResetIterator -> hashtableCleanupIterator

…able deleted from under safe iterator Signed-off-by: Rain Valentine <rsg000@gmail.com>

codecov · 2025-11-05T22:18:47Z

Codecov Report

❌ Patch coverage is 80.82192% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.45%. Comparing base (c88c94e) to head (00d2073).
⚠️ Report is 50 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/hashtable.c	82.35%	6 Missing ⚠️
src/module.c	0.00%	2 Missing ⚠️
src/vset.c	33.33%	2 Missing ⚠️
src/aof.c	66.66%	1 Missing ⚠️
src/defrag.c	0.00%	1 Missing ⚠️
src/object.c	50.00%	1 Missing ⚠️
src/server.c	87.50%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2807      +/-   ##
============================================
+ Coverage     72.43%   72.45%   +0.02%     
============================================
  Files           128      129       +1     
  Lines         70245    70522     +277     
============================================
+ Hits          50880    51098     +218     
- Misses        19365    19424      +59

Files with missing lines	Coverage Δ
src/acl.c	`90.30% <100.00%> (-0.29%)`	⬇️
src/debug.c	`54.58% <ø> (-0.01%)`	⬇️
src/kvstore.c	`95.91% <100.00%> (ø)`
src/latency.c	`80.97% <100.00%> (ø)`
src/pubsub.c	`97.19% <100.00%> (ø)`
src/rdb.c	`77.42% <100.00%> (+0.62%)`	⬆️
src/sort.c	`94.80% <ø> (+0.01%)`	⬆️
src/t_hash.c	`96.17% <100.00%> (-0.08%)`	⬇️
src/t_set.c	`97.82% <100.00%> (ø)`
src/t_zset.c	`96.84% <100.00%> (+0.58%)`	⬆️
... and 8 more

... and 22 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Rain Valentine <rsg000@gmail.com>

JimB123

Glad to see a proposal for this.

src/hashtable.c

JimB123 · 2025-11-14T17:12:58Z

src/hashtable.c

+    iter *it = ht->safe_iterators;
+    while (it) {
+        it->hashtable = NULL; /* Mark as invalid */
+        it = it->next_safe_iter;
+    }
+    ht->safe_iterators = NULL;


How about:

while (ht->safe_iterators) untrackSafeIterator(ht->safe_iterators);

Ensures that the cleanup/invalidation is identical (like setting next_safe_iter to NULL, which was missed here).

Oh that's much better. Very DRY

JimB123 · 2025-11-14T17:35:09Z

src/hashtable.c

    iter->index = -1;
    iter->flags = flags;
+    iter->next_safe_iter = NULL;
+    if (isSafe(iter) && ht != NULL) {


How can ht be NULL here? Maybe put an assertion at the top if you want to check this.

kvstore's iterator contains a hashtableIterator, and in kvstoreIteratorInit() they want to init their hashtableIterator to something and redo it later when they decide which hashtable to actually start with. Or that was my impression. I discovered this from a failed tcl test 😅

Hmmm, that sounds logically wrong to me. The hashtable is defining the hashtable-iterator API - not kvstore. It makes no logical sense for hashtable to support creation of an iterator with a null hashtable.

src/hashtable.c

JimB123 · 2025-11-14T17:42:52Z

src/hashtable.c

@@ -2000,10 +2048,13 @@ void hashtableReinitIterator(hashtableIterator *iterator, hashtable *ht) {
 /* Resets a stack-allocated iterator. */
 void hashtableResetIterator(hashtableIterator *iterator) {


I'm not clear on what this function is trying to do. I see that it can call resumeRehashing, but is that the extent of "reset"? I'm expecting that this would essentially rewind the iterator, allowing me to begin iteration again. However, it's not resetting index or table. Is this right?

Also, how is this different than hashtableReinitIterator (above)? It looks wrong to me that the reinit function above isn't checking if the iterator needs to be untracked - it just NULLs out next_safe_iterator. This looks like an issue.

Good catch! I've fixed that, and I'm also renaming (which accounts for all the extra files changed):

hashtableReinitIterator -> hashtableRetargetIterator

hashtableResetIterator -> hashtableCleanupIterator

src/hashtable.c

Signed-off-by: Rain Valentine <rsg000@gmail.com>

JimB123 · 2025-11-17T20:51:34Z

src/hashtable.c

+        ht->safe_iterators = it->next_safe_iter;
+    } else {
+        iter *current = ht->safe_iterators;
+        assert(current != NULL);


Not really needed. Has limited documentation value, and code will NPE in any case.

JimB123 · 2025-11-17T20:51:41Z

src/hashtable.c

+        assert(current != NULL);
+        while (current->next_safe_iter != it) {
+            current = current->next_safe_iter;
+            assert(current != NULL);


Not really needed. Has limited documentation value, and code will NPE in any case.

zuiderkwast

LGTM, except a trailing slash.

src/unit/test_hashtable.c

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

src/unit/test_hashtable.c

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

… hashtable delete (valkey-io#2807) This makes it safe to delete hashtable while a safe iterator is iterating it. This currently isn't possible, but this improvement is required for fork-less replication valkey-io#1754 which is being actively worked on. We discussed these issues in valkey-io#2611 which guards against a different but related issue: calling hashtableNext again after it has already returned false. I implemented a singly linked list that hashtable uses to track its current safe iterators. It is used to invalidate all associated safe iterators when the hashtable is released. A singly linked list is acceptable because the list length is always very small - typically zero and no more than a handful. Also, renames the internal functions: hashtableReinitIterator -> hashtableRetargetIterator hashtableResetIterator -> hashtableCleanupIterator --------- Signed-off-by: Rain Valentine <rsg000@gmail.com> Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

zuiderkwast · 2026-01-10T16:11:56Z

src/valkey-microbench

This binary file was added to the repo in this PR. We need to remove it.

To avoid this in the future, NEVER use git add -a.

Introduced by mistake in #2807. Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

in #2807 we added safe-iterator invalidation when a hashtable is deleted. When we create a safeIterator we initialize it to it->table=0 and it-index=-1. HOWEVER say someone creates a safe iterator never progress it and later on just delete the iterator using `hashtableReleaseIterator`. the iterator will NOT be untracked since, `hashtableCleanupIterator` will skip the untrack: ```c if (!(iter->index == -1 && iter->table == 0)) { if (isSafe(iter)) { hashtableResumeRehashing(iter->hashtable); untrackSafeIterator(iter); } else { assert(iter->fingerprint == hashtableFingerprint(iter->hashtable)); } } ``` and then when the hashtable is freed the iterator will be referenced leading to a use-after-free. For example: ```c it = hashtableCreateIterator(ht, HASHTABLE_ITER_SAFE); hashtableReleaseIterator(it); hashtableRelease(ht); ``` This PR fix it by ALWAYS untrack safe iterators which have a valid hashtable when they are being cleaned up. Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

Introduced by mistake in valkey-io#2807. Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

in valkey-io#2807 we added safe-iterator invalidation when a hashtable is deleted. When we create a safeIterator we initialize it to it->table=0 and it-index=-1. HOWEVER say someone creates a safe iterator never progress it and later on just delete the iterator using `hashtableReleaseIterator`. the iterator will NOT be untracked since, `hashtableCleanupIterator` will skip the untrack: ```c if (!(iter->index == -1 && iter->table == 0)) { if (isSafe(iter)) { hashtableResumeRehashing(iter->hashtable); untrackSafeIterator(iter); } else { assert(iter->fingerprint == hashtableFingerprint(iter->hashtable)); } } ``` and then when the hashtable is freed the iterator will be referenced leading to a use-after-free. For example: ```c it = hashtableCreateIterator(ht, HASHTABLE_ITER_SAFE); hashtableReleaseIterator(it); hashtableRelease(ht); ``` This PR fix it by ALWAYS untrack safe iterators which have a valid hashtable when they are being cleaned up. Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

github-actions bot assigned rainsupreme Nov 5, 2025

safe iterator tracking in hashtable. prevents invalid access if hasht…

56db3fd

…able deleted from under safe iterator Signed-off-by: Rain Valentine <rsg000@gmail.com>

rainsupreme force-pushed the safe-iterator-tracking branch from d67a6b7 to 56db3fd Compare November 5, 2025 21:59

clang format

014b26b

Signed-off-by: Rain Valentine <rsg000@gmail.com>

JimB123 reviewed Nov 14, 2025

View reviewed changes

improve method names, safety improvements, other feedback

a1497a5

Signed-off-by: Rain Valentine <rsg000@gmail.com>

JimB123 approved these changes Nov 17, 2025

View reviewed changes

zuiderkwast approved these changes Dec 3, 2025

View reviewed changes

src/unit/test_hashtable.c Outdated Show resolved Hide resolved

Remove trailing slash

6da6a41

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

zuiderkwast reviewed Dec 3, 2025

View reviewed changes

src/unit/test_hashtable.c Outdated Show resolved Hide resolved

Newline at end of file

00d2073

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

zuiderkwast changed the title ~~add safe iterator tracking in hashtable. prevents invalid access on hashtable delete~~ Add safe iterator tracking in hashtable to prevents invalid access on hashtable delete Dec 3, 2025

zuiderkwast merged commit 70196ee into valkey-io:unstable Dec 3, 2025
55 checks passed

zuiderkwast reviewed Jan 10, 2026

View reviewed changes

zuiderkwast mentioned this pull request Jan 13, 2026

Delete large binary junk file valkey-microbench #3055

Merged

zuiderkwast added a commit that referenced this pull request Jan 13, 2026

Delete large binary junk file valkey-microbench (#3055)

b7f1a6c

Introduced by mistake in #2807. Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

rainsupreme deleted the safe-iterator-tracking branch February 13, 2026 22:05

ranshid mentioned this pull request Feb 17, 2026

Always untrack initialized safe iterator #3223

Merged

harrylin98 pushed a commit to harrylin98/valkey_forked that referenced this pull request Feb 19, 2026

Delete large binary junk file valkey-microbench (valkey-io#3055)

fdfc82c

Introduced by mistake in valkey-io#2807. Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

		@@ -2000,10 +2048,13 @@ void hashtableReinitIterator(hashtableIterator iterator, hashtable ht) {
		/* Resets a stack-allocated iterator. */
		void hashtableResetIterator(hashtableIterator *iterator) {

Comments

Conversation

rainsupreme commented Nov 5, 2025 • edited by zuiderkwast Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JimB123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rainsupreme Nov 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rainsupreme Nov 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rainsupreme commented Nov 5, 2025 •

edited by zuiderkwast

Loading

codecov bot commented Nov 5, 2025 •

edited

Loading

rainsupreme Nov 15, 2025 •

edited

Loading

rainsupreme Nov 15, 2025 •

edited

Loading