Skip to content

Fix robin_hash::erase() method#92

Open
zhicme wants to merge 1 commit into
Tessil:masterfrom
zhicme:master
Open

Fix robin_hash::erase() method#92
zhicme wants to merge 1 commit into
Tessil:masterfrom
zhicme:master

Conversation

@zhicme

@zhicme zhicme commented Apr 17, 2026

Copy link
Copy Markdown

Bug Description

There is a bug in the range-erase implementation:

if (first == last) {
  return mutable_iterator(first);
}

auto first_mutable = mutable_iterator(first);
auto last_mutable = mutable_iterator(last);
for (auto it = first_mutable.m_bucket; it != last_mutable.m_bucket; ++it) {
  if (!it->empty()) {
    it->clear();
    m_nb_elements--;
  }
}

The problem is that erase(first, last) treats the bucket array as linear rather than circular when clearing the erased range.

As a result, when the erased range logically wraps around the end of the bucket array, the implementation does not correctly account for the circular probe sequence before performing the backward-shift cleanup. This can leave the table in an inconsistent state and may cause incorrect behavior afterward.

In particular, this may lead to a situation where the map ends up containing duplicate keys, which violates the map invariant.

#include <tsl/robin_map.h>
#include <iostream>

struct BadHash {
  std::size_t operator()(int) const noexcept {
    return 6;
  }
};

int main() {
  tsl::robin_map<int, int, BadHash> map(8);
  map.max_load_factor(0.95f);

  map.insert({1, 10});
  map.insert({2, 20});
  map.insert({3, 30});
  map.insert({4, 40});

  for (const auto& kv : map) {
    std::cout << kv.first << ":" << kv.second << "\n";
  }

  map.erase(map.find(1), map.end());
  auto res = map.insert({3, 300});

  std::cout << "inserted_again = " << res.second << "\n";
  std::cout << "size = " << map.size() << "\n";

  for (const auto& kv : map) {
    std::cout << kv.first << ":" << kv.second << "\n";
  }
}
// outputs:
// 3:30
// 4:40
// 1:10
// 2:20
// inserted_again = 1
// size = 3
// 3:30
// 4:40
// 3:300

To fix this issue, I reimplemented erase, added two overloaded version of next_bucket(), and introduced a new helper function cyclic_distance() to correctly handle circular bucket traversal.
I am not sure if there is a better way to fix this issue, but I hope this fix is helpful for you. 😄

@zhicme

zhicme commented Apr 27, 2026

Copy link
Copy Markdown
Author

I’m very sorry, I forgot to switch branches before making the changes. Would you like me to submit a new PR?

@Tessil

Tessil commented Jun 13, 2026

Copy link
Copy Markdown
Owner

Very sorry for the long delay and thanks for the catch!

The change looks good, could you just apply the clang-format one the change and eventually add a small regression test? Thanks!

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