add safe iterator tracking in hashtable. prevents invalid access on hashtable delete #2807
Conversation
…able deleted from under safe iterator Signed-off-by: Rain Valentine <[email protected]>
rainsupreme
force-pushed
the
safe-iterator-tracking
branch
from
d67a6b7 to
56db3fd
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #2807 +/- ##
============================================
+ Coverage 72.43% 72.45% +0.01%
============================================
Files 128 128
Lines 70245 70400 +155
============================================
+ Hits 50880 51005 +125
- Misses 19365 19395 +30
🚀 New features to boost your workflow:
|
Signed-off-by: Rain Valentine <[email protected]>
JimB123
left a comment
JimB123
left a comment
There was a problem hiding this comment.
Glad to see a proposal for this.
src/hashtable.c
Outdated
| iter *it = ht->safe_iterators; | ||
| while (it) { | ||
| it->hashtable = NULL; /* Mark as invalid */ | ||
| it = it->next_safe_iter; | ||
| } | ||
| ht->safe_iterators = NULL; |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Oh that's much better. Very DRY
| iter->index = -1; | ||
| iter->flags = flags; | ||
| iter->next_safe_iter = NULL; | ||
| if (isSafe(iter) && ht != NULL) { |
There was a problem hiding this comment.
How can ht be NULL here? Maybe put an assertion at the top if you want to check this.
There was a problem hiding this comment.
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 😅
src/hashtable.c
Outdated
| } | ||
|
|
||
| /* Resets a stack-allocated iterator. */ | ||
| void hashtableResetIterator(hashtableIterator *iterator) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good catch! I've fixed that, and I'm also renaming (which accounts for all the extra files changed):
- hashtableReinitIterator -> hashtableRetargetIterator
- hashtableResetIterator -> hashtableCleanupIterator
Signed-off-by: Rain Valentine <[email protected]>
2 participants
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.