From 2bf08e6223b153592bc71685d09e0ec83af47cfb Mon Sep 17 00:00:00 2001 From: Clemens Backes Date: Tue, 14 Jun 2022 14:39:15 +0200 Subject: [PATCH 01/20] Detect blocked signal installation by sanitizers Sanitizers can prevent the installation of signal handlers, but sigaction would still return 0 (for success). Detect this by checking the installed signal handler via a second call to sigaction. R=mark@chromium.org Bug: chromium:1328749 Change-Id: I62a5777379ec5c6b1ca2d5a62e7cd3fb8ed1437b Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3702302 Reviewed-by: Mark Mentovai Commit-Queue: Clemens Backes --- util/posix/signals.cc | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/util/posix/signals.cc b/util/posix/signals.cc index f53ceb2a2f..93874e5426 100644 --- a/util/posix/signals.cc +++ b/util/posix/signals.cc @@ -147,6 +147,25 @@ bool Signals::InstallHandler(int sig, PLOG(ERROR) << "sigaction " << sig; return false; } + +// Sanitizers can prevent the installation of signal handlers, but sigaction +// does not report this as failure. Attempt to detect this by checking the +// currently installed signal handler. +#if defined(ADDRESS_SANITIZER) || defined(MEMORY_SANITIZER) || \ + defined(THREAD_SANITIZER) || defined(LEAK_SANITIZER) || \ + defined(UNDEFINED_SANITIZER) + struct sigaction installed_handler; + CHECK_EQ(sigaction(sig, nullptr, &installed_handler), 0); + // If the installed handler does not point to the just installed handler, then + // the allow_user_segv_handler sanitizer flag is (probably) disabled. + if (installed_handler.sa_sigaction != handler) { + LOG(WARNING) + << "sanitizers are preventing signal handler installation (sig " << sig + << ")"; + return false; + } +#endif + return true; } From f19ef3c6077e5318b723b92fa854c1000b3ba66f Mon Sep 17 00:00:00 2001 From: Alex Pankhurst Date: Mon, 13 Jun 2022 15:49:56 -0700 Subject: [PATCH 02/20] [fuchsia] Fix uninitialized fields Fuchsia's Crashpad roller was broken due to uninitialized fields in structs. Bug: fxbug.dev/101498 Change-Id: I1283afea9c5ac4eddb432590f9a5ec5cb1856a7c Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3704517 Reviewed-by: Joshua Peraza Commit-Queue: Alex Pankhurst --- snapshot/fuchsia/process_reader_fuchsia_test.cc | 2 +- snapshot/minidump/process_snapshot_minidump_test.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/snapshot/fuchsia/process_reader_fuchsia_test.cc b/snapshot/fuchsia/process_reader_fuchsia_test.cc index 3c8f827001..b8e71afb5f 100644 --- a/snapshot/fuchsia/process_reader_fuchsia_test.cc +++ b/snapshot/fuchsia/process_reader_fuchsia_test.cc @@ -147,7 +147,7 @@ CRASHPAD_CHILD_TEST_MAIN(ProcessReaderChildThreadsTestMain) { EXPECT_EQ(status, ZX_OK); constexpr size_t kNumThreads = 5; - struct ThreadData thread_data[kNumThreads] = {{0}}; + struct ThreadData thread_data[kNumThreads] = {{0, 0}}; for (size_t i = 0; i < kNumThreads; ++i) { thread_data[i] = { diff --git a/snapshot/minidump/process_snapshot_minidump_test.cc b/snapshot/minidump/process_snapshot_minidump_test.cc index 902d699ee8..7fb4388820 100644 --- a/snapshot/minidump/process_snapshot_minidump_test.cc +++ b/snapshot/minidump/process_snapshot_minidump_test.cc @@ -792,7 +792,7 @@ TEST(ProcessSnapshotMinidump, ThreadsWithNames) { for (uint32_t minidump_thread_index = 0; minidump_thread_index < kMinidumpThreadCount; ++minidump_thread_index) { - MINIDUMP_THREAD_NAME minidump_thread_name = {0}; + MINIDUMP_THREAD_NAME minidump_thread_name = {0, 0}; minidump_thread_name.ThreadId = kBaseThreadId + minidump_thread_index; minidump_thread_name.RvaOfThreadName = thread_name_rva64s[minidump_thread_index]; From 02bdf8f9d75490eea23968818ed4e80c8b576c0b Mon Sep 17 00:00:00 2001 From: Ben Hamilton Date: Mon, 13 Jun 2022 15:18:29 -0600 Subject: [PATCH 03/20] [snapshot] Add missing #include in process_reader_win_test.cc The Chromium presubmits flagged a missing #include in process_reader_win_test.cc. This adds the missing #include. Change-Id: I68aed4328f976bba547a0cb7a9ea833fdf71873b Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3703312 Reviewed-by: Mark Mentovai Commit-Queue: Mark Mentovai --- snapshot/win/process_reader_win_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/snapshot/win/process_reader_win_test.cc b/snapshot/win/process_reader_win_test.cc index 6cfd8a4f45..f63277da76 100644 --- a/snapshot/win/process_reader_win_test.cc +++ b/snapshot/win/process_reader_win_test.cc @@ -25,6 +25,7 @@ #include "base/strings/stringprintf.h" #include "base/strings/utf_string_conversions.h" +#include "build/build_config.h" #include "gmock/gmock.h" #include "gtest/gtest.h" #include "test/scoped_set_thread_name.h" From 3ae34b169b30c7cd47db244bb277f8c5fe0c06c6 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 14 Jun 2022 19:00:14 -0400 Subject: [PATCH 04/20] [test] Fix test build failures in Chromium Importing Crashpad into Chromium revealed a few build failures: 1) The MSVC compiler needed assistance constructing SleepingThreads 2) scoped_set_thread_name_posix.cc did not build on Android, where BUILDFLAG(IS_LINUX) is not defined and __ANDROID_API__ must be set to 24 or higher to use pthread_getname_np() This fixes the build failures, which I tested with a Chromium CQ dry-run: https://crrev.com/c/3703491 Change-Id: Ibde7cacaa45d384272890ea9b1ee2d707048ab03 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3703446 Commit-Queue: Mark Mentovai Reviewed-by: Joshua Peraza --- snapshot/win/process_reader_win_test.cc | 8 +++---- test/scoped_set_thread_name_posix.cc | 29 +++++++++++++++++-------- 2 files changed, 24 insertions(+), 13 deletions(-) diff --git a/snapshot/win/process_reader_win_test.cc b/snapshot/win/process_reader_win_test.cc index f63277da76..98cb11b7af 100644 --- a/snapshot/win/process_reader_win_test.cc +++ b/snapshot/win/process_reader_win_test.cc @@ -145,7 +145,7 @@ class ProcessReaderChildThreadSuspendCount final : public WinMultiprocess { class SleepingThread : public Thread { public: - SleepingThread(const std::string& thread_name) + explicit SleepingThread(const std::string& thread_name) : done_(nullptr), thread_name_(thread_name) {} void SetHandle(Semaphore* done) { @@ -222,9 +222,9 @@ class ProcessReaderChildThreadSuspendCount final : public WinMultiprocess { // Create three dummy threads so we can confirm we read successfully read // more than just the main thread. std::array threads = { - "WinMultiprocessChild-1", - "WinMultiprocessChild-2", - "WinMultiprocessChild-3", + SleepingThread(std::string("WinMultiprocessChild-1")), + SleepingThread(std::string("WinMultiprocessChild-2")), + SleepingThread(std::string("WinMultiprocessChild-3")), }; Semaphore done(0); for (auto& thread : threads) diff --git a/test/scoped_set_thread_name_posix.cc b/test/scoped_set_thread_name_posix.cc index 66146c066a..4720bc0045 100644 --- a/test/scoped_set_thread_name_posix.cc +++ b/test/scoped_set_thread_name_posix.cc @@ -20,10 +20,13 @@ #include #include "base/check.h" +#include "base/check_op.h" #include "build/build_config.h" #if BUILDFLAG(IS_APPLE) #include +#elif BUILDFLAG(IS_ANDROID) +#include #endif namespace crashpad { @@ -31,36 +34,44 @@ namespace test { namespace { -#if BUILDFLAG(IS_LINUX) +#if BUILDFLAG(IS_APPLE) +constexpr size_t kPthreadNameMaxLen = MAXTHREADNAMESIZE; +#elif BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_CHROMEOS) // The kernel headers define this in linux/sched.h as TASK_COMM_LEN, but the // userspace copy of that header does not define it. constexpr size_t kPthreadNameMaxLen = 16; -#elif BUILDFLAG(IS_APPLE) -constexpr size_t kPthreadNameMaxLen = MAXTHREADNAMESIZE; #else #error Port to your platform #endif void SetCurrentThreadName(const std::string& thread_name) { -#if BUILDFLAG(IS_LINUX) - PCHECK((errno = pthread_setname_np(pthread_self(), thread_name.c_str())) == 0) - << "pthread_setname_np"; -#elif BUILDFLAG(IS_APPLE) +#if BUILDFLAG(IS_APPLE) // Apple's pthread_setname_np() sets errno instead of returning it. PCHECK(pthread_setname_np(thread_name.c_str()) == 0) << "pthread_setname_np"; +#elif BUILDFLAG(IS_ANDROID) && __ANDROID_API__ < 24 + // pthread_setname_np() requires Android API 24 or later. + CHECK_LT(thread_name.length(), kPthreadNameMaxLen); + PCHECK(prctl(PR_SET_NAME, thread_name.c_str()) == 0) << "prctl(PR_SET_NAME)"; #else -#error Port to your platform + PCHECK((errno = pthread_setname_np(pthread_self(), thread_name.c_str())) == 0) + << "pthread_setname_np"; #endif } std::string GetCurrentThreadName() { std::string result(kPthreadNameMaxLen, '\0'); +#if BUILDFLAG(IS_ANDROID) && __ANDROID_API__ < 24 + static constexpr char kGetThreadNameFunctionName[] = "prctl"; + PCHECK(prctl(PR_GET_NAME, result.data()) == 0) << "prctl(PR_GET_NAME)"; +#else + static constexpr char kGetThreadNameFunctionName[] = "pthread_getname_np"; PCHECK((errno = pthread_getname_np( pthread_self(), result.data(), result.length())) == 0) << "pthread_getname_np"; +#endif const auto result_nul_idx = result.find('\0'); CHECK(result_nul_idx != std::string::npos) - << "pthread_getname_np did not NUL terminate"; + << kGetThreadNameFunctionName << " did not NUL terminate"; result.resize(result_nul_idx); return result; } From 07ef17371d476fc77e737813cfcb434fc4aabddd Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 15 Jun 2022 09:51:59 -0400 Subject: [PATCH 05/20] Add buildtools/clang_format/script to DEPS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit clang-format doesn’t work after week’s buildtools update to 0a14d52dad27 without separately checking out buildtools/clang_format/script. Change-Id: I8330aacb85d1ba96318e5f2cd4563b6d32615963 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3707851 Commit-Queue: Mark Mentovai Reviewed-by: Justin Cohen --- DEPS | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/DEPS b/DEPS index 9829da3433..4a2012548c 100644 --- a/DEPS +++ b/DEPS @@ -27,6 +27,10 @@ deps = { 'buildtools': Var('chromium_git') + '/chromium/src/buildtools.git@' + '8b16338d17cd71b04a6ba28da7322ab6739892c2', + 'buildtools/clang_format/script': + Var('chromium_git') + + '/external/github.com/llvm/llvm-project/clang/tools/clang-format.git@' + + 'c912837e0d82b5ca4b6e790b573b3956d3744c1c', 'crashpad/third_party/edo/edo': { 'url': Var('chromium_git') + '/external/github.com/google/eDistantObject.git@' + '727e556705278598fce683522beedbb9946bfda0', From 460943dd9a71dc76f68182a8ede766d5543e5341 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Thu, 16 Jun 2022 12:09:22 +0530 Subject: [PATCH 06/20] posix: Replace DoubleForkAndExec() with ForkAndSpawn() The DoubleForkAndExec() function was taking over 622 milliseconds to run on macOS 11 (BigSur) on Intel i5-1038NG7. I did some debugging by adding some custom traces and found that the fork() syscall is the bottleneck here, i.e., the first fork() takes around 359 milliseconds and the nested fork() takes around 263 milliseconds. Replacing the nested fork() and exec() with posix_spawn() reduces the time consumption to 257 milliseconds! See https://github.com/libuv/libuv/pull/3064 to know why fork() is so slow on macOS and why posix_spawn() is a better replacement. Another point to note is that even base::LaunchProcess() from Chromium calls posix_spawnp() on macOS - https://source.chromium.org/chromium/chromium/src/+/8f8d82dea0fa8f11f57c74dbb65126f8daba58f7:base/process/launch_mac.cc;l=295-296 Change-Id: I25c6ee9629a1ae5d0c32b361b56a1ce0b4b0fd26 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3641386 Reviewed-by: Mark Mentovai Commit-Queue: Mark Mentovai --- AUTHORS | 1 + client/crashpad_client_linux.cc | 10 +- client/crashpad_client_mac.cc | 4 +- .../cros_crash_report_exception_handler.cc | 13 +- util/BUILD.gn | 4 +- util/posix/double_fork_and_exec.cc | 166 ------------- util/posix/fork_and_spawn.cc | 235 ++++++++++++++++++ ...ouble_fork_and_exec.h => fork_and_spawn.h} | 29 +-- 8 files changed, 264 insertions(+), 198 deletions(-) delete mode 100644 util/posix/double_fork_and_exec.cc create mode 100644 util/posix/fork_and_spawn.cc rename util/posix/{double_fork_and_exec.h => fork_and_spawn.h} (76%) diff --git a/AUTHORS b/AUTHORS index 8dcac32388..0210392433 100644 --- a/AUTHORS +++ b/AUTHORS @@ -12,3 +12,4 @@ Opera Software ASA Vewd Software AS LG Electronics, Inc. MIPS Technologies, Inc. +Darshan Sen diff --git a/client/crashpad_client_linux.cc b/client/crashpad_client_linux.cc index 295ec1615d..94c1cb02ef 100644 --- a/client/crashpad_client_linux.cc +++ b/client/crashpad_client_linux.cc @@ -45,7 +45,7 @@ #include "util/linux/socket.h" #include "util/misc/address_sanitizer.h" #include "util/misc/from_pointer_cast.h" -#include "util/posix/double_fork_and_exec.h" +#include "util/posix/fork_and_spawn.h" #include "util/posix/scoped_mmap.h" #include "util/posix/signals.h" @@ -459,7 +459,7 @@ bool CrashpadClient::StartHandler( argv.push_back(FormatArgumentInt("initial-client-fd", handler_sock.get())); argv.push_back("--shared-client-connection"); - if (!DoubleForkAndExec(argv, nullptr, handler_sock.get(), false, nullptr)) { + if (!ForkAndSpawn(argv, nullptr, handler_sock.get(), false, nullptr)) { return false; } @@ -614,7 +614,7 @@ bool CrashpadClient::StartJavaHandlerForClient( int socket) { std::vector argv = BuildAppProcessArgs( class_name, database, metrics_dir, url, annotations, arguments, socket); - return DoubleForkAndExec(argv, env, socket, false, nullptr); + return ForkAndSpawn(argv, env, socket, false, nullptr); } bool CrashpadClient::StartHandlerWithLinkerAtCrash( @@ -663,7 +663,7 @@ bool CrashpadClient::StartHandlerWithLinkerForClient( annotations, arguments, socket); - return DoubleForkAndExec(argv, env, socket, false, nullptr); + return ForkAndSpawn(argv, env, socket, false, nullptr); } #endif @@ -697,7 +697,7 @@ bool CrashpadClient::StartHandlerForClient( argv.push_back(FormatArgumentInt("initial-client-fd", socket)); - return DoubleForkAndExec(argv, nullptr, socket, true, nullptr); + return ForkAndSpawn(argv, nullptr, socket, true, nullptr); } // static diff --git a/client/crashpad_client_mac.cc b/client/crashpad_client_mac.cc index d25bfb7163..585bac910a 100644 --- a/client/crashpad_client_mac.cc +++ b/client/crashpad_client_mac.cc @@ -36,7 +36,7 @@ #include "util/mach/notify_server.h" #include "util/misc/clock.h" #include "util/misc/implicit_cast.h" -#include "util/posix/double_fork_and_exec.h" +#include "util/posix/fork_and_spawn.h" namespace crashpad { @@ -343,7 +343,7 @@ class HandlerStarter final : public NotifyServer::DefaultInterface { // this parent process, which was probably using the exception server now // being restarted. The handler can’t monitor itself for its own crashes via // this interface. - if (!DoubleForkAndExec( + if (!ForkAndSpawn( argv, nullptr, server_write_fd.get(), diff --git a/handler/linux/cros_crash_report_exception_handler.cc b/handler/linux/cros_crash_report_exception_handler.cc index 9e58d94aa4..3caa3b987b 100644 --- a/handler/linux/cros_crash_report_exception_handler.cc +++ b/handler/linux/cros_crash_report_exception_handler.cc @@ -29,7 +29,7 @@ #include "util/linux/ptrace_client.h" #include "util/misc/metrics.h" #include "util/misc/uuid.h" -#include "util/posix/double_fork_and_exec.h" +#include "util/posix/fork_and_spawn.h" namespace crashpad { @@ -266,12 +266,11 @@ bool CrosCrashReportExceptionHandler::HandleExceptionWithConnection( argv.push_back("--always_allow_feedback"); } - if (!DoubleForkAndExec(argv, - nullptr /* envp */, - file_writer.fd() /* preserve_fd */, - false /* use_path */, - nullptr /* child_function */)) { - LOG(ERROR) << "DoubleForkAndExec failed"; + if (!ForkAndSpawn(argv, + nullptr /* envp */, + file_writer.fd() /* preserve_fd */, + false /* use_path */, + nullptr /* child_function */)) { Metrics::ExceptionCaptureResult( Metrics::CaptureResult::kFinishedWritingCrashReportFailed); return false; diff --git a/util/BUILD.gn b/util/BUILD.gn index c372ff49b3..79cf878e43 100644 --- a/util/BUILD.gn +++ b/util/BUILD.gn @@ -288,10 +288,10 @@ crashpad_static_library("util") { sources += [ "posix/close_multiple.cc", "posix/close_multiple.h", - "posix/double_fork_and_exec.cc", - "posix/double_fork_and_exec.h", "posix/drop_privileges.cc", "posix/drop_privileges.h", + "posix/fork_and_spawn.cc", + "posix/fork_and_spawn.h", "posix/process_info.h", # These map signals to and from strings. While Fuchsia defines some of diff --git a/util/posix/double_fork_and_exec.cc b/util/posix/double_fork_and_exec.cc deleted file mode 100644 index 1960430954..0000000000 --- a/util/posix/double_fork_and_exec.cc +++ /dev/null @@ -1,166 +0,0 @@ -// 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/posix/double_fork_and_exec.h" - -#include -#include -#include -#include - -#include "base/check_op.h" -#include "base/logging.h" -#include "base/posix/eintr_wrapper.h" -#include "base/strings/stringprintf.h" -#include "util/posix/close_multiple.h" - -namespace crashpad { - -bool DoubleForkAndExec(const std::vector& argv, - const std::vector* envp, - int preserve_fd, - bool use_path, - void (*child_function)()) { - DCHECK(!envp || !use_path); - - // argv_c contains const char* pointers and is terminated by nullptr. This is - // suitable for passing to execv(). Although argv_c is not used in the parent - // process, it must be built in the parent process because it’s unsafe to do - // so in the child or grandchild process. - std::vector argv_c; - argv_c.reserve(argv.size() + 1); - for (const std::string& argument : argv) { - argv_c.push_back(argument.c_str()); - } - argv_c.push_back(nullptr); - - std::vector envp_c; - if (envp) { - envp_c.reserve(envp->size() + 1); - for (const std::string& variable : *envp) { - envp_c.push_back(variable.c_str()); - } - envp_c.push_back(nullptr); - } - - // Double-fork(). The three processes involved are parent, child, and - // grandchild. The grandchild will call execv(). The child exits immediately - // after spawning the grandchild, so the grandchild becomes an orphan and its - // parent process ID becomes 1. This relieves the parent and child of the - // responsibility to reap the grandchild with waitpid() or similar. The - // grandchild is expected to outlive the parent process, so the parent - // shouldn’t be concerned with reaping it. This approach means that accidental - // early termination of the handler process will not result in a zombie - // process. - pid_t pid = fork(); - if (pid < 0) { - PLOG(ERROR) << "fork"; - return false; - } - - if (pid == 0) { - // Child process. - - if (child_function) { - child_function(); - } - - // Call setsid(), creating a new process group and a new session, both led - // by this process. The new process group has no controlling terminal. This - // disconnects it from signals generated by the parent process’ terminal. - // - // setsid() is done in the child instead of the grandchild so that the - // grandchild will not be a session leader. If it were a session leader, an - // accidental open() of a terminal device without O_NOCTTY would make that - // terminal the controlling terminal. - // - // It’s not desirable for the grandchild to have a controlling terminal. The - // grandchild manages its own lifetime, such as by monitoring clients on its - // own and exiting when it loses all clients and when it deems it - // appropraite to do so. It may serve clients in different process groups or - // sessions than its original client, and receiving signals intended for its - // original client’s process group could be harmful in that case. - PCHECK(setsid() != -1) << "setsid"; - - pid = fork(); - if (pid < 0) { - PLOG(FATAL) << "fork"; - } - - if (pid > 0) { - // Child process. - - // _exit() instead of exit(), because fork() was called. - _exit(EXIT_SUCCESS); - } - - // Grandchild process. - - CloseMultipleNowOrOnExec(STDERR_FILENO + 1, preserve_fd); - - // &argv_c[0] is a pointer to a pointer to const char data, but because of - // how C (not C++) works, execvp() wants a pointer to a const pointer to - // char data. It modifies neither the data nor the pointers, so the - // const_cast is safe. - char* const* argv_for_execv = const_cast(&argv_c[0]); - - if (envp) { - // This cast is safe for the same reason that the argv_for_execv cast is. - char* const* envp_for_execv = const_cast(&envp_c[0]); - execve(argv_for_execv[0], argv_for_execv, envp_for_execv); - PLOG(FATAL) << "execve " << argv_for_execv[0]; - } - - if (use_path) { - execvp(argv_for_execv[0], argv_for_execv); - PLOG(FATAL) << "execvp " << argv_for_execv[0]; - } - - execv(argv_for_execv[0], argv_for_execv); - PLOG(FATAL) << "execv " << argv_for_execv[0]; - } - - // waitpid() for the child, so that it does not become a zombie process. The - // child normally exits quickly. - // - // Failures from this point on may result in the accumulation of a zombie, but - // should not be considered fatal. Log only warnings, but don’t treat these - // failures as a failure of the function overall. - int status; - pid_t wait_pid = HANDLE_EINTR(waitpid(pid, &status, 0)); - if (wait_pid == -1) { - PLOG(WARNING) << "waitpid"; - return true; - } - DCHECK_EQ(wait_pid, pid); - - if (WIFSIGNALED(status)) { - int sig = WTERMSIG(status); - LOG(WARNING) << base::StringPrintf( - "intermediate process terminated by signal %d (%s)%s", - sig, - strsignal(sig), - WCOREDUMP(status) ? " (core dumped)" : ""); - } else if (!WIFEXITED(status)) { - LOG(WARNING) << base::StringPrintf( - "intermediate process: unknown termination 0x%x", status); - } else if (WEXITSTATUS(status) != EXIT_SUCCESS) { - LOG(WARNING) << "intermediate process exited with code " - << WEXITSTATUS(status); - } - - return true; -} - -} // namespace crashpad diff --git a/util/posix/fork_and_spawn.cc b/util/posix/fork_and_spawn.cc new file mode 100644 index 0000000000..c6a95bbfdc --- /dev/null +++ b/util/posix/fork_and_spawn.cc @@ -0,0 +1,235 @@ +// 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/posix/fork_and_spawn.h" + +#include +#include +#include +#include +#include +#include + +#include "base/check.h" +#include "base/check_op.h" +#include "base/logging.h" +#include "base/posix/eintr_wrapper.h" +#include "base/strings/stringprintf.h" +#include "build/build_config.h" +#include "util/posix/close_multiple.h" + +extern char** environ; + +namespace crashpad { + +namespace { + +#if BUILDFLAG(IS_APPLE) + +class PosixSpawnAttr { + public: + PosixSpawnAttr() { + PCHECK((errno = posix_spawnattr_init(&attr_)) == 0) + << "posix_spawnattr_init"; + } + + PosixSpawnAttr(const PosixSpawnAttr&) = delete; + PosixSpawnAttr& operator=(const PosixSpawnAttr&) = delete; + + ~PosixSpawnAttr() { + PCHECK((errno = posix_spawnattr_destroy(&attr_)) == 0) + << "posix_spawnattr_destroy"; + } + + void SetFlags(short flags) { + PCHECK((errno = posix_spawnattr_setflags(&attr_, flags)) == 0) + << "posix_spawnattr_setflags"; + } + + const posix_spawnattr_t* Get() const { return &attr_; } + + private: + posix_spawnattr_t attr_; +}; + +class PosixSpawnFileActions { + public: + PosixSpawnFileActions() { + PCHECK((errno = posix_spawn_file_actions_init(&file_actions_)) == 0) + << "posix_spawn_file_actions_init"; + } + + PosixSpawnFileActions(const PosixSpawnFileActions&) = delete; + PosixSpawnFileActions& operator=(const PosixSpawnFileActions&) = delete; + + ~PosixSpawnFileActions() { + PCHECK((errno = posix_spawn_file_actions_destroy(&file_actions_)) == 0) + << "posix_spawn_file_actions_destroy"; + } + + void AddInheritedFileDescriptor(int fd) { + PCHECK((errno = posix_spawn_file_actions_addinherit_np(&file_actions_, + fd)) == 0) + << "posix_spawn_file_actions_addinherit_np"; + } + + const posix_spawn_file_actions_t* Get() const { return &file_actions_; } + + private: + posix_spawn_file_actions_t file_actions_; +}; + +#endif + +} // namespace + +bool ForkAndSpawn(const std::vector& argv, + const std::vector* envp, + int preserve_fd, + bool use_path, + void (*child_function)()) { + // argv_c contains const char* pointers and is terminated by nullptr. This is + // suitable for passing to posix_spawn() or posix_spawnp(). Although argv_c is + // not used in the parent process, it must be built in the parent process + // because it’s unsafe to do so in the child. + std::vector argv_c; + argv_c.reserve(argv.size() + 1); + for (const std::string& argument : argv) { + argv_c.push_back(argument.c_str()); + } + argv_c.push_back(nullptr); + + std::vector envp_c; + if (envp) { + envp_c.reserve(envp->size() + 1); + for (const std::string& variable : *envp) { + envp_c.push_back(variable.c_str()); + } + envp_c.push_back(nullptr); + } + + // The three processes involved are parent, child, and grandchild. The child + // exits immediately after spawning the grandchild, so the grandchild becomes + // an orphan and its parent process ID becomes 1. This relieves the parent and + // child of the responsibility to reap the grandchild with waitpid() or + // similar. The grandchild is expected to outlive the parent process, so the + // parent shouldn’t be concerned with reaping it. This approach means that + // accidental early termination of the handler process will not result in a + // zombie process. + pid_t pid = fork(); + if (pid < 0) { + PLOG(ERROR) << "fork"; + return false; + } + + if (pid == 0) { + // Child process. + + if (child_function) { + child_function(); + } + + // Call setsid(), creating a new process group and a new session, both led + // by this process. The new process group has no controlling terminal. This + // disconnects it from signals generated by the parent process’ terminal. + // + // setsid() is done in the child instead of the grandchild so that the + // grandchild will not be a session leader. If it were a session leader, an + // accidental open() of a terminal device without O_NOCTTY would make that + // terminal the controlling terminal. + // + // It’s not desirable for the grandchild to have a controlling terminal. The + // grandchild manages its own lifetime, such as by monitoring clients on its + // own and exiting when it loses all clients and when it deems it + // appropraite to do so. It may serve clients in different process groups or + // sessions than its original client, and receiving signals intended for its + // original client’s process group could be harmful in that case. + PCHECK(setsid() != -1) << "setsid"; + + // &argv_c[0] is a pointer to a pointer to const char data, but because of + // how C (not C++) works, posix_spawn() and posix_spawnp() want a pointer to + // a const pointer to char data. They modifies neither the data nor the + // pointers, so the const_cast is safe. + char* const* argv_for_spawn = const_cast(argv_c.data()); + + // This cast is safe for the same reason that the argv_for_spawn cast is. + char* const* envp_for_spawn = + envp ? const_cast(envp_c.data()) : environ; + +#if BUILDFLAG(IS_APPLE) + PosixSpawnAttr attr; + attr.SetFlags(POSIX_SPAWN_CLOEXEC_DEFAULT); + + PosixSpawnFileActions file_actions; + for (int fd = 0; fd <= STDERR_FILENO; ++fd) { + file_actions.AddInheritedFileDescriptor(fd); + } + file_actions.AddInheritedFileDescriptor(preserve_fd); + + const posix_spawnattr_t* attr_p = attr.Get(); + const posix_spawn_file_actions_t* file_actions_p = file_actions.Get(); +#else + CloseMultipleNowOrOnExec(STDERR_FILENO + 1, preserve_fd); + + const posix_spawnattr_t* attr_p = nullptr; + const posix_spawn_file_actions_t* file_actions_p = nullptr; +#endif + + auto posix_spawn_fp = use_path ? posix_spawnp : posix_spawn; + if ((errno = posix_spawn_fp(&pid, + argv_for_spawn[0], + file_actions_p, + attr_p, + argv_for_spawn, + envp_for_spawn)) != 0) { + PLOG(FATAL) << (use_path ? "posix_spawnp" : "posix_spawn"); + } + + // _exit() instead of exit(), because fork() was called. + _exit(EXIT_SUCCESS); + } + + // waitpid() for the child, so that it does not become a zombie process. The + // child normally exits quickly. + // + // Failures from this point on may result in the accumulation of a zombie, but + // should not be considered fatal. Log only warnings, but don’t treat these + // failures as a failure of the function overall. + int status; + pid_t wait_pid = HANDLE_EINTR(waitpid(pid, &status, 0)); + if (wait_pid == -1) { + PLOG(WARNING) << "waitpid"; + return true; + } + DCHECK_EQ(wait_pid, pid); + + if (WIFSIGNALED(status)) { + int sig = WTERMSIG(status); + LOG(WARNING) << base::StringPrintf( + "intermediate process terminated by signal %d (%s)%s", + sig, + strsignal(sig), + WCOREDUMP(status) ? " (core dumped)" : ""); + } else if (!WIFEXITED(status)) { + LOG(WARNING) << base::StringPrintf( + "intermediate process: unknown termination 0x%x", status); + } else if (WEXITSTATUS(status) != EXIT_SUCCESS) { + LOG(WARNING) << "intermediate process exited with code " + << WEXITSTATUS(status); + } + + return true; +} + +} // namespace crashpad diff --git a/util/posix/double_fork_and_exec.h b/util/posix/fork_and_spawn.h similarity index 76% rename from util/posix/double_fork_and_exec.h rename to util/posix/fork_and_spawn.h index 02fc0f28f1..fc55aa3a37 100644 --- a/util/posix/double_fork_and_exec.h +++ b/util/posix/fork_and_spawn.h @@ -12,8 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. -#ifndef CRASHPAD_UTIL_POSIX_DOUBLE_FORK_AND_EXEC_H_ -#define CRASHPAD_UTIL_POSIX_DOUBLE_FORK_AND_EXEC_H_ +#ifndef CRASHPAD_UTIL_POSIX_FORK_AND_SPAWN_H_ +#define CRASHPAD_UTIL_POSIX_FORK_AND_SPAWN_H_ #include #include @@ -23,7 +23,7 @@ namespace crashpad { //! \brief Executes a (grand-)child process. //! //! The grandchild process will be started through the -//! double-`fork()`-and-`execv()` pattern. This allows the grandchild to fully +//! `fork()`-and-`posix_spawn()` pattern. This allows the grandchild to fully //! disassociate from the parent. The grandchild will not be a member of the //! parent’s process group or session and will not have a controlling terminal, //! providing isolation from signals not intended for it. The grandchild’s @@ -37,7 +37,7 @@ namespace crashpad { //! \param[in] argv The argument vector to start the grandchild process with. //! `argv[0]` is used as the path to the executable. //! \param[in] envp A vector of environment variables of the form `var=value` to -//! be passed to `execve()`. If this value is `nullptr`, the current +//! be passed to `posix_spawn()`. If this value is `nullptr`, the current //! environment is used. //! \param[in] preserve_fd A file descriptor to be inherited by the grandchild //! process. This file descriptor is inherited in addition to the three file @@ -45,16 +45,13 @@ namespace crashpad { //! if no additional file descriptors are to be inherited. //! \param[in] use_path Whether to consult the `PATH` environment variable when //! requested to start an executable at a non-absolute path. If `false`, -//! `execv()`, which does not consult `PATH`, will be used. If `true`, -//! `execvp()`, which does consult `PATH`, will be used. +//! `posix_spawn()`, which does not consult `PATH`, will be used. If `true`, +//! `posix_spawnp()`, which does consult `PATH`, will be used. //! \param[in] child_function If not `nullptr`, this function will be called in -//! the intermediate child process, prior to the second `fork()`. Take note +//! the intermediate child process, prior to the `posix_spawn()`. Take note //! that this function will run in the context of a forked process, and must //! be safe for that purpose. //! -//! Setting both \a envp to a value other than `nullptr` and \a use_path to -//! `true` is not currently supported. -//! //! \return `true` on success, and `false` on failure with a message logged. //! Only failures that occur in the parent process that indicate a definite //! failure to start the the grandchild are reported in the return value. @@ -63,12 +60,12 @@ namespace crashpad { //! terminating. The caller assumes the responsibility for detecting such //! failures, for example, by observing a failure to perform a successful //! handshake with the grandchild process. -bool DoubleForkAndExec(const std::vector& argv, - const std::vector* envp, - int preserve_fd, - bool use_path, - void (*child_function)()); +bool ForkAndSpawn(const std::vector& argv, + const std::vector* envp, + int preserve_fd, + bool use_path, + void (*child_function)()); } // namespace crashpad -#endif // CRASHPAD_UTIL_POSIX_DOUBLE_FORK_AND_EXEC_H_ +#endif // CRASHPAD_UTIL_POSIX_FORK_AND_SPAWN_H_ From 7c30a508eb1c5fba3533a1e5570e79b9b2ad37d5 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Fri, 17 Jun 2022 07:39:41 -0400 Subject: [PATCH 07/20] Build actual crashpad .asm files in win/cross builds Now that we have llvm-ml, we no longer need the workaround for this. This upstreams https://chromium-review.googlesource.com/c/chromium/src/+/3708412 Bug: chromium:762167 Change-Id: Iadc8ba9753bb7dd079415ee744f3b176b7e2f629 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3707748 Reviewed-by: Mark Mentovai Commit-Queue: Mark Mentovai --- util/BUILD.gn | 47 ++++++++--------------- util/misc/capture_context_broken.cc | 29 -------------- util/win/safe_terminate_process_broken.cc | 34 ---------------- 3 files changed, 15 insertions(+), 95 deletions(-) delete mode 100644 util/misc/capture_context_broken.cc delete mode 100644 util/win/safe_terminate_process_broken.cc diff --git a/util/BUILD.gn b/util/BUILD.gn index 79cf878e43..17406edb0b 100644 --- a/util/BUILD.gn +++ b/util/BUILD.gn @@ -509,40 +509,23 @@ crashpad_static_library("util") { "win/xp_compat.h", ] - # There's no ml.exe yet in cross builds, so provide broken-but-not-asm - # versions of the functions defined in .asm files. - # - # CaptureContext() in capture_context_broken.cc just calls CHECK(false). - # SafeTerminateProcess() in safe_terminate_process.cc just calls regular - # TerminateProcess() without the protection against broken third-party - # patching of TerminateProcess(). - # - # TODO(thakis): Use the .asm file in cross builds somehow, - # https://crbug.com/762167. - if (host_os == "win") { - if (current_cpu != "arm64") { - sources += [ - "misc/capture_context_win.asm", - "win/safe_terminate_process.asm", - ] - } else { - # Most Crashpad builds use Microsoft's armasm64.exe macro assembler for - # .asm source files. When building in Chromium, clang-cl is used as the - # assembler instead. Since the two assemblers recognize different - # assembly dialects, the same .asm file can't be used for each. As a - # workaround, use a prebuilt .obj file when the Microsoft-dialect - # assembler isn't available. - if (crashpad_is_in_chromium) { - sources += [ "misc/capture_context_win_arm64.obj" ] - } else { - sources += [ "misc/capture_context_win_arm64.asm" ] - } - } - } else { + if (current_cpu != "arm64") { sources += [ - "misc/capture_context_broken.cc", - "win/safe_terminate_process_broken.cc", + "misc/capture_context_win.asm", + "win/safe_terminate_process.asm", ] + } else { + # Most Crashpad builds use Microsoft's armasm64.exe macro assembler for + # .asm source files. When building in Chromium, clang-cl is used as the + # assembler instead. Since the two assemblers recognize different + # assembly dialects, the same .asm file can't be used for each. As a + # workaround, use a prebuilt .obj file when the Microsoft-dialect + # assembler isn't available. + if (crashpad_is_in_chromium) { + sources += [ "misc/capture_context_win_arm64.obj" ] + } else { + sources += [ "misc/capture_context_win_arm64.asm" ] + } } } diff --git a/util/misc/capture_context_broken.cc b/util/misc/capture_context_broken.cc deleted file mode 100644 index ea6d0a0cf9..0000000000 --- a/util/misc/capture_context_broken.cc +++ /dev/null @@ -1,29 +0,0 @@ -// 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/misc/capture_context.h" - -#include - -#include "base/check.h" - -namespace crashpad { - -void CaptureContext(NativeCPUContext* context) { - // Don't use this file in production. - CHECK(false) - << "Don't use this! For cross builds only. See https://crbug.com/762167."; -} - -} // namespace crashpad diff --git a/util/win/safe_terminate_process_broken.cc b/util/win/safe_terminate_process_broken.cc deleted file mode 100644 index 2429c726ca..0000000000 --- a/util/win/safe_terminate_process_broken.cc +++ /dev/null @@ -1,34 +0,0 @@ -// 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 "base/logging.h" - -#if defined(ARCH_CPU_X86) - -namespace crashpad { - -bool SafeTerminateProcess(HANDLE process, UINT exit_code) { - // Third-party software that hooks TerminateProcess() incorrectly has been - // encountered in the wild. This version of SafeTerminateProcess() lacks - // protection against that, so don't use it in production. - LOG(WARNING) - << "Don't use this! For cross builds only. See https://crbug.com/777924."; - return TerminateProcess(process, exit_code) != FALSE; -} - -} // namespace crashpad - -#endif // defined(ARCH_CPU_X86) From 21546d85140d9e19689707a88c092e7c5eace1c8 Mon Sep 17 00:00:00 2001 From: Justin Cohen Date: Wed, 22 Jun 2022 12:22:26 -0400 Subject: [PATCH 08/20] Use call_once in lazy settings load. This fixes a test case that accesses settings for the first time in multiple threads simultaneously. Fixed: crashpad:417 Change-Id: I6539682f171563f8ff5a1203fdd550ab92afc276 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3711807 Commit-Queue: Justin Cohen Reviewed-by: Robert Sesek --- client/crash_report_database_generic.cc | 7 ++++--- client/crash_report_database_mac.mm | 9 +++++---- client/crash_report_database_win.cc | 9 +++++---- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/client/crash_report_database_generic.cc b/client/crash_report_database_generic.cc index c4e0401096..742850ab87 100644 --- a/client/crash_report_database_generic.cc +++ b/client/crash_report_database_generic.cc @@ -18,6 +18,7 @@ #include #include +#include #include #include @@ -258,15 +259,15 @@ class CrashReportDatabaseGeneric : public CrashReportDatabase { static bool WriteMetadata(const base::FilePath& path, const Report& report); Settings& SettingsInternal() { - if (!settings_init_) + std::call_once(settings_init_, [this]() { settings_.Initialize(base_dir_.Append(kSettings)); - settings_init_ = true; + }); return settings_; } base::FilePath base_dir_; Settings settings_; - bool settings_init_ = false; + std::once_flag settings_init_; InitializationStateDcheck initialized_; }; diff --git a/client/crash_report_database_mac.mm b/client/crash_report_database_mac.mm index 4c9e5e72e8..e33b932528 100644 --- a/client/crash_report_database_mac.mm +++ b/client/crash_report_database_mac.mm @@ -27,6 +27,7 @@ #include #include +#include #include #include "base/logging.h" @@ -263,15 +264,15 @@ OperationStatus ReportsInDirectory(const base::FilePath& path, void CleanOrphanedAttachments(); Settings& SettingsInternal() { - if (!settings_init_) + std::call_once(settings_init_, [this]() { settings_.Initialize(base_dir_.Append(kSettings)); - settings_init_ = true; + }); return settings_; } base::FilePath base_dir_; Settings settings_; - bool settings_init_; + std::once_flag settings_init_; bool xattr_new_names_; InitializationStateDcheck initialized_; }; @@ -280,7 +281,7 @@ OperationStatus ReportsInDirectory(const base::FilePath& path, : CrashReportDatabase(), base_dir_(path), settings_(), - settings_init_(false), + settings_init_(), xattr_new_names_(false), initialized_() {} diff --git a/client/crash_report_database_win.cc b/client/crash_report_database_win.cc index e16aa104fb..e111d8e13b 100644 --- a/client/crash_report_database_win.cc +++ b/client/crash_report_database_win.cc @@ -21,6 +21,7 @@ #include #include +#include #include #include @@ -663,15 +664,15 @@ class CrashReportDatabaseWin : public CrashReportDatabase { std::unique_ptr AcquireMetadata(); Settings& SettingsInternal() { - if (!settings_init_) + std::call_once(settings_init_, [this]() { settings_.Initialize(base_dir_.Append(kSettings)); - settings_init_ = true; + }); return settings_; } base::FilePath base_dir_; Settings settings_; - bool settings_init_; + std::once_flag settings_init_; InitializationStateDcheck initialized_; }; @@ -679,7 +680,7 @@ CrashReportDatabaseWin::CrashReportDatabaseWin(const base::FilePath& path) : CrashReportDatabase(), base_dir_(path), settings_(), - settings_init_(false), + settings_init_(), initialized_() {} CrashReportDatabaseWin::~CrashReportDatabaseWin() { From 23cefd04177be4020c00edc681d1765b76e2cd0b Mon Sep 17 00:00:00 2001 From: Justin Cohen Date: Wed, 22 Jun 2022 19:34:31 -0400 Subject: [PATCH 09/20] Fix Chromium compile. Fixes error: invalid operands to binary expression ('std::ostream' (aka 'basic_ostream') and 'const char[19]') << "pthread_setname_np"; in test/scoped_set_thread_name_posix.cc Change-Id: I77eeeee9c828d563aaa15331733001e522a04642 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3714964 Reviewed-by: Mark Mentovai Commit-Queue: Justin Cohen --- test/scoped_set_thread_name_posix.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/test/scoped_set_thread_name_posix.cc b/test/scoped_set_thread_name_posix.cc index 4720bc0045..92c61bbf41 100644 --- a/test/scoped_set_thread_name_posix.cc +++ b/test/scoped_set_thread_name_posix.cc @@ -17,6 +17,7 @@ #include #include +#include #include #include "base/check.h" From 6e946c4af851722b9d444af89590d65cd2c3ec38 Mon Sep 17 00:00:00 2001 From: Justin Cohen Date: Thu, 23 Jun 2022 03:12:41 +0000 Subject: [PATCH 10/20] Revert "posix: Replace DoubleForkAndExec() with ForkAndSpawn()" This reverts commit 460943dd9a71dc76f68182a8ede766d5543e5341. Reason for revert: This fails to compile in Chromium Android. posix_spawn and posix_spawnp are available in Android NDK 28, but Chromium is building with version 23. https://ci.chromium.org/ui/p/chromium/builders/try/android_compile_dbg/1179765/overview Original change's description: > posix: Replace DoubleForkAndExec() with ForkAndSpawn() > > The DoubleForkAndExec() function was taking over 622 milliseconds to run > on macOS 11 (BigSur) on Intel i5-1038NG7. I did some debugging by adding > some custom traces and found that the fork() syscall is the bottleneck > here, i.e., the first fork() takes around 359 milliseconds and the > nested fork() takes around 263 milliseconds. Replacing the nested fork() > and exec() with posix_spawn() reduces the time consumption to 257 > milliseconds! > > See https://github.com/libuv/libuv/pull/3064 to know why fork() is so > slow on macOS and why posix_spawn() is a better replacement. > > Another point to note is that even base::LaunchProcess() from Chromium > calls posix_spawnp() on macOS - > https://source.chromium.org/chromium/chromium/src/+/8f8d82dea0fa8f11f57c74dbb65126f8daba58f7:base/process/launch_mac.cc;l=295-296 > > Change-Id: I25c6ee9629a1ae5d0c32b361b56a1ce0b4b0fd26 > Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3641386 > Reviewed-by: Mark Mentovai > Commit-Queue: Mark Mentovai Change-Id: I7f6161bc4734c50308438cdde1e193023ee9bfb8 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3719439 Reviewed-by: Mark Mentovai Commit-Queue: Justin Cohen --- AUTHORS | 1 - client/crashpad_client_linux.cc | 10 +- client/crashpad_client_mac.cc | 4 +- .../cros_crash_report_exception_handler.cc | 13 +- util/BUILD.gn | 4 +- util/posix/double_fork_and_exec.cc | 166 +++++++++++++ ...ork_and_spawn.h => double_fork_and_exec.h} | 29 ++- util/posix/fork_and_spawn.cc | 235 ------------------ 8 files changed, 198 insertions(+), 264 deletions(-) create mode 100644 util/posix/double_fork_and_exec.cc rename util/posix/{fork_and_spawn.h => double_fork_and_exec.h} (76%) delete mode 100644 util/posix/fork_and_spawn.cc diff --git a/AUTHORS b/AUTHORS index 0210392433..8dcac32388 100644 --- a/AUTHORS +++ b/AUTHORS @@ -12,4 +12,3 @@ Opera Software ASA Vewd Software AS LG Electronics, Inc. MIPS Technologies, Inc. -Darshan Sen diff --git a/client/crashpad_client_linux.cc b/client/crashpad_client_linux.cc index 94c1cb02ef..295ec1615d 100644 --- a/client/crashpad_client_linux.cc +++ b/client/crashpad_client_linux.cc @@ -45,7 +45,7 @@ #include "util/linux/socket.h" #include "util/misc/address_sanitizer.h" #include "util/misc/from_pointer_cast.h" -#include "util/posix/fork_and_spawn.h" +#include "util/posix/double_fork_and_exec.h" #include "util/posix/scoped_mmap.h" #include "util/posix/signals.h" @@ -459,7 +459,7 @@ bool CrashpadClient::StartHandler( argv.push_back(FormatArgumentInt("initial-client-fd", handler_sock.get())); argv.push_back("--shared-client-connection"); - if (!ForkAndSpawn(argv, nullptr, handler_sock.get(), false, nullptr)) { + if (!DoubleForkAndExec(argv, nullptr, handler_sock.get(), false, nullptr)) { return false; } @@ -614,7 +614,7 @@ bool CrashpadClient::StartJavaHandlerForClient( int socket) { std::vector argv = BuildAppProcessArgs( class_name, database, metrics_dir, url, annotations, arguments, socket); - return ForkAndSpawn(argv, env, socket, false, nullptr); + return DoubleForkAndExec(argv, env, socket, false, nullptr); } bool CrashpadClient::StartHandlerWithLinkerAtCrash( @@ -663,7 +663,7 @@ bool CrashpadClient::StartHandlerWithLinkerForClient( annotations, arguments, socket); - return ForkAndSpawn(argv, env, socket, false, nullptr); + return DoubleForkAndExec(argv, env, socket, false, nullptr); } #endif @@ -697,7 +697,7 @@ bool CrashpadClient::StartHandlerForClient( argv.push_back(FormatArgumentInt("initial-client-fd", socket)); - return ForkAndSpawn(argv, nullptr, socket, true, nullptr); + return DoubleForkAndExec(argv, nullptr, socket, true, nullptr); } // static diff --git a/client/crashpad_client_mac.cc b/client/crashpad_client_mac.cc index 585bac910a..d25bfb7163 100644 --- a/client/crashpad_client_mac.cc +++ b/client/crashpad_client_mac.cc @@ -36,7 +36,7 @@ #include "util/mach/notify_server.h" #include "util/misc/clock.h" #include "util/misc/implicit_cast.h" -#include "util/posix/fork_and_spawn.h" +#include "util/posix/double_fork_and_exec.h" namespace crashpad { @@ -343,7 +343,7 @@ class HandlerStarter final : public NotifyServer::DefaultInterface { // this parent process, which was probably using the exception server now // being restarted. The handler can’t monitor itself for its own crashes via // this interface. - if (!ForkAndSpawn( + if (!DoubleForkAndExec( argv, nullptr, server_write_fd.get(), diff --git a/handler/linux/cros_crash_report_exception_handler.cc b/handler/linux/cros_crash_report_exception_handler.cc index 3caa3b987b..9e58d94aa4 100644 --- a/handler/linux/cros_crash_report_exception_handler.cc +++ b/handler/linux/cros_crash_report_exception_handler.cc @@ -29,7 +29,7 @@ #include "util/linux/ptrace_client.h" #include "util/misc/metrics.h" #include "util/misc/uuid.h" -#include "util/posix/fork_and_spawn.h" +#include "util/posix/double_fork_and_exec.h" namespace crashpad { @@ -266,11 +266,12 @@ bool CrosCrashReportExceptionHandler::HandleExceptionWithConnection( argv.push_back("--always_allow_feedback"); } - if (!ForkAndSpawn(argv, - nullptr /* envp */, - file_writer.fd() /* preserve_fd */, - false /* use_path */, - nullptr /* child_function */)) { + if (!DoubleForkAndExec(argv, + nullptr /* envp */, + file_writer.fd() /* preserve_fd */, + false /* use_path */, + nullptr /* child_function */)) { + LOG(ERROR) << "DoubleForkAndExec failed"; Metrics::ExceptionCaptureResult( Metrics::CaptureResult::kFinishedWritingCrashReportFailed); return false; diff --git a/util/BUILD.gn b/util/BUILD.gn index 17406edb0b..3fe82c395e 100644 --- a/util/BUILD.gn +++ b/util/BUILD.gn @@ -288,10 +288,10 @@ crashpad_static_library("util") { sources += [ "posix/close_multiple.cc", "posix/close_multiple.h", + "posix/double_fork_and_exec.cc", + "posix/double_fork_and_exec.h", "posix/drop_privileges.cc", "posix/drop_privileges.h", - "posix/fork_and_spawn.cc", - "posix/fork_and_spawn.h", "posix/process_info.h", # These map signals to and from strings. While Fuchsia defines some of diff --git a/util/posix/double_fork_and_exec.cc b/util/posix/double_fork_and_exec.cc new file mode 100644 index 0000000000..1960430954 --- /dev/null +++ b/util/posix/double_fork_and_exec.cc @@ -0,0 +1,166 @@ +// 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/posix/double_fork_and_exec.h" + +#include +#include +#include +#include + +#include "base/check_op.h" +#include "base/logging.h" +#include "base/posix/eintr_wrapper.h" +#include "base/strings/stringprintf.h" +#include "util/posix/close_multiple.h" + +namespace crashpad { + +bool DoubleForkAndExec(const std::vector& argv, + const std::vector* envp, + int preserve_fd, + bool use_path, + void (*child_function)()) { + DCHECK(!envp || !use_path); + + // argv_c contains const char* pointers and is terminated by nullptr. This is + // suitable for passing to execv(). Although argv_c is not used in the parent + // process, it must be built in the parent process because it’s unsafe to do + // so in the child or grandchild process. + std::vector argv_c; + argv_c.reserve(argv.size() + 1); + for (const std::string& argument : argv) { + argv_c.push_back(argument.c_str()); + } + argv_c.push_back(nullptr); + + std::vector envp_c; + if (envp) { + envp_c.reserve(envp->size() + 1); + for (const std::string& variable : *envp) { + envp_c.push_back(variable.c_str()); + } + envp_c.push_back(nullptr); + } + + // Double-fork(). The three processes involved are parent, child, and + // grandchild. The grandchild will call execv(). The child exits immediately + // after spawning the grandchild, so the grandchild becomes an orphan and its + // parent process ID becomes 1. This relieves the parent and child of the + // responsibility to reap the grandchild with waitpid() or similar. The + // grandchild is expected to outlive the parent process, so the parent + // shouldn’t be concerned with reaping it. This approach means that accidental + // early termination of the handler process will not result in a zombie + // process. + pid_t pid = fork(); + if (pid < 0) { + PLOG(ERROR) << "fork"; + return false; + } + + if (pid == 0) { + // Child process. + + if (child_function) { + child_function(); + } + + // Call setsid(), creating a new process group and a new session, both led + // by this process. The new process group has no controlling terminal. This + // disconnects it from signals generated by the parent process’ terminal. + // + // setsid() is done in the child instead of the grandchild so that the + // grandchild will not be a session leader. If it were a session leader, an + // accidental open() of a terminal device without O_NOCTTY would make that + // terminal the controlling terminal. + // + // It’s not desirable for the grandchild to have a controlling terminal. The + // grandchild manages its own lifetime, such as by monitoring clients on its + // own and exiting when it loses all clients and when it deems it + // appropraite to do so. It may serve clients in different process groups or + // sessions than its original client, and receiving signals intended for its + // original client’s process group could be harmful in that case. + PCHECK(setsid() != -1) << "setsid"; + + pid = fork(); + if (pid < 0) { + PLOG(FATAL) << "fork"; + } + + if (pid > 0) { + // Child process. + + // _exit() instead of exit(), because fork() was called. + _exit(EXIT_SUCCESS); + } + + // Grandchild process. + + CloseMultipleNowOrOnExec(STDERR_FILENO + 1, preserve_fd); + + // &argv_c[0] is a pointer to a pointer to const char data, but because of + // how C (not C++) works, execvp() wants a pointer to a const pointer to + // char data. It modifies neither the data nor the pointers, so the + // const_cast is safe. + char* const* argv_for_execv = const_cast(&argv_c[0]); + + if (envp) { + // This cast is safe for the same reason that the argv_for_execv cast is. + char* const* envp_for_execv = const_cast(&envp_c[0]); + execve(argv_for_execv[0], argv_for_execv, envp_for_execv); + PLOG(FATAL) << "execve " << argv_for_execv[0]; + } + + if (use_path) { + execvp(argv_for_execv[0], argv_for_execv); + PLOG(FATAL) << "execvp " << argv_for_execv[0]; + } + + execv(argv_for_execv[0], argv_for_execv); + PLOG(FATAL) << "execv " << argv_for_execv[0]; + } + + // waitpid() for the child, so that it does not become a zombie process. The + // child normally exits quickly. + // + // Failures from this point on may result in the accumulation of a zombie, but + // should not be considered fatal. Log only warnings, but don’t treat these + // failures as a failure of the function overall. + int status; + pid_t wait_pid = HANDLE_EINTR(waitpid(pid, &status, 0)); + if (wait_pid == -1) { + PLOG(WARNING) << "waitpid"; + return true; + } + DCHECK_EQ(wait_pid, pid); + + if (WIFSIGNALED(status)) { + int sig = WTERMSIG(status); + LOG(WARNING) << base::StringPrintf( + "intermediate process terminated by signal %d (%s)%s", + sig, + strsignal(sig), + WCOREDUMP(status) ? " (core dumped)" : ""); + } else if (!WIFEXITED(status)) { + LOG(WARNING) << base::StringPrintf( + "intermediate process: unknown termination 0x%x", status); + } else if (WEXITSTATUS(status) != EXIT_SUCCESS) { + LOG(WARNING) << "intermediate process exited with code " + << WEXITSTATUS(status); + } + + return true; +} + +} // namespace crashpad diff --git a/util/posix/fork_and_spawn.h b/util/posix/double_fork_and_exec.h similarity index 76% rename from util/posix/fork_and_spawn.h rename to util/posix/double_fork_and_exec.h index fc55aa3a37..02fc0f28f1 100644 --- a/util/posix/fork_and_spawn.h +++ b/util/posix/double_fork_and_exec.h @@ -12,8 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. -#ifndef CRASHPAD_UTIL_POSIX_FORK_AND_SPAWN_H_ -#define CRASHPAD_UTIL_POSIX_FORK_AND_SPAWN_H_ +#ifndef CRASHPAD_UTIL_POSIX_DOUBLE_FORK_AND_EXEC_H_ +#define CRASHPAD_UTIL_POSIX_DOUBLE_FORK_AND_EXEC_H_ #include #include @@ -23,7 +23,7 @@ namespace crashpad { //! \brief Executes a (grand-)child process. //! //! The grandchild process will be started through the -//! `fork()`-and-`posix_spawn()` pattern. This allows the grandchild to fully +//! double-`fork()`-and-`execv()` pattern. This allows the grandchild to fully //! disassociate from the parent. The grandchild will not be a member of the //! parent’s process group or session and will not have a controlling terminal, //! providing isolation from signals not intended for it. The grandchild’s @@ -37,7 +37,7 @@ namespace crashpad { //! \param[in] argv The argument vector to start the grandchild process with. //! `argv[0]` is used as the path to the executable. //! \param[in] envp A vector of environment variables of the form `var=value` to -//! be passed to `posix_spawn()`. If this value is `nullptr`, the current +//! be passed to `execve()`. If this value is `nullptr`, the current //! environment is used. //! \param[in] preserve_fd A file descriptor to be inherited by the grandchild //! process. This file descriptor is inherited in addition to the three file @@ -45,13 +45,16 @@ namespace crashpad { //! if no additional file descriptors are to be inherited. //! \param[in] use_path Whether to consult the `PATH` environment variable when //! requested to start an executable at a non-absolute path. If `false`, -//! `posix_spawn()`, which does not consult `PATH`, will be used. If `true`, -//! `posix_spawnp()`, which does consult `PATH`, will be used. +//! `execv()`, which does not consult `PATH`, will be used. If `true`, +//! `execvp()`, which does consult `PATH`, will be used. //! \param[in] child_function If not `nullptr`, this function will be called in -//! the intermediate child process, prior to the `posix_spawn()`. Take note +//! the intermediate child process, prior to the second `fork()`. Take note //! that this function will run in the context of a forked process, and must //! be safe for that purpose. //! +//! Setting both \a envp to a value other than `nullptr` and \a use_path to +//! `true` is not currently supported. +//! //! \return `true` on success, and `false` on failure with a message logged. //! Only failures that occur in the parent process that indicate a definite //! failure to start the the grandchild are reported in the return value. @@ -60,12 +63,12 @@ namespace crashpad { //! terminating. The caller assumes the responsibility for detecting such //! failures, for example, by observing a failure to perform a successful //! handshake with the grandchild process. -bool ForkAndSpawn(const std::vector& argv, - const std::vector* envp, - int preserve_fd, - bool use_path, - void (*child_function)()); +bool DoubleForkAndExec(const std::vector& argv, + const std::vector* envp, + int preserve_fd, + bool use_path, + void (*child_function)()); } // namespace crashpad -#endif // CRASHPAD_UTIL_POSIX_FORK_AND_SPAWN_H_ +#endif // CRASHPAD_UTIL_POSIX_DOUBLE_FORK_AND_EXEC_H_ diff --git a/util/posix/fork_and_spawn.cc b/util/posix/fork_and_spawn.cc deleted file mode 100644 index c6a95bbfdc..0000000000 --- a/util/posix/fork_and_spawn.cc +++ /dev/null @@ -1,235 +0,0 @@ -// 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/posix/fork_and_spawn.h" - -#include -#include -#include -#include -#include -#include - -#include "base/check.h" -#include "base/check_op.h" -#include "base/logging.h" -#include "base/posix/eintr_wrapper.h" -#include "base/strings/stringprintf.h" -#include "build/build_config.h" -#include "util/posix/close_multiple.h" - -extern char** environ; - -namespace crashpad { - -namespace { - -#if BUILDFLAG(IS_APPLE) - -class PosixSpawnAttr { - public: - PosixSpawnAttr() { - PCHECK((errno = posix_spawnattr_init(&attr_)) == 0) - << "posix_spawnattr_init"; - } - - PosixSpawnAttr(const PosixSpawnAttr&) = delete; - PosixSpawnAttr& operator=(const PosixSpawnAttr&) = delete; - - ~PosixSpawnAttr() { - PCHECK((errno = posix_spawnattr_destroy(&attr_)) == 0) - << "posix_spawnattr_destroy"; - } - - void SetFlags(short flags) { - PCHECK((errno = posix_spawnattr_setflags(&attr_, flags)) == 0) - << "posix_spawnattr_setflags"; - } - - const posix_spawnattr_t* Get() const { return &attr_; } - - private: - posix_spawnattr_t attr_; -}; - -class PosixSpawnFileActions { - public: - PosixSpawnFileActions() { - PCHECK((errno = posix_spawn_file_actions_init(&file_actions_)) == 0) - << "posix_spawn_file_actions_init"; - } - - PosixSpawnFileActions(const PosixSpawnFileActions&) = delete; - PosixSpawnFileActions& operator=(const PosixSpawnFileActions&) = delete; - - ~PosixSpawnFileActions() { - PCHECK((errno = posix_spawn_file_actions_destroy(&file_actions_)) == 0) - << "posix_spawn_file_actions_destroy"; - } - - void AddInheritedFileDescriptor(int fd) { - PCHECK((errno = posix_spawn_file_actions_addinherit_np(&file_actions_, - fd)) == 0) - << "posix_spawn_file_actions_addinherit_np"; - } - - const posix_spawn_file_actions_t* Get() const { return &file_actions_; } - - private: - posix_spawn_file_actions_t file_actions_; -}; - -#endif - -} // namespace - -bool ForkAndSpawn(const std::vector& argv, - const std::vector* envp, - int preserve_fd, - bool use_path, - void (*child_function)()) { - // argv_c contains const char* pointers and is terminated by nullptr. This is - // suitable for passing to posix_spawn() or posix_spawnp(). Although argv_c is - // not used in the parent process, it must be built in the parent process - // because it’s unsafe to do so in the child. - std::vector argv_c; - argv_c.reserve(argv.size() + 1); - for (const std::string& argument : argv) { - argv_c.push_back(argument.c_str()); - } - argv_c.push_back(nullptr); - - std::vector envp_c; - if (envp) { - envp_c.reserve(envp->size() + 1); - for (const std::string& variable : *envp) { - envp_c.push_back(variable.c_str()); - } - envp_c.push_back(nullptr); - } - - // The three processes involved are parent, child, and grandchild. The child - // exits immediately after spawning the grandchild, so the grandchild becomes - // an orphan and its parent process ID becomes 1. This relieves the parent and - // child of the responsibility to reap the grandchild with waitpid() or - // similar. The grandchild is expected to outlive the parent process, so the - // parent shouldn’t be concerned with reaping it. This approach means that - // accidental early termination of the handler process will not result in a - // zombie process. - pid_t pid = fork(); - if (pid < 0) { - PLOG(ERROR) << "fork"; - return false; - } - - if (pid == 0) { - // Child process. - - if (child_function) { - child_function(); - } - - // Call setsid(), creating a new process group and a new session, both led - // by this process. The new process group has no controlling terminal. This - // disconnects it from signals generated by the parent process’ terminal. - // - // setsid() is done in the child instead of the grandchild so that the - // grandchild will not be a session leader. If it were a session leader, an - // accidental open() of a terminal device without O_NOCTTY would make that - // terminal the controlling terminal. - // - // It’s not desirable for the grandchild to have a controlling terminal. The - // grandchild manages its own lifetime, such as by monitoring clients on its - // own and exiting when it loses all clients and when it deems it - // appropraite to do so. It may serve clients in different process groups or - // sessions than its original client, and receiving signals intended for its - // original client’s process group could be harmful in that case. - PCHECK(setsid() != -1) << "setsid"; - - // &argv_c[0] is a pointer to a pointer to const char data, but because of - // how C (not C++) works, posix_spawn() and posix_spawnp() want a pointer to - // a const pointer to char data. They modifies neither the data nor the - // pointers, so the const_cast is safe. - char* const* argv_for_spawn = const_cast(argv_c.data()); - - // This cast is safe for the same reason that the argv_for_spawn cast is. - char* const* envp_for_spawn = - envp ? const_cast(envp_c.data()) : environ; - -#if BUILDFLAG(IS_APPLE) - PosixSpawnAttr attr; - attr.SetFlags(POSIX_SPAWN_CLOEXEC_DEFAULT); - - PosixSpawnFileActions file_actions; - for (int fd = 0; fd <= STDERR_FILENO; ++fd) { - file_actions.AddInheritedFileDescriptor(fd); - } - file_actions.AddInheritedFileDescriptor(preserve_fd); - - const posix_spawnattr_t* attr_p = attr.Get(); - const posix_spawn_file_actions_t* file_actions_p = file_actions.Get(); -#else - CloseMultipleNowOrOnExec(STDERR_FILENO + 1, preserve_fd); - - const posix_spawnattr_t* attr_p = nullptr; - const posix_spawn_file_actions_t* file_actions_p = nullptr; -#endif - - auto posix_spawn_fp = use_path ? posix_spawnp : posix_spawn; - if ((errno = posix_spawn_fp(&pid, - argv_for_spawn[0], - file_actions_p, - attr_p, - argv_for_spawn, - envp_for_spawn)) != 0) { - PLOG(FATAL) << (use_path ? "posix_spawnp" : "posix_spawn"); - } - - // _exit() instead of exit(), because fork() was called. - _exit(EXIT_SUCCESS); - } - - // waitpid() for the child, so that it does not become a zombie process. The - // child normally exits quickly. - // - // Failures from this point on may result in the accumulation of a zombie, but - // should not be considered fatal. Log only warnings, but don’t treat these - // failures as a failure of the function overall. - int status; - pid_t wait_pid = HANDLE_EINTR(waitpid(pid, &status, 0)); - if (wait_pid == -1) { - PLOG(WARNING) << "waitpid"; - return true; - } - DCHECK_EQ(wait_pid, pid); - - if (WIFSIGNALED(status)) { - int sig = WTERMSIG(status); - LOG(WARNING) << base::StringPrintf( - "intermediate process terminated by signal %d (%s)%s", - sig, - strsignal(sig), - WCOREDUMP(status) ? " (core dumped)" : ""); - } else if (!WIFEXITED(status)) { - LOG(WARNING) << base::StringPrintf( - "intermediate process: unknown termination 0x%x", status); - } else if (WEXITSTATUS(status) != EXIT_SUCCESS) { - LOG(WARNING) << "intermediate process exited with code " - << WEXITSTATUS(status); - } - - return true; -} - -} // namespace crashpad From 1c37daa5acd500633cdffc7f6db61d8c6b32b771 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Fri, 24 Jun 2022 19:20:54 +0530 Subject: [PATCH 11/20] Reland "posix: Replace DoubleForkAndExec() with ForkAndSpawn()" This is a reland of 460943dd9a71dc76f68182a8ede766d5543e5341 Original change's description: > The DoubleForkAndExec() function was taking over 622 milliseconds to run > on macOS 11 (BigSur) on Intel i5-1038NG7. I did some debugging by adding > some custom traces and found that the fork() syscall is the bottleneck > here, i.e., the first fork() takes around 359 milliseconds and the > nested fork() takes around 263 milliseconds. Replacing the nested fork() > and exec() with posix_spawn() reduces the time consumption to 257 > milliseconds! > > See https://github.com/libuv/libuv/pull/3064 to know why fork() is so > slow on macOS and why posix_spawn() is a better replacement. > > Another point to note is that even base::LaunchProcess() from Chromium > calls posix_spawnp() on macOS - > https://source.chromium.org/chromium/chromium/src/+/8f8d82dea0fa8f11f57c74dbb65126f8daba58f7:base/process/launch_mac.cc;l=295-296 The reland isolates the change to non-Android POSIX systems because posix_spawn and posix_spawnp are available in Android NDK 28, but Chromium is building with version 23. Change-Id: If44629f5445bb0e3d0a1d3698b85f047d1cbf04f Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3721655 Reviewed-by: Mark Mentovai Commit-Queue: Mark Mentovai --- AUTHORS | 1 + client/crashpad_client_linux.cc | 10 +- client/crashpad_client_mac.cc | 4 +- .../cros_crash_report_exception_handler.cc | 13 +- util/BUILD.gn | 4 +- util/posix/double_fork_and_exec.cc | 166 ----------- util/posix/spawn_subprocess.cc | 261 ++++++++++++++++++ ...ble_fork_and_exec.h => spawn_subprocess.h} | 57 ++-- 8 files changed, 303 insertions(+), 213 deletions(-) delete mode 100644 util/posix/double_fork_and_exec.cc create mode 100644 util/posix/spawn_subprocess.cc rename util/posix/{double_fork_and_exec.h => spawn_subprocess.h} (53%) diff --git a/AUTHORS b/AUTHORS index 8dcac32388..0210392433 100644 --- a/AUTHORS +++ b/AUTHORS @@ -12,3 +12,4 @@ Opera Software ASA Vewd Software AS LG Electronics, Inc. MIPS Technologies, Inc. +Darshan Sen diff --git a/client/crashpad_client_linux.cc b/client/crashpad_client_linux.cc index 295ec1615d..9cd452b577 100644 --- a/client/crashpad_client_linux.cc +++ b/client/crashpad_client_linux.cc @@ -45,9 +45,9 @@ #include "util/linux/socket.h" #include "util/misc/address_sanitizer.h" #include "util/misc/from_pointer_cast.h" -#include "util/posix/double_fork_and_exec.h" #include "util/posix/scoped_mmap.h" #include "util/posix/signals.h" +#include "util/posix/spawn_subprocess.h" namespace crashpad { @@ -459,7 +459,7 @@ bool CrashpadClient::StartHandler( argv.push_back(FormatArgumentInt("initial-client-fd", handler_sock.get())); argv.push_back("--shared-client-connection"); - if (!DoubleForkAndExec(argv, nullptr, handler_sock.get(), false, nullptr)) { + if (!SpawnSubprocess(argv, nullptr, handler_sock.get(), false, nullptr)) { return false; } @@ -614,7 +614,7 @@ bool CrashpadClient::StartJavaHandlerForClient( int socket) { std::vector argv = BuildAppProcessArgs( class_name, database, metrics_dir, url, annotations, arguments, socket); - return DoubleForkAndExec(argv, env, socket, false, nullptr); + return SpawnSubprocess(argv, env, socket, false, nullptr); } bool CrashpadClient::StartHandlerWithLinkerAtCrash( @@ -663,7 +663,7 @@ bool CrashpadClient::StartHandlerWithLinkerForClient( annotations, arguments, socket); - return DoubleForkAndExec(argv, env, socket, false, nullptr); + return SpawnSubprocess(argv, env, socket, false, nullptr); } #endif @@ -697,7 +697,7 @@ bool CrashpadClient::StartHandlerForClient( argv.push_back(FormatArgumentInt("initial-client-fd", socket)); - return DoubleForkAndExec(argv, nullptr, socket, true, nullptr); + return SpawnSubprocess(argv, nullptr, socket, true, nullptr); } // static diff --git a/client/crashpad_client_mac.cc b/client/crashpad_client_mac.cc index d25bfb7163..7e92cb86fd 100644 --- a/client/crashpad_client_mac.cc +++ b/client/crashpad_client_mac.cc @@ -36,7 +36,7 @@ #include "util/mach/notify_server.h" #include "util/misc/clock.h" #include "util/misc/implicit_cast.h" -#include "util/posix/double_fork_and_exec.h" +#include "util/posix/spawn_subprocess.h" namespace crashpad { @@ -343,7 +343,7 @@ class HandlerStarter final : public NotifyServer::DefaultInterface { // this parent process, which was probably using the exception server now // being restarted. The handler can’t monitor itself for its own crashes via // this interface. - if (!DoubleForkAndExec( + if (!SpawnSubprocess( argv, nullptr, server_write_fd.get(), diff --git a/handler/linux/cros_crash_report_exception_handler.cc b/handler/linux/cros_crash_report_exception_handler.cc index 9e58d94aa4..4804eacf85 100644 --- a/handler/linux/cros_crash_report_exception_handler.cc +++ b/handler/linux/cros_crash_report_exception_handler.cc @@ -29,7 +29,7 @@ #include "util/linux/ptrace_client.h" #include "util/misc/metrics.h" #include "util/misc/uuid.h" -#include "util/posix/double_fork_and_exec.h" +#include "util/posix/spawn_subprocess.h" namespace crashpad { @@ -266,12 +266,11 @@ bool CrosCrashReportExceptionHandler::HandleExceptionWithConnection( argv.push_back("--always_allow_feedback"); } - if (!DoubleForkAndExec(argv, - nullptr /* envp */, - file_writer.fd() /* preserve_fd */, - false /* use_path */, - nullptr /* child_function */)) { - LOG(ERROR) << "DoubleForkAndExec failed"; + if (!SpawnSubprocess(argv, + nullptr /* envp */, + file_writer.fd() /* preserve_fd */, + false /* use_path */, + nullptr /* child_function */)) { Metrics::ExceptionCaptureResult( Metrics::CaptureResult::kFinishedWritingCrashReportFailed); return false; diff --git a/util/BUILD.gn b/util/BUILD.gn index 3fe82c395e..6e6a8eec00 100644 --- a/util/BUILD.gn +++ b/util/BUILD.gn @@ -288,11 +288,11 @@ crashpad_static_library("util") { sources += [ "posix/close_multiple.cc", "posix/close_multiple.h", - "posix/double_fork_and_exec.cc", - "posix/double_fork_and_exec.h", "posix/drop_privileges.cc", "posix/drop_privileges.h", "posix/process_info.h", + "posix/spawn_subprocess.cc", + "posix/spawn_subprocess.h", # These map signals to and from strings. While Fuchsia defines some of # the common SIGx defines, signals are never raised on Fuchsia, so diff --git a/util/posix/double_fork_and_exec.cc b/util/posix/double_fork_and_exec.cc deleted file mode 100644 index 1960430954..0000000000 --- a/util/posix/double_fork_and_exec.cc +++ /dev/null @@ -1,166 +0,0 @@ -// 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/posix/double_fork_and_exec.h" - -#include -#include -#include -#include - -#include "base/check_op.h" -#include "base/logging.h" -#include "base/posix/eintr_wrapper.h" -#include "base/strings/stringprintf.h" -#include "util/posix/close_multiple.h" - -namespace crashpad { - -bool DoubleForkAndExec(const std::vector& argv, - const std::vector* envp, - int preserve_fd, - bool use_path, - void (*child_function)()) { - DCHECK(!envp || !use_path); - - // argv_c contains const char* pointers and is terminated by nullptr. This is - // suitable for passing to execv(). Although argv_c is not used in the parent - // process, it must be built in the parent process because it’s unsafe to do - // so in the child or grandchild process. - std::vector argv_c; - argv_c.reserve(argv.size() + 1); - for (const std::string& argument : argv) { - argv_c.push_back(argument.c_str()); - } - argv_c.push_back(nullptr); - - std::vector envp_c; - if (envp) { - envp_c.reserve(envp->size() + 1); - for (const std::string& variable : *envp) { - envp_c.push_back(variable.c_str()); - } - envp_c.push_back(nullptr); - } - - // Double-fork(). The three processes involved are parent, child, and - // grandchild. The grandchild will call execv(). The child exits immediately - // after spawning the grandchild, so the grandchild becomes an orphan and its - // parent process ID becomes 1. This relieves the parent and child of the - // responsibility to reap the grandchild with waitpid() or similar. The - // grandchild is expected to outlive the parent process, so the parent - // shouldn’t be concerned with reaping it. This approach means that accidental - // early termination of the handler process will not result in a zombie - // process. - pid_t pid = fork(); - if (pid < 0) { - PLOG(ERROR) << "fork"; - return false; - } - - if (pid == 0) { - // Child process. - - if (child_function) { - child_function(); - } - - // Call setsid(), creating a new process group and a new session, both led - // by this process. The new process group has no controlling terminal. This - // disconnects it from signals generated by the parent process’ terminal. - // - // setsid() is done in the child instead of the grandchild so that the - // grandchild will not be a session leader. If it were a session leader, an - // accidental open() of a terminal device without O_NOCTTY would make that - // terminal the controlling terminal. - // - // It’s not desirable for the grandchild to have a controlling terminal. The - // grandchild manages its own lifetime, such as by monitoring clients on its - // own and exiting when it loses all clients and when it deems it - // appropraite to do so. It may serve clients in different process groups or - // sessions than its original client, and receiving signals intended for its - // original client’s process group could be harmful in that case. - PCHECK(setsid() != -1) << "setsid"; - - pid = fork(); - if (pid < 0) { - PLOG(FATAL) << "fork"; - } - - if (pid > 0) { - // Child process. - - // _exit() instead of exit(), because fork() was called. - _exit(EXIT_SUCCESS); - } - - // Grandchild process. - - CloseMultipleNowOrOnExec(STDERR_FILENO + 1, preserve_fd); - - // &argv_c[0] is a pointer to a pointer to const char data, but because of - // how C (not C++) works, execvp() wants a pointer to a const pointer to - // char data. It modifies neither the data nor the pointers, so the - // const_cast is safe. - char* const* argv_for_execv = const_cast(&argv_c[0]); - - if (envp) { - // This cast is safe for the same reason that the argv_for_execv cast is. - char* const* envp_for_execv = const_cast(&envp_c[0]); - execve(argv_for_execv[0], argv_for_execv, envp_for_execv); - PLOG(FATAL) << "execve " << argv_for_execv[0]; - } - - if (use_path) { - execvp(argv_for_execv[0], argv_for_execv); - PLOG(FATAL) << "execvp " << argv_for_execv[0]; - } - - execv(argv_for_execv[0], argv_for_execv); - PLOG(FATAL) << "execv " << argv_for_execv[0]; - } - - // waitpid() for the child, so that it does not become a zombie process. The - // child normally exits quickly. - // - // Failures from this point on may result in the accumulation of a zombie, but - // should not be considered fatal. Log only warnings, but don’t treat these - // failures as a failure of the function overall. - int status; - pid_t wait_pid = HANDLE_EINTR(waitpid(pid, &status, 0)); - if (wait_pid == -1) { - PLOG(WARNING) << "waitpid"; - return true; - } - DCHECK_EQ(wait_pid, pid); - - if (WIFSIGNALED(status)) { - int sig = WTERMSIG(status); - LOG(WARNING) << base::StringPrintf( - "intermediate process terminated by signal %d (%s)%s", - sig, - strsignal(sig), - WCOREDUMP(status) ? " (core dumped)" : ""); - } else if (!WIFEXITED(status)) { - LOG(WARNING) << base::StringPrintf( - "intermediate process: unknown termination 0x%x", status); - } else if (WEXITSTATUS(status) != EXIT_SUCCESS) { - LOG(WARNING) << "intermediate process exited with code " - << WEXITSTATUS(status); - } - - return true; -} - -} // namespace crashpad diff --git a/util/posix/spawn_subprocess.cc b/util/posix/spawn_subprocess.cc new file mode 100644 index 0000000000..43fca18deb --- /dev/null +++ b/util/posix/spawn_subprocess.cc @@ -0,0 +1,261 @@ +// 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/posix/spawn_subprocess.h" + +#include +#include +#include +#include +#include +#include + +#include "base/check.h" +#include "base/check_op.h" +#include "base/logging.h" +#include "base/posix/eintr_wrapper.h" +#include "base/strings/stringprintf.h" +#include "build/build_config.h" +#include "util/posix/close_multiple.h" + +#if BUILDFLAG(IS_ANDROID) +#include +#endif + +extern char** environ; + +namespace crashpad { + +namespace { + +#if BUILDFLAG(IS_APPLE) + +class PosixSpawnAttr { + public: + PosixSpawnAttr() { + PCHECK((errno = posix_spawnattr_init(&attr_)) == 0) + << "posix_spawnattr_init"; + } + + PosixSpawnAttr(const PosixSpawnAttr&) = delete; + PosixSpawnAttr& operator=(const PosixSpawnAttr&) = delete; + + ~PosixSpawnAttr() { + PCHECK((errno = posix_spawnattr_destroy(&attr_)) == 0) + << "posix_spawnattr_destroy"; + } + + void SetFlags(short flags) { + PCHECK((errno = posix_spawnattr_setflags(&attr_, flags)) == 0) + << "posix_spawnattr_setflags"; + } + + const posix_spawnattr_t* Get() const { return &attr_; } + + private: + posix_spawnattr_t attr_; +}; + +class PosixSpawnFileActions { + public: + PosixSpawnFileActions() { + PCHECK((errno = posix_spawn_file_actions_init(&file_actions_)) == 0) + << "posix_spawn_file_actions_init"; + } + + PosixSpawnFileActions(const PosixSpawnFileActions&) = delete; + PosixSpawnFileActions& operator=(const PosixSpawnFileActions&) = delete; + + ~PosixSpawnFileActions() { + PCHECK((errno = posix_spawn_file_actions_destroy(&file_actions_)) == 0) + << "posix_spawn_file_actions_destroy"; + } + + void AddInheritedFileDescriptor(int fd) { + PCHECK((errno = posix_spawn_file_actions_addinherit_np(&file_actions_, + fd)) == 0) + << "posix_spawn_file_actions_addinherit_np"; + } + + const posix_spawn_file_actions_t* Get() const { return &file_actions_; } + + private: + posix_spawn_file_actions_t file_actions_; +}; + +#endif + +} // namespace + +bool SpawnSubprocess(const std::vector& argv, + const std::vector* envp, + int preserve_fd, + bool use_path, + void (*child_function)()) { + // argv_c contains const char* pointers and is terminated by nullptr. This is + // suitable for passing to posix_spawn*() and execv*(). Although argv_c is not + // used in the parent process, it must be built in the parent process because + // it’s unsafe to do so in the child or grandchild process. + std::vector argv_c; + argv_c.reserve(argv.size() + 1); + for (const std::string& argument : argv) { + argv_c.push_back(argument.c_str()); + } + argv_c.push_back(nullptr); + + std::vector envp_c; + if (envp) { + envp_c.reserve(envp->size() + 1); + for (const std::string& variable : *envp) { + envp_c.push_back(variable.c_str()); + } + envp_c.push_back(nullptr); + } + + // The three processes involved are parent, child, and grandchild. The child + // exits immediately after spawning the grandchild, so the grandchild becomes + // an orphan and its parent process ID becomes 1. This relieves the parent and + // child of the responsibility to reap the grandchild with waitpid() or + // similar. The grandchild is expected to outlive the parent process, so the + // parent shouldn’t be concerned with reaping it. This approach means that + // accidental early termination of the handler process will not result in a + // zombie process. + pid_t pid = fork(); + if (pid < 0) { + PLOG(ERROR) << "fork"; + return false; + } + + if (pid == 0) { + // Child process. + + if (child_function) { + child_function(); + } + + // Call setsid(), creating a new process group and a new session, both led + // by this process. The new process group has no controlling terminal. This + // disconnects it from signals generated by the parent process’ terminal. + // + // setsid() is done in the child instead of the grandchild so that the + // grandchild will not be a session leader. If it were a session leader, an + // accidental open() of a terminal device without O_NOCTTY would make that + // terminal the controlling terminal. + // + // It’s not desirable for the grandchild to have a controlling terminal. The + // grandchild manages its own lifetime, such as by monitoring clients on its + // own and exiting when it loses all clients and when it deems it + // appropraite to do so. It may serve clients in different process groups or + // sessions than its original client, and receiving signals intended for its + // original client’s process group could be harmful in that case. + PCHECK(setsid() != -1) << "setsid"; + + // &argv_c[0] is a pointer to a pointer to const char data, but because of + // how C (not C++) works, posix_spawn*() and execv*() want a pointer to + // a const pointer to char data. They modify neither the data nor the + // pointers, so the const_cast is safe. + char* const* argv_for_spawn = const_cast(argv_c.data()); + + // This cast is safe for the same reason that the argv_for_spawn cast is. + char* const* envp_for_spawn = + envp ? const_cast(envp_c.data()) : environ; + +#if BUILDFLAG(IS_ANDROID) && __ANDROID_API__ < 28 + pid = fork(); + if (pid < 0) { + PLOG(FATAL) << "fork"; + } + + if (pid > 0) { + // Child process. + + // _exit() instead of exit(), because fork() was called. + _exit(EXIT_SUCCESS); + } + + // Grandchild process. + + CloseMultipleNowOrOnExec(STDERR_FILENO + 1, preserve_fd); + + auto execve_fp = use_path ? execvpe : execve; + execve_fp(argv_for_spawn[0], argv_for_spawn, envp_for_spawn); + PLOG(FATAL) << (use_path ? "execvpe" : "execve"); +#else +#if BUILDFLAG(IS_APPLE) + PosixSpawnAttr attr; + attr.SetFlags(POSIX_SPAWN_CLOEXEC_DEFAULT); + + PosixSpawnFileActions file_actions; + for (int fd = 0; fd <= STDERR_FILENO; ++fd) { + file_actions.AddInheritedFileDescriptor(fd); + } + file_actions.AddInheritedFileDescriptor(preserve_fd); + + const posix_spawnattr_t* attr_p = attr.Get(); + const posix_spawn_file_actions_t* file_actions_p = file_actions.Get(); +#else + CloseMultipleNowOrOnExec(STDERR_FILENO + 1, preserve_fd); + + const posix_spawnattr_t* attr_p = nullptr; + const posix_spawn_file_actions_t* file_actions_p = nullptr; +#endif + + auto posix_spawn_fp = use_path ? posix_spawnp : posix_spawn; + if ((errno = posix_spawn_fp(nullptr, + argv_for_spawn[0], + file_actions_p, + attr_p, + argv_for_spawn, + envp_for_spawn)) != 0) { + PLOG(FATAL) << (use_path ? "posix_spawnp" : "posix_spawn"); + } + + // _exit() instead of exit(), because fork() was called. + _exit(EXIT_SUCCESS); +#endif + } + + // waitpid() for the child, so that it does not become a zombie process. The + // child normally exits quickly. + // + // Failures from this point on may result in the accumulation of a zombie, but + // should not be considered fatal. Log only warnings, but don’t treat these + // failures as a failure of the function overall. + int status; + pid_t wait_pid = HANDLE_EINTR(waitpid(pid, &status, 0)); + if (wait_pid == -1) { + PLOG(WARNING) << "waitpid"; + return true; + } + DCHECK_EQ(wait_pid, pid); + + if (WIFSIGNALED(status)) { + int sig = WTERMSIG(status); + LOG(WARNING) << base::StringPrintf( + "intermediate process terminated by signal %d (%s)%s", + sig, + strsignal(sig), + WCOREDUMP(status) ? " (core dumped)" : ""); + } else if (!WIFEXITED(status)) { + LOG(WARNING) << base::StringPrintf( + "intermediate process: unknown termination 0x%x", status); + } else if (WEXITSTATUS(status) != EXIT_SUCCESS) { + LOG(WARNING) << "intermediate process exited with code " + << WEXITSTATUS(status); + } + + return true; +} + +} // namespace crashpad diff --git a/util/posix/double_fork_and_exec.h b/util/posix/spawn_subprocess.h similarity index 53% rename from util/posix/double_fork_and_exec.h rename to util/posix/spawn_subprocess.h index 02fc0f28f1..23910810b1 100644 --- a/util/posix/double_fork_and_exec.h +++ b/util/posix/spawn_subprocess.h @@ -12,48 +12,43 @@ // See the License for the specific language governing permissions and // limitations under the License. -#ifndef CRASHPAD_UTIL_POSIX_DOUBLE_FORK_AND_EXEC_H_ -#define CRASHPAD_UTIL_POSIX_DOUBLE_FORK_AND_EXEC_H_ +#ifndef CRASHPAD_UTIL_POSIX_SPAWN_SUBPROCESS_H_ +#define CRASHPAD_UTIL_POSIX_SPAWN_SUBPROCESS_H_ #include #include namespace crashpad { -//! \brief Executes a (grand-)child process. +//! \brief Spawns a subprocess. //! -//! The grandchild process will be started through the -//! double-`fork()`-and-`execv()` pattern. This allows the grandchild to fully -//! disassociate from the parent. The grandchild will not be a member of the -//! parent’s process group or session and will not have a controlling terminal, -//! providing isolation from signals not intended for it. The grandchild’s -//! parent process, in terms of the process tree hierarchy, will be the process -//! with process ID 1, relieving any other process of the responsibility to reap -//! it via `waitpid()`. Aside from the three file descriptors associated with -//! the standard input/output streams and any file descriptor passed in \a -//! preserve_fd, the grandchild will not inherit any file descriptors from the -//! parent process. +//! A grandchild process will be started through the +//! `fork()`-and-`posix_spawn()` pattern where supported, and +//! double-`fork()`-and-`execv()` pattern elsewhere. This allows the grandchild +//! to fully disassociate from the parent. The grandchild will not be a member +//! of the parent’s process group or session and will not have a controlling +//! terminal, providing isolation from signals not intended for it. The +//! grandchild’s parent process, in terms of the process tree hierarchy, will be +//! the process with process ID 1, relieving any other process of the +//! responsibility to reap it via `waitpid()`. Aside from the three file +//! descriptors associated with the standard input/output streams and any file +//! descriptor passed in \a preserve_fd, the grandchild will not inherit any +//! file descriptors from the parent process. //! //! \param[in] argv The argument vector to start the grandchild process with. //! `argv[0]` is used as the path to the executable. //! \param[in] envp A vector of environment variables of the form `var=value` to -//! be passed to `execve()`. If this value is `nullptr`, the current -//! environment is used. +//! be passed to the spawned process. If this value is `nullptr`, the +//! current environment is used. //! \param[in] preserve_fd A file descriptor to be inherited by the grandchild //! process. This file descriptor is inherited in addition to the three file //! descriptors associated with the standard input/output streams. Use `-1` //! if no additional file descriptors are to be inherited. //! \param[in] use_path Whether to consult the `PATH` environment variable when -//! requested to start an executable at a non-absolute path. If `false`, -//! `execv()`, which does not consult `PATH`, will be used. If `true`, -//! `execvp()`, which does consult `PATH`, will be used. +//! requested to start an executable at a non-absolute path. //! \param[in] child_function If not `nullptr`, this function will be called in -//! the intermediate child process, prior to the second `fork()`. Take note -//! that this function will run in the context of a forked process, and must -//! be safe for that purpose. -//! -//! Setting both \a envp to a value other than `nullptr` and \a use_path to -//! `true` is not currently supported. +//! the intermediate child process. Take note that this function will run in +//! the context of a forked process, and must be safe for that purpose. //! //! \return `true` on success, and `false` on failure with a message logged. //! Only failures that occur in the parent process that indicate a definite @@ -63,12 +58,12 @@ namespace crashpad { //! terminating. The caller assumes the responsibility for detecting such //! failures, for example, by observing a failure to perform a successful //! handshake with the grandchild process. -bool DoubleForkAndExec(const std::vector& argv, - const std::vector* envp, - int preserve_fd, - bool use_path, - void (*child_function)()); +bool SpawnSubprocess(const std::vector& argv, + const std::vector* envp, + int preserve_fd, + bool use_path, + void (*child_function)()); } // namespace crashpad -#endif // CRASHPAD_UTIL_POSIX_DOUBLE_FORK_AND_EXEC_H_ +#endif // CRASHPAD_UTIL_POSIX_SPAWN_SUBPROCESS_H_ From 80f383327eb5c55bc11d5d1d4917bd00a860871b Mon Sep 17 00:00:00 2001 From: Justin Cohen Date: Mon, 27 Jun 2022 12:36:34 -0400 Subject: [PATCH 12/20] [win] Fix ScopedSetThreadName for Windows 7 Windows 7 doesn't support SetThreadDescription/GetThreadDescription. Add an IsSupported to ScopedSetThreadName test to wrap unsupported calls. Change-Id: I70d4e20b94efea03e41c5f7ed8d8e1b886192923 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3722556 Commit-Queue: Justin Cohen Reviewed-by: Mark Mentovai --- snapshot/win/process_reader_win_test.cc | 43 ++++++++++++---------- test/scoped_set_thread_name.h | 7 +++- test/scoped_set_thread_name_win.cc | 48 +++++++++++++++++++------ 3 files changed, 68 insertions(+), 30 deletions(-) diff --git a/snapshot/win/process_reader_win_test.cc b/snapshot/win/process_reader_win_test.cc index 98cb11b7af..2ff9f1146d 100644 --- a/snapshot/win/process_reader_win_test.cc +++ b/snapshot/win/process_reader_win_test.cc @@ -123,7 +123,9 @@ TEST(ProcessReaderWin, SelfOneThread) { ASSERT_GE(threads.size(), 1u); EXPECT_EQ(threads[0].id, GetCurrentThreadId()); - EXPECT_EQ(threads[0].name, "SelfBasic"); + if (ScopedSetThreadName::IsSupported()) { + EXPECT_EQ(threads[0].name, "SelfBasic"); + } EXPECT_NE(ProgramCounterFromCONTEXT(threads[0].context.context()), nullptr); EXPECT_EQ(threads[0].suspend_count, 0u); @@ -174,28 +176,31 @@ class ProcessReaderChildThreadSuspendCount final : public WinMultiprocess { const auto& threads = process_reader.Threads(); ASSERT_GE(threads.size(), kCreatedThreads + 1); - EXPECT_EQ(threads[0].name, "WinMultiprocessChild-Main"); - - const std::set expected_thread_names = { - "WinMultiprocessChild-1", - "WinMultiprocessChild-2", - "WinMultiprocessChild-3", - }; - // Windows can create threads besides the ones created in - // WinMultiprocessChild(), so keep track of the (non-main) thread names - // and make sure all the expected names are present. - std::set thread_names; - for (size_t i = 1; i < threads.size(); i++) { - if (!threads[i].name.empty()) { - thread_names.emplace(threads[i].name); - } - } - - EXPECT_THAT(thread_names, IsSupersetOf(expected_thread_names)); for (const auto& thread : threads) { EXPECT_EQ(thread.suspend_count, 0u); } + + if (ScopedSetThreadName::IsSupported()) { + EXPECT_EQ(threads[0].name, "WinMultiprocessChild-Main"); + + const std::set expected_thread_names = { + "WinMultiprocessChild-1", + "WinMultiprocessChild-2", + "WinMultiprocessChild-3", + }; + // Windows can create threads besides the ones created in + // WinMultiprocessChild(), so keep track of the (non-main) thread names + // and make sure all the expected names are present. + std::set thread_names; + for (size_t i = 1; i < threads.size(); i++) { + if (!threads[i].name.empty()) { + thread_names.emplace(threads[i].name); + } + } + + EXPECT_THAT(thread_names, IsSupersetOf(expected_thread_names)); + } } { diff --git a/test/scoped_set_thread_name.h b/test/scoped_set_thread_name.h index 69998c03b9..dc149e4f15 100644 --- a/test/scoped_set_thread_name.h +++ b/test/scoped_set_thread_name.h @@ -32,9 +32,14 @@ class ScopedSetThreadName final { ~ScopedSetThreadName(); +#if BUILDFLAG(IS_WIN) || DOXYGEN + //! \brief Returns `true` if Windows supports setting and getting thread name. + static bool IsSupported(); +#endif + private: #if BUILDFLAG(IS_WIN) - const std::wstring original_name_; + std::wstring original_name_; #else const std::string original_name_; #endif diff --git a/test/scoped_set_thread_name_win.cc b/test/scoped_set_thread_name_win.cc index 482d3c12e5..37c87ee86c 100644 --- a/test/scoped_set_thread_name_win.cc +++ b/test/scoped_set_thread_name_win.cc @@ -19,6 +19,7 @@ #include "base/check.h" #include "base/logging.h" #include "base/strings/utf_string_conversions.h" +#include "util/win/get_function.h" #include "util/win/scoped_local_alloc.h" namespace crashpad { @@ -26,30 +27,57 @@ namespace test { namespace { +auto GetThreadDescriptionFuncPtr() { + static const auto get_thread_description = + GET_FUNCTION(L"kernel32.dll", ::GetThreadDescription); + return get_thread_description; +} + +auto SetThreadDescriptionFuncPtr() { + static const auto set_thread_description = + GET_FUNCTION(L"kernel32.dll", ::SetThreadDescription); + return set_thread_description; +} + std::wstring GetCurrentThreadName() { wchar_t* thread_description; - HRESULT hr = GetThreadDescription(GetCurrentThread(), &thread_description); + const auto get_thread_description = GetThreadDescriptionFuncPtr(); + DCHECK(get_thread_description); + HRESULT hr = get_thread_description(GetCurrentThread(), &thread_description); CHECK(SUCCEEDED(hr)) << "GetThreadDescription: " << logging::SystemErrorCodeToString(hr); ScopedLocalAlloc thread_description_owner(thread_description); return std::wstring(thread_description); } -} // namespace - -ScopedSetThreadName::ScopedSetThreadName(const std::string& new_thread_name) - : original_name_(GetCurrentThreadName()) { - const std::wstring wnew_thread_name = base::UTF8ToWide(new_thread_name); +void SetCurrentThreadName(const std::wstring& new_thread_name) { + const auto set_thread_description = SetThreadDescriptionFuncPtr(); + DCHECK(set_thread_description); HRESULT hr = - SetThreadDescription(GetCurrentThread(), wnew_thread_name.c_str()); + set_thread_description(GetCurrentThread(), new_thread_name.c_str()); CHECK(SUCCEEDED(hr)) << "SetThreadDescription: " << logging::SystemErrorCodeToString(hr); } +} // namespace + +ScopedSetThreadName::ScopedSetThreadName(const std::string& new_thread_name) + : original_name_() { + if (IsSupported()) { + original_name_.assign(GetCurrentThreadName()); + SetCurrentThreadName(base::UTF8ToWide(new_thread_name)); + } +} + ScopedSetThreadName::~ScopedSetThreadName() { - HRESULT hr = SetThreadDescription(GetCurrentThread(), original_name_.c_str()); - CHECK(SUCCEEDED(hr)) << "SetThreadDescription: " - << logging::SystemErrorCodeToString(hr); + if (IsSupported()) { + SetCurrentThreadName(original_name_); + } +} + +// static +bool ScopedSetThreadName::IsSupported() { + return GetThreadDescriptionFuncPtr() && SetThreadDescriptionFuncPtr(); } } // namespace test From bac699ef47cd7650575446d4a266d56f95097974 Mon Sep 17 00:00:00 2001 From: Justin Cohen Date: Mon, 27 Jun 2022 14:00:13 -0400 Subject: [PATCH 13/20] ios: Correct xcode-hybrid setup for Xcode 14. Changes copied verbatim from Chromium with one exception to remove Chromium specific gn args. This includes a mini_chromium roll to not codesign within Xcode. Change-Id: I89b35bee08f9bc9e37f902f2b57e02acb2113ae1 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3726509 Reviewed-by: Rohit Rao Reviewed-by: Mark Mentovai Commit-Queue: Justin Cohen --- DEPS | 2 +- build/ios/convert_gn_xcodeproj.py | 184 +++++++++++++----------- build/ios/setup_ios_gn.py | 11 +- build/ios/xcodescheme-testable.template | 63 ++++++++ build/ios/xcodescheme.template | 2 - 5 files changed, 166 insertions(+), 96 deletions(-) diff --git a/DEPS b/DEPS index 4a2012548c..0f29f6d024 100644 --- a/DEPS +++ b/DEPS @@ -44,7 +44,7 @@ deps = { 'e1e7b0ad8ee99a875b272c8e33e308472e897660', 'crashpad/third_party/mini_chromium/mini_chromium': Var('chromium_git') + '/chromium/mini_chromium@' + - '5654edb4225bcad13901155c819febb5748e502b', + '75dcb8dc417af77fdb9ec23c7b51cb1d57dfcee2', 'crashpad/third_party/libfuzzer/src': Var('chromium_git') + '/chromium/llvm-project/compiler-rt/lib/fuzzer.git@' + 'fda403cf93ecb8792cb1d061564d89a6553ca020', diff --git a/build/ios/convert_gn_xcodeproj.py b/build/ios/convert_gn_xcodeproj.py index 8c5c6d073f..2bce72d694 100755 --- a/build/ios/convert_gn_xcodeproj.py +++ b/build/ios/convert_gn_xcodeproj.py @@ -28,6 +28,7 @@ import filecmp import functools import hashlib +import io import json import os import re @@ -65,7 +66,7 @@ class Template(string.Template): @functools.lru_cache def LoadSchemeTemplate(root, name): """Return a string.Template object for scheme file loaded relative to root.""" - path = os.path.join(root, 'build', 'ios', name) + path = os.path.join(root, 'build', 'ios', name + '.template') with open(path) as file: return Template(file.read()) @@ -75,7 +76,7 @@ def CreateIdentifier(str_id): return hashlib.sha1(str_id.encode("utf-8")).hexdigest()[:24].upper() -def GenerateSchemeForTarget(root, project, old_project, name, path, tests): +def GenerateSchemeForTarget(root, project, old_project, name, path, is_test): """Generates the .xcsheme file for target named |name|. The file is generated in the new project schemes directory from a template. @@ -91,9 +92,23 @@ def GenerateSchemeForTarget(root, project, old_project, name, path, tests): if not os.path.isdir(os.path.dirname(scheme_path)): os.makedirs(os.path.dirname(scheme_path)) + substitutions = { + 'LLDBINIT_PATH': LLDBINIT_PATH, + 'BLUEPRINT_IDENTIFIER': identifier, + 'BUILDABLE_NAME': path, + 'BLUEPRINT_NAME': name, + 'PROJECT_NAME': project_name + } + + if is_test: + template = LoadSchemeTemplate(root, 'xcodescheme-testable') + substitutions['PATH'] = os.environ['PATH'] + + else: + template = LoadSchemeTemplate(root, 'xcodescheme') + old_scheme_path = os.path.join(old_project, relative_path) if os.path.exists(old_scheme_path): - made_changes = False tree = xml.etree.ElementTree.parse(old_scheme_path) tree_root = tree.getroot() @@ -105,7 +120,6 @@ def GenerateSchemeForTarget(root, project, old_project, name, path, tests): ('BlueprintIdentifier', identifier)): if reference.get(attr) != value: reference.set(attr, value) - made_changes = True for child in tree_root: if child.tag not in ('TestAction', 'LaunchAction'): @@ -113,59 +127,29 @@ def GenerateSchemeForTarget(root, project, old_project, name, path, tests): if child.get('customLLDBInitFile') != LLDBINIT_PATH: child.set('customLLDBInitFile', LLDBINIT_PATH) - made_changes = True - # Override the list of testables. - if child.tag == 'TestAction': - for subchild in child: - if subchild.tag != 'Testables': - continue + if is_test: - for elt in list(subchild): - subchild.remove(elt) + template_tree = xml.etree.ElementTree.parse( + io.StringIO(template.substitute(**substitutions))) - if tests: - template = LoadSchemeTemplate(root, 'xcodescheme-testable.template') - for (key, test_path, test_name) in sorted(tests): - testable = ''.join(template.substitute( - BLUEPRINT_IDENTIFIER=key, - BUILDABLE_NAME=test_path, - BLUEPRINT_NAME=test_name, - PROJECT_NAME=project_name)) + template_tree_root = template_tree.getroot() + for child in tree_root: + if child.tag != 'BuildAction': + continue - testable_elt = xml.etree.ElementTree.fromstring(testable) - subchild.append(testable_elt) + for subchild in list(child): + child.remove(subchild) - if made_changes: - tree.write(scheme_path, xml_declaration=True, encoding='UTF-8') + for post_action in template_tree_root.findall('.//PostActions'): + child.append(post_action) - else: - shutil.copyfile(old_scheme_path, scheme_path) + tree.write(scheme_path, xml_declaration=True, encoding='UTF-8') else: - testables = '' - if tests: - template = LoadSchemeTemplate(root, 'xcodescheme-testable.template') - testables = '\n' + ''.join( - template.substitute( - BLUEPRINT_IDENTIFIER=key, - BUILDABLE_NAME=test_path, - BLUEPRINT_NAME=test_name, - PROJECT_NAME=project_name) - for (key, test_path, test_name) in sorted(tests)).rstrip() - - template = LoadSchemeTemplate(root, 'xcodescheme.template') - with open(scheme_path, 'w') as scheme_file: - scheme_file.write( - template.substitute( - TESTABLES=testables, - LLDBINIT_PATH=LLDBINIT_PATH, - BLUEPRINT_IDENTIFIER=identifier, - BUILDABLE_NAME=path, - BLUEPRINT_NAME=name, - PROJECT_NAME=project_name)) + scheme_file.write(template.substitute(**substitutions)) class XcodeProject(object): @@ -225,6 +209,7 @@ def UpdateBuildConfigurations(self, configurations): # because objects will be added to/removed from the project upon # iterating this list and python dictionaries cannot be mutated # during iteration. + for key, obj in list(self.IterObjectsByIsa('XCConfigurationList')): # Use the first build configuration as template for creating all the # new build configurations. @@ -232,7 +217,6 @@ def UpdateBuildConfigurations(self, configurations): build_config_template['buildSettings']['CONFIGURATION_BUILD_DIR'] = \ '$(PROJECT_DIR)/../$(CONFIGURATION)$(EFFECTIVE_PLATFORM_NAME)' - # Remove the existing build configurations from the project before # creating the new ones. for build_config_id in obj['buildConfigurations']: @@ -342,48 +326,56 @@ def UpdateXcodeProject(project_dir, old_project_dir, configurations, root_dir): product = project.objects[obj['productReference']] product_path = product['path'] - # For XCTests, the key is the product path, while for XCUITests, the key - # is the target name. Use a sum of both possible keys (there should not - # be overlaps since different hosts are used for XCTests and XCUITests - # but this make the code simpler). + # Do not generate scheme for the XCTests and XXCUITests target app. + # Instead, a scheme will be generated for each test modules. tests = mapping.get(product_path, []) + mapping.get(obj['name'], []) - GenerateSchemeForTarget( - root_dir, project_dir, old_project_dir, - obj['name'], product_path, tests) + if not tests: + GenerateSchemeForTarget( + root_dir, project_dir, old_project_dir, + obj['name'], product_path, False) + + else: + for (_, test_name, test_path) in tests: + GenerateSchemeForTarget( + root_dir, project_dir, old_project_dir, + test_name, test_path, True) + + root_object = project.objects[json_data['rootObject']] + main_group = project.objects[root_object['mainGroup']] + + sources = None + for child_key in main_group['children']: + child = project.objects[child_key] + if child.get('name') == 'Source' or child.get('name') == 'Sources': + sources = child + break + if sources is None: + sources = main_group - source = GetOrCreateRootGroup(project, json_data['rootObject'], 'Source') - AddMarkdownToProject(project, root_dir, source) - SortFileReferencesByName(project, source) + AddMarkdownToProject(project, root_dir, sources, sources is main_group) + SortFileReferencesByName(project, sources, root_object.get('productRefGroup')) objects = collections.OrderedDict(sorted(project.objects.items())) - WriteXcodeProject(project_dir, json.dumps(json_data)) + # WriteXcodeProject(project_dir, json.dumps(json_data)) -def CreateGroup(project, parent_group, group_name, path=None): +def CreateGroup(project, parent_group, group_name, use_relative_paths): group_object = { 'children': [], 'isa': 'PBXGroup', - 'name': group_name, 'sourceTree': '', } - if path is not None: - group_object['path'] = path + if use_relative_paths: + group_object['path'] = group_name + else: + group_object['name'] = group_name parent_group_name = parent_group.get('name', '') group_object_key = project.AddObject(parent_group_name, group_object) parent_group['children'].append(group_object_key) return group_object -def GetOrCreateRootGroup(project, root_object, group_name): - main_group = project.objects[project.objects[root_object]['mainGroup']] - for child_key in main_group['children']: - child = project.objects[child_key] - if child['name'] == group_name: - return child - return CreateGroup(project, main_group, group_name, path='../..') - - class ObjectKey(object): """Wrapper around PBXFileReference and PBXGroup for sorting. @@ -401,19 +393,24 @@ class ObjectKey(object): is checked and compared in alphabetic order. """ - def __init__(self, obj): + def __init__(self, obj, last): self.isa = obj['isa'] if 'name' in obj: self.name = obj['name'] else: self.name = obj['path'] + self.last = last def __lt__(self, other): + if self.last != other.last: + return other.last if self.isa != other.isa: return self.isa > other.isa return self.name < other.name def __gt__(self, other): + if self.last != other.last: + return self.last if self.isa != other.isa: return self.isa < other.isa return self.name > other.name @@ -422,9 +419,10 @@ def __eq__(self, other): return self.isa == other.isa and self.name == other.name -def SortFileReferencesByName(project, group_object): +def SortFileReferencesByName(project, group_object, products_group_ref): SortFileReferencesByNameWithSortKey( - project, group_object, lambda ref: ObjectKey(project.objects[ref])) + project, group_object, + lambda ref: ObjectKey(project.objects[ref], ref == products_group_ref)) def SortFileReferencesByNameWithSortKey(project, group_object, sort_key): @@ -435,7 +433,7 @@ def SortFileReferencesByNameWithSortKey(project, group_object, sort_key): SortFileReferencesByNameWithSortKey(project, child, sort_key) -def AddMarkdownToProject(project, root_dir, group_object): +def AddMarkdownToProject(project, root_dir, group_object, use_relative_paths): list_files_cmd = ['git', '-C', root_dir, 'ls-files', '*.md'] paths = check_output(list_files_cmd).splitlines() ios_internal_dir = os.path.join(root_dir, 'ios_internal') @@ -448,31 +446,43 @@ def AddMarkdownToProject(project, root_dir, group_object): "fileEncoding": "4", "isa": "PBXFileReference", "lastKnownFileType": "net.daringfireball.markdown", - "name": os.path.basename(path), - "path": path, "sourceTree": "" } - new_markdown_entry_id = project.AddObject('sources', new_markdown_entry) - folder = GetFolderForPath(project, group_object, os.path.dirname(path)) + if use_relative_paths: + new_markdown_entry['path'] = os.path.basename(path) + else: + new_markdown_entry['name'] = os.path.basename(path) + new_markdown_entry['path'] = path + folder = GetFolderForPath( + project, group_object, os.path.dirname(path), + use_relative_paths) + folder_name = folder.get('name', None) + if folder_name is None: + folder_name = folder.get('path', 'sources') + new_markdown_entry_id = project.AddObject(folder_name, new_markdown_entry) folder['children'].append(new_markdown_entry_id) -def GetFolderForPath(project, group_object, path): +def GetFolderForPath(project, group_object, path, use_relative_paths): objects = project.objects if not path: return group_object for folder in path.split('/'): children = group_object['children'] new_root = None - for child in children: - if objects[child]['isa'] == 'PBXGroup' and \ - objects[child]['name'] == folder: - new_root = objects[child] - break + for child_key in children: + child = objects[child_key] + if child['isa'] == 'PBXGroup': + child_name = child.get('name', None) + if child_name is None: + child_name = child.get('path') + if child_name == folder: + new_root = child + break if not new_root: # If the folder isn't found we could just cram it into the leaf existing # folder, but that leads to folders with tons of README.md inside. - new_root = CreateGroup(project, group_object, folder) + new_root = CreateGroup(project, group_object, folder, use_relative_paths) group_object = new_root return group_object diff --git a/build/ios/setup_ios_gn.py b/build/ios/setup_ios_gn.py index aacc8ec7bd..333864eedf 100755 --- a/build/ios/setup_ios_gn.py +++ b/build/ios/setup_ios_gn.py @@ -110,7 +110,7 @@ def __init__(self, settings, config, target): self._config = config self._target = target - def _GetGnArgs(self, extra_args=None): + def _GetGnArgs(self): """Build the list of arguments to pass to gn. Returns: @@ -134,11 +134,6 @@ def _GetGnArgs(self, extra_args=None): 'target_environment', self.TARGET_ENVIRONMENT_VALUES[self._target])) - # If extra arguments are passed to the function, pass them before the - # user overrides (if any). - if extra_args is not None: - args.extend(extra_args) - # Add user overrides after the other configurations so that they can # refer to them and override them. args.extend(self._settings.items('gn_args')) @@ -218,6 +213,10 @@ def GetGnCommand(self, gn_path, src_path, out_path, xcode_project_name=None): gn_command.append('--ninja-executable=autoninja') gn_command.append('--xcode-build-system=new') gn_command.append('--xcode-project=%s' % xcode_project_name) + gn_command.append('--xcode-additional-files-patterns=*.md') + gn_command.append('--xcode-configs=' + ';'.join(SUPPORTED_CONFIGS)) + gn_command.append('--xcode-config-build-dir=' + '//out/${CONFIGURATION}${EFFECTIVE_PLATFORM_NAME}') if self._settings.has_section('filters'): target_filters = self._settings.values('filters') if target_filters: diff --git a/build/ios/xcodescheme-testable.template b/build/ios/xcodescheme-testable.template index 61b6f471b7..24ba39a97d 100644 --- a/build/ios/xcodescheme-testable.template +++ b/build/ios/xcodescheme-testable.template @@ -1,3 +1,37 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/build/ios/xcodescheme.template b/build/ios/xcodescheme.template index 514bea4ef0..b7cda4b10b 100644 --- a/build/ios/xcodescheme.template +++ b/build/ios/xcodescheme.template @@ -28,8 +28,6 @@ selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB" customLLDBInitFile = "@{LLDBINIT_PATH}" shouldUseLaunchSchemeArgsEnv = "YES"> - @{TESTABLES} - Date: Wed, 6 Jul 2022 13:53:12 -0700 Subject: [PATCH 14/20] Add WER runtime exception helper module for Windows This adds a runtime exception helper (& test module) for Windows and plumbing to allow the module to be registered by the crashpad client, and to trigger the crashpad handler. Embedders can build their own module to control which exceptions are passed to the handler. See: go/chrome-windows-runtime-exception-helper for motivation. When registered (which is the responsibility of the embedding application), the helper is loaded by WerFault.exe when Windows Error Reporting receives crashes that are not caught by crashpad's normal handlers - for instance a control-flow violation when a module is compiled with /guard:cf. Registration: The embedder must arrange for the full path to the helper to be added in the appropriate Windows Error Reporting\ RuntimeExceptionHelperModules registry key. Once an embedder's crashpad client is connected to a crashpad handler (e.g. through SetIpcPipeName()) the embedder calls RegisterWerModule. Internally, this registration includes handles used to trigger the crashpad handler, an area reserved to hold an exception and context, and structures needed by the crashpad handler. Following a crash: WerFault.exe handles the crash then validates and loads the helper module. WER hands the helper module a handle to the crashing target process and copies of the exception and context for the faulting thread. The helper then copies out the client's registration data and duplicates handles to the crashpad handler, then fills back the various structures in the paused client that the crashpad handler will need. The helper then signals the crashpad handler, which collects a dump then notifies the helper that it is done. Support: WerRegisterExceptionHelperModule has been availble since at least Windows 7 but WerFault would not pass on the exceptions that crashpad could not already handle. This changed in Windows 10 20H1 (19041), which supports HKCU and HKLM registrations, and passes in more types of crashes. It is harmless to register the module for earlier versions of Windows as it simply won't be loaded by WerFault.exe. Tests: snapshot/win/end_to_end_test.py has been refactored slightly to group crash generation and output validation in main() by breaking up RunTests into smaller functions. As the module works by being loaded in WerFault.exe it is tested in end_to_end_test.py. Bug: crashpad:133, 866033, 865632 Change-Id: Id668bd15a510a24c79753e1bb03e9456f41a9780 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3677284 Reviewed-by: Joshua Peraza Commit-Queue: Alex Gough --- client/BUILD.gn | 5 +- client/crashpad_client.h | 15 ++ client/crashpad_client_win.cc | 74 +++++--- handler/BUILD.gn | 21 ++- handler/win/fastfail_test_program.cc | 146 +++++++++++++++ handler/win/wer/BUILD.gn | 47 +++++ handler/win/wer/crashpad_wer.cc | 177 ++++++++++++++++++ handler/win/wer/crashpad_wer.def | 20 ++ handler/win/wer/crashpad_wer.h | 45 +++++ handler/win/wer/crashpad_wer.ver | 2 + handler/win/wer/crashpad_wer_main.cc | 83 ++++++++ .../win/wer/crashpad_wer_module_unittest.cc | 86 +++++++++ snapshot/win/end_to_end_test.py | 121 +++++++++--- util/BUILD.gn | 13 ++ util/win/registration_protocol_win.h | 42 +++++ 15 files changed, 849 insertions(+), 48 deletions(-) create mode 100644 handler/win/fastfail_test_program.cc create mode 100644 handler/win/wer/BUILD.gn create mode 100644 handler/win/wer/crashpad_wer.cc create mode 100644 handler/win/wer/crashpad_wer.def create mode 100644 handler/win/wer/crashpad_wer.h create mode 100644 handler/win/wer/crashpad_wer.ver create mode 100644 handler/win/wer/crashpad_wer_main.cc create mode 100644 handler/win/wer/crashpad_wer_module_unittest.cc diff --git a/client/BUILD.gn b/client/BUILD.gn index 1a83efa2bb..359d57eda8 100644 --- a/client/BUILD.gn +++ b/client/BUILD.gn @@ -195,7 +195,10 @@ source_set("client_test") { } if (crashpad_is_win) { - data_deps += [ "../handler:crashpad_handler_console" ] + data_deps += [ + "../handler:crashpad_handler_console", + "../handler/win/wer:crashpad_wer_handler", + ] } } diff --git a/client/crashpad_client.h b/client/crashpad_client.h index 1063dcf740..17103d263f 100644 --- a/client/crashpad_client.h +++ b/client/crashpad_client.h @@ -669,6 +669,21 @@ class CrashpadClient { //! error message will have been logged. bool WaitForHandlerStart(unsigned int timeout_ms); + //! \brief Register a DLL using WerRegisterExceptionModule(). + //! + //! This method should only be called after a successful call to + //! SetHandlerIPCPipe() or StartHandler(). The registration is valid for the + //! lifetime of this object. + //! + //! \param[in] full_path The full path to the DLL that will be registered. + //! The DLL path should also be set in an appropriate + //! `Windows Error Reporting` registry key. + //! + //! \return `true` if the DLL was registered. Note: Windows just stashes the + //! path somewhere so this returns `true` even if the DLL is not yet + //! set in an appropriate registry key, or does not exist. + bool RegisterWerModule(const std::wstring& full_path); + //! \brief Requests that the handler capture a dump even though there hasn't //! been a crash. //! diff --git a/client/crashpad_client_win.cc b/client/crashpad_client_win.cc index 1275d7d75b..c443029dda 100644 --- a/client/crashpad_client_win.cc +++ b/client/crashpad_client_win.cc @@ -16,6 +16,8 @@ #include +#include + #include #include #include @@ -61,19 +63,25 @@ HANDLE g_signal_exception = INVALID_HANDLE_VALUE; // Where we store the exception information that the crash handler reads. ExceptionInformation g_crash_exception_information; -// These handles are never closed. g_signal_non_crash_dump is used to signal to -// the server to take a dump (not due to an exception), and the server will -// signal g_non_crash_dump_done when the dump is completed. -HANDLE g_signal_non_crash_dump = INVALID_HANDLE_VALUE; -HANDLE g_non_crash_dump_done = INVALID_HANDLE_VALUE; - -// Guards multiple simultaneous calls to DumpWithoutCrash(). This is leaked. +// Guards multiple simultaneous calls to DumpWithoutCrash() in the client. +// This is leaked. base::Lock* g_non_crash_dump_lock; // Where we store a pointer to the context information when taking a non-crash // dump. ExceptionInformation g_non_crash_exception_information; +// Context for the out-of-process exception handler module and holds non-crash +// dump handles. Handles are never closed once created. +WerRegistration g_wer_registration = {WerRegistration::kWerRegistrationVersion, + INVALID_HANDLE_VALUE, + INVALID_HANDLE_VALUE, + false, + nullptr, + {0}, + {0}, + {0}}; + enum class StartupState : int { kNotReady = 0, // This must be value 0 because it is the initial value of a // global AtomicWord. @@ -402,8 +410,8 @@ bool StartHandlerProcess( InitialClientData initial_client_data( g_signal_exception, - g_signal_non_crash_dump, - g_non_crash_dump_done, + g_wer_registration.dump_without_crashing, + g_wer_registration.dump_completed, data->ipc_pipe_handle.get(), this_process.get(), FromPointerCast(&g_crash_exception_information), @@ -467,8 +475,8 @@ bool StartHandlerProcess( handle_list.reserve(8); handle_list.push_back(g_signal_exception); - handle_list.push_back(g_signal_non_crash_dump); - handle_list.push_back(g_non_crash_dump_done); + handle_list.push_back(g_wer_registration.dump_without_crashing); + handle_list.push_back(g_wer_registration.dump_completed); handle_list.push_back(data->ipc_pipe_handle.get()); handle_list.push_back(this_process.get()); AddHandleToListIfValidAndInheritable(&handle_list, @@ -625,9 +633,9 @@ bool CrashpadClient::StartHandler( g_signal_exception = CreateEvent(&security_attributes, false /* auto reset */, false, nullptr); - g_signal_non_crash_dump = + g_wer_registration.dump_without_crashing = CreateEvent(&security_attributes, false /* auto reset */, false, nullptr); - g_non_crash_dump_done = + g_wer_registration.dump_completed = CreateEvent(&security_attributes, false /* auto reset */, false, nullptr); CommonInProcessInitialization(); @@ -679,8 +687,8 @@ bool CrashpadClient::SetHandlerIPCPipe(const std::wstring& ipc_pipe) { DCHECK(!ipc_pipe_.empty()); DCHECK_EQ(g_signal_exception, INVALID_HANDLE_VALUE); - DCHECK_EQ(g_signal_non_crash_dump, INVALID_HANDLE_VALUE); - DCHECK_EQ(g_non_crash_dump_done, INVALID_HANDLE_VALUE); + DCHECK_EQ(g_wer_registration.dump_without_crashing, INVALID_HANDLE_VALUE); + DCHECK_EQ(g_wer_registration.dump_completed, INVALID_HANDLE_VALUE); DCHECK(!g_critical_section_with_debug_info.DebugInfo); DCHECK(!g_non_crash_dump_lock); @@ -712,9 +720,9 @@ bool CrashpadClient::SetHandlerIPCPipe(const std::wstring& ipc_pipe) { // The server returns these already duplicated to be valid in this process. g_signal_exception = IntToHandle(response.registration.request_crash_dump_event); - g_signal_non_crash_dump = + g_wer_registration.dump_without_crashing = IntToHandle(response.registration.request_non_crash_dump_event); - g_non_crash_dump_done = + g_wer_registration.dump_completed = IntToHandle(response.registration.non_crash_dump_completed_event); return true; @@ -749,10 +757,29 @@ bool CrashpadClient::WaitForHandlerStart(unsigned int timeout_ms) { return exit_code == 0; } +bool CrashpadClient::RegisterWerModule(const std::wstring& path) { + if (g_wer_registration.dump_completed == INVALID_HANDLE_VALUE || + g_wer_registration.dump_without_crashing == INVALID_HANDLE_VALUE) { + LOG(ERROR) << "not connected"; + return false; + } + // We cannot point (*context).exception_pointers to our pointers yet as it + // might get used for other non-crash dumps. + g_wer_registration.crashpad_exception_info = + &g_non_crash_exception_information; + // we can point these as we are the only users. + g_wer_registration.pointers.ExceptionRecord = &g_wer_registration.exception; + g_wer_registration.pointers.ContextRecord = &g_wer_registration.context; + + HRESULT res = + WerRegisterRuntimeExceptionModule(path.c_str(), &g_wer_registration); + return res == S_OK; +} + // static void CrashpadClient::DumpWithoutCrash(const CONTEXT& context) { - if (g_signal_non_crash_dump == INVALID_HANDLE_VALUE || - g_non_crash_dump_done == INVALID_HANDLE_VALUE) { + if (g_wer_registration.dump_without_crashing == INVALID_HANDLE_VALUE || + g_wer_registration.dump_completed == INVALID_HANDLE_VALUE) { LOG(ERROR) << "not connected"; return; } @@ -760,7 +787,7 @@ void CrashpadClient::DumpWithoutCrash(const CONTEXT& context) { if (BlockUntilHandlerStartedOrFailed() == StartupState::kFailed) { // If we know for certain that the handler has failed to start, then abort // here, as we would otherwise wait indefinitely for the - // g_non_crash_dump_done event that would never be signalled. + // g_wer_registration.dump_completed event that would never be signalled. LOG(ERROR) << "crash server failed to launch, no dump captured"; return; } @@ -798,11 +825,14 @@ void CrashpadClient::DumpWithoutCrash(const CONTEXT& context) { g_non_crash_exception_information.exception_pointers = FromPointerCast(&exception_pointers); - bool set_event_result = !!SetEvent(g_signal_non_crash_dump); + g_wer_registration.in_dump_without_crashing = true; + bool set_event_result = !!SetEvent(g_wer_registration.dump_without_crashing); PLOG_IF(ERROR, !set_event_result) << "SetEvent"; - DWORD wfso_result = WaitForSingleObject(g_non_crash_dump_done, INFINITE); + DWORD wfso_result = + WaitForSingleObject(g_wer_registration.dump_completed, INFINITE); PLOG_IF(ERROR, wfso_result != WAIT_OBJECT_0) << "WaitForSingleObject"; + g_wer_registration.in_dump_without_crashing = false; } // static diff --git a/handler/BUILD.gn b/handler/BUILD.gn index 0fe4760db7..e79f4719b3 100644 --- a/handler/BUILD.gn +++ b/handler/BUILD.gn @@ -145,7 +145,10 @@ source_set("handler_test") { ] if (crashpad_is_win) { - deps += [ "../minidump:test_support" ] + deps += [ + "../minidump:test_support", + "win/wer:crashpad_wer_test", + ] data_deps = [ ":crashpad_handler_test_extended_handler", @@ -353,4 +356,20 @@ if (crashpad_is_win) { ] } } + + config("enable_cfg") { + cflags = [ "/guard:cf" ] + ldflags = [ "/guard:cf" ] + } + crashpad_executable("fastfail_program") { + testonly = true + + sources = [ "win/fastfail_test_program.cc" ] + configs = [ ":enable_cfg" ] + + deps = [ + "../client", + "../third_party/mini_chromium:base", + ] + } } diff --git a/handler/win/fastfail_test_program.cc b/handler/win/fastfail_test_program.cc new file mode 100644 index 0000000000..1c33a6a06f --- /dev/null +++ b/handler/win/fastfail_test_program.cc @@ -0,0 +1,146 @@ +// Copyright 2022 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 "base/files/file_path.h" +#include "base/logging.h" +#include "client/crashpad_client.h" +#include "util/misc/paths.h" + +#include + +// We set up a program that crashes with a CFG exception so must be built and +// linked with /guard:cf. We register the crashpad runtime exception helper +// module to intercept and trigger the crashpad handler. Note that Windows only +// loads the module in WerFault after the crash for Windows 10 >= 20h1 (19041). +namespace crashpad { +namespace { + +// This function should not be on our stack as CFG prevents the modified +// icall from happening. +int CallRffeManyTimes() { + RaiseFailFastException(nullptr, nullptr, 0); + RaiseFailFastException(nullptr, nullptr, 0); + RaiseFailFastException(nullptr, nullptr, 0); + RaiseFailFastException(nullptr, nullptr, 0); + return 1; +} + +using FuncType = decltype(&CallRffeManyTimes); +void IndirectCall(FuncType* func) { + // This code always generates CFG guards. + (*func)(); +} + +void CfgCrash() { + // Call into the middle of the Crashy function. + FuncType func = reinterpret_cast( + (reinterpret_cast(CallRffeManyTimes)) + 16); + __try { + // Generates a STATUS_STACK_BUFFER_OVERRUN exception if CFG triggers. + IndirectCall(&func); + } __except (EXCEPTION_EXECUTE_HANDLER) { + // CFG fast fail should never be caught. + CHECK(false); + } + // Should only reach here if CFG is disabled. + abort(); +} + +void FastFailCrash() { + __fastfail(77); +} + +int CrashyMain(int argc, wchar_t* argv[]) { + static CrashpadClient* client = new crashpad::CrashpadClient(); + + std::wstring type; + if (argc == 3) { + type = argv[2]; + // We call this from end_to_end_test.py. + if (!client->SetHandlerIPCPipe(argv[1])) { + LOG(ERROR) << "SetHandler"; + return EXIT_FAILURE; + } + } else if (argc == 4) { + type = argv[3]; + // This is helpful for debugging. + if (!client->StartHandler(base::FilePath(argv[1]), + base::FilePath(argv[2]), + base::FilePath(), + std::string(), + std::map(), + std::vector(), + false, + true)) { + LOG(ERROR) << "StartHandler"; + return EXIT_FAILURE; + } + // Got to have a handler & registration. + if (!client->WaitForHandlerStart(10000)) { + LOG(ERROR) << "Handler failed to start"; + return EXIT_FAILURE; + } + } else { + fprintf(stderr, "Usage: %ls [cf|ff]\n", argv[0]); + fprintf( + stderr, " %ls [cf|ff]\n", argv[0]); + return EXIT_FAILURE; + } + + base::FilePath exe_path; + if (!Paths::Executable(&exe_path)) { + LOG(ERROR) << "Failed getting path"; + return EXIT_FAILURE; + } + + // Find the module. + auto mod_path = exe_path.DirName().Append(L"crashpad_wer.dll"); + + // Make sure it is registered in the registry. + DWORD dwOne = 1; + LSTATUS reg_res = + RegSetKeyValueW(HKEY_CURRENT_USER, + L"Software\\Microsoft\\Windows\\Windows Error Reporting\\" + L"RuntimeExceptionHelperModules", + mod_path.value().c_str(), + REG_DWORD, + &dwOne, + sizeof(DWORD)); + if (reg_res != ERROR_SUCCESS) { + LOG(ERROR) << "RegSetKeyValueW"; + return EXIT_FAILURE; + } + + if (!client->RegisterWerModule(mod_path.value())) { + LOG(ERROR) << "WerRegisterRuntimeExceptionModule"; + return EXIT_FAILURE; + } + + if (type == L"cf") + CfgCrash(); + if (type == L"ff") + FastFailCrash(); + + LOG(ERROR) << "Invalid type or exception failed."; + return EXIT_FAILURE; +} + +} // namespace +} // namespace crashpad + +int wmain(int argc, wchar_t* argv[]) { + return crashpad::CrashyMain(argc, argv); +} diff --git a/handler/win/wer/BUILD.gn b/handler/win/wer/BUILD.gn new file mode 100644 index 0000000000..5fc38efdd9 --- /dev/null +++ b/handler/win/wer/BUILD.gn @@ -0,0 +1,47 @@ +# Copyright 2022 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. + +import("../../../build/crashpad_buildconfig.gni") + +assert(crashpad_is_win) + +# Allows other projects (e.g. Chrome) to wrap this as a dll. +source_set("crashpad_wer_handler") { + sources = [ + "crashpad_wer.cc", + "crashpad_wer.h", + ] + deps = [ "../../../util:util_registration_protocol" ] +} + +crashpad_loadable_module("crashpad_wer") { + sources = [ + "crashpad_wer.def", + "crashpad_wer_main.cc", + ] + deps = [ ":crashpad_wer_handler" ] + configs = [ "../../../:crashpad_config" ] +} + +source_set("crashpad_wer_test") { + testonly = true + sources = [ "crashpad_wer_module_unittest.cc" ] + deps = [ + ":crashpad_wer", + ":crashpad_wer_handler", + "../../../client:client", + "../../../test:test", + "../../../third_party/googletest:googletest", + ] +} diff --git a/handler/win/wer/crashpad_wer.cc b/handler/win/wer/crashpad_wer.cc new file mode 100644 index 0000000000..f3de028e0e --- /dev/null +++ b/handler/win/wer/crashpad_wer.cc @@ -0,0 +1,177 @@ +// Copyright 2022 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. + +// See: +// https://docs.microsoft.com/en-us/windows/win32/api/werapi/nf-werapi-werregisterruntimeexceptionmodule + +#include "handler/win/wer/crashpad_wer.h" + +#include "util/misc/address_types.h" +#include "util/win/registration_protocol_win.h" + +#include +#include + +namespace crashpad::wer { +namespace { +using crashpad::WerRegistration; + +// We have our own version of this to avoid pulling in //base. +class ScopedHandle { + public: + ScopedHandle() : handle_(INVALID_HANDLE_VALUE) {} + ScopedHandle(HANDLE from) : handle_(from) {} + ~ScopedHandle() { + if (IsValid()) + CloseHandle(handle_); + } + bool IsValid() { + if (handle_ == INVALID_HANDLE_VALUE || handle_ == 0) + return false; + return true; + } + HANDLE Get() { return handle_; } + + private: + HANDLE handle_; +}; + +ScopedHandle DuplicateFromTarget(HANDLE target_process, HANDLE target_handle) { + HANDLE hTmp; + if (!DuplicateHandle(target_process, + target_handle, + GetCurrentProcess(), + &hTmp, + SYNCHRONIZE | EVENT_MODIFY_STATE, + false, + 0)) { + return ScopedHandle(); + } + return ScopedHandle(hTmp); +} + +bool ProcessException(std::vector& handled_exceptions, + const PVOID pContext, + const PWER_RUNTIME_EXCEPTION_INFORMATION e_info) { + // Need to have been given a context. + if (!pContext) + return false; + + if (!e_info->bIsFatal) + return false; + + // Only deal with exceptions that crashpad would not have handled. + if (handled_exceptions.size() && + std::find(handled_exceptions.begin(), + handled_exceptions.end(), + e_info->exceptionRecord.ExceptionCode) == + handled_exceptions.end()) { + return false; + } + + // Grab out the handles to the crashpad server. + WerRegistration target_registration = {}; + if (!ReadProcessMemory(e_info->hProcess, + pContext, + &target_registration, + sizeof(target_registration), + nullptr)) { + return false; + } + + // Validate version of registration struct. + if (target_registration.version != WerRegistration::kWerRegistrationVersion) + return false; + + // Dupe handles for triggering the dump. + auto dump_start = DuplicateFromTarget( + e_info->hProcess, target_registration.dump_without_crashing); + auto dump_done = + DuplicateFromTarget(e_info->hProcess, target_registration.dump_completed); + + if (!dump_start.IsValid() || !dump_done.IsValid()) + return false; + + // It's possible that the target crashed while inside a DumpWithoutCrashing + // call - either in the DumpWithoutCrashing call or in another thread - if so + // we cannot trigger the dump until the first call's crash is dealth with as + // the crashpad handler might be reading from structures we will write to. We + // give the event a short while to be triggered and give up if it is not + // signalled. + if (target_registration.in_dump_without_crashing) { + constexpr DWORD kOneSecondInMs = 1000; + DWORD wait_result = WaitForSingleObject(dump_done.Get(), kOneSecondInMs); + if (wait_result != WAIT_OBJECT_0) + return false; + } + + // Set up the crashpad handler's info structure. + crashpad::ExceptionInformation target_non_crash_exception_info{}; + target_non_crash_exception_info.thread_id = GetThreadId(e_info->hThread); + target_non_crash_exception_info.exception_pointers = + static_cast(reinterpret_cast(pContext)) + + offsetof(WerRegistration, pointers); + + if (!WriteProcessMemory(e_info->hProcess, + target_registration.crashpad_exception_info, + &target_non_crash_exception_info, + sizeof(target_non_crash_exception_info), + nullptr)) { + return false; + } + + // Write Exception & Context to the areas reserved by the client. + if (!WriteProcessMemory( + e_info->hProcess, + reinterpret_cast(target_registration.pointers.ExceptionRecord), + &e_info->exceptionRecord, + sizeof(e_info->exceptionRecord), + nullptr)) { + return false; + } + if (!WriteProcessMemory( + e_info->hProcess, + reinterpret_cast(target_registration.pointers.ContextRecord), + &e_info->context, + sizeof(e_info->context), + nullptr)) { + return false; + } + + // Request dump. + if (!SetEvent(dump_start.Get())) + return false; + + constexpr DWORD kTenSecondsInMs = 10 * 1000; + DWORD result = WaitForSingleObject(dump_done.Get(), kTenSecondsInMs); + + if (result == WAIT_OBJECT_0) { + // The handler signalled that it has written a dump, so we can terminate the + // target - this takes over from WER, sorry WER. + TerminateProcess(e_info->hProcess, e_info->exceptionRecord.ExceptionCode); + return true; + } + // Maybe some other handler can have a go. + return false; +} +} // namespace + +bool ExceptionEvent( + std::vector& handled_exceptions, + const PVOID pContext, + const PWER_RUNTIME_EXCEPTION_INFORMATION pExceptionInformation) { + return ProcessException(handled_exceptions, pContext, pExceptionInformation); +} + +} // namespace crashpad::wer diff --git a/handler/win/wer/crashpad_wer.def b/handler/win/wer/crashpad_wer.def new file mode 100644 index 0000000000..57ec676fe8 --- /dev/null +++ b/handler/win/wer/crashpad_wer.def @@ -0,0 +1,20 @@ +; Copyright 2022 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. +LIBRARY "crashpad_wer.dll" + +EXPORTS + ; These are required for the WER api. + OutOfProcessExceptionEventCallback + OutOfProcessExceptionEventSignatureCallback + OutOfProcessExceptionEventDebuggerLaunchCallback diff --git a/handler/win/wer/crashpad_wer.h b/handler/win/wer/crashpad_wer.h new file mode 100644 index 0000000000..5a3a52f901 --- /dev/null +++ b/handler/win/wer/crashpad_wer.h @@ -0,0 +1,45 @@ +// Copyright 2022 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_HANDLER_WIN_WER_CRASHPAD_WER_H_ +#define CRASHPAD_HANDLER_WIN_WER_CRASHPAD_WER_H_ + +#include + +#include +#include + +namespace crashpad::wer { +//! \brief Embedder calls this from OutOfProcessExceptionEventCallback(). +//! +//! In the embedder's WER runtime exception helper, call this during +//! OutOfProcessExceptionEventCallback(). +//! +//! \param[in] handled_exceptions is a list of exception codes that the helper +//! should pass on to crashpad handler (if possible). Provide an empty list +//! to pass every exception on to the crashpad handler. +//! \param[in] pContext is the context provided by WerFault to the helper. +//! \param[in] pExceptionInformation is the exception information provided by +//! WerFault to the helper DLL. +//! +//! \return `true` if the target process was dumped by the crashpad handler then +//! terminated, or `false` otherwise. +bool ExceptionEvent( + std::vector& handled_exceptions, + const PVOID pContext, + const PWER_RUNTIME_EXCEPTION_INFORMATION pExceptionInformation); + +} // namespace crashpad::wer + +#endif // CRASHPAD_HANDLER_WIN_WER_CRASHPAD_WER_H_ diff --git a/handler/win/wer/crashpad_wer.ver b/handler/win/wer/crashpad_wer.ver new file mode 100644 index 0000000000..0fee2e9f91 --- /dev/null +++ b/handler/win/wer/crashpad_wer.ver @@ -0,0 +1,2 @@ +INTERNAL_NAME=crashpad_wer +ORIGINAL_FILENAME=crashpad_wer.dll diff --git a/handler/win/wer/crashpad_wer_main.cc b/handler/win/wer/crashpad_wer_main.cc new file mode 100644 index 0000000000..4613b35353 --- /dev/null +++ b/handler/win/wer/crashpad_wer_main.cc @@ -0,0 +1,83 @@ +// Copyright 2022 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. + +// See: +// https://docs.microsoft.com/en-us/windows/win32/api/werapi/nf-werapi-werregisterruntimeexceptionmodule + +#include "handler/win/wer/crashpad_wer.h" + +#include +#include + +// Functions that will be exported from the DLL. +extern "C" { +BOOL WINAPI DllMain(HINSTANCE instance, DWORD reason, LPVOID reserved) { + return true; +} + +// PFN_WER_RUNTIME_EXCEPTION_EVENT +// pContext is the address of a crashpad::internal::WerRegistration in the +// target process. +HRESULT OutOfProcessExceptionEventCallback( + PVOID pContext, + const PWER_RUNTIME_EXCEPTION_INFORMATION pExceptionInformation, + BOOL* pbOwnershipClaimed, + PWSTR pwszEventName, + PDWORD pchSize, + PDWORD pdwSignatureCount) { + std::vector wanted_exceptions = { + 0xC0000602, // STATUS_FAIL_FAST_EXCEPTION + 0xC0000409, // STATUS_STACK_BUFFER_OVERRUN + }; + // Default to not-claiming as bailing out is easier. + *pbOwnershipClaimed = FALSE; + bool result = crashpad::wer::ExceptionEvent( + wanted_exceptions, pContext, pExceptionInformation); + + if (result) { + *pbOwnershipClaimed = TRUE; + // Technically we failed as we terminated the process. + return E_FAIL; + } + // Pass. + return S_OK; +} + +// PFN_WER_RUNTIME_EXCEPTION_EVENT_SIGNATURE +HRESULT OutOfProcessExceptionEventSignatureCallback( + PVOID pContext, + const PWER_RUNTIME_EXCEPTION_INFORMATION pExceptionInformation, + DWORD dwIndex, + PWSTR pwszName, + PDWORD pchName, + PWSTR pwszValue, + PDWORD pchValue) { + // We handle everything in the call to OutOfProcessExceptionEventCallback. + // This function should never be called. + return E_FAIL; +} + +// PFN_WER_RUNTIME_EXCEPTION_DEBUGGER_LAUNCH +HRESULT OutOfProcessExceptionEventDebuggerLaunchCallback( + PVOID pContext, + const PWER_RUNTIME_EXCEPTION_INFORMATION pExceptionInformation, + PBOOL pbIsCustomDebugger, + PWSTR pwszDebuggerLaunch, + PDWORD pchDebuggerLaunch, + PBOOL pbIsDebuggerAutolaunch) { + // We handle everything in the call to OutOfProcessExceptionEventCallback. + // This function should never be called. + return E_FAIL; +} +} // extern "C" diff --git a/handler/win/wer/crashpad_wer_module_unittest.cc b/handler/win/wer/crashpad_wer_module_unittest.cc new file mode 100644 index 0000000000..efd8392ca2 --- /dev/null +++ b/handler/win/wer/crashpad_wer_module_unittest.cc @@ -0,0 +1,86 @@ +// Copyright 2022 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 "client/crashpad_client.h" + +#include "base/files/file_path.h" +#include "gtest/gtest.h" +#include "test/test_paths.h" +#include "util/win/registration_protocol_win.h" + +#include +#include + +namespace crashpad { +namespace test { +namespace { +base::FilePath ModulePath() { + auto dir = TestPaths::Executable().DirName(); + return dir.Append(FILE_PATH_LITERAL("crashpad_wer.dll")); +} + +// Quick sanity check of the module, can't really test dumping etc. outside of +// WerFault.exe loading it. +TEST(CrashpadWerModule, Basic) { + HRESULT res = 0; + // Module loads. + HMODULE hMod = LoadLibraryW(ModulePath().value().c_str()); + ASSERT_TRUE(hMod); + + // Required functions exist. + auto wref = reinterpret_cast( + GetProcAddress(hMod, WER_RUNTIME_EXCEPTION_EVENT_FUNCTION)); + ASSERT_TRUE(wref); + auto wrees = reinterpret_cast( + GetProcAddress(hMod, WER_RUNTIME_EXCEPTION_EVENT_SIGNATURE_FUNCTION)); + ASSERT_TRUE(wrees); + auto wredl = reinterpret_cast( + GetProcAddress(hMod, WER_RUNTIME_EXCEPTION_DEBUGGER_LAUNCH)); + ASSERT_TRUE(wredl); + + // Not-implemented functions return E_FAIL as expected. + res = wrees(nullptr, nullptr, 0, nullptr, nullptr, nullptr, nullptr); + ASSERT_EQ(res, E_FAIL); + res = wredl(nullptr, nullptr, nullptr, nullptr, nullptr, nullptr); + ASSERT_EQ(res, E_FAIL); + + // Dummy args for OutOfProcessExceptionEventCallback. + crashpad::WerRegistration registration; + WER_RUNTIME_EXCEPTION_INFORMATION wer_ex; + BOOL bClaimed = FALSE; + + // No context => skip. + res = wref(nullptr, &wer_ex, &bClaimed, nullptr, nullptr, nullptr); + ASSERT_EQ(res, S_OK); + ASSERT_EQ(bClaimed, FALSE); + + // Non-fatal exceptions are skipped. + wer_ex.bIsFatal = FALSE; + res = wref(®istration, &wer_ex, &bClaimed, nullptr, nullptr, nullptr); + ASSERT_EQ(res, S_OK); + ASSERT_EQ(bClaimed, FALSE); + + // Fatal exception with unhandled code is skipped. + wer_ex.bIsFatal = TRUE; + wer_ex.exceptionRecord.ExceptionCode = 0xc0000005; + res = wref(®istration, &wer_ex, &bClaimed, nullptr, nullptr, nullptr); + ASSERT_EQ(res, S_OK); + ASSERT_EQ(bClaimed, FALSE); + + FreeLibrary(hMod); +} + +} // namespace +} // namespace test +} // namespace crashpad diff --git a/snapshot/win/end_to_end_test.py b/snapshot/win/end_to_end_test.py index 9d319078d8..ff307e123c 100755 --- a/snapshot/win/end_to_end_test.py +++ b/snapshot/win/end_to_end_test.py @@ -87,6 +87,15 @@ def GetCdbPath(): return None +def Win32_20H1(): + (major, _, build) = platform.win32_ver()[1].split('.') + if int(major) < 10: + return False + if int(build) >= 19041: + return True + return False + + def NamedPipeExistsAndReady(pipe_name): """Returns False if pipe_name does not exist. If pipe_name does exist, blocks until the pipe is ready to service clients, and then returns True. @@ -203,6 +212,12 @@ def GetDumpFromZ7Program(out_dir, pipe_name): win32con.EXCEPTION_ACCESS_VIOLATION) +def GetDumpFromFastFailProgram(out_dir, pipe_name, *args): + STATUS_STACK_BUFFER_OVERRUN = 0xc0000409 + return GetDumpFromProgram(out_dir, pipe_name, 'fastfail_program.exe', + STATUS_STACK_BUFFER_OVERRUN, *args) + + class CdbRun(object): """Run cdb.exe passing it a cdb command and capturing the output. `Check()` searches for regex patterns in sequence allowing verification of @@ -215,18 +230,25 @@ def __init__(self, cdb_path, dump_path, command): self.out = subprocess.check_output( [cdb_path, '-z', dump_path, '-c', command + ';q'], text=True) - def Check(self, pattern, message, re_flags=0): + def Check(self, pattern, message, re_flags=0, must_not_match=False): match_obj = re.search(pattern, self.out, re_flags) - if match_obj: + if match_obj and not must_not_match: # Matched. Consume up to end of match. self.out = self.out[match_obj.end(0):] print('ok - %s' % message) sys.stdout.flush() + elif must_not_match and not match_obj: + # Did not match and did not want to match. + print('ok - %s' % message) + sys.stdout.flush() else: print('-' * 80, file=sys.stderr) print('FAILED - %s' % message, file=sys.stderr) print('-' * 80, file=sys.stderr) - print('did not match:\n %s' % pattern, file=sys.stderr) + if must_not_match: + print('unexpected match:\n %s' % pattern, file=sys.stderr) + else: + print('did not match:\n %s' % pattern, file=sys.stderr) print('-' * 80, file=sys.stderr) print('remaining output was:\n %s' % self.out, file=sys.stderr) print('-' * 80, file=sys.stderr) @@ -244,8 +266,7 @@ def Find(self, pattern, re_flags=0): def RunTests(cdb_path, dump_path, start_handler_dump_path, destroyed_dump_path, - z7_dump_path, other_program_path, other_program_no_exception_path, - sigabrt_main_path, sigabrt_background_path, pipe_name): + pipe_name): """Runs various tests in sequence. Runs a new cdb instance on the dump for each block of tests to reduce the chances that output from one command is confused for output from another. @@ -373,17 +394,22 @@ def RunTests(cdb_path, dump_path, start_handler_dump_path, destroyed_dump_path, out.Check(r'type \?\?\? \(333333\), size 00001000', 'first user stream') out.Check(r'type \?\?\? \(222222\), size 00000080', 'second user stream') - if z7_dump_path: - out = CdbRun(cdb_path, z7_dump_path, '.ecxr;lm') - out.Check('This dump file has an exception of interest stored in it', - 'captured exception in z7 module') - # Older versions of cdb display relative to exports for /Z7 modules, - # newer ones just display the offset. - out.Check(r'z7_test(!CrashMe\+0xe|\+0x100e):', - 'exception in z7 at correct location') - out.Check(r'z7_test C \(codeview symbols\) z7_test\.dll', - 'expected non-pdb symbol format') +def Run7zDumpTest(cdb_path, z7_dump_path): + """Validate output when non-pdb symbols are in a module.""" + out = CdbRun(cdb_path, z7_dump_path, '.ecxr;lm') + out.Check('This dump file has an exception of interest stored in it', + 'captured exception in z7 module') + # Older versions of cdb display relative to exports for /Z7 modules, + # newer ones just display the offset. + out.Check(r'z7_test(!CrashMe\+0xe|\+0x100e):', + 'exception in z7 at correct location') + out.Check(r'z7_test C \(codeview symbols\) z7_test\.dll', + 'expected non-pdb symbol format') + + +def RunOtherProgramTests(cdb_path, other_program_path, + other_program_no_exception_path): out = CdbRun(cdb_path, other_program_path, '.ecxr;k;~') out.Check('Unknown exception - code deadbea7', 'other program dump exception code') @@ -407,6 +433,9 @@ def RunTests(cdb_path, dump_path, start_handler_dump_path, destroyed_dump_path, 'other program with no exception given') out.Check('!RaiseException', 'other program in RaiseException()') + +def RunSigAbrtTest(cdb_path, sigabrt_main_path, sigabrt_background_path): + """Validate that abort signals are collected.""" out = CdbRun(cdb_path, sigabrt_main_path, '.ecxr') out.Check('code 40000015', 'got sigabrt signal') out.Check('::HandleAbortSignal', ' stack in expected location') @@ -415,6 +444,32 @@ def RunTests(cdb_path, dump_path, start_handler_dump_path, destroyed_dump_path, out.Check('code 40000015', 'got sigabrt signal from background thread') +def RunFastFailDumpTest(cdb_path, fastfail_path): + """Runs tests on __fastfail() caught using the runtime exception helper.""" + out = CdbRun(cdb_path, fastfail_path, '.ecxr;k') + out.Check('This dump file has an exception of interest stored in it', + 'captured exception from __fastfail() crash()') + out.Check(r'Subcode: 0x4d \(unknown subcode\)', 'See expected subcode.') + out.Check('FastFailCrash', 'See expected throwing function.') + out = CdbRun(cdb_path, fastfail_path, '.ecxr;k') + + +def RunCfgDumpTest(cdb_path, cfg_path): + """Runs tests on a cfg crash caught using the runtime exception helper.""" + out = CdbRun(cdb_path, cfg_path, '.ecxr;k') + out.Check('This dump file has an exception of interest stored in it', + 'captured exception from cfg crash()') + out.Check('Subcode: 0xa FAST_FAIL_GUARD_ICALL_CHECK_FAILURE', + 'See expected cfg error code.') + out.Check('RtlFailFast', + 'See expected Windows exception throwing function.') + out.Check('::CfgCrash', 'expected crashy function is on the stack.') + out = CdbRun(cdb_path, cfg_path, '.ecxr;k') + out.Check(r'CallRffeManyTimes', + 'Do not see the function we fiddled the pointer for.', + must_not_match=True) + + def main(args): try: if len(args) != 1: @@ -437,6 +492,7 @@ def main(args): pipe_name = r'\\.\pipe\end-to-end_%s_%s' % (os.getpid(), str(random.getrandbits(64))) + # Basic tests. crashy_dump_path = GetDumpFromCrashyProgram(args[0], pipe_name) if not crashy_dump_path: return 1 @@ -450,12 +506,10 @@ def main(args): if not destroyed_dump_path: return 1 - z7_dump_path = None - if not args[0].endswith('_x64'): - z7_dump_path = GetDumpFromZ7Program(args[0], pipe_name) - if not z7_dump_path: - return 1 + RunTests(cdb_path, crashy_dump_path, start_handler_dump_path, + destroyed_dump_path, pipe_name) + # Other program dumps. other_program_path = GetDumpFromOtherProgram(args[0], pipe_name) if not other_program_path: return 1 @@ -465,6 +519,10 @@ def main(args): if not other_program_no_exception_path: return 1 + RunOtherProgramTests(cdb_path, other_program_path, + other_program_no_exception_path) + + # SIGABRT. sigabrt_main_path = GetDumpFromSignal(args[0], pipe_name, 'main') if not sigabrt_main_path: return 1 @@ -474,10 +532,25 @@ def main(args): if not sigabrt_background_path: return 1 - RunTests(cdb_path, crashy_dump_path, start_handler_dump_path, - destroyed_dump_path, z7_dump_path, other_program_path, - other_program_no_exception_path, sigabrt_main_path, - sigabrt_background_path, pipe_name) + RunSigAbrtTest(cdb_path, sigabrt_main_path, sigabrt_background_path) + + # Can only build the z7 program on x86. + if not args[0].endswith('_x64'): + z7_dump_path = GetDumpFromZ7Program(args[0], pipe_name) + if not z7_dump_path: + return 1 + Run7zDumpTest(cdb_path, z7_dump_path) + + # __fastfail() & CFG crash caught by WerRuntimeExceptionHelperModule. + if (Win32_20H1()): + cfg_path = GetDumpFromFastFailProgram(args[0], pipe_name, "cf") + if not cfg_path: + return 1 + RunCfgDumpTest(cdb_path, cfg_path) + fastfail_path = GetDumpFromFastFailProgram(args[0], pipe_name, "ff") + if not fastfail_path: + return 1 + RunFastFailDumpTest(cdb_path, fastfail_path) return 1 if g_had_failures else 0 finally: diff --git a/util/BUILD.gn b/util/BUILD.gn index 6e6a8eec00..8a0a89062a 100644 --- a/util/BUILD.gn +++ b/util/BUILD.gn @@ -159,6 +159,19 @@ if (crashpad_is_mac || crashpad_is_ios) { } } +# Used by crashpad_wer_handler to avoid linking all of :util. +if (crashpad_is_win) { + source_set("util_registration_protocol") { + sources = [ + "misc/address_types.h", + "win/address_types.h", + "win/registration_protocol_win.h", + ] + public_deps = [ "../third_party/mini_chromium:build" ] + public_configs = [ "..:crashpad_config" ] + } +} + crashpad_static_library("util") { sources = [ "file/delimited_file_reader.cc", diff --git a/util/win/registration_protocol_win.h b/util/win/registration_protocol_win.h index 690d61242e..e467fd84a2 100644 --- a/util/win/registration_protocol_win.h +++ b/util/win/registration_protocol_win.h @@ -37,6 +37,48 @@ struct ExceptionInformation { DWORD thread_id; }; +//! \brief Context to be passed to WerRegisterRuntimeExceptionModule(). +//! +//! Used by the crashpad client, and the WER exception DLL. +struct WerRegistration { + //! \brief The expected value of `version`. This should be changed whenever + //! this struct is modified incompatibly. + enum { kWerRegistrationVersion = 1 }; + //! \brief Version field to detect skew between target process and helper. + //! Should be set to kWerRegistrationVersion. + int version; + //! \brief Used by DumpWithoutCrashing and the WER module to initiate a dump. + //! These handles are leaked in the client process. + HANDLE dump_without_crashing; + //! \brief Used by DumpWithoutCrashing to signal that a dump has been taken. + //! These handles are leaked in the client process. + HANDLE dump_completed; + //! \brief Set just before and cleared just after the events above are + //! triggered or signalled in a normal DumpWithoutCrashing call. + //! When `true` the WER handler should not set the exception structures until + //! after dump_completed has been signalled. + bool in_dump_without_crashing; + //! \brief Address of g_non_crash_exception_information. + //! + //! Provided by the target process. Just before dumping we will point + //! (*crashpad_exception_info).exception_pointers at `pointers`. As WerFault + //! loads the helper with the same bitness as the client this can be void*. + void* crashpad_exception_info; + //! \brief These will point into the `exception` and `context` members in this + //! structure. + //! + //! Filled in by the helper DLL. + EXCEPTION_POINTERS pointers; + //! \brief The exception provided by WerFault. + //! + //! Filled in by the helper DLL. + EXCEPTION_RECORD exception; + //! \brief The context provided by WerFault. + //! + //! Filled in by the helper DLL. + CONTEXT context; +}; + //! \brief A client registration request. struct RegistrationRequest { //! \brief The expected value of `version`. This should be changed whenever From 7a622b2f6baaacdff5c8f3acf7efe32b378b4ee7 Mon Sep 17 00:00:00 2001 From: Stephan Hartmann Date: Fri, 8 Jul 2022 19:24:46 +0200 Subject: [PATCH 15/20] GCC: fix invalid bind of packed field to uint32_t& GCC does not allow binding a packed field to an address. Assign to a intermediate variable instead before pushing to map. Bug: chromium:819294 Change-Id: I806e5f99c2b19e656b91a60f72172b59c961ba5f Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3751392 Reviewed-by: Mark Mentovai Commit-Queue: Mark Mentovai --- snapshot/minidump/process_snapshot_minidump.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/snapshot/minidump/process_snapshot_minidump.cc b/snapshot/minidump/process_snapshot_minidump.cc index 28942447dc..b9e7ea294c 100644 --- a/snapshot/minidump/process_snapshot_minidump.cc +++ b/snapshot/minidump/process_snapshot_minidump.cc @@ -643,7 +643,9 @@ bool ProcessSnapshotMinidump::InitializeThreadNames() { return false; } - thread_names_.emplace(minidump_thread_name.ThreadId, std::move(name)); + // See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36566 + const uint32_t thread_id = minidump_thread_name.ThreadId; + thread_names_.emplace(thread_id, std::move(name)); } return true; From 14246325920a4dc4c5d2862a93721cc8a9590044 Mon Sep 17 00:00:00 2001 From: Justin Cohen Date: Wed, 13 Jul 2022 13:15:36 -0400 Subject: [PATCH 16/20] ios: Fix testCrashWithDyldErrorString on arm64. Fixed when running a simulator on arm64 Apple Silicon. Change-Id: I6a6e917b4d5ff009683214794fe6a6af833be3c0 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3758362 Reviewed-by: Rohit Rao Commit-Queue: Justin Cohen --- test/ios/crash_type_xctest.mm | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/ios/crash_type_xctest.mm b/test/ios/crash_type_xctest.mm index 27b4e1f7fe..0bf0980e5e 100644 --- a/test/ios/crash_type_xctest.mm +++ b/test/ios/crash_type_xctest.mm @@ -286,7 +286,13 @@ - (void)testCrashWithDyldErrorString { return; } [rootObject_ crashWithDyldErrorString]; +#if defined(ARCH_CPU_X86_64) [self verifyCrashReportException:EXC_BAD_INSTRUCTION]; +#elif defined(ARCH_CPU_ARM64) + [self verifyCrashReportException:EXC_BREAKPOINT]; +#else +#error Port to your CPU architecture +#endif NSArray* vector = [rootObject_ getAnnotations][@"vector"]; // This message is set by dyld-353.2.1/src/ImageLoaderMachO.cpp // ImageLoaderMachO::doInitialization(). From b7db85b62d791fbb006fd87acb81b07eba71efa0 Mon Sep 17 00:00:00 2001 From: Justin Cohen Date: Thu, 14 Jul 2022 13:41:55 -0400 Subject: [PATCH 17/20] ios: vm_read module file path before calling strlen. Adds a new IOSIntermediateDumpWriter::AddPropertyCString method which takes an address to a cstring of unknown length and page-by-page searches for a NUL-byte terminator. This is necessary because currently WriteModuleInfo calls strlen directly on the dyld and module filePath without first using vm_read. On iOS14 this occasionally crashes, and is generally unwise. Instead, use AddPropertyCString. This patch also removes WriteDyldErrorStringAnnotation, as it's no longer used going forward with iOS 15. Bug: 1332862 Change-Id: I3801693bc39259a0127e5175dccf286a1cd97ba7 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3689516 Reviewed-by: Mark Mentovai Commit-Queue: Justin Cohen --- .../in_process_intermediate_dump_handler.cc | 135 +++--------------- util/ios/ios_intermediate_dump_writer.cc | 80 ++++++++++- util/ios/ios_intermediate_dump_writer.h | 28 +++- util/ios/ios_intermediate_dump_writer_test.cc | 74 ++++++++++ 4 files changed, 196 insertions(+), 121 deletions(-) diff --git a/client/ios_handler/in_process_intermediate_dump_handler.cc b/client/ios_handler/in_process_intermediate_dump_handler.cc index e7e9003e3b..6b431581d2 100644 --- a/client/ios_handler/in_process_intermediate_dump_handler.cc +++ b/client/ios_handler/in_process_intermediate_dump_handler.cc @@ -118,7 +118,7 @@ void WriteProperty(IOSIntermediateDumpWriter* writer, //! \param[in] writer The dump writer //! \param[in] key The key to write. //! \param[in] value Memory to be written. -//! \param[in] count Length of \a data. +//! \param[in] value_length Length of \a data. void WritePropertyBytes(IOSIntermediateDumpWriter* writer, IntermediateDumpKey key, const void* value, @@ -127,6 +127,20 @@ void WritePropertyBytes(IOSIntermediateDumpWriter* writer, WriteError(key); } +//! \brief Call AddPropertyCString with raw error log. +//! +//! \param[in] writer The dump writer +//! \param[in] key The key to write. +//! \param[in] max_length The maximum string length. +//! \param[in] value Memory to be written. +void WritePropertyCString(IOSIntermediateDumpWriter* writer, + IntermediateDumpKey key, + size_t max_length, + const char* value) { + if (!writer->AddPropertyCString(key, max_length, value)) + WriteError(key); +} + kern_return_t MachVMRegionRecurseDeepest(task_t task, vm_address_t* address, vm_size_t* size, @@ -498,80 +512,6 @@ void WriteAppleCrashReporterAnnotations( } } -void WriteDyldErrorStringAnnotation( - IOSIntermediateDumpWriter* writer, - const uint64_t address, - const symtab_command* symtab_command_ptr, - const dysymtab_command* dysymtab_command_ptr, - const segment_command_64* text_seg_ptr, - const segment_command_64* linkedit_seg_ptr, - vm_size_t slide) { - if (text_seg_ptr == nullptr || linkedit_seg_ptr == nullptr || - symtab_command_ptr == nullptr) { - return; - } - - ScopedVMRead symtab_command; - ScopedVMRead dysymtab_command; - ScopedVMRead text_seg; - ScopedVMRead linkedit_seg; - if (!symtab_command.Read(symtab_command_ptr) || - !text_seg.Read(text_seg_ptr) || !linkedit_seg.Read(linkedit_seg_ptr) || - (dysymtab_command_ptr && !dysymtab_command.Read(dysymtab_command_ptr))) { - CRASHPAD_RAW_LOG("Unable to load dyld symbol table."); - } - - uint64_t file_slide = - (linkedit_seg->vmaddr - text_seg->vmaddr) - linkedit_seg->fileoff; - uint64_t strings = address + (symtab_command->stroff + file_slide); - nlist_64* symbol_ptr = reinterpret_cast( - address + (symtab_command->symoff + file_slide)); - - // If a dysymtab is present, use it to filter the symtab for just the - // portion used for extdefsym. If no dysymtab is present, the entire symtab - // will need to be consulted. - uint32_t symbol_count = symtab_command->nsyms; - if (dysymtab_command_ptr) { - symbol_ptr += dysymtab_command->iextdefsym; - symbol_count = dysymtab_command->nextdefsym; - } - - for (uint32_t i = 0; i < symbol_count; i++, symbol_ptr++) { - ScopedVMRead symbol; - if (!symbol.Read(symbol_ptr)) { - CRASHPAD_RAW_LOG("Unable to load dyld symbol table symbol."); - return; - } - - if (!symbol->n_value) - continue; - - ScopedVMRead symbol_name; - if (!symbol_name.Read(strings + symbol->n_un.n_strx)) { - CRASHPAD_RAW_LOG("Unable to load dyld symbol name."); - } - - if (strcmp(symbol_name.get(), "_error_string") == 0) { - ScopedVMRead symbol_value; - if (!symbol_value.Read(symbol->n_value + slide)) { - CRASHPAD_RAW_LOG("Unable to load dyld symbol value."); - } - // 1024 here is distinct from kMaxMessageSize above, because it refers to - // a precisely-sized buffer inside dyld. - const size_t value_len = strnlen(symbol_value.get(), 1024); - if (value_len) { - WriteProperty(writer, - IntermediateDumpKey::kAnnotationsDyldErrorString, - symbol_value.get(), - value_len); - } - return; - } - - continue; - } -} - } // namespace // static @@ -980,10 +920,8 @@ void InProcessIntermediateDumpHandler::WriteModuleInfo( } if (image->imageFilePath) { - WriteProperty(writer, - IntermediateDumpKey::kName, - image->imageFilePath, - strlen(image->imageFilePath)); + WritePropertyCString( + writer, IntermediateDumpKey::kName, PATH_MAX, image->imageFilePath); } uint64_t address = FromPointerCast(image->imageLoadAddress); WriteProperty(writer, IntermediateDumpKey::kAddress, &address); @@ -995,10 +933,8 @@ void InProcessIntermediateDumpHandler::WriteModuleInfo( { IOSIntermediateDumpWriter::ScopedArrayMap modules(writer); if (image_infos->dyldPath) { - WriteProperty(writer, - IntermediateDumpKey::kName, - image_infos->dyldPath, - strlen(image_infos->dyldPath)); + WritePropertyCString( + writer, IntermediateDumpKey::kName, PATH_MAX, image_infos->dyldPath); } uint64_t address = FromPointerCast(image_infos->dyldImageLoadAddress); @@ -1129,10 +1065,6 @@ void InProcessIntermediateDumpHandler::WriteModuleInfoAtAddress( // Make sure that the basic load command structure doesn’t overflow the // space allotted for load commands, as well as iterating through ncmds. vm_size_t slide = 0; - const symtab_command* symtab_command = nullptr; - const dysymtab_command* dysymtab_command = nullptr; - const segment_command_64* linkedit_seg = nullptr; - const segment_command_64* text_seg = nullptr; for (uint32_t cmd_index = 0, cumulative_cmd_size = 0; cmd_index <= header->ncmds && cumulative_cmd_size < header->sizeofcmds; ++cmd_index, cumulative_cmd_size += command->cmdsize) { @@ -1145,20 +1077,11 @@ void InProcessIntermediateDumpHandler::WriteModuleInfoAtAddress( const segment_command_64* segment_ptr = reinterpret_cast(command_ptr); if (strcmp(segment->segname, SEG_TEXT) == 0) { - text_seg = segment_ptr; WriteProperty(writer, IntermediateDumpKey::kSize, &segment->vmsize); slide = address - segment->vmaddr; } else if (strcmp(segment->segname, SEG_DATA) == 0) { WriteDataSegmentAnnotations(writer, segment_ptr, slide); - } else if (strcmp(segment->segname, SEG_LINKEDIT) == 0) { - linkedit_seg = segment_ptr; } - } else if (command->cmd == LC_SYMTAB) { - symtab_command = - reinterpret_cast(command_ptr); - } else if (command->cmd == LC_DYSYMTAB) { - dysymtab_command = - reinterpret_cast(command_ptr); } else if (command->cmd == LC_ID_DYLIB) { ScopedVMRead dylib; if (!dylib.Read(command_ptr)) { @@ -1195,16 +1118,6 @@ void InProcessIntermediateDumpHandler::WriteModuleInfoAtAddress( } WriteProperty(writer, IntermediateDumpKey::kFileType, &header->filetype); - - if (is_dyld && header->filetype == MH_DYLINKER) { - WriteDyldErrorStringAnnotation(writer, - address, - symtab_command, - dysymtab_command, - text_seg, - linkedit_seg, - slide); - } } void InProcessIntermediateDumpHandler::WriteDataSegmentAnnotations( @@ -1286,12 +1199,10 @@ void InProcessIntermediateDumpHandler::WriteCrashpadAnnotationsList( } IOSIntermediateDumpWriter::ScopedArrayMap annotation_map(writer); - const size_t name_len = strnlen(reinterpret_cast(node->name()), - Annotation::kNameMaxLength); - WritePropertyBytes(writer, - IntermediateDumpKey::kAnnotationName, - reinterpret_cast(node->name()), - name_len); + WritePropertyCString(writer, + IntermediateDumpKey::kAnnotationName, + Annotation::kNameMaxLength, + reinterpret_cast(node->name())); WritePropertyBytes(writer, IntermediateDumpKey::kAnnotationValue, reinterpret_cast(node->value()), diff --git a/util/ios/ios_intermediate_dump_writer.cc b/util/ios/ios_intermediate_dump_writer.cc index 0fd3851697..bfbb596c2d 100644 --- a/util/ios/ios_intermediate_dump_writer.cc +++ b/util/ios/ios_intermediate_dump_writer.cc @@ -15,8 +15,10 @@ #include "util/ios/ios_intermediate_dump_writer.h" #include +#include #include +#include #include #include "base/check.h" @@ -86,6 +88,73 @@ bool IOSIntermediateDumpWriter::Close() { return RawLoggingCloseFile(fd); } +bool IOSIntermediateDumpWriter::AddPropertyCString(IntermediateDumpKey key, + size_t max_length, + const char* value) { + constexpr size_t kMaxStringBytes = 1024; + if (max_length > kMaxStringBytes) { + CRASHPAD_RAW_LOG("AddPropertyCString max_length too large"); + return false; + } + + char buffer[kMaxStringBytes]; + size_t string_length; + if (ReadCStringInternal(value, buffer, max_length, &string_length)) { + return Property(key, buffer, string_length); + } + return false; +} + +bool IOSIntermediateDumpWriter::ReadCStringInternal(const char* value, + char* buffer, + size_t max_length, + size_t* string_length) { + size_t length = 0; + while (length < max_length) { + vm_address_t data_address = reinterpret_cast(value + length); + // Calculate bytes to read past `data_address`, either the number of bytes + // to the end of the page, or the remaining bytes in `buffer`, whichever is + // smaller. + size_t data_to_end_of_page = + getpagesize() - (data_address - trunc_page(data_address)); + size_t remaining_bytes_in_buffer = max_length - length; + size_t bytes_to_read = + std::min(data_to_end_of_page, remaining_bytes_in_buffer); + + char* buffer_start = buffer + length; + size_t bytes_read = 0; + kern_return_t kr = + vm_read_overwrite(mach_task_self(), + data_address, + bytes_to_read, + reinterpret_cast(buffer_start), + &bytes_read); + if (kr != KERN_SUCCESS || bytes_read <= 0) { + CRASHPAD_RAW_LOG("ReadCStringInternal vm_read_overwrite failed"); + return false; + } + + char* nul = static_cast(memchr(buffer_start, '\0', bytes_read)); + if (nul != nullptr) { + length += nul - buffer_start; + *string_length = length; + return true; + } + length += bytes_read; + } + CRASHPAD_RAW_LOG("unterminated string"); + return false; +} + +bool IOSIntermediateDumpWriter::AddPropertyInternal(IntermediateDumpKey key, + const char* value, + size_t value_length) { + ScopedVMRead vmread; + if (!vmread.Read(value, value_length)) + return false; + return Property(key, vmread.get(), value_length); +} + bool IOSIntermediateDumpWriter::ArrayMapStart() { const CommandType command_type = CommandType::kMapStart; return RawLoggingWriteFile(fd_, &command_type, sizeof(command_type)); @@ -123,17 +192,14 @@ bool IOSIntermediateDumpWriter::RootMapEnd() { return RawLoggingWriteFile(fd_, &command_type, sizeof(command_type)); } -bool IOSIntermediateDumpWriter::AddPropertyInternal(IntermediateDumpKey key, - const char* value, - size_t value_length) { - ScopedVMRead vmread; - if (!vmread.Read(value, value_length)) - return false; +bool IOSIntermediateDumpWriter::Property(IntermediateDumpKey key, + const void* value, + size_t value_length) { const CommandType command_type = CommandType::kProperty; return RawLoggingWriteFile(fd_, &command_type, sizeof(command_type)) && RawLoggingWriteFile(fd_, &key, sizeof(key)) && RawLoggingWriteFile(fd_, &value_length, sizeof(size_t)) && - RawLoggingWriteFile(fd_, vmread.get(), value_length); + RawLoggingWriteFile(fd_, value, value_length); } } // namespace internal diff --git a/util/ios/ios_intermediate_dump_writer.h b/util/ios/ios_intermediate_dump_writer.h index d4f7a7b770..ea178353ed 100644 --- a/util/ios/ios_intermediate_dump_writer.h +++ b/util/ios/ios_intermediate_dump_writer.h @@ -175,9 +175,27 @@ class IOSIntermediateDumpWriter final { key, reinterpret_cast(value), value_length); } + //! \return Returns `true` if able to vm_read a string of \a value and write + //! a kProperty command with the \a key \a value up to a NUL byte. + //! The string cannot be longer than \a max_length with a maximum string + //! length of 1024. + bool AddPropertyCString(IntermediateDumpKey key, + size_t max_length, + const char* value); + private: - //! \return Returns `true` if able to write a kProperty command with the - //! \a key \a value \a count tuple. + //! \return Returns `true` if able to vm_read_overwrite \a value into + //! \a buffer while only reading one page at a time up to a NUL byte. + //! Sets the final length of \a buffer to \a string_length. + //! Returns `false` if unable to vm_read \a value or when no NUL byte can + //! be found within /a max_length (unterminated). + bool ReadCStringInternal(const char* value, + char* buffer, + size_t max_length, + size_t* string_length); + + //! \return Returns `true` if able to vm_read \a value \a count and write a + //! kProperty command with the \a key \a value \a count tuple. bool AddPropertyInternal(IntermediateDumpKey key, const char* value, size_t value_length); @@ -205,6 +223,12 @@ class IOSIntermediateDumpWriter final { //! \return Returns `true` if able to write a kRootMapEnd command. bool RootMapEnd(); + //! \return Returns `true` if able to write a kProperty command with the + //! \a key \a value \a value_length tuple. + bool Property(IntermediateDumpKey key, + const void* value, + size_t value_length); + int fd_; }; diff --git a/util/ios/ios_intermediate_dump_writer_test.cc b/util/ios/ios_intermediate_dump_writer_test.cc index 055ae70cca..5e70899625 100644 --- a/util/ios/ios_intermediate_dump_writer_test.cc +++ b/util/ios/ios_intermediate_dump_writer_test.cc @@ -15,8 +15,10 @@ #include "util/ios/ios_intermediate_dump_writer.h" #include +#include #include "base/files/scoped_file.h" +#include "base/mac/scoped_mach_vm.h" #include "base/posix/eintr_wrapper.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -102,6 +104,78 @@ TEST_F(IOSIntermediateDumpWriterTest, Property) { ASSERT_EQ(contents, result); } +TEST_F(IOSIntermediateDumpWriterTest, PropertyString) { + EXPECT_TRUE(writer_->Open(path())); + EXPECT_TRUE(writer_->AddPropertyCString(Key::kVersion, 64, "version")); + + std::string contents; + ASSERT_TRUE(LoggingReadEntireFile(path(), &contents)); + std::string result("\5\1\0\a\0\0\0\0\0\0\0version", 18); + ASSERT_EQ(contents, result); +} + +TEST_F(IOSIntermediateDumpWriterTest, PropertyStringShort) { + EXPECT_TRUE(writer_->Open(path())); + EXPECT_FALSE( + writer_->AddPropertyCString(Key::kVersion, 7, "versionnnnnnnnnnnn")); +} + +TEST_F(IOSIntermediateDumpWriterTest, PropertyStringLong) { + EXPECT_TRUE(writer_->Open(path())); + + char* bad_string = nullptr; + EXPECT_FALSE(writer_->AddPropertyCString(Key::kVersion, 1025, bad_string)); +} + +TEST_F(IOSIntermediateDumpWriterTest, MissingPropertyString) { + char* region; + vm_size_t page_size = getpagesize(); + vm_size_t region_size = page_size * 2; + ASSERT_EQ(vm_allocate(mach_task_self(), + reinterpret_cast(®ion), + region_size, + VM_FLAGS_ANYWHERE), + 0); + base::mac::ScopedMachVM vm_owner(reinterpret_cast(region), + region_size); + + // Fill first page with 'A' and second with 'B'. + memset(region, 'A', page_size); + memset(region + page_size, 'B', page_size); + + // Drop a NUL 10 bytes from the end of the first page and into the second + // page. + region[page_size - 10] = '\0'; + region[page_size + 10] = '\0'; + + // Read a string that spans two pages. + EXPECT_TRUE(writer_->Open(path())); + EXPECT_TRUE( + writer_->AddPropertyCString(Key::kVersion, 64, region + page_size - 5)); + std::string contents; + ASSERT_TRUE(LoggingReadEntireFile(path(), &contents)); + std::string result("\x5\x1\0\xF\0\0\0\0\0\0\0AAAAABBBBBBBBBB", 26); + ASSERT_EQ(contents, result); + + // Dealloc second page. + ASSERT_EQ(vm_deallocate(mach_task_self(), + reinterpret_cast(region + page_size), + page_size), + 0); + + // Reading the same string should fail when the next page is dealloc-ed. + EXPECT_FALSE( + writer_->AddPropertyCString(Key::kVersion, 64, region + page_size - 5)); + + // Ensure we can read the first string without loading the second page. + EXPECT_TRUE(writer_->Open(path())); + EXPECT_TRUE( + writer_->AddPropertyCString(Key::kVersion, 64, region + page_size - 20)); + ASSERT_TRUE(LoggingReadEntireFile(path(), &contents)); + result.assign("\x5\x1\0\n\0\0\0\0\0\0\0AAAAAAAAAA", 21); + ASSERT_EQ(contents, result); +} + TEST_F(IOSIntermediateDumpWriterTest, BadProperty) { EXPECT_TRUE(writer_->Open(path())); ASSERT_FALSE(writer_->AddProperty(Key::kVersion, "version", -1)); From df86075acc33314e611b351b33bf1c671b8cbc2f Mon Sep 17 00:00:00 2001 From: Justin Cohen Date: Thu, 14 Jul 2022 11:00:29 -0400 Subject: [PATCH 18/20] ios: Prevent duplicate uploads and watchdog kills with slow uploads. On iOS, holding a lock during a slow upload can lead to watchdog kills if the app is suspended mid-upload. Instead, if the client can obtain the lock, the database sets a lock-time file attribute and releases the flock. The file attribute is cleared when the upload is completed. The lock-time attribute can be used to prevent file access from other processes, or to discard reports that likely were terminated mid-upload. Bug:chromium:1342051 Change-Id: Ib878f6ade8eae467ee39acb52288296759c84582 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3739019 Reviewed-by: Robert Sesek Commit-Queue: Justin Cohen Reviewed-by: Mark Mentovai --- client/crash_report_database.h | 10 ++++- client/crash_report_database_mac.mm | 58 ++++++++++++++++++++++++++- client/crash_report_database_test.cc | 44 ++++++++++++++++++++ client/settings.h | 8 ++++ handler/crash_report_upload_thread.cc | 8 +--- util/misc/metrics.cc | 7 ++++ util/misc/metrics.h | 6 +++ util/net/http_transport_mac.mm | 2 + 8 files changed, 134 insertions(+), 9 deletions(-) diff --git a/client/crash_report_database.h b/client/crash_report_database.h index fea49853a0..4404959fad 100644 --- a/client/crash_report_database.h +++ b/client/crash_report_database.h @@ -328,7 +328,8 @@ class CrashReportDatabase { virtual OperationStatus GetCompletedReports(std::vector* reports) = 0; //! \brief Obtains and locks a report object for uploading to a collection - //! server. + //! server. On iOS the file lock is released and mutual-exclusion is kept + //! via a file attribute. //! //! Callers should upload the crash report using the FileReader provided. //! Callers should then call RecordUploadComplete() to record a successful @@ -336,6 +337,13 @@ class CrashReportDatabase { //! be recorded as unsuccessful and the report lock released when \a report is //! destroyed. //! + //! On iOS, holding a lock during a slow upload can lead to watchdog kills if + //! the app is suspended mid-upload. Instead, if the client can obtain the + //! lock, the database sets a lock-time file attribute and releases the lock. + //! The attribute is cleared when the upload is completed. The lock-time + //! attribute can be used to prevent file access from other processes, or to + //! discard reports that likely were terminated mid-upload. + //! //! \param[in] uuid The unique identifier for the crash report record. //! \param[out] report A crash report record for the report to be uploaded. //! Only valid if this returns #kNoError. diff --git a/client/crash_report_database_mac.mm b/client/crash_report_database_mac.mm index e33b932528..813e5b8fb9 100644 --- a/client/crash_report_database_mac.mm +++ b/client/crash_report_database_mac.mm @@ -71,6 +71,9 @@ constexpr char kXattrCollectorID[] = "id"; constexpr char kXattrCreationTime[] = "creation_time"; constexpr char kXattrIsUploaded[] = "uploaded"; +#if BUILDFLAG(IS_IOS) +constexpr char kXattrUploadStartTime[] = "upload_start_time"; +#endif constexpr char kXattrLastUploadTime[] = "last_upload_time"; constexpr char kXattrUploadAttemptCount[] = "upload_count"; constexpr char kXattrIsUploadExplicitlyRequested[] = @@ -189,10 +192,11 @@ OperationStatus RecordUploadAttempt(UploadReport* report, //! \brief Obtain a background task assertion while a flock is in use. //! Ensure this is defined first so it is destroyed last. internal::ScopedBackgroundTask ios_background_task{"UploadReportMac"}; -#endif // BUILDFLAG(IS_IOS) +#else //! \brief Stores the flock of the file for the duration of //! GetReportForUploading() and RecordUploadAttempt(). base::ScopedFD lock_fd; +#endif // BUILDFLAG(IS_IOS) }; //! \brief Locates a crash report in the database by UUID. @@ -474,12 +478,50 @@ OperationStatus ReportsInDirectory(const base::FilePath& path, if (!ReadReportMetadataLocked(upload_report->file_path, upload_report.get())) return kDatabaseError; +#if BUILDFLAG(IS_IOS) + time_t upload_start_time = 0; + if (ReadXattrTimeT(upload_report->file_path, + XattrName(kXattrUploadStartTime), + &upload_start_time) == XattrStatus::kOtherError) { + return kDatabaseError; + } + + time_t now = time(nullptr); + if (upload_start_time) { + // If we were able to ObtainReportLock but kXattrUploadStartTime is set, + // either another client is uploading this report or a client was terminated + // during an upload. CrashReportUploadThread sets the timeout to 20 seconds + // for iOS. If kXattrUploadStartTime is less than 5 minutes ago, consider + // the report locked and return kBusyError. Otherwise, consider the upload a + // failure and skip the report. + if (upload_start_time > now - 15 * internal::kUploadReportTimeoutSeconds) { + return kBusyError; + } else { + // SkipReportUpload expects an unlocked report. + lock.reset(); + CrashReportDatabase::OperationStatus os = SkipReportUpload( + upload_report->uuid, Metrics::CrashSkippedReason::kUploadFailed); + if (os != kNoError) { + return kDatabaseError; + } + return kReportNotFound; + } + } + + if (!WriteXattrTimeT( + upload_report->file_path, XattrName(kXattrUploadStartTime), now)) { + return kDatabaseError; + } +#endif + if (!upload_report->Initialize(upload_report->file_path, this)) { return kFileSystemError; } upload_report->database_ = this; +#if !BUILDFLAG(IS_IOS) upload_report->lock_fd.reset(lock.release()); +#endif upload_report->report_metrics_ = report_metrics; report->reset(upload_report.release()); return kNoError; @@ -510,6 +552,13 @@ OperationStatus ReportsInDirectory(const base::FilePath& path, return os; } +#if BUILDFLAG(IS_IOS) + if (RemoveXattr(report_path, XattrName(kXattrUploadStartTime)) == + XattrStatus::kOtherError) { + return kDatabaseError; + } +#endif + if (!WriteXattrBool(report_path, XattrName(kXattrIsUploaded), successful)) { return kDatabaseError; } @@ -554,6 +603,13 @@ OperationStatus ReportsInDirectory(const base::FilePath& path, if (!lock.is_valid()) return kBusyError; +#if BUILDFLAG(IS_IOS) + if (RemoveXattr(report_path, XattrName(kXattrUploadStartTime)) == + XattrStatus::kOtherError) { + return kDatabaseError; + } +#endif + return MarkReportCompletedLocked(report_path, nullptr); } diff --git a/client/crash_report_database_test.cc b/client/crash_report_database_test.cc index bdb6dc242c..67b5edcc2c 100644 --- a/client/crash_report_database_test.cc +++ b/client/crash_report_database_test.cc @@ -24,6 +24,10 @@ #include "util/file/file_io.h" #include "util/file/filesystem.h" +#if BUILDFLAG(IS_IOS) +#include "util/mac/xattr.h" +#endif + namespace crashpad { namespace test { namespace { @@ -511,6 +515,46 @@ TEST_F(CrashReportDatabaseTest, DuelingUploads) { CrashReportDatabase::kNoError); } +#if BUILDFLAG(IS_IOS) +TEST_F(CrashReportDatabaseTest, InterruptedIOSUploads) { + CrashReportDatabase::Report report; + CreateCrashReport(&report); + + std::unique_ptr upload_report; + EXPECT_EQ(db()->GetReportForUploading(report.uuid, &upload_report), + CrashReportDatabase::kNoError); + + // Set upload_start_time to 10 minutes ago. + time_t ten_minutes_ago = time(nullptr) - 10 * 60; + ASSERT_TRUE( + WriteXattrTimeT(report.file_path, + "org.chromium.crashpad.database.upload_start_time", + ten_minutes_ago)); + + std::vector reports; + EXPECT_EQ(db()->GetPendingReports(&reports), CrashReportDatabase::kNoError); + ASSERT_EQ(reports.size(), 1u); + reports.clear(); + EXPECT_EQ(db()->GetCompletedReports(&reports), CrashReportDatabase::kNoError); + EXPECT_TRUE(reports.empty()); + + // Getting a stale report will automatically skip it. + std::unique_ptr upload_report_2; + EXPECT_EQ(db()->GetReportForUploading(report.uuid, &upload_report_2), + CrashReportDatabase::kReportNotFound); + EXPECT_FALSE(upload_report_2); + + // Confirm report was moved from pending to completed. + EXPECT_EQ(db()->GetPendingReports(&reports), CrashReportDatabase::kNoError); + EXPECT_TRUE(reports.empty()); + EXPECT_EQ(db()->GetCompletedReports(&reports), CrashReportDatabase::kNoError); + ASSERT_EQ(reports.size(), 1u); + + EXPECT_EQ(db()->RecordUploadComplete(std::move(upload_report), std::string()), + CrashReportDatabase::kReportNotFound); +} +#endif + TEST_F(CrashReportDatabaseTest, UploadAlreadyUploaded) { CrashReportDatabase::Report report; CreateCrashReport(&report); diff --git a/client/settings.h b/client/settings.h index 8ad8a2b16f..e4a5cedc43 100644 --- a/client/settings.h +++ b/client/settings.h @@ -37,6 +37,14 @@ struct ScopedLockedFileHandleTraits { static void Free(FileHandle handle); }; +// TODO(mark): The timeout should be configurable by the client. +#if BUILDFLAG(IS_IOS) +// iOS background assertions only last 30 seconds, keep the timeout shorter. +constexpr double kUploadReportTimeoutSeconds = 20; +#else +constexpr double kUploadReportTimeoutSeconds = 60; +#endif + } // namespace internal //! \brief An interface for accessing and modifying the settings of a diff --git a/handler/crash_report_upload_thread.cc b/handler/crash_report_upload_thread.cc index 138cf80026..4d1548dbf8 100644 --- a/handler/crash_report_upload_thread.cc +++ b/handler/crash_report_upload_thread.cc @@ -311,13 +311,7 @@ CrashReportUploadThread::UploadResult CrashReportUploadThread::UploadReport( } http_transport->SetBodyStream(http_multipart_builder.GetBodyStream()); // TODO(mark): The timeout should be configurable by the client. -#if BUILDFLAG(IS_IOS) - // iOS background assertions only last 30 seconds, keep the timeout shorter. - double timeout_seconds = 20; -#else - double timeout_seconds = 60; -#endif - http_transport->SetTimeout(timeout_seconds); + http_transport->SetTimeout(internal::kUploadReportTimeoutSeconds); std::string url = url_; if (options_.identify_client_via_url) { diff --git a/util/misc/metrics.cc b/util/misc/metrics.cc index 2e8459c71d..dac7300d5d 100644 --- a/util/misc/metrics.cc +++ b/util/misc/metrics.cc @@ -78,6 +78,13 @@ void Metrics::CrashUploadAttempted(bool successful) { UMA_HISTOGRAM_BOOLEAN("Crashpad.CrashUpload.AttemptSuccessful", successful); } +#if BUILDFLAG(IS_APPLE) +// static +void Metrics::CrashUploadErrorCode(int error_code) { + base::UmaHistogramSparse("Crashpad.CrashUpload.ErrorCode", error_code); +} +#endif + // static void Metrics::CrashUploadSkipped(CrashSkippedReason reason) { UMA_HISTOGRAM_ENUMERATION( diff --git a/util/misc/metrics.h b/util/misc/metrics.h index c870b3a22a..e11fcc1303 100644 --- a/util/misc/metrics.h +++ b/util/misc/metrics.h @@ -63,6 +63,12 @@ class Metrics { //! \brief Reports on a crash upload attempt, and if it succeeded. static void CrashUploadAttempted(bool successful); +#if BUILDFLAG(IS_APPLE) || DOXYGEN + //! \brief Records error codes from + //! `+[NSURLConnection sendSynchronousRequest:returningResponse:error:]`. + static void CrashUploadErrorCode(int error_code); +#endif + //! \brief Values for CrashUploadSkipped(). //! //! \note These are used as metrics enumeration values, so new values should diff --git a/util/net/http_transport_mac.mm b/util/net/http_transport_mac.mm index 50b9206880..445e895d13 100644 --- a/util/net/http_transport_mac.mm +++ b/util/net/http_transport_mac.mm @@ -25,6 +25,7 @@ #include "package.h" #include "util/file/file_io.h" #include "util/misc/implicit_cast.h" +#include "util/misc/metrics.h" #include "util/net/http_body.h" // An implementation of NSInputStream that reads from a @@ -257,6 +258,7 @@ - (BOOL)setProperty:(id)property forKey:(NSStreamPropertyKey)key { #pragma clang diagnostic pop if (error) { + Metrics::CrashUploadErrorCode(error.code); LOG(ERROR) << [[error localizedDescription] UTF8String] << " (" << [[error domain] UTF8String] << " " << [error code] << ")"; return false; From ae7d8a9ba461134ae8eb7f7a86dfc02bb41a85d6 Mon Sep 17 00:00:00 2001 From: Justin Cohen Date: Fri, 15 Jul 2022 14:36:38 -0400 Subject: [PATCH 19/20] ios: Use fewer vm_reads when iterating modules. Rather than vm_reading each individual module load_command, load all of the commands at once. This saves nearly 200ms on an iPhone 12 Pro. Change-Id: I06f56c3ecbdf74f78759648ea62bcccd027f304c Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3764242 Reviewed-by: Mark Mentovai Commit-Queue: Justin Cohen --- .../in_process_intermediate_dump_handler.cc | 123 ++++++++---------- .../in_process_intermediate_dump_handler.h | 10 +- 2 files changed, 61 insertions(+), 72 deletions(-) diff --git a/client/ios_handler/in_process_intermediate_dump_handler.cc b/client/ios_handler/in_process_intermediate_dump_handler.cc index 6b431581d2..93128931d1 100644 --- a/client/ios_handler/in_process_intermediate_dump_handler.cc +++ b/client/ios_handler/in_process_intermediate_dump_handler.cc @@ -1053,68 +1053,63 @@ void InProcessIntermediateDumpHandler::WriteModuleInfoAtAddress( return; } - const load_command* command_ptr = reinterpret_cast( - reinterpret_cast(address) + 1); - - ScopedVMRead command; - if (!command.Read(command_ptr)) { - CRASHPAD_RAW_LOG("Invalid module command"); + const load_command* unsafe_command_ptr = + reinterpret_cast( + reinterpret_cast(address) + 1); + + // Rather than using an individual ScopedVMRead for each load_command, load + // the entire block of commands at once. + ScopedVMRead all_commands; + if (!all_commands.Read(unsafe_command_ptr, header->sizeofcmds)) { + CRASHPAD_RAW_LOG("Unable to read module load_commands."); return; } + // All the *_vm_read_ptr variables in the load_command loop below have been + // vm_read in `all_commands` above, and may be dereferenced without additional + // ScopedVMReads. + const load_command* command_vm_read_ptr = + reinterpret_cast(all_commands.get()); + // Make sure that the basic load command structure doesn’t overflow the // space allotted for load commands, as well as iterating through ncmds. vm_size_t slide = 0; for (uint32_t cmd_index = 0, cumulative_cmd_size = 0; - cmd_index <= header->ncmds && cumulative_cmd_size < header->sizeofcmds; - ++cmd_index, cumulative_cmd_size += command->cmdsize) { - if (command->cmd == LC_SEGMENT_64) { - ScopedVMRead segment; - if (!segment.Read(command_ptr)) { - CRASHPAD_RAW_LOG("Invalid LC_SEGMENT_64 segment"); - return; - } - const segment_command_64* segment_ptr = - reinterpret_cast(command_ptr); - if (strcmp(segment->segname, SEG_TEXT) == 0) { - WriteProperty(writer, IntermediateDumpKey::kSize, &segment->vmsize); - slide = address - segment->vmaddr; - } else if (strcmp(segment->segname, SEG_DATA) == 0) { - WriteDataSegmentAnnotations(writer, segment_ptr, slide); - } - } else if (command->cmd == LC_ID_DYLIB) { - ScopedVMRead dylib; - if (!dylib.Read(command_ptr)) { - CRASHPAD_RAW_LOG("Invalid LC_ID_DYLIB segment"); - return; + cmd_index < header->ncmds && cumulative_cmd_size < header->sizeofcmds; + ++cmd_index) { + if (command_vm_read_ptr->cmd == LC_SEGMENT_64) { + const segment_command_64* segment_vm_read_ptr = + reinterpret_cast(command_vm_read_ptr); + if (strcmp(segment_vm_read_ptr->segname, SEG_TEXT) == 0) { + WriteProperty( + writer, IntermediateDumpKey::kSize, &segment_vm_read_ptr->vmsize); + slide = address - segment_vm_read_ptr->vmaddr; + } else if (strcmp(segment_vm_read_ptr->segname, SEG_DATA) == 0) { + WriteDataSegmentAnnotations(writer, segment_vm_read_ptr, slide); } + } else if (command_vm_read_ptr->cmd == LC_ID_DYLIB) { + const dylib_command* dylib_vm_read_ptr = + reinterpret_cast(command_vm_read_ptr); WriteProperty(writer, IntermediateDumpKey::kDylibCurrentVersion, - &dylib->dylib.current_version); - } else if (command->cmd == LC_SOURCE_VERSION) { - ScopedVMRead source_version; - if (!source_version.Read(command_ptr)) { - CRASHPAD_RAW_LOG("Invalid LC_SOURCE_VERSION segment"); - return; - } + &dylib_vm_read_ptr->dylib.current_version); + } else if (command_vm_read_ptr->cmd == LC_SOURCE_VERSION) { + const source_version_command* source_version_vm_read_ptr = + reinterpret_cast(command_vm_read_ptr); WriteProperty(writer, IntermediateDumpKey::kSourceVersion, - &source_version->version); - } else if (command->cmd == LC_UUID) { - ScopedVMRead uuid; - if (!uuid.Read(command_ptr)) { - CRASHPAD_RAW_LOG("Invalid LC_UUID segment"); - return; - } - WriteProperty(writer, IntermediateDumpKey::kUUID, &uuid->uuid); + &source_version_vm_read_ptr->version); + } else if (command_vm_read_ptr->cmd == LC_UUID) { + const uuid_command* uuid_vm_read_ptr = + reinterpret_cast(command_vm_read_ptr); + WriteProperty( + writer, IntermediateDumpKey::kUUID, &uuid_vm_read_ptr->uuid); } - command_ptr = reinterpret_cast( - reinterpret_cast(command_ptr) + command->cmdsize); - if (!command.Read(command_ptr)) { - CRASHPAD_RAW_LOG("Invalid module command"); - return; - } + cumulative_cmd_size += command_vm_read_ptr->cmdsize; + command_vm_read_ptr = reinterpret_cast( + reinterpret_cast(command_vm_read_ptr) + + command_vm_read_ptr->cmdsize); } WriteProperty(writer, IntermediateDumpKey::kFileType, &header->filetype); @@ -1122,40 +1117,32 @@ void InProcessIntermediateDumpHandler::WriteModuleInfoAtAddress( void InProcessIntermediateDumpHandler::WriteDataSegmentAnnotations( IOSIntermediateDumpWriter* writer, - const segment_command_64* segment_ptr, + const segment_command_64* segment_vm_read_ptr, vm_size_t slide) { - ScopedVMRead segment; - if (!segment.Read(segment_ptr)) { - CRASHPAD_RAW_LOG("Unable to read SEG_DATA."); - return; - } - const section_64* section_ptr = reinterpret_cast( - reinterpret_cast(segment_ptr) + sizeof(segment_command_64)); - for (uint32_t sect_index = 0; sect_index <= segment->nsects; ++sect_index) { - ScopedVMRead section; - if (!section.Read(section_ptr)) { - CRASHPAD_RAW_LOG("Unable to read SEG_DATA section."); - return; - } - if (strcmp(section->sectname, "crashpad_info") == 0) { + const section_64* section_vm_read_ptr = reinterpret_cast( + reinterpret_cast(segment_vm_read_ptr) + + sizeof(segment_command_64)); + for (uint32_t sect_index = 0; sect_index <= segment_vm_read_ptr->nsects; + ++sect_index) { + if (strcmp(section_vm_read_ptr->sectname, "crashpad_info") == 0) { ScopedVMRead crashpad_info; - if (crashpad_info.Read(section->addr + slide) && + if (crashpad_info.Read(section_vm_read_ptr->addr + slide) && crashpad_info->size() == sizeof(CrashpadInfo) && crashpad_info->signature() == CrashpadInfo::kSignature && crashpad_info->version() == 1) { WriteCrashpadAnnotationsList(writer, crashpad_info.get()); WriteCrashpadSimpleAnnotationsDictionary(writer, crashpad_info.get()); } - } else if (strcmp(section->sectname, "__crash_info") == 0) { + } else if (strcmp(section_vm_read_ptr->sectname, "__crash_info") == 0) { ScopedVMRead crash_info; - if (!crash_info.Read(section->addr + slide) || + if (!crash_info.Read(section_vm_read_ptr->addr + slide) || (crash_info->version != 4 && crash_info->version != 5)) { continue; } WriteAppleCrashReporterAnnotations(writer, crash_info.get()); } - section_ptr = reinterpret_cast( - reinterpret_cast(section_ptr) + sizeof(section_64)); + section_vm_read_ptr = reinterpret_cast( + reinterpret_cast(section_vm_read_ptr) + sizeof(section_64)); } } diff --git a/client/ios_handler/in_process_intermediate_dump_handler.h b/client/ios_handler/in_process_intermediate_dump_handler.h index f6db28d510..49a520dc86 100644 --- a/client/ios_handler/in_process_intermediate_dump_handler.h +++ b/client/ios_handler/in_process_intermediate_dump_handler.h @@ -139,10 +139,12 @@ class InProcessIntermediateDumpHandler final { bool is_dyld); //! \brief Extract and write Apple crashreporter_annotations_t data and - //! Crashpad annotations. - static void WriteDataSegmentAnnotations(IOSIntermediateDumpWriter* writer, - const segment_command_64* segment_ptr, - vm_size_t slide); + //! Crashpad annotations. Note that \a segment_vm_read_ptr has already + //! been read via vm_read and may be dereferenced without a ScopedVMRead. + static void WriteDataSegmentAnnotations( + IOSIntermediateDumpWriter* writer, + const segment_command_64* segment_vm_read_ptr, + vm_size_t slide); //! \brief Write Crashpad annotations list. static void WriteCrashpadAnnotationsList(IOSIntermediateDumpWriter* writer, From c25a4a77588e81d3c4ac09f89edeed587fa30a9a Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Mon, 18 Jul 2022 13:04:25 +0200 Subject: [PATCH 20/20] update submodules and add WER handler to CMake --- handler/CMakeLists.txt | 24 ++++++++++++++++++++++++ third_party/mini_chromium/mini_chromium | 2 +- util/CMakeLists.txt | 4 ++-- 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/handler/CMakeLists.txt b/handler/CMakeLists.txt index 86dcdff9a3..fde7d26f64 100644 --- a/handler/CMakeLists.txt +++ b/handler/CMakeLists.txt @@ -122,3 +122,27 @@ if(NOT IOS) RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}" ) endif() + +if(WIN32) + add_library(crashpad_wer SHARED + win/wer/crashpad_wer.cc + win/wer/crashpad_wer.h + win/wer/crashpad_wer.def + win/wer/crashpad_wer_main.cc + ../util/misc/address_types.h + ../util/win/address_types.h + ../util/win/registration_protocol_win.h + ) + + target_link_libraries(crashpad_wer + PRIVATE + $ + ) + + set_property(TARGET crashpad_wer PROPERTY EXPORT_NAME crashpad_wer) + add_library(crashpad::wer ALIAS crashpad_wer) + + install(TARGETS crashpad_wer EXPORT crashpad_export + RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}" + ) +endif() diff --git a/third_party/mini_chromium/mini_chromium b/third_party/mini_chromium/mini_chromium index 5654edb422..75dcb8dc41 160000 --- a/third_party/mini_chromium/mini_chromium +++ b/third_party/mini_chromium/mini_chromium @@ -1 +1 @@ -Subproject commit 5654edb4225bcad13901155c819febb5748e502b +Subproject commit 75dcb8dc417af77fdb9ec23c7b51cb1d57dfcee2 diff --git a/util/CMakeLists.txt b/util/CMakeLists.txt index e37bfb155e..8a72636ff9 100644 --- a/util/CMakeLists.txt +++ b/util/CMakeLists.txt @@ -132,11 +132,11 @@ if(NOT WIN32) thread/thread_posix.cc posix/close_multiple.cc posix/close_multiple.h - posix/double_fork_and_exec.cc - posix/double_fork_and_exec.h posix/drop_privileges.cc posix/drop_privileges.h posix/process_info.h + posix/spawn_subprocess.cc + posix/spawn_subprocess.h posix/symbolic_constants_posix.cc posix/symbolic_constants_posix.h )