commit 25d207ad0f5b662d56d0d744069e660780f7af1d
author Lukasz Anforowicz <lukasza@google.com> Wed Jul 26 08:51:25 2023 -0700
committer Copybara-Service <copybara-worker@google.com> Wed Jul 26 08:52:00 2023 -0700
tree 8aa1d3eb6b76baeb4e2422c927a8fdd582acc7fe
parent 2e9a797fd78ee52854878a63d905c58b916ed081

Refactor `ReturnValueSlot.Test` to avoid reading destroyed objects.

The test wants to check that the object pointed by `slot_ptr` has been
destroyed.  Before this CL the test would read the `slot_ptr->state`
field and assert that it is equal to `kDestroyedState`.  Reading an
already destroyed object is UB and was caught by MSAN.  After this CL,
the assertion is done through `destroyed_locations` instead.

PiperOrigin-RevId: 551219313
Change-Id: I4a5e085ad6a7ae8943721b93e52197fd11245a1c

support/internal/return_value_slot_test.cc

diff --git a/support/internal/return_value_slot_test.cc b/support/internal/return_value_slot_test.cc
index 06ec89c..6d140a8 100644
--- a/support/internal/return_value_slot_test.cc
+++ b/support/internal/return_value_slot_test.cc
@@ -22,8 +22,11 @@
   MonitoringHelper(const MonitoringHelper&) = delete;
   MonitoringHelper& operator=(const MonitoringHelper&) = delete;
 
-  explicit MonitoringHelper(int new_state, std::vector<int>* destroyed_states)
-      : state(new_state), destroyed_states(destroyed_states) {
+  explicit MonitoringHelper(int new_state, std::vector<int>* destroyed_states,
+                            std::vector<MonitoringHelper*>* destroyed_locations)
+      : state(new_state),
+        destroyed_states(destroyed_states),
+        destroyed_locations(destroyed_locations) {
     CHECK_NE(state, kMovedAwayState);
     CHECK_NE(state, kDestroyedState);
   }
@@ -31,6 +34,7 @@
   MonitoringHelper(MonitoringHelper&& other) {
     state = other.state;
     destroyed_states = other.destroyed_states;
+    destroyed_locations = other.destroyed_locations;
     other.state = kMovedAwayState;
   }
 
@@ -44,9 +48,11 @@
 
     // Pretend to destroy old field values.
     destroyed_states->push_back(state);
+    destroyed_locations->push_back(this);
 
     state = other.state;
     destroyed_states = other.destroyed_states;
+    destroyed_locations = other.destroyed_locations;
     other.state = kMovedAwayState;
     return *this;
   }
@@ -56,20 +62,24 @@
     CHECK_NE(state, kUninitializedState);
     CHECK_NE(state, kDestroyedState);
     destroyed_states->push_back(state);
+    destroyed_locations->push_back(this);
     state = kDestroyedState;
   }
 
   int state;
   std::vector<int>* destroyed_states;
+  std::vector<MonitoringHelper*>* destroyed_locations;
 };
 
 TEST(ReturnValueSlot, Test) {
   std::vector<int> destroyed_states;
+  std::vector<MonitoringHelper*> destroyed_locations;
 
   constexpr int kInitialValue = 1;
   constexpr int kReturnedValue = 2;
 
-  MonitoringHelper return_value(kInitialValue, &destroyed_states);
+  MonitoringHelper return_value(kInitialValue, &destroyed_states,
+                                &destroyed_locations);
 
   {
     // At this point `slot` is in an uninitialized state.
@@ -77,12 +87,16 @@
     MonitoringHelper* slot_ptr = slot.Get();
     slot_ptr->state = kUninitializedState;
     slot_ptr->destroyed_states = &destroyed_states;
+    slot_ptr->destroyed_locations = &destroyed_locations;
     // No destructors should run up to this point.
     EXPECT_THAT(destroyed_states, testing::IsEmpty());
+    EXPECT_THAT(destroyed_locations, testing::IsEmpty());
 
     // Initialize the memory.
-    new (slot_ptr) MonitoringHelper(kReturnedValue, &destroyed_states);
+    new (slot_ptr) MonitoringHelper(kReturnedValue, &destroyed_states,
+                                    &destroyed_locations);
     EXPECT_THAT(destroyed_states, testing::IsEmpty());
+    EXPECT_THAT(destroyed_locations, testing::IsEmpty());
 
     // Move the return value from `slot` to `return_value`.
     return_value = std::move(slot).AssumeInitAndTakeValue();
@@ -90,17 +104,16 @@
     // overwritten by the assignment - this is where `kInitialValue` comes from.
     //
     // AssumeInitAndTakeValue will destroy `ReturnValueSlot::value_` in a
-    // kMovedAwayState.
+    // kMovedAwayState.  This is asserted by checking that `slot_ptr` is covered
+    // by `destroyed_locations`.
     //
     // Additionally, a temporary `MonitoringHelper` value in a moved-away state
     // will be destroyed.
     EXPECT_THAT(
         destroyed_states,
         testing::ElementsAre(kMovedAwayState, kInitialValue, kMovedAwayState));
-    // The value inside `ReturnValueSlot` (pointed to by `slot_ptr`) should be
-    // in a `kDestroyedState` state at this point due to
-    // `AssumeInitAndTakeValue()`.
-    EXPECT_EQ(kDestroyedState, slot_ptr->state);
+    EXPECT_THAT(destroyed_locations,
+                testing::ElementsAre(slot_ptr, testing::_, testing::_));
     EXPECT_EQ(kReturnedValue, return_value.state);
   }