Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ExtentTree use of invalidated iterator #4416

Merged
merged 1 commit into from
Jan 7, 2025
Merged

fix: ExtentTree use of invalidated iterator #4416

merged 1 commit into from
Jan 7, 2025

Conversation

kostasrim
Copy link
Contributor

The changes introduced in https://github.com/dragonflydb/dragonfly/blob/main/src/core/extent_tree.cc#L41 do not considerate that the cached iterator might be invalidated by the erase() above. As such, it causes an illegal access and it was caught by the sanitizers: https://github.com/dragonflydb/dragonfly/actions/runs/12644997917/job/35233466191#step:8:8607

@kostasrim kostasrim self-assigned this Jan 7, 2025
@@ -29,7 +29,7 @@ void ExtentTree::Add(size_t start, size_t len) {
prev->second += len;
start = prev->first;
len += prev_len;
prev_extent = prev;
prev_extent_key = prev->first;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absl also issued a warning but the test did not fail. Maybe we should somehow be aware of these warnings (because they usually point to the root cause of the problem)

@romange
Copy link
Collaborator

romange commented Jan 7, 2025

I thought it's fully compatible with map. Now I see it's not:
https://github.com/abseil/abseil-cpp/blob/master/absl/container/btree_map.h#L42C1-L48C41

@kostasrim kostasrim merged commit e8f43d6 into main Jan 7, 2025
9 checks passed
@kostasrim kostasrim deleted the kpr2 branch January 7, 2025 12:38
romange pushed a commit that referenced this pull request Jan 12, 2025
Fixes accessing and invalidated iterator within ExtendTree
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants