From e6773f2dc332d1570f646638a7f8038352f7a822 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 22 Mar 2021 18:54:11 -0500 Subject: [PATCH 1/2] RaiiMapOfListElement: fix invalid reference due to rehashing instability Signed-off-by: Jose Nino --- source/common/common/cleanup.h | 17 +++++++++++------ test/common/common/cleanup_test.cc | 26 +++++++++++++++++++++++--- 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/source/common/common/cleanup.h b/source/common/common/cleanup.h index 0c1db1da1d8b..abcf23f6a7b7 100644 --- a/source/common/common/cleanup.h +++ b/source/common/common/cleanup.h @@ -64,8 +64,11 @@ template class RaiiMapOfListElement { template RaiiMapOfListElement(MapOfList& map, const ConvertibleToKey& key, Value value) - : map_(map), list_(map_.try_emplace(key).first->second), key_(key), cancelled_(false) { - it_ = list_.emplace(list_.begin(), value); + : map_(map), key_(key), cancelled_(false) { + // The list reference itself cannot be saved because it is not stable in the event of a + // absl::flat_hash_map rehash. + std::list& list = map_.try_emplace(key).first->second; + it_ = list.emplace(list.begin(), value); } virtual ~RaiiMapOfListElement() { @@ -79,16 +82,18 @@ template class RaiiMapOfListElement { private: void erase() { ASSERT(!cancelled_); - list_.erase(it_); - if (list_.empty()) { + auto list_it = map_.find(key_); + ASSERT(list_it != map_.end()); + + list_it->second.erase(it_); + if (list_it->second.empty()) { map_.erase(key_); } cancelled_ = true; } MapOfList& map_; - std::list& list_; - // Because of absl::flat_hash_map iterator instability we have to keep a copy of the key + // Because of absl::flat_hash_map iterator instability we have to keep a copy of the key. const Key key_; typename MapOfList::mapped_type::iterator it_; bool cancelled_; diff --git a/test/common/common/cleanup_test.cc b/test/common/common/cleanup_test.cc index 25c3ea2b29f4..7832914a6caa 100644 --- a/test/common/common/cleanup_test.cc +++ b/test/common/common/cleanup_test.cc @@ -61,7 +61,7 @@ TEST(RaiiListElementTest, DeleteOnErase) { } TEST(RaiiMapOfListElement, DeleteOnDestruction) { - absl::flat_hash_map> map; + absl::node_hash_map> map; { EXPECT_EQ(map.size(), 0); RaiiMapOfListElement element(map, 1, 1); @@ -74,7 +74,7 @@ TEST(RaiiMapOfListElement, DeleteOnDestruction) { } TEST(RaiiMapOfListElementTest, CancelDelete) { - absl::flat_hash_map> map; + absl::node_hash_map> map; { EXPECT_EQ(map.size(), 0); @@ -92,7 +92,7 @@ TEST(RaiiMapOfListElementTest, CancelDelete) { } TEST(RaiiMapOfListElement, MultipleEntriesSameKey) { - absl::flat_hash_map> map; + absl::node_hash_map> map; { EXPECT_EQ(map.size(), 0); RaiiMapOfListElement element(map, 1, 1); @@ -114,4 +114,24 @@ TEST(RaiiMapOfListElement, MultipleEntriesSameKey) { EXPECT_EQ(map.size(), 0); } +TEST(RaiiMapOfListElement, DeleteAfterMapRehash) { + absl::node_hash_map> map; + std::list> list; + // According to https://abseil.io/docs/cpp/guides/container the max load factor on + // absl::flat_hash_map is 87.5%. Using bucket_count and multiplying by 2 should give us enough + // head room to cause rehashing. + int rehash_limit = (map.bucket_count() == 0 ? 1 : map.bucket_count()) * 2; + + for (int i = 0; i <= rehash_limit; i++) { + list.emplace_back(map, i, i); + EXPECT_EQ(map.size(), i + 1); + auto it = map.find(i); + ASSERT_NE(map.end(), it); + EXPECT_EQ(it->second.size(), 1); + } + + list.clear(); + EXPECT_EQ(map.size(), 0); +} + } // namespace Envoy From c13a4d29001775a9e69d0af55fef1d0f63e57a6e Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 26 Mar 2021 16:32:44 -0500 Subject: [PATCH 2/2] back Signed-off-by: Jose Nino --- test/common/common/cleanup_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/common/common/cleanup_test.cc b/test/common/common/cleanup_test.cc index 7832914a6caa..d76faf78e445 100644 --- a/test/common/common/cleanup_test.cc +++ b/test/common/common/cleanup_test.cc @@ -61,7 +61,7 @@ TEST(RaiiListElementTest, DeleteOnErase) { } TEST(RaiiMapOfListElement, DeleteOnDestruction) { - absl::node_hash_map> map; + absl::flat_hash_map> map; { EXPECT_EQ(map.size(), 0); RaiiMapOfListElement element(map, 1, 1); @@ -74,7 +74,7 @@ TEST(RaiiMapOfListElement, DeleteOnDestruction) { } TEST(RaiiMapOfListElementTest, CancelDelete) { - absl::node_hash_map> map; + absl::flat_hash_map> map; { EXPECT_EQ(map.size(), 0); @@ -92,7 +92,7 @@ TEST(RaiiMapOfListElementTest, CancelDelete) { } TEST(RaiiMapOfListElement, MultipleEntriesSameKey) { - absl::node_hash_map> map; + absl::flat_hash_map> map; { EXPECT_EQ(map.size(), 0); RaiiMapOfListElement element(map, 1, 1); @@ -115,7 +115,7 @@ TEST(RaiiMapOfListElement, MultipleEntriesSameKey) { } TEST(RaiiMapOfListElement, DeleteAfterMapRehash) { - absl::node_hash_map> map; + absl::flat_hash_map> map; std::list> list; // According to https://abseil.io/docs/cpp/guides/container the max load factor on // absl::flat_hash_map is 87.5%. Using bucket_count and multiplying by 2 should give us enough