From 44e32fe1233dc949503a1d0f9d02004b7b5412f2 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 25 Apr 2017 13:37:23 -0400 Subject: [PATCH] Tweak InitializationState tests that rely on undefined behavior These tests: - InitializationState.InitializationState - InitializationStateDcheckDeathTest.Destroyed_NotUninitialized - InitializationStateDcheckDeathTest.Destroyed_NotValid rely on certain behavior from destroyed objects. This is undefined behavior and we know it, but the whole point of the of InitializationState and InitializationStateDcheck destructors is to try to help catch other parts of the program making use of undefined behavior. To make it impossible for the memory that formerly hosted these objects to be repurposed during tests after the objects are destroyed, these tests that attempt to work with destroyed objects are changed to use placement new, so that the lifetimes of the objects can be decoupled from the lifetimes of the buffers. Test: crashpad_util_test InitializationState* Change-Id: Ie972a54116c8b90a21a502d3ba13623583dfac06 Reviewed-on: https://chromium-review.googlesource.com/486383 Reviewed-by: Joshua Peraza --- util/misc/initialization_state_dcheck_test.cc | 57 ++++++++++++------- util/misc/initialization_state_test.cc | 56 ++++++++++-------- 2 files changed, 70 insertions(+), 43 deletions(-) diff --git a/util/misc/initialization_state_dcheck_test.cc b/util/misc/initialization_state_dcheck_test.cc index b293ab72..2c407675 100644 --- a/util/misc/initialization_state_dcheck_test.cc +++ b/util/misc/initialization_state_dcheck_test.cc @@ -14,7 +14,12 @@ #include "util/misc/initialization_state_dcheck.h" +#include + +#include + #include "base/logging.h" +#include "base/memory/free_deleter.h" #include "gtest/gtest.h" #include "test/gtest_death_check.h" @@ -98,33 +103,45 @@ TEST(InitializationStateDcheckDeathTest, Destroyed_NotUninitialized) { // This tests that an attempt to reinitialize a destroyed object fails. See // the InitializationState.InitializationState test for an explanation of this // use-after-free test. - InitializationStateDcheck* initialization_state_dcheck_pointer; - { - InitializationStateDcheck initialization_state_dcheck; - initialization_state_dcheck_pointer = &initialization_state_dcheck; - INITIALIZATION_STATE_SET_INITIALIZING(initialization_state_dcheck); - INITIALIZATION_STATE_SET_VALID(initialization_state_dcheck); - INITIALIZATION_STATE_DCHECK_VALID(initialization_state_dcheck); - } - ASSERT_DEATH_CHECK(INITIALIZATION_STATE_SET_INITIALIZING( - *initialization_state_dcheck_pointer), - "kStateUninitialized"); + std::unique_ptr + initialization_state_dcheck_buffer(static_cast( + malloc(sizeof(InitializationStateDcheck)))); + + InitializationStateDcheck* initialization_state_dcheck = + new (initialization_state_dcheck_buffer.get()) + InitializationStateDcheck(); + + INITIALIZATION_STATE_SET_INITIALIZING(*initialization_state_dcheck); + INITIALIZATION_STATE_SET_VALID(*initialization_state_dcheck); + INITIALIZATION_STATE_DCHECK_VALID(*initialization_state_dcheck); + + initialization_state_dcheck->~InitializationStateDcheck(); + + ASSERT_DEATH_CHECK( + INITIALIZATION_STATE_SET_INITIALIZING(*initialization_state_dcheck), + "kStateUninitialized"); } TEST(InitializationStateDcheckDeathTest, Destroyed_NotValid) { // This tests that an attempt to use a destroyed object fails. See the // InitializationState.InitializationState test for an explanation of this // use-after-free test. - InitializationStateDcheck* initialization_state_dcheck_pointer; - { - InitializationStateDcheck initialization_state_dcheck; - initialization_state_dcheck_pointer = &initialization_state_dcheck; - INITIALIZATION_STATE_SET_INITIALIZING(initialization_state_dcheck); - INITIALIZATION_STATE_SET_VALID(initialization_state_dcheck); - INITIALIZATION_STATE_DCHECK_VALID(initialization_state_dcheck); - } + std::unique_ptr + initialization_state_dcheck_buffer(static_cast( + malloc(sizeof(InitializationStateDcheck)))); + + InitializationStateDcheck* initialization_state_dcheck = + new (initialization_state_dcheck_buffer.get()) + InitializationStateDcheck(); + + INITIALIZATION_STATE_SET_INITIALIZING(*initialization_state_dcheck); + INITIALIZATION_STATE_SET_VALID(*initialization_state_dcheck); + INITIALIZATION_STATE_DCHECK_VALID(*initialization_state_dcheck); + + initialization_state_dcheck->~InitializationStateDcheck(); + ASSERT_DEATH_CHECK( - INITIALIZATION_STATE_DCHECK_VALID(*initialization_state_dcheck_pointer), + INITIALIZATION_STATE_DCHECK_VALID(*initialization_state_dcheck), "kStateValid"); } diff --git a/util/misc/initialization_state_test.cc b/util/misc/initialization_state_test.cc index d427a140..9f462826 100644 --- a/util/misc/initialization_state_test.cc +++ b/util/misc/initialization_state_test.cc @@ -14,6 +14,11 @@ #include "util/misc/initialization_state.h" +#include + +#include + +#include "base/memory/free_deleter.h" #include "gtest/gtest.h" namespace crashpad { @@ -21,36 +26,41 @@ namespace test { namespace { TEST(InitializationState, InitializationState) { - InitializationState* initialization_state_pointer; - { - InitializationState initialization_state; - initialization_state_pointer = &initialization_state; + // Use placement new so that the buffer used to host the object remains live + // even after the object is destroyed. + std::unique_ptr + initialization_state_buffer( + static_cast(malloc(sizeof(InitializationState)))); - EXPECT_TRUE(initialization_state.is_uninitialized()); - EXPECT_FALSE(initialization_state.is_valid()); + InitializationState* initialization_state = + new (initialization_state_buffer.get()) InitializationState(); - initialization_state.set_invalid(); + EXPECT_TRUE(initialization_state->is_uninitialized()); + EXPECT_FALSE(initialization_state->is_valid()); - EXPECT_FALSE(initialization_state.is_uninitialized()); - EXPECT_FALSE(initialization_state.is_valid()); + initialization_state->set_invalid(); - initialization_state.set_valid(); + EXPECT_FALSE(initialization_state->is_uninitialized()); + EXPECT_FALSE(initialization_state->is_valid()); - EXPECT_FALSE(initialization_state.is_uninitialized()); - EXPECT_TRUE(initialization_state.is_valid()); - } + initialization_state->set_valid(); - // initialization_state_pointer points to something that no longer exists. - // This portion of the test is intended to check that after an - // InitializationState object goes out of scope, it will not be considered - // valid on a use-after-free, assuming that nothing else was written to its - // former home in memory. + EXPECT_FALSE(initialization_state->is_uninitialized()); + EXPECT_TRUE(initialization_state->is_valid()); + + initialization_state->~InitializationState(); + + // initialization_state points to something that no longer exists. This + // portion of the test is intended to check that after an InitializationState + // object is destroyed, it will not be considered valid on a use-after-free, + // assuming that nothing else was written to its former home in memory. // - // This portion of the test is technically not valid C++, but it exists to - // test that the behavior is as desired when other code uses the language - // improperly. - EXPECT_FALSE(initialization_state_pointer->is_uninitialized()); - EXPECT_FALSE(initialization_state_pointer->is_valid()); + // Because initialization_state was constructed via placement new into a + // buffer that’s still valid and its destructor was called directly, this + // approximates use-after-free without risking that the memory formerly used + // for the InitializationState object has been repurposed. + EXPECT_FALSE(initialization_state->is_uninitialized()); + EXPECT_FALSE(initialization_state->is_valid()); } } // namespace