From 26804a0be1f2a9d012cf1026130a3747fbcce409 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Mon, 9 Mar 2015 18:02:14 -0400 Subject: [PATCH] Add ASSERT_DEATH_CHECK() to do ASSERT_DEATH() of CHECK() failures. Likewise for EXPECT_DEATH_CHECK() and EXPECT_DEATH(). In the in-Chromium build configured for official builds in Release mode, CHECK() throws away its condition string and stream parameters without ever printing them, although it still evaluates the condition and triggers death appropriately. {ASSERT,EXPECT}_DEATH(statement, regex) will not work correctly for any regex that attempts to match what CHECK() prints. In these build configurations, {ASSERT,EXPECT}_DEATH_CHECK() use a match-all regex (""). In other build configurations, they transparently wrap {ASSERT,EXPECT}_DEATH(). BUG=crashpad:12 R=rsesek@chromium.org, scottmg@chromium.org Review URL: https://codereview.chromium.org/992693003 --- client/simple_string_dictionary_test.cc | 7 ++- minidump/minidump.gyp | 2 + minidump/minidump_exception_writer_test.cc | 9 ++- minidump/minidump_file_writer_test.cc | 5 +- minidump/minidump_module_writer_test.cc | 4 +- minidump/minidump_system_info_writer_test.cc | 5 +- minidump/minidump_thread_writer_test.cc | 6 +- .../composite_mach_message_server_test.cc | 3 +- util/misc/initialization_state_dcheck_test.cc | 33 ++++++----- util/misc/scoped_forbid_return_test.cc | 13 ++++- util/net/http_multipart_builder_test.cc | 13 +++-- util/test/gtest_death_check.h | 55 +++++++++++++++++++ util/test/multiprocess_posix_test.cc | 11 ++-- util/util.gyp | 1 + 14 files changed, 127 insertions(+), 40 deletions(-) create mode 100644 util/test/gtest_death_check.h diff --git a/client/simple_string_dictionary_test.cc b/client/simple_string_dictionary_test.cc index 173fee31..26a918bc 100644 --- a/client/simple_string_dictionary_test.cc +++ b/client/simple_string_dictionary_test.cc @@ -16,6 +16,7 @@ #include "base/logging.h" #include "gtest/gtest.h" +#include "util/test/gtest_death_check.h" namespace crashpad { namespace test { @@ -279,13 +280,13 @@ TEST(SimpleStringDictionary, OutOfSpace) { TEST(SimpleStringDictionaryDeathTest, NullKey) { TSimpleStringDictionary<4, 6, 6> map; - ASSERT_DEATH(map.SetKeyValue(nullptr, "hello"), "key"); + ASSERT_DEATH_CHECK(map.SetKeyValue(nullptr, "hello"), "key"); map.SetKeyValue("hi", "there"); - ASSERT_DEATH(map.GetValueForKey(nullptr), "key"); + ASSERT_DEATH_CHECK(map.GetValueForKey(nullptr), "key"); EXPECT_STREQ("there", map.GetValueForKey("hi")); - ASSERT_DEATH(map.GetValueForKey(nullptr), "key"); + ASSERT_DEATH_CHECK(map.GetValueForKey(nullptr), "key"); map.RemoveKey("hi"); EXPECT_EQ(0u, map.GetCount()); } diff --git a/minidump/minidump.gyp b/minidump/minidump.gyp index 3fcd2494..cf1d7f37 100644 --- a/minidump/minidump.gyp +++ b/minidump/minidump.gyp @@ -81,6 +81,8 @@ '../third_party/gtest/gtest.gyp:gtest', '../third_party/gtest/gtest.gyp:gtest_main', '../third_party/mini_chromium/mini_chromium.gyp:base', + '../util/util.gyp:crashpad_util', + '../util/util.gyp:crashpad_util_test', ], 'include_dirs': [ '..', diff --git a/minidump/minidump_exception_writer_test.cc b/minidump/minidump_exception_writer_test.cc index a0866ba0..c8adeb43 100644 --- a/minidump/minidump_exception_writer_test.cc +++ b/minidump/minidump_exception_writer_test.cc @@ -34,6 +34,7 @@ #include "snapshot/test/test_cpu_context.h" #include "snapshot/test/test_exception_snapshot.h" #include "util/file/string_file.h" +#include "util/test/gtest_death_check.h" namespace crashpad { namespace test { @@ -262,15 +263,17 @@ TEST(MinidumpExceptionWriterDeathTest, NoContext) { minidump_file_writer.AddStream(exception_writer.Pass()); StringFile string_file; - ASSERT_DEATH(minidump_file_writer.WriteEverything(&string_file), "context_"); + ASSERT_DEATH_CHECK(minidump_file_writer.WriteEverything(&string_file), + "context_"); } TEST(MinidumpExceptionWriterDeathTest, TooMuchInformation) { MinidumpExceptionWriter exception_writer; std::vector exception_information(EXCEPTION_MAXIMUM_PARAMETERS + 1, 0x5a5a5a5a5a5a5a5a); - ASSERT_DEATH(exception_writer.SetExceptionInformation(exception_information), - "kMaxParameters"); + ASSERT_DEATH_CHECK( + exception_writer.SetExceptionInformation(exception_information), + "kMaxParameters"); } } // namespace diff --git a/minidump/minidump_file_writer_test.cc b/minidump/minidump_file_writer_test.cc index 5046b936..89eb8051 100644 --- a/minidump/minidump_file_writer_test.cc +++ b/minidump/minidump_file_writer_test.cc @@ -33,7 +33,7 @@ #include "snapshot/test/test_system_snapshot.h" #include "snapshot/test/test_thread_snapshot.h" #include "util/file/string_file.h" -#include "util/file/string_file.h" +#include "util/test/gtest_death_check.h" namespace crashpad { namespace test { @@ -436,7 +436,8 @@ TEST(MinidumpFileWriterDeathTest, SameStreamType) { const uint8_t kStream1Value = 0xa5; auto stream1 = make_scoped_ptr( new TestStream(kStream1Type, kStream1Size, kStream1Value)); - ASSERT_DEATH(minidump_file.AddStream(stream1.Pass()), "already present"); + ASSERT_DEATH_CHECK(minidump_file.AddStream(stream1.Pass()), + "already present"); } } // namespace diff --git a/minidump/minidump_module_writer_test.cc b/minidump/minidump_module_writer_test.cc index 6024c2ca..8087e36b 100644 --- a/minidump/minidump_module_writer_test.cc +++ b/minidump/minidump_module_writer_test.cc @@ -33,6 +33,7 @@ #include "util/file/string_file.h" #include "util/misc/uuid.h" #include "util/stdlib/pointer_container.h" +#include "util/test/gtest_death_check.h" namespace crashpad { namespace test { @@ -753,7 +754,8 @@ TEST(MinidumpModuleWriterDeathTest, NoModuleName) { minidump_file_writer.AddStream(module_list_writer.Pass()); StringFile string_file; - ASSERT_DEATH(minidump_file_writer.WriteEverything(&string_file), "name_"); + ASSERT_DEATH_CHECK(minidump_file_writer.WriteEverything(&string_file), + "name_"); } } // namespace diff --git a/minidump/minidump_system_info_writer_test.cc b/minidump/minidump_system_info_writer_test.cc index 09aa5c15..2ad7d7ea 100644 --- a/minidump/minidump_system_info_writer_test.cc +++ b/minidump/minidump_system_info_writer_test.cc @@ -30,6 +30,7 @@ #include "minidump/test/minidump_writable_test_util.h" #include "snapshot/test/test_system_snapshot.h" #include "util/file/string_file.h" +#include "util/test/gtest_death_check.h" namespace crashpad { namespace test { @@ -471,8 +472,8 @@ TEST(MinidumpSystemInfoWriterDeathTest, NoCSDVersion) { minidump_file_writer.AddStream(system_info_writer.Pass()); StringFile string_file; - ASSERT_DEATH(minidump_file_writer.WriteEverything(&string_file), - "csd_version_"); + ASSERT_DEATH_CHECK(minidump_file_writer.WriteEverything(&string_file), + "csd_version_"); } } // namespace diff --git a/minidump/minidump_thread_writer_test.cc b/minidump/minidump_thread_writer_test.cc index 291f70a3..94ec200a 100644 --- a/minidump/minidump_thread_writer_test.cc +++ b/minidump/minidump_thread_writer_test.cc @@ -34,6 +34,7 @@ #include "snapshot/test/test_memory_snapshot.h" #include "snapshot/test/test_thread_snapshot.h" #include "util/file/string_file.h" +#include "util/test/gtest_death_check.h" namespace crashpad { namespace test { @@ -674,11 +675,12 @@ TEST(MinidumpThreadWriterDeathTest, NoContext) { minidump_file_writer.AddStream(thread_list_writer.Pass()); StringFile string_file; - ASSERT_DEATH(minidump_file_writer.WriteEverything(&string_file), "context_"); + ASSERT_DEATH_CHECK(minidump_file_writer.WriteEverything(&string_file), + "context_"); } TEST(MinidumpThreadWriterDeathTest, InitializeFromSnapshot_NoContext) { - ASSERT_DEATH( + ASSERT_DEATH_CHECK( RunInitializeFromSnapshotTest( false), "context_"); } diff --git a/util/mach/composite_mach_message_server_test.cc b/util/mach/composite_mach_message_server_test.cc index f1ec963c..95f7d952 100644 --- a/util/mach/composite_mach_message_server_test.cc +++ b/util/mach/composite_mach_message_server_test.cc @@ -17,6 +17,7 @@ #include "base/strings/stringprintf.h" #include "gtest/gtest.h" #include "util/mach/mach_message.h" +#include "util/test/gtest_death_check.h" namespace crashpad { namespace test { @@ -295,7 +296,7 @@ TEST(CompositeMachMessageServerDeathTest, DuplicateRequestID) { CompositeMachMessageServer server; server.AddHandler(&handlers[0]); - EXPECT_DEATH(server.AddHandler(&handlers[1]), "duplicate request ID"); + EXPECT_DEATH_CHECK(server.AddHandler(&handlers[1]), "duplicate request ID"); } } // namespace diff --git a/util/misc/initialization_state_dcheck_test.cc b/util/misc/initialization_state_dcheck_test.cc index c9b595ac..c1c8e7e2 100644 --- a/util/misc/initialization_state_dcheck_test.cc +++ b/util/misc/initialization_state_dcheck_test.cc @@ -16,6 +16,7 @@ #include "base/logging.h" #include "gtest/gtest.h" +#include "util/test/gtest_death_check.h" namespace crashpad { namespace test { @@ -37,16 +38,18 @@ TEST(InitializationStateDcheckDeathTest, Uninitialized_NotInvalid) { // This tests that an attempt to set an uninitialized object as valid without // transitioning through the initializing (invalid) state fails. InitializationStateDcheck initialization_state_dcheck; - ASSERT_DEATH(INITIALIZATION_STATE_SET_VALID(initialization_state_dcheck), - "kStateInvalid"); + ASSERT_DEATH_CHECK( + INITIALIZATION_STATE_SET_VALID(initialization_state_dcheck), + "kStateInvalid"); } TEST(InitializationStateDcheckDeathTest, Uninitialized_NotValid) { // This tests that an attempt to use an uninitialized object as though it // were valid fails. InitializationStateDcheck initialization_state_dcheck; - ASSERT_DEATH(INITIALIZATION_STATE_DCHECK_VALID(initialization_state_dcheck), - "kStateValid"); + ASSERT_DEATH_CHECK( + INITIALIZATION_STATE_DCHECK_VALID(initialization_state_dcheck), + "kStateValid"); } TEST(InitializationStateDcheckDeathTest, Invalid_NotUninitialized) { @@ -54,7 +57,7 @@ TEST(InitializationStateDcheckDeathTest, Invalid_NotUninitialized) { // initialization was already attempted fails. InitializationStateDcheck initialization_state_dcheck; INITIALIZATION_STATE_SET_INITIALIZING(initialization_state_dcheck); - ASSERT_DEATH( + ASSERT_DEATH_CHECK( INITIALIZATION_STATE_SET_INITIALIZING(initialization_state_dcheck), "kStateUninitialized"); } @@ -64,8 +67,9 @@ TEST(InitializationStateDcheckDeathTest, Invalid_NotValid) { // were valid fails. InitializationStateDcheck initialization_state_dcheck; INITIALIZATION_STATE_SET_INITIALIZING(initialization_state_dcheck); - ASSERT_DEATH(INITIALIZATION_STATE_DCHECK_VALID(initialization_state_dcheck), - "kStateValid"); + ASSERT_DEATH_CHECK( + INITIALIZATION_STATE_DCHECK_VALID(initialization_state_dcheck), + "kStateValid"); } TEST(InitializationStateDcheckDeathTest, Valid_NotUninitialized) { @@ -74,7 +78,7 @@ TEST(InitializationStateDcheckDeathTest, Valid_NotUninitialized) { InitializationStateDcheck initialization_state_dcheck; INITIALIZATION_STATE_SET_INITIALIZING(initialization_state_dcheck); INITIALIZATION_STATE_SET_VALID(initialization_state_dcheck); - ASSERT_DEATH( + ASSERT_DEATH_CHECK( INITIALIZATION_STATE_SET_INITIALIZING(initialization_state_dcheck), "kStateUninitialized"); } @@ -85,8 +89,9 @@ TEST(InitializationStateDcheckDeathTest, Valid_NotInvalid) { InitializationStateDcheck initialization_state_dcheck; INITIALIZATION_STATE_SET_INITIALIZING(initialization_state_dcheck); INITIALIZATION_STATE_SET_VALID(initialization_state_dcheck); - ASSERT_DEATH(INITIALIZATION_STATE_SET_VALID(initialization_state_dcheck), - "kStateInvalid"); + ASSERT_DEATH_CHECK( + INITIALIZATION_STATE_SET_VALID(initialization_state_dcheck), + "kStateInvalid"); } TEST(InitializationStateDcheckDeathTest, Destroyed_NotUninitialized) { @@ -101,9 +106,9 @@ TEST(InitializationStateDcheckDeathTest, Destroyed_NotUninitialized) { INITIALIZATION_STATE_SET_VALID(initialization_state_dcheck); INITIALIZATION_STATE_DCHECK_VALID(initialization_state_dcheck); } - ASSERT_DEATH(INITIALIZATION_STATE_SET_INITIALIZING( - *initialization_state_dcheck_pointer), - "kStateUninitialized"); + ASSERT_DEATH_CHECK(INITIALIZATION_STATE_SET_INITIALIZING( + *initialization_state_dcheck_pointer), + "kStateUninitialized"); } TEST(InitializationStateDcheckDeathTest, Destroyed_NotValid) { @@ -118,7 +123,7 @@ TEST(InitializationStateDcheckDeathTest, Destroyed_NotValid) { INITIALIZATION_STATE_SET_VALID(initialization_state_dcheck); INITIALIZATION_STATE_DCHECK_VALID(initialization_state_dcheck); } - ASSERT_DEATH( + ASSERT_DEATH_CHECK( INITIALIZATION_STATE_DCHECK_VALID(*initialization_state_dcheck_pointer), "kStateValid"); } diff --git a/util/misc/scoped_forbid_return_test.cc b/util/misc/scoped_forbid_return_test.cc index 9a1b6d90..93a3e327 100644 --- a/util/misc/scoped_forbid_return_test.cc +++ b/util/misc/scoped_forbid_return_test.cc @@ -14,7 +14,9 @@ #include "util/misc/scoped_forbid_return.h" +#include "base/compiler_specific.h" #include "gtest/gtest.h" +#include "util/test/gtest_death_check.h" namespace crashpad { namespace test { @@ -44,12 +46,17 @@ void ScopedForbidReturnHelper(ForbidReturnType type) { const char kForbiddenMessage[] = "attempt to exit scope forbidden"; TEST(ScopedForbidReturnDeathTest, Default) { - ASSERT_DEATH(ScopedForbidReturnHelper(kForbidReturnDefault), - kForbiddenMessage); + // kForbiddenMessage may appear to be unused if ASSERT_DEATH_CHECK() throws it + // away. + ALLOW_UNUSED_LOCAL(kForbiddenMessage); + + ASSERT_DEATH_CHECK(ScopedForbidReturnHelper(kForbidReturnDefault), + kForbiddenMessage); } TEST(ScopedForbidReturnDeathTest, Armed) { - ASSERT_DEATH(ScopedForbidReturnHelper(kForbidReturnArmed), kForbiddenMessage); + ASSERT_DEATH_CHECK(ScopedForbidReturnHelper(kForbidReturnArmed), + kForbiddenMessage); } TEST(ScopedForbidReturn, Disarmed) { diff --git a/util/net/http_multipart_builder_test.cc b/util/net/http_multipart_builder_test.cc index e185e4ac..5517f67b 100644 --- a/util/net/http_multipart_builder_test.cc +++ b/util/net/http_multipart_builder_test.cc @@ -19,6 +19,7 @@ #include "gtest/gtest.h" #include "util/net/http_body.h" #include "util/net/http_body_test_util.h" +#include "util/test/gtest_death_check.h" #include "util/test/paths.h" namespace crashpad { @@ -273,10 +274,14 @@ TEST(HTTPMultipartBuilder, SharedFormDataAndAttachmentKeyNamespace) { TEST(HTTPMultipartBuilderDeathTest, AssertUnsafeMIMEType) { HTTPMultipartBuilder builder; // Invalid and potentially dangerous: - ASSERT_DEATH(builder.SetFileAttachment("", "", base::FilePath(), "\r\n"), ""); - ASSERT_DEATH(builder.SetFileAttachment("", "", base::FilePath(), "\""), ""); - ASSERT_DEATH(builder.SetFileAttachment("", "", base::FilePath(), "\x12"), ""); - ASSERT_DEATH(builder.SetFileAttachment("", "", base::FilePath(), "<>"), ""); + ASSERT_DEATH_CHECK( + builder.SetFileAttachment("", "", base::FilePath(), "\r\n"), ""); + ASSERT_DEATH_CHECK( + builder.SetFileAttachment("", "", base::FilePath(), "\""), ""); + ASSERT_DEATH_CHECK( + builder.SetFileAttachment("", "", base::FilePath(), "\x12"), ""); + ASSERT_DEATH_CHECK( + builder.SetFileAttachment("", "", base::FilePath(), "<>"), ""); // Invalid but safe: builder.SetFileAttachment("", "", base::FilePath(), "0/totally/-invalid.pdf"); // Valid and safe: diff --git a/util/test/gtest_death_check.h b/util/test/gtest_death_check.h new file mode 100644 index 00000000..b3900b8b --- /dev/null +++ b/util/test/gtest_death_check.h @@ -0,0 +1,55 @@ +// Copyright 2015 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef CRASHPAD_UTIL_TEST_GTEST_DEATH_CHECK_H_ +#define CRASHPAD_UTIL_TEST_GTEST_DEATH_CHECK_H_ + +#include "base/logging.h" +#include "gtest/gtest.h" + +//! \file + +#if !(!defined(MINI_CHROMIUM_BASE_LOGGING_H_) && \ + defined(OFFICIAL_BUILD) && \ + defined(NDEBUG)) || \ + DOXYGEN + +//! \brief Wraps the gtest `ASSERT_DEATH()` macro to make assertions about death +//! caused by `CHECK()` failures. +//! +//! In an in-Chromium build in the official configuration in Release mode, +//! `CHECK()` does not print its condition or streamed messages. In that case, +//! this macro uses an empty \a regex pattern when calling `ASSERT_DEATH()` to +//! avoid looking for any particular output on the standard error stream. In +//! other build configurations, the \a regex pattern is left intact. +#define ASSERT_DEATH_CHECK(statement, regex) ASSERT_DEATH(statement, regex) + +//! \brief Wraps the gtest `EXPECT_DEATH()` macro to make assertions about death +//! caused by `CHECK()` failures. +//! +//! In an in-Chromium build in the official configuration in Release mode, +//! `CHECK()` does not print its condition or streamed messages. In that case, +//! this macro uses an empty \a regex pattern when calling `EXPECT_DEATH()` to +//! avoid looking for any particular output on the standard error stream. In +//! other build configurations, the \a regex pattern is left intact. +#define EXPECT_DEATH_CHECK(statement, regex) EXPECT_DEATH(statement, regex) + +#else + +#define ASSERT_DEATH_CHECK(statement, regex) ASSERT_DEATH(statement, "") +#define EXPECT_DEATH_CHECK(statement, regex) EXPECT_DEATH(statement, "") + +#endif + +#endif // CRASHPAD_UTIL_TEST_GTEST_DEATH_CHECK_H_ diff --git a/util/test/multiprocess_posix_test.cc b/util/test/multiprocess_posix_test.cc index a8c7d9da..d0f133da 100644 --- a/util/test/multiprocess_posix_test.cc +++ b/util/test/multiprocess_posix_test.cc @@ -21,6 +21,7 @@ #include "base/basictypes.h" #include "gtest/gtest.h" #include "util/file/file_io.h" +#include "util/test/gtest_death_check.h" namespace crashpad { namespace test { @@ -189,7 +190,7 @@ class TestMultiprocessClosePipe final : public Multiprocess { // abort execution. Regardless of how SIGPIPE is handled, the process will // be terminated. Because the actual termination mechanism is not known, // no regex can be specified. - EXPECT_DEATH(CheckedWriteFile(WritePipeHandle(), &c, 1), ""); + EXPECT_DEATH_CHECK(CheckedWriteFile(WritePipeHandle(), &c, 1), ""); } } @@ -198,18 +199,18 @@ class TestMultiprocessClosePipe final : public Multiprocess { case kReadCloses: CloseReadPipe(); EXPECT_NE(-1, WritePipeHandle()); - EXPECT_DEATH(ReadPipeHandle(), "fd"); + EXPECT_DEATH_CHECK(ReadPipeHandle(), "fd"); break; case kWriteCloses: CloseWritePipe(); EXPECT_NE(-1, ReadPipeHandle()); - EXPECT_DEATH(WritePipeHandle(), "fd"); + EXPECT_DEATH_CHECK(WritePipeHandle(), "fd"); break; case kReadAndWriteClose: CloseReadPipe(); CloseWritePipe(); - EXPECT_DEATH(ReadPipeHandle(), "fd"); - EXPECT_DEATH(WritePipeHandle(), "fd"); + EXPECT_DEATH_CHECK(ReadPipeHandle(), "fd"); + EXPECT_DEATH_CHECK(WritePipeHandle(), "fd"); break; } } diff --git a/util/util.gyp b/util/util.gyp index ac95c855..4fe0009c 100644 --- a/util/util.gyp +++ b/util/util.gyp @@ -224,6 +224,7 @@ 'sources': [ 'test/errors.cc', 'test/errors.h', + 'test/gtest_death_check.h', 'test/mac/dyld.h', 'test/mac/mach_errors.cc', 'test/mac/mach_errors.h',