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

[Bug]: new flat_hash_map to empty string could cause memory corruption in c++20 #1661

Closed
JuniorHsu opened this issue Apr 24, 2024 · 7 comments

Comments

@JuniorHsu
Copy link

JuniorHsu commented Apr 24, 2024

Describe the issue

while we migrate to c++20, we see regression for flat_hash_map<absl::string_view, absl::string_view> of empty c-string. see STR for detail

Steps to reproduce the problem

while we do

using StringViewMap = absl::flat_hash_map<absl::string_view, absl::string_view>;

const StringViewMap& replacements() {
  CONSTRUCT_ON_FIRST_USE(StringViewMap, {{"xxxxxxxxx", ""}, {".", "_"}});
}

auto& r = replacements();

std::cout << "r.size: " << r.size() << std::endl;
for (const auto& [k, v] : r) {
   std::cout << "k: " << k << " v: " << v << std::endl;
}

where the definition is in https://github.com/envoyproxy/envoy/blob/4af52310675fbaf7d1e05cf89134332421c8ba2e/source/common/common/macros.h#L34

this will output
r.size: 1 from my side and see the element in the flat_hash_map contains lots of code section in the key of map like [Alignment = 8UL, Alloc = std::allocator<std::pair<const std::string, Envoy::Regex::EngineFactory *> with size 2^64 -1

replacing "" with string() is one workaround

What version of Abseil are you using?

version = "20230802.1",

What operating system and version are you using?

ubuntu

What compiler and version are you using?

clang 12.0.1

What build system are you using?

bazel 6.1.0

Additional context

No response

@derekmauro
Copy link
Member

The example code provided contains syntax errors. Sure, I could try to fix them, but then I would just be guessing what your code actually says and possibly hiding the error. Please provide a working reproduction.

@JuniorHsu
Copy link
Author

The example code provided contains syntax errors. Sure, I could try to fix them, but then I would just be guessing what your code actually says and possibly hiding the error. Please provide a working reproduction.

const StringViewMap& replacements() const {

remove second const should work, sorry i try to have a minimal viable test but didn't compile again

I modify the initial STR too

@derekmauro
Copy link
Member

I will not look at this until I have a complete reproduction.

@JuniorHsu
Copy link
Author

I'll try to have a unit test

@JuniorHsu
Copy link
Author

JuniorHsu commented Apr 25, 2024

it's reproducible in my macOS from this commit
JuniorHsu@432bd53

~/code/abseil-cpp $ bazel test --sandbox_debug -c dbg  --test_output=streamed --verbose_failures --cxxopt=-std=c++20 absl/container:flat_hash_map_test --test_filter=FlatHashMap.EmptyStringViewMap
WARNING: Streamed test output requested. All tests will be run locally, without sharding, one at a time
INFO: Analyzed target //absl/container:flat_hash_map_test (0 packages loaded, 3 targets configured).
Running main() from gmock_main.cc
Note: Google Test filter = FlatHashMap.EmptyStringViewMap
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from FlatHashMap
[ RUN      ] FlatHashMap.EmptyStringViewMap
absl/container/flat_hash_map_test.cc:91: Failure
Expected equality of these values:
  r.size()
    Which is: 1
  2

[  FAILED  ] FlatHashMap.EmptyStringViewMap (0 ms)
[----------] 1 test from FlatHashMap (0 ms total)

@derekmauro
Copy link
Member

derekmauro commented Apr 25, 2024

Thank you for the reproduction.

This isn't an absl::flat_hash_map issue. std::unordered_map demonstrates the same behavior.

If you replace new type{__VA_ARGS__} with new type(__VA_ARGS__) in CONSTRUCT_ON_FIRST_USE then it will work, but I don't think you want to do that, because it will change the meaning of other code.

Basically this is a more complicated form of

std::vector<int> v(5, 0);  // Vector v holds 5 zeros -> {0, 0, 0, 0, 0}
std::vector<int> v{5, 0};  // Vector v holds a 5 and a 0 -> {5, 0}

C++20 added a new string_view constructor which is why you are seeing this issue in C++20 only. A different constructor is getting called.

My only advice is not to use CONSTRUCT_ON_FIRST_USE at all. absl::NoDestructor might be helpful here.
See also https://abseil.io/tips/88.

@JuniorHsu
Copy link
Author

Thanks for digging on this. I filed envoyproxy/envoy#33804
Could be a design defect on c++ 20 if we could reproduce in std::unordered_map

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

No branches or pull requests

2 participants