From ffe4c1018c1b46ddf945261fe1c61f549ddc4fa9 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 18 Apr 2017 22:07:33 -0400 Subject: [PATCH 1/4] net: Update Blink source code references MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The references to RFC 2388 §3 and RFC 2047 are removed. RFC 7578 has replaced RFC 2388, and RFC 7578 acknowledges that the area of RFC 2388 called into question by the previous comment in this code was not widely adopted. The code does not violate RFC 7578, so the TODO is removed. Change-Id: Ie68cba49f9fbc95a4ae3a156783a6db3b406950c Reviewed-on: https://chromium-review.googlesource.com/481244 Reviewed-by: Robert Sesek --- util/net/http_multipart_builder.cc | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/util/net/http_multipart_builder.cc b/util/net/http_multipart_builder.cc index 640d540a..91105be9 100644 --- a/util/net/http_multipart_builder.cc +++ b/util/net/http_multipart_builder.cc @@ -40,8 +40,8 @@ std::string GenerateBoundaryString() { // characters from the set “'()+_,-./:=? ”, and not ending in a space. // However, some servers have been observed as dealing poorly with certain // nonalphanumeric characters. See - // blink/Source/platform/network/FormDataBuilder.cpp - // blink::FormDataBuilder::generateUniqueBoundaryString(). + // blink/Source/platform/network/FormDataEncoder.cpp + // blink::FormDataEncoder::GenerateUniqueBoundaryString(). // // This implementation produces a 56-character string with over 190 bits of // randomness (62^32 > 2^190). @@ -60,17 +60,12 @@ std::string GenerateBoundaryString() { // Escapes the specified name to be suitable for the name field of a // form-data part. std::string EncodeMIMEField(const std::string& name) { - // RFC 2388 §3 says to encode non-ASCII field names according to RFC 2047, but - // no browsers implement that behavior. Instead, they send field names in the - // page hosting the form’s encoding. However, some form of escaping is needed. // This URL-escapes the quote character and newline characters, per Blink. See - // blink/Source/platform/network/FormDataBuilder.cpp - // blink::appendQuotedString(). - // - // TODO(mark): This encoding is not necessarily correct, and the same code in - // Blink is marked with a FIXME. Blink does not escape the '%' character, - // that’s a local addition, but it seems appropriate to be able to decode the - // string properly. + // blink/Source/platform/network/FormDataEncoder.cpp + // blink::AppendQuotedString(). %-encoding is endorsed by RFC 7578 §2, with + // approval for otherwise unencoded UTF-8 given by RFC 7578 §5.1. Blink does + // not escape the '%' character, but it seems appropriate to do so in order to + // be able to decode the string properly. std::string encoded; for (char character : name) { switch (character) { From e04194afd91d4998b51f06214e71129da73e67a0 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 19 Apr 2017 13:22:08 -0400 Subject: [PATCH 2/4] win: Wrap TerminateProcess() to accept cdecl patches on x86 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TerminateProcess(), like most of the Windows API, is declared WINAPI, which is __stdcall on 32-bit x86. That means that the callee, TerminateProcess() itself, is responsible for cleaning up parameters on the stack on return. In https://crashpad.chromium.org/bug/179, crashes in ExceptionHandlerServer::OnNonCrashDumpEvent() were observed in ways that make it evident that TerminateProcess() has been patched with a __cdecl routine. The crucial difference between __stdcall and __cdecl is that the caller is responsible for stack parameter cleanup in __cdecl. The mismatch means that nobody cleans parameters from the stack, and the stack pointer has an unexpected value, which in the case of the Crashpad handler crash, results in TerminateProcess()’s second argument erroneously being used as the lock address in the call to ReleaseSRWLockExclusive() or LeaveCriticalSection(). As a workaround, on 32-bit x86, call through SafeTerminateProcess(), a custom assembly routine that’s compatible with either __stdcall or __cdecl implementations of TerminateProcess() by not trusting the value of the stack pointer on return from that function. Instead, the stack pointer is restored directly from the frame pointer. Bug: crashpad:179 Test: crashpad_util_test SafeTerminateProcess.*, others Change-Id: If9508f4eb7631020ea69ddbbe4a22eb335cdb325 Reviewed-on: https://chromium-review.googlesource.com/481180 Reviewed-by: Scott Graham --- client/crashpad_client_win.cc | 13 +- util/util.gyp | 3 + util/util_test.gyp | 9 + util/win/capture_context.asm | 4 +- util/win/exception_handler_server.cc | 3 +- util/win/safe_terminate_process.asm | 74 +++++++ util/win/safe_terminate_process.h | 55 ++++++ util/win/safe_terminate_process_test.cc | 185 ++++++++++++++++++ util/win/safe_terminate_process_test_child.cc | 22 +++ util/win/termination_codes.h | 2 +- 10 files changed, 360 insertions(+), 10 deletions(-) create mode 100644 util/win/safe_terminate_process.asm create mode 100644 util/win/safe_terminate_process.h create mode 100644 util/win/safe_terminate_process_test.cc create mode 100644 util/win/safe_terminate_process_test_child.cc diff --git a/client/crashpad_client_win.cc b/client/crashpad_client_win.cc index 7a3d0047..1a183c8a 100644 --- a/client/crashpad_client_win.cc +++ b/client/crashpad_client_win.cc @@ -42,6 +42,7 @@ #include "util/win/ntstatus_logging.h" #include "util/win/process_info.h" #include "util/win/registration_protocol_win.h" +#include "util/win/safe_terminate_process.h" #include "util/win/scoped_process_suspend.h" #include "util/win/termination_codes.h" #include "util/win/xp_compat.h" @@ -126,7 +127,7 @@ LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) { // here, rather than trying to signal to a handler that will never arrive, // and then sleeping unnecessarily. LOG(ERROR) << "crash server failed to launch, self-terminating"; - TerminateProcess(GetCurrentProcess(), kTerminationCodeCrashNoDump); + SafeTerminateProcess(GetCurrentProcess(), kTerminationCodeCrashNoDump); return EXCEPTION_CONTINUE_SEARCH; } @@ -171,7 +172,7 @@ LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) { LOG(ERROR) << "crash server did not respond, self-terminating"; - TerminateProcess(GetCurrentProcess(), kTerminationCodeCrashNoDump); + SafeTerminateProcess(GetCurrentProcess(), kTerminationCodeCrashNoDump); return EXCEPTION_CONTINUE_SEARCH; } @@ -733,8 +734,8 @@ void CrashpadClient::DumpWithoutCrash(const CONTEXT& context) { // Win32 APIs, so just use regular locking here in case of multiple threads // calling this function. If a crash occurs while we're in here, the worst // that can happen is that the server captures a partial dump for this path - // because on the other thread gathering a crash dump, it TerminateProcess()d, - // causing this one to abort. + // because another thread’s crash processing finished and the process was + // terminated before this thread’s non-crash processing could be completed. base::AutoLock lock(*g_non_crash_dump_lock); // Create a fake EXCEPTION_POINTERS to give the handler something to work @@ -777,8 +778,8 @@ void CrashpadClient::DumpWithoutCrash(const CONTEXT& context) { void CrashpadClient::DumpAndCrash(EXCEPTION_POINTERS* exception_pointers) { if (g_signal_exception == INVALID_HANDLE_VALUE) { LOG(ERROR) << "not connected"; - TerminateProcess(GetCurrentProcess(), - kTerminationCodeNotConnectedToHandler); + SafeTerminateProcess(GetCurrentProcess(), + kTerminationCodeNotConnectedToHandler); return; } diff --git a/util/util.gyp b/util/util.gyp index 5bc4de6f..74613f93 100644 --- a/util/util.gyp +++ b/util/util.gyp @@ -212,6 +212,8 @@ 'win/process_structs.h', 'win/registration_protocol_win.cc', 'win/registration_protocol_win.h', + 'win/safe_terminate_process.asm', + 'win/safe_terminate_process.h', 'win/scoped_handle.cc', 'win/scoped_handle.h', 'win/scoped_local_alloc.cc', @@ -316,6 +318,7 @@ }, { # else: OS!="win" 'sources!': [ 'win/capture_context.asm', + 'win/safe_terminate_process.asm', ], }], ['OS=="linux"', { diff --git a/util/util_test.gyp b/util/util_test.gyp index 95dac082..20e0c954 100644 --- a/util/util_test.gyp +++ b/util/util_test.gyp @@ -102,6 +102,7 @@ 'win/initial_client_data_test.cc', 'win/process_info_test.cc', 'win/registration_protocol_win_test.cc', + 'win/safe_terminate_process_test.cc', 'win/scoped_process_suspend_test.cc', 'win/session_end_watcher_test.cc', 'win/time_test.cc', @@ -117,6 +118,7 @@ ['OS=="win"', { 'dependencies': [ 'crashpad_util_test_process_info_test_child', + 'crashpad_util_test_safe_terminate_process_test_child', ], 'link_settings': { 'libraries': [ @@ -171,6 +173,13 @@ }, }, }, + { + 'target_name': 'crashpad_util_test_safe_terminate_process_test_child', + 'type': 'executable', + 'sources': [ + 'win/safe_terminate_process_test_child.cc', + ], + }, ] }], ], diff --git a/util/win/capture_context.asm b/util/win/capture_context.asm index 56efec7f..b0093ba1 100644 --- a/util/win/capture_context.asm +++ b/util/win/capture_context.asm @@ -21,7 +21,7 @@ endif ifdef _M_IX86 .586 -.XMM +.xmm .model flat endif @@ -212,7 +212,7 @@ CONTEXT ends endif ; namespace crashpad { -; void CaptureContext(CONTEXT* context) +; void CaptureContext(CONTEXT* context); ; } // namespace crashpad ifdef _M_IX86 CAPTURECONTEXT_SYMBOL equ ?CaptureContext@crashpad@@YAXPAU_CONTEXT@@@Z diff --git a/util/win/exception_handler_server.cc b/util/win/exception_handler_server.cc index fdc159c0..6642665c 100644 --- a/util/win/exception_handler_server.cc +++ b/util/win/exception_handler_server.cc @@ -34,6 +34,7 @@ #include "util/win/get_function.h" #include "util/win/handle.h" #include "util/win/registration_protocol_win.h" +#include "util/win/safe_terminate_process.h" #include "util/win/xp_compat.h" namespace crashpad { @@ -546,7 +547,7 @@ void __stdcall ExceptionHandlerServer::OnCrashDumpEvent(void* ctx, BOOLEAN) { client->crash_exception_information_address(), client->debug_critical_section_address()); - TerminateProcess(client->process(), exit_code); + SafeTerminateProcess(client->process(), exit_code); } // static diff --git a/util/win/safe_terminate_process.asm b/util/win/safe_terminate_process.asm new file mode 100644 index 00000000..b219a9e6 --- /dev/null +++ b/util/win/safe_terminate_process.asm @@ -0,0 +1,74 @@ +; Copyright 2017 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. + +; Detect ml64 assembling for x86_64 by checking for rax. +ifdef rax +_M_X64 equ 1 +else +_M_IX86 equ 1 +endif + +ifdef _M_IX86 +.586 +.xmm +.model flat + +includelib kernel32.lib + +extern __imp__TerminateProcess@8:proc + +; namespace crashpad { +; bool SafeTerminateProcess(HANDLE process, UINT exit_code); +; } // namespace crashpad +SAFETERMINATEPROCESS_SYMBOL equ ?SafeTerminateProcess@crashpad@@YA_NPAXI@Z + +_TEXT segment +public SAFETERMINATEPROCESS_SYMBOL + +SAFETERMINATEPROCESS_SYMBOL proc + + ; This function is written in assembler source because it’s important for it + ; to not be inlined, for it to allocate a stack frame, and most critically, + ; for it to not trust esp on return from TerminateProcess(). + ; __declspec(noinline) can prevent inlining and #pragma optimize("y", off) can + ; disable frame pointer omission, but there’s no way to force a C compiler to + ; distrust esp, and even if there was a way, it’d probably be fragile. + + push ebp + mov ebp, esp + + push [ebp+12] + push [ebp+8] + call dword ptr [__imp__TerminateProcess@8] + + ; Convert from BOOL to bool. + test eax, eax + setne al + + ; TerminateProcess() is supposed to be stdcall (callee clean-up), and esp and + ; ebp are expected to already be equal. But if it’s been patched badly by + ; something that’s cdecl (caller clean-up), this next move will get things + ; back on track. + mov esp, ebp + pop ebp + + ret + +SAFETERMINATEPROCESS_SYMBOL endp + +_TEXT ends + +endif + +end diff --git a/util/win/safe_terminate_process.h b/util/win/safe_terminate_process.h new file mode 100644 index 00000000..7079eb2a --- /dev/null +++ b/util/win/safe_terminate_process.h @@ -0,0 +1,55 @@ +// Copyright 2017 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_WIN_SAFE_TERMINATE_PROCESS_H_ +#define CRASHPAD_UTIL_WIN_SAFE_TERMINATE_PROCESS_H_ + +#include + +#include "build/build_config.h" + +namespace crashpad { + +//! \brief Calls `TerminateProcess()`. +//! +//! `TerminateProcess()` has been observed in the wild as being patched badly on +//! 32-bit x86: it’s patched with code adhering to the `cdecl` (caller clean-up) +//! convention, although it’s supposed to be `stdcall` (callee clean-up). The +//! mix-up means that neither caller nor callee perform parameter clean-up from +//! the stack, causing the stack pointer to have an unexpected value on return +//! from the patched function. This typically results in a crash shortly +//! thereafter. See Crashpad bug +//! 179. +//! +//! On 32-bit x86, this replacement function calls `TerminateProcess()` without +//! making any assumptions about the stack pointer on its return. As such, it’s +//! compatible with the badly patched `cdecl` version as well as the native +//! `stdcall` version (and other less badly patched versions). +//! +//! Elsewhere, this function calls `TerminateProcess()` directly without any +//! additional fanfare. +//! +//! Call this function instead of `TerminateProcess()` anywhere that +//! `TerminateProcess()` would normally be called. +bool SafeTerminateProcess(HANDLE process, UINT exit_code); + +#if !defined(ARCH_CPU_X86) +inline bool SafeTerminateProcess(HANDLE process, UINT exit_code) { + return TerminateProcess(process, exit_code) != FALSE; +} +#endif // !ARCH_CPU_X86 + +} // namespace crashpad + +#endif // CRASHPAD_UTIL_WIN_SAFE_TERMINATE_PROCESS_H_ diff --git a/util/win/safe_terminate_process_test.cc b/util/win/safe_terminate_process_test.cc new file mode 100644 index 00000000..2fbc8483 --- /dev/null +++ b/util/win/safe_terminate_process_test.cc @@ -0,0 +1,185 @@ +// Copyright 2017 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. + +#include "util/win/safe_terminate_process.h" + +#include + +#include +#include + +#include "base/files/file_path.h" +#include "base/logging.h" +#include "base/macros.h" +#include "build/build_config.h" +#include "gtest/gtest.h" +#include "test/errors.h" +#include "test/test_paths.h" +#include "test/win/child_launcher.h" +#include "util/win/scoped_handle.h" + +namespace crashpad { +namespace test { +namespace { + +// Patches executable code, saving a copy of the original code so that it can be +// restored on destruction. +class ScopedExecutablePatch { + public: + ScopedExecutablePatch(void* target, const void* source, size_t size) + : original_(new uint8_t[size]), target_(target), size_(size) { + memcpy(original_.get(), target_, size_); + + ScopedVirtualProtectRWX protect_rwx(target_, size_); + memcpy(target_, source, size_); + } + + ~ScopedExecutablePatch() { + ScopedVirtualProtectRWX protect_rwx(target_, size_); + memcpy(target_, original_.get(), size_); + } + + private: + // Sets the protection on (address, size) to PAGE_EXECUTE_READWRITE by calling + // VirtualProtect(), and restores the original protection on destruction. Note + // that the region may span multiple pages, but the first page’s original + // protection will be applied to the entire region on destruction. This + // shouldn’t be a problem in practice for patching a function for this test’s + // purposes. + class ScopedVirtualProtectRWX { + public: + // If either the constructor or destructor fails, PCHECK() to terminate + // immediately, because the process will be in a weird and untrustworthy + // state, and gtest error handling isn’t worthwhile at that point. + + ScopedVirtualProtectRWX(void* address, size_t size) + : address_(address), size_(size) { + PCHECK(VirtualProtect( + address_, size_, PAGE_EXECUTE_READWRITE, &old_protect_)) + << "VirtualProtect"; + } + + ~ScopedVirtualProtectRWX() { + DWORD last_protect_; + PCHECK(VirtualProtect(address_, size_, old_protect_, &last_protect_)) + << "VirtualProtect"; + } + + private: + void* address_; + size_t size_; + DWORD old_protect_; + + DISALLOW_COPY_AND_ASSIGN(ScopedVirtualProtectRWX); + }; + + std::unique_ptr original_; + void* target_; + size_t size_; + + DISALLOW_COPY_AND_ASSIGN(ScopedExecutablePatch); +}; + +TEST(SafeTerminateProcess, PatchBadly) { + // This is a test of SafeTerminateProcess(), but it doesn’t actually terminate + // anything. Instead, it works with a process handle for the current process + // that doesn’t have PROCESS_TERMINATE access. The whole point of this test is + // to patch the real TerminateProcess() badly with a cdecl implementation to + // ensure that SafeTerminateProcess() can recover from such gross misconduct. + // The actual termination isn’t relevant to this test. + // + // Notably, don’t duplicate the process handle with PROCESS_TERMINATE access + // or with the DUPLICATE_SAME_ACCESS option. The SafeTerminateProcess() calls + // that follow operate on a duplicate of the current process’ process handle, + // and they’re supposed to fail, not terminate this process. + HANDLE process; + ASSERT_TRUE(DuplicateHandle(GetCurrentProcess(), + GetCurrentProcess(), + GetCurrentProcess(), + &process, + PROCESS_QUERY_INFORMATION, + false, + 0)) + << ErrorMessage("DuplicateHandle"); + ScopedKernelHANDLE process_owner(process); + + // Make sure that TerminateProcess() works as a baseline. + SetLastError(ERROR_SUCCESS); + EXPECT_FALSE(TerminateProcess(process, 0)); + EXPECT_EQ(GetLastError(), ERROR_ACCESS_DENIED); + + // Make sure that SafeTerminateProcess() works, calling through to + // TerminateProcess() properly. + SetLastError(ERROR_SUCCESS); + EXPECT_FALSE(SafeTerminateProcess(process, 0)); + EXPECT_EQ(GetLastError(), ERROR_ACCESS_DENIED); + + { + // Patch TerminateProcess() badly. This turns it into a no-op that returns 0 + // without cleaning up arguments from the stack, as a stdcall function is + // expected to do. + // + // This simulates the unexpected cdecl-patched TerminateProcess() as seen at + // https://crashpad.chromium.org/bug/179. In reality, this only affects + // 32-bit x86, as there’s no calling convention confusion on x86_64. It + // doesn’t hurt to run this test in the 64-bit environment, though. + const uint8_t patch[] = { +#if defined(ARCH_CPU_X86) + 0x31, 0xc0, // xor eax, eax +#elif defined(ARCH_CPU_X86_64) + 0x48, 0x31, 0xc0, // xor rax, rax +#else +#error Port +#endif + 0xc3, // ret + }; + + void* target = reinterpret_cast(TerminateProcess); + ScopedExecutablePatch executable_patch(target, patch, arraysize(patch)); + + // Make sure that SafeTerminateProcess() can be called. Since it’s been + // patched with a no-op stub, GetLastError() shouldn’t be modified. + SetLastError(ERROR_SUCCESS); + EXPECT_FALSE(SafeTerminateProcess(process, 0)); + EXPECT_EQ(GetLastError(), ERROR_SUCCESS); + } + + // Now that the real TerminateProcess() has been restored, verify that it + // still works properly. + SetLastError(ERROR_SUCCESS); + EXPECT_FALSE(SafeTerminateProcess(process, 0)); + EXPECT_EQ(GetLastError(), ERROR_ACCESS_DENIED); +} + +TEST(SafeTerminateProcess, TerminateChild) { + base::FilePath test_executable = TestPaths::Executable(); + std::wstring child_executable = + test_executable.DirName() + .Append(test_executable.BaseName().RemoveFinalExtension().value() + + L"_safe_terminate_process_test_child.exe") + .value(); + + ChildLauncher child(child_executable, std::wstring()); + ASSERT_NO_FATAL_FAILURE(child.Start()); + + constexpr DWORD kExitCode = 0x51ee9d1e; // Sort of like “sleep and die.” + + ASSERT_TRUE(SafeTerminateProcess(child.process_handle(), kExitCode)) + << ErrorMessage("TerminateProcess"); + EXPECT_EQ(child.WaitForExit(), kExitCode); +} + +} // namespace +} // namespace test +} // namespace crashpad diff --git a/util/win/safe_terminate_process_test_child.cc b/util/win/safe_terminate_process_test_child.cc new file mode 100644 index 00000000..605778b0 --- /dev/null +++ b/util/win/safe_terminate_process_test_child.cc @@ -0,0 +1,22 @@ +// Copyright 2017 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. + +#include +#include +#include + +int wmain(int argc, wchar_t** argv) { + Sleep(INFINITE); + return EXIT_FAILURE; +} diff --git a/util/win/termination_codes.h b/util/win/termination_codes.h index 1bcbf5dd..ce7fdba9 100644 --- a/util/win/termination_codes.h +++ b/util/win/termination_codes.h @@ -18,7 +18,7 @@ namespace crashpad { //! \brief Crashpad-specific codes that are used as arguments to -//! `TerminateProcess()` in unusual circumstances. +//! SafeTerminateProcess() or `TerminateProcess()` in unusual circumstances. enum TerminationCodes : unsigned int { //! \brief The crash handler did not respond, and the client self-terminated. kTerminationCodeCrashNoDump = 0xffff7001, From 74fddc3fed2a983bc335b03cd94ed60cc9dbc908 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 19 Apr 2017 13:38:26 -0400 Subject: [PATCH 3/4] win: Wrap test::ChildLauncher::Start() in ASSERT_NO_FATAL_FAILURE() Test: crashpad_snapshot_test, crashpad_util_test, end_to_end_test Change-Id: I09581521678fe3b083c409f308eeab2e583b3c9f Reviewed-on: https://chromium-review.googlesource.com/481245 Commit-Queue: Mark Mentovai Reviewed-by: Scott Graham --- handler/handler.gyp | 1 + handler/win/crash_other_program.cc | 5 +++++ snapshot/win/exception_snapshot_win_test.cc | 4 ++-- snapshot/win/extra_memory_ranges_test.cc | 2 +- snapshot/win/pe_image_annotations_reader_test.cc | 2 +- snapshot/win/process_snapshot_win_test.cc | 2 +- test/win/child_launcher.h | 3 +++ util/win/process_info_test.cc | 2 +- 8 files changed, 15 insertions(+), 6 deletions(-) diff --git a/handler/handler.gyp b/handler/handler.gyp index 50479828..1f8fd751 100644 --- a/handler/handler.gyp +++ b/handler/handler.gyp @@ -147,6 +147,7 @@ 'dependencies': [ '../client/client.gyp:crashpad_client', '../test/test.gyp:crashpad_test', + '../third_party/gtest/gtest.gyp:gtest', '../third_party/mini_chromium/mini_chromium.gyp:base', '../util/util.gyp:crashpad_util', ], diff --git a/handler/win/crash_other_program.cc b/handler/win/crash_other_program.cc index 389aee1f..93a3a07a 100644 --- a/handler/win/crash_other_program.cc +++ b/handler/win/crash_other_program.cc @@ -20,6 +20,7 @@ #include "base/logging.h" #include "base/strings/stringprintf.h" #include "client/crashpad_client.h" +#include "gtest/gtest.h" #include "test/test_paths.h" #include "test/win/child_launcher.h" #include "util/file/file_io.h" @@ -93,6 +94,10 @@ int CrashOtherProgram(int argc, wchar_t* argv[]) { test_executable.DirName().Append(L"hanging_program.exe").value(); ChildLauncher child(child_test_executable, argv[1]); child.Start(); + if (testing::Test::HasFatalFailure()) { + LOG(ERROR) << "failed to start child"; + return EXIT_FAILURE; + } // Wait until it's ready. char c; diff --git a/snapshot/win/exception_snapshot_win_test.cc b/snapshot/win/exception_snapshot_win_test.cc index e2776964..286e7a6f 100644 --- a/snapshot/win/exception_snapshot_win_test.cc +++ b/snapshot/win/exception_snapshot_win_test.cc @@ -148,7 +148,7 @@ void TestCrashingChild(const base::string16& directory_modification) { L"_crashing_child.exe") .value(); ChildLauncher child(child_test_executable, pipe_name); - child.Start(); + ASSERT_NO_FATAL_FAILURE(child.Start()); // The child tells us (approximately) where it will crash. WinVMAddress break_near_address; @@ -256,7 +256,7 @@ void TestDumpWithoutCrashingChild( L"_dump_without_crashing.exe") .value(); ChildLauncher child(child_test_executable, pipe_name); - child.Start(); + ASSERT_NO_FATAL_FAILURE(child.Start()); // The child tells us (approximately) where it will capture a dump. WinVMAddress dump_near_address; diff --git a/snapshot/win/extra_memory_ranges_test.cc b/snapshot/win/extra_memory_ranges_test.cc index 49d835f0..a012027b 100644 --- a/snapshot/win/extra_memory_ranges_test.cc +++ b/snapshot/win/extra_memory_ranges_test.cc @@ -53,7 +53,7 @@ void TestExtraMemoryRanges(TestType type, L"_extra_memory_ranges.exe") .value(); ChildLauncher child(child_test_executable, L""); - child.Start(); + ASSERT_NO_FATAL_FAILURE(child.Start()); // Wait for the child process to indicate that it's done setting up its // annotations via the CrashpadInfo interface. diff --git a/snapshot/win/pe_image_annotations_reader_test.cc b/snapshot/win/pe_image_annotations_reader_test.cc index 02a61232..a20e986b 100644 --- a/snapshot/win/pe_image_annotations_reader_test.cc +++ b/snapshot/win/pe_image_annotations_reader_test.cc @@ -57,7 +57,7 @@ void TestAnnotationsOnCrash(TestType type, L"_simple_annotations.exe") .value(); ChildLauncher child(child_test_executable, L""); - child.Start(); + ASSERT_NO_FATAL_FAILURE(child.Start()); // Wait for the child process to indicate that it's done setting up its // annotations via the CrashpadInfo interface. diff --git a/snapshot/win/process_snapshot_win_test.cc b/snapshot/win/process_snapshot_win_test.cc index 75e6e708..a0e5cef4 100644 --- a/snapshot/win/process_snapshot_win_test.cc +++ b/snapshot/win/process_snapshot_win_test.cc @@ -46,7 +46,7 @@ void TestImageReaderChild(const base::string16& directory_modification) { L"_image_reader.exe") .value(); ChildLauncher child(child_test_executable, done_uuid.ToString16()); - child.Start(); + ASSERT_NO_FATAL_FAILURE(child.Start()); char c; ASSERT_TRUE( diff --git a/test/win/child_launcher.h b/test/win/child_launcher.h index 22463fa5..6674efe2 100644 --- a/test/win/child_launcher.h +++ b/test/win/child_launcher.h @@ -39,6 +39,9 @@ class ChildLauncher { //! \brief Starts the child process, after which the handle functions below //! will be valid. + //! + //! Errors are signaled via gtest assertions. This method may be invoked via + //! `ASSERT_NO_FATAL_FAILURE()` to assert that it succeeds. void Start(); //! \brief Waits for the child process to exit. diff --git a/util/win/process_info_test.cc b/util/win/process_info_test.cc index 54459424..322a987f 100644 --- a/util/win/process_info_test.cc +++ b/util/win/process_info_test.cc @@ -151,7 +151,7 @@ void TestOtherProcess(const base::string16& directory_modification) { AppendCommandLineArgument(done_uuid.ToString16(), &args); ChildLauncher child(child_test_executable, args); - child.Start(); + ASSERT_NO_FATAL_FAILURE(child.Start()); // The child sends us a code address we can look up in the memory map. WinVMAddress code_address; From f487da4ff2c47a129e2f8f3a7e0c769b54e4585f Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 19 Apr 2017 13:49:37 -0400 Subject: [PATCH 4/4] win handler: Move test targets to handler_test.gyp Test: end_to_end_test Change-Id: I1fb01e0a6e701c8ec3958b68e2665cd4348a2242 Reviewed-on: https://chromium-review.googlesource.com/481083 Reviewed-by: Scott Graham --- handler/handler.gyp | 112 ---------------------------- handler/handler_test.gyp | 156 +++++++++++++++++++++++++++++++++------ 2 files changed, 134 insertions(+), 134 deletions(-) diff --git a/handler/handler.gyp b/handler/handler.gyp index 1f8fd751..aefd4de5 100644 --- a/handler/handler.gyp +++ b/handler/handler.gyp @@ -112,119 +112,7 @@ }, ], }, - { - 'target_name': 'crashy_program', - 'type': 'executable', - 'dependencies': [ - '../client/client.gyp:crashpad_client', - '../third_party/mini_chromium/mini_chromium.gyp:base', - '../util/util.gyp:crashpad_util', - ], - 'include_dirs': [ - '..', - ], - 'sources': [ - 'win/crashy_test_program.cc', - ], - }, - { - 'target_name': 'crashy_signal', - 'type': 'executable', - 'dependencies': [ - '../client/client.gyp:crashpad_client', - '../third_party/mini_chromium/mini_chromium.gyp:base', - ], - 'include_dirs': [ - '..', - ], - 'sources': [ - 'win/crashy_signal.cc', - ], - }, - { - 'target_name': 'crash_other_program', - 'type': 'executable', - 'dependencies': [ - '../client/client.gyp:crashpad_client', - '../test/test.gyp:crashpad_test', - '../third_party/gtest/gtest.gyp:gtest', - '../third_party/mini_chromium/mini_chromium.gyp:base', - '../util/util.gyp:crashpad_util', - ], - 'sources': [ - 'win/crash_other_program.cc', - ], - }, - { - 'target_name': 'fake_handler_that_crashes_at_startup', - 'type': 'executable', - 'sources': [ - 'win/fake_handler_that_crashes_at_startup.cc', - ], - }, - { - 'target_name': 'hanging_program', - 'type': 'executable', - 'dependencies': [ - '../client/client.gyp:crashpad_client', - '../third_party/mini_chromium/mini_chromium.gyp:base', - ], - 'sources': [ - 'win/hanging_program.cc', - ], - }, - { - 'target_name': 'loader_lock_dll', - 'type': 'loadable_module', - 'sources': [ - 'win/loader_lock_dll.cc', - ], - 'msvs_settings': { - 'NoImportLibrary': 'true', - }, - }, - { - 'target_name': 'self_destroying_program', - 'type': 'executable', - 'dependencies': [ - '../client/client.gyp:crashpad_client', - '../compat/compat.gyp:crashpad_compat', - '../snapshot/snapshot.gyp:crashpad_snapshot', - '../third_party/mini_chromium/mini_chromium.gyp:base', - '../util/util.gyp:crashpad_util', - ], - 'include_dirs': [ - '..', - ], - 'sources': [ - 'win/self_destroying_test_program.cc', - ], - }, ], - 'conditions': [ - # Cannot create an x64 DLL with embedded debug info. - ['target_arch=="ia32"', { - 'targets': [ - { - 'target_name': 'crashy_z7_loader', - 'type': 'executable', - 'dependencies': [ - '../client/client.gyp:crashpad_client', - '../test/test.gyp:crashpad_test', - '../third_party/mini_chromium/mini_chromium.gyp:base', - ], - 'include_dirs': [ - '..', - ], - 'sources': [ - 'win/crashy_test_z7_loader.cc', - ], - }, - ], - }], - ], - }, { - 'targets': [], }], ], } diff --git a/handler/handler_test.gyp b/handler/handler_test.gyp index e5264182..588305f8 100644 --- a/handler/handler_test.gyp +++ b/handler/handler_test.gyp @@ -37,28 +37,140 @@ ], 'conditions': [ ['OS=="win"', { - 'targets': [{ - # The handler is only tested on Windows for now. - 'target_name': 'crashpad_handler_test', - 'type': 'executable', - 'dependencies': [ - 'crashpad_handler_test_extended_handler', - 'handler.gyp:crashpad_handler_lib', - '../client/client.gyp:crashpad_client', - '../compat/compat.gyp:crashpad_compat', - '../test/test.gyp:crashpad_gtest_main', - '../test/test.gyp:crashpad_test', - '../third_party/gtest/gtest.gyp:gtest', - '../third_party/mini_chromium/mini_chromium.gyp:base', - '../util/util.gyp:crashpad_util', - ], - 'include_dirs': [ - '..', - ], - 'sources': [ - 'crashpad_handler_test.cc', - ], - }], + 'targets': [ + { + 'target_name': 'crash_other_program', + 'type': 'executable', + 'dependencies': [ + '../client/client.gyp:crashpad_client', + '../test/test.gyp:crashpad_test', + '../third_party/gtest/gtest.gyp:gtest', + '../third_party/mini_chromium/mini_chromium.gyp:base', + '../util/util.gyp:crashpad_util', + ], + 'sources': [ + 'win/crash_other_program.cc', + ], + }, + { + # The handler is only tested on Windows for now. + 'target_name': 'crashpad_handler_test', + 'type': 'executable', + 'dependencies': [ + 'crashpad_handler_test_extended_handler', + 'handler.gyp:crashpad_handler_lib', + '../client/client.gyp:crashpad_client', + '../compat/compat.gyp:crashpad_compat', + '../test/test.gyp:crashpad_gtest_main', + '../test/test.gyp:crashpad_test', + '../third_party/gtest/gtest.gyp:gtest', + '../third_party/mini_chromium/mini_chromium.gyp:base', + '../util/util.gyp:crashpad_util', + ], + 'include_dirs': [ + '..', + ], + 'sources': [ + 'crashpad_handler_test.cc', + ], + }, + { + 'target_name': 'crashy_program', + 'type': 'executable', + 'dependencies': [ + '../client/client.gyp:crashpad_client', + '../third_party/mini_chromium/mini_chromium.gyp:base', + '../util/util.gyp:crashpad_util', + ], + 'include_dirs': [ + '..', + ], + 'sources': [ + 'win/crashy_test_program.cc', + ], + }, + { + 'target_name': 'crashy_signal', + 'type': 'executable', + 'dependencies': [ + '../client/client.gyp:crashpad_client', + '../third_party/mini_chromium/mini_chromium.gyp:base', + ], + 'include_dirs': [ + '..', + ], + 'sources': [ + 'win/crashy_signal.cc', + ], + }, + { + 'target_name': 'fake_handler_that_crashes_at_startup', + 'type': 'executable', + 'sources': [ + 'win/fake_handler_that_crashes_at_startup.cc', + ], + }, + { + 'target_name': 'hanging_program', + 'type': 'executable', + 'dependencies': [ + '../client/client.gyp:crashpad_client', + '../third_party/mini_chromium/mini_chromium.gyp:base', + ], + 'sources': [ + 'win/hanging_program.cc', + ], + }, + { + 'target_name': 'loader_lock_dll', + 'type': 'loadable_module', + 'sources': [ + 'win/loader_lock_dll.cc', + ], + 'msvs_settings': { + 'NoImportLibrary': 'true', + }, + }, + { + 'target_name': 'self_destroying_program', + 'type': 'executable', + 'dependencies': [ + '../client/client.gyp:crashpad_client', + '../compat/compat.gyp:crashpad_compat', + '../snapshot/snapshot.gyp:crashpad_snapshot', + '../third_party/mini_chromium/mini_chromium.gyp:base', + '../util/util.gyp:crashpad_util', + ], + 'include_dirs': [ + '..', + ], + 'sources': [ + 'win/self_destroying_test_program.cc', + ], + }, + ], + 'conditions': [ + # Cannot create an x64 DLL with embedded debug info. + ['target_arch=="ia32"', { + 'targets': [ + { + 'target_name': 'crashy_z7_loader', + 'type': 'executable', + 'dependencies': [ + '../client/client.gyp:crashpad_client', + '../test/test.gyp:crashpad_test', + '../third_party/mini_chromium/mini_chromium.gyp:base', + ], + 'include_dirs': [ + '..', + ], + 'sources': [ + 'win/crashy_test_z7_loader.cc', + ], + }, + ], + }], + ], }], ], }