sparse-map icon indicating copy to clipboard operation
sparse-map copied to clipboard

Avoid iterator invalidation without effective insert

Open artem opened this issue 2 years ago • 4 comments

There's an issue with accessing an existing value with operator[] when hashtable size reaches a certain value: iterator is sometimes being invalidated leading to broken iterators invalidation contract:

  • insert, emplace, emplace_hint, operator[]: if there is an effective insert, invalidate the iterators.

or use-after-free from calling code if user is unaware of such behaviour.

Fix this issue by actually issuing a rehash in effective erase and insert routines.

artem avatar Aug 30 '22 15:08 artem

PS: docs also say that erase() always invalidates the iterators, though the current implementation does that only on effective erase. Should the doc be adjusted as well?

artem avatar Aug 30 '22 15:08 artem

Thank you, we effectively should avoid rehashing when no actual insert is done.

I added some comments to the PR code.

Tessil avatar Sep 09 '22 19:09 Tessil

Thanks for the feedback. I have pushed fixes for review and additionally reverted + 1 in load threshold check to preserve original behaviour

artem avatar Sep 14 '22 13:09 artem

Thank you for the change, looks good. Just a small comment left.

Note that the map doesn't guarantee that erase is noexcept per the commit message. The problem is more that an exception could be thrown during the rehash after an element was successfully erased which goes against the strong exception guarantee (see https://en.cppreference.com/w/cpp/language/exceptions).

I also would like to limit the rehash to insertions as done in std::unordered_map

Tessil avatar Sep 20 '22 20:09 Tessil

I think I have addressed all of your comments in the last revision and it is safe to merge. Also, I have squashed all the commits into a single one (since they no longer make sense on their own).

Sorry for the rather long delay, been busy with other things.

artem avatar Jan 06 '23 21:01 artem

Thank you, it looks good, I merged the change.

Tessil avatar Jan 15 '23 15:01 Tessil