Add validation against use of moved-from hash tables. We enable this validation when sanitizers are enabled rather than in debug mode because debug mode can be enabled/disabled separately in different translation units, which can cause bugs for this type of validation. E.g. a hashtable that was moved from in a TU with debug enabled is then destroyed in a TU with debug disabled. PiperOrigin-RevId: 668492783 Change-Id: Ifab6894de0890aa0a7d8ea338b53b6759b097cb1
diff --git a/absl/container/internal/raw_hash_set.h b/absl/container/internal/raw_hash_set.h index 711dabe..c371832 100644 --- a/absl/container/internal/raw_hash_set.h +++ b/absl/container/internal/raw_hash_set.h
@@ -545,6 +545,7 @@ kAboveMaxValidCapacity = ~size_t{} - 100, kReentrance, kDestroyed, + kMovedFrom, }; // Returns a pointer to a control byte group that can be used by empty tables. @@ -2402,6 +2403,10 @@ alignof(slot_type) <= alignof(HeapOrSoo); } + constexpr static size_t DefaultCapacity() { + return SooEnabled() ? SooCapacity() : 0; + } + // Whether `size` fits in the SOO capacity of this table. bool fits_in_soo(size_t size) const { return SooEnabled() && size <= SooCapacity(); @@ -2639,7 +2644,7 @@ const allocator_type& alloc = allocator_type()) : settings_(CommonFields::CreateDefault<SooEnabled()>(), hash, eq, alloc) { - if (bucket_count > (SooEnabled() ? SooCapacity() : 0)) { + if (bucket_count > DefaultCapacity()) { resize(NormalizeCapacity(bucket_count)); } } @@ -2822,7 +2827,7 @@ transfer(soo_slot(), that.soo_slot()); } that.common() = CommonFields::CreateDefault<SooEnabled()>(); - maybe_increment_generation_or_rehash_on_move(); + annotate_for_bug_detection_on_move(that); } raw_hash_set(raw_hash_set&& that, const allocator_type& a) @@ -2830,7 +2835,7 @@ that.eq_ref(), a) { if (a == that.alloc_ref()) { swap_common(that); - maybe_increment_generation_or_rehash_on_move(); + annotate_for_bug_detection_on_move(that); } else { move_elements_allocs_unequal(std::move(that)); } @@ -2896,15 +2901,19 @@ size_t size() const { return common().size(); } size_t capacity() const { const size_t cap = common().capacity(); - // Compiler complains when using functions in assume so use local variables. - ABSL_ATTRIBUTE_UNUSED static constexpr bool kEnabled = SooEnabled(); - ABSL_ATTRIBUTE_UNUSED static constexpr size_t kCapacity = SooCapacity(); - ABSL_ASSUME(!kEnabled || cap >= kCapacity); + // Compiler complains when using functions in ASSUME so use local variable. + ABSL_ATTRIBUTE_UNUSED static constexpr size_t kDefaultCapacity = + DefaultCapacity(); + ABSL_ASSUME(cap >= kDefaultCapacity); return cap; } size_t max_size() const { return (std::numeric_limits<size_t>::max)(); } ABSL_ATTRIBUTE_REINITIALIZES void clear() { + if (SwisstableGenerationsEnabled() && + capacity() == InvalidCapacity::kMovedFrom) { + common().set_capacity(DefaultCapacity()); + } AssertNotDebugCapacity(); // Iterating over this container is O(bucket_count()). When bucket_count() // is much greater than size(), iteration becomes prohibitively expensive. @@ -3586,6 +3595,10 @@ } inline void destructor_impl() { + if (SwisstableGenerationsEnabled() && + capacity() == InvalidCapacity::kMovedFrom) { + return; + } if (capacity() == 0) return; if (is_soo()) { if (!empty()) { @@ -3759,8 +3772,16 @@ move_common(that_is_full_soo, that.alloc_ref(), common(), std::move(tmp)); } - void maybe_increment_generation_or_rehash_on_move() { - if (!SwisstableGenerationsEnabled() || capacity() == 0 || is_soo()) { + void annotate_for_bug_detection_on_move( + ABSL_ATTRIBUTE_UNUSED raw_hash_set& that) { + // We only enable moved-from validation when generations are enabled (rather + // than using NDEBUG) to avoid issues in which NDEBUG is enabled in some + // translation units but not in others. + if (SwisstableGenerationsEnabled() && this != &that) { + that.common().set_capacity(InvalidCapacity::kMovedFrom); + } + if (!SwisstableGenerationsEnabled() || capacity() == DefaultCapacity() || + capacity() > kAboveMaxValidCapacity) { return; } common().increment_generation(); @@ -3782,7 +3803,7 @@ CopyAlloc(alloc_ref(), that.alloc_ref(), std::integral_constant<bool, propagate_alloc>()); that.common() = CommonFields::CreateDefault<SooEnabled()>(); - maybe_increment_generation_or_rehash_on_move(); + annotate_for_bug_detection_on_move(that); return *this; } @@ -3796,7 +3817,7 @@ } if (!that.is_soo()) that.dealloc(); that.common() = CommonFields::CreateDefault<SooEnabled()>(); - maybe_increment_generation_or_rehash_on_move(); + annotate_for_bug_detection_on_move(that); return *this; } @@ -3875,27 +3896,30 @@ // Asserts for correctness that we run on find/find_or_prepare_insert. template <class K> void AssertOnFind(ABSL_ATTRIBUTE_UNUSED const K& key) { -#ifdef NDEBUG - return; -#endif AssertHashEqConsistent(key); AssertNotDebugCapacity(); } // Asserts that the capacity is not a sentinel invalid value. - // TODO(b/296061262): also add asserts for moved-from state. void AssertNotDebugCapacity() const { assert(capacity() != InvalidCapacity::kReentrance && - "reentrant container access during element construction/destruction " + "Reentrant container access during element construction/destruction " "is not allowed."); assert(capacity() != InvalidCapacity::kDestroyed && - "use of destroyed hash table."); + "Use of destroyed hash table."); + if (SwisstableGenerationsEnabled() && + ABSL_PREDICT_FALSE(capacity() == InvalidCapacity::kMovedFrom)) { + ABSL_RAW_LOG(FATAL, "Use of moved-from hash table."); + } } // Asserts that hash and equal functors provided by the user are consistent, // meaning that `eq(k1, k2)` implies `hash(k1)==hash(k2)`. template <class K> void AssertHashEqConsistent(const K& key) { +#ifdef NDEBUG + return; +#endif // If the hash/eq functors are known to be consistent, then skip validation. if (std::is_same<hasher, absl::container_internal::StringHash>::value && std::is_same<key_equal, absl::container_internal::StringEq>::value) {
diff --git a/absl/container/internal/raw_hash_set_test.cc b/absl/container/internal/raw_hash_set_test.cc index 81397ad..3d5f59a 100644 --- a/absl/container/internal/raw_hash_set_test.cc +++ b/absl/container/internal/raw_hash_set_test.cc
@@ -3674,6 +3674,29 @@ #endif } +TEST(Table, MovedFromCallsFail) { + if (!SwisstableGenerationsEnabled()) { + GTEST_SKIP() << "Moved-from checks only enabled in sanitizer mode."; + return; + } + + { + ABSL_ATTRIBUTE_UNUSED IntTable t1, t2; + t1.insert(1); + t2 = std::move(t1); + // NOLINTNEXTLINE(bugprone-use-after-move) + EXPECT_DEATH_IF_SUPPORTED(t1.contains(1), ""); + } + { + ABSL_ATTRIBUTE_UNUSED IntTable t1; + t1.insert(1); + ABSL_ATTRIBUTE_UNUSED IntTable t2(std::move(t1)); + // NOLINTNEXTLINE(bugprone-use-after-move) + EXPECT_DEATH_IF_SUPPORTED(t1.contains(1), ""); + t1.clear(); // Clearing a moved-from table is allowed. + } +} + } // namespace } // namespace container_internal ABSL_NAMESPACE_END