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

Use STL containers when requiring pointer/iterator stability #49

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Muirey03
Copy link

@Muirey03 Muirey03 commented Aug 4, 2024

From https://abseil.io/about/design/btree:

When values are inserted or removed from a B-tree, nodes can be split or merged and values can be moved within and between nodes (for the purpose of maintaining tree balance). This means that when values are inserted or removed from a B-tree container, pointers and iterators to other values in the B-tree can be invalidated. Abseil B-trees therefore do not provide pointer stability or iterator stability - this is in contrast to STL sorted containers that do provide these guarantees.

BinDiff incorrectly relies on pointer stability and iterator stability when using btree data structures in a number of places:

  • indexed_fixed_points_ is a vector of pointers to FixedPointInfos held in a absl::btree_set
  • indexed_flow_graphs1_ and indexed_flow_graphs2_ are vectors of pointers to FlowGraphInfos held in a absl::btree_map
  • Results::DeleteMatches calls histogram_.erase while iterating over histogram_ which is a absl::btree_map, invalidating the iterator

You can see the effects of these incorrect assumptions by manually removing and adding matches in the IDA plugin. You will see duplicate matches appearing and functions being erased from the database entirely.

This PR fixes the issue by replacing the types that depend on pointer/iterator stability with their STL equivalents.

@cblichmann cblichmann added the bug Something isn't working label Aug 12, 2024
@cblichmann
Copy link
Member

Thank you for your PR!
While replacing Abseil maps/sets with their STL equivalents will fix the issues you mentioned, we deliberately removed all/most uses of STL maps/sets from the code base.
It seems we did not read the API contracts carefully enough and there are not enough tests to catch these issues.
Let's first fix the obvious deficiency in Results::DeleteMatches() and then debug the others.
Switching back to std::map<>/std::set<> is not really a good option for us. We may need to add another indirection by having the containers store pointers instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants