[Bugfix][Support] Fix copy constructor for support::OrderedSet #17044
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Prior to this commit, the `support::OrderedSet<T>` utility used the default copy constructor and copy assignment, which would copy both the `OrderedSet::elements_` and `OrderedSet::elem_to_iter_` members. While this is the correct behavior for `elements_`, the copy of `elem_to_iter_` would contain references to the original's `element_`, rather than to its own. While `elem_to_iter_` is used in both `OrderedSet::push_back` and `OrderedSet::erase`, the implementation of `OrderedSet::push_back` only depends on the keys used in `elem_to_iter_`, and does not depend on the values stored. As a result, this bug could go undetected for append-only usage, which is the most frequent use of `OrderedSet`. This commit updates `support::OrderedSet` to have an explicit copy constructor and copy assignment. Only the `std::list<T> elements_` member may be copied, while the `elem_to_iter_` must instead be rebuilt.
|
While I'm unaware of any way to trigger this bug from an externally-visible API, I ran across this during implementation of a debug feature. This bug can result in segfaults when accessing elements of a |
vinx13
approved these changes
May 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this commit, the
support::OrderedSet<T>utility used the default copy constructor and copy assignment, which would copy both theOrderedSet::elements_andOrderedSet::elem_to_iter_members. While this is the correct behavior forelements_, the copy ofelem_to_iter_would contain references to the original'selement_, rather than to its own.While
elem_to_iter_is used in bothOrderedSet::push_backandOrderedSet::erase, the implementation ofOrderedSet::push_backonly depends on the keys used inelem_to_iter_, and does not depend on the values stored. As a result, this bug could go undetected for append-only usage, which is the most frequent use ofOrderedSet.This commit updates
support::OrderedSetto have an explicit copy constructor and copy assignment. Only thestd::list<T> elements_member may be copied, while theelem_to_iter_must instead be rebuilt.