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/handler/handler.gyp b/handler/handler.gyp index 50479828..aefd4de5 100644 --- a/handler/handler.gyp +++ b/handler/handler.gyp @@ -112,118 +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/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', + ], + }, + ], + }], + ], }], ], } 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/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) { 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/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; 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,