From 8b2ec2aae4b2721079c3a28a4cde1d13d2cca999 Mon Sep 17 00:00:00 2001 From: Vlad Tsyrklevich Date: Thu, 20 Dec 2018 11:12:51 -0800 Subject: [PATCH] Make TaskMemory a child class of ProcessMemory Currently TaskMemory re-implements a number of Read* routines that are implemented in a platform-independent way in ProcessMemory with access to a single platform-specific ReadUpTo method. Implement the ReadUpTo method for TaskMemory and subclass it from ProcessMemory to inherit the remaining methods. The ProcessMemoryTests didn't work on macOS because MultiprocessExec can not access the child process' task port without root privileges or the task_for_pid entitlement. Create an adaptor class for those tests to use MachMultiprocess so that the child process sends its task port to the parent. Bug: crashpad:263 Change-Id: Id8e1788a74fe957f05703a5eb569ca3bf9870369 Reviewed-on: https://chromium-review.googlesource.com/c/1387265 Commit-Queue: Vlad Tsyrklevich Reviewed-by: Mark Mentovai --- test/multiprocess_exec.h | 4 +- test/multiprocess_exec_posix.cc | 8 ++ test/process_type.cc | 6 +- test/process_type.h | 8 +- util/BUILD.gn | 29 ++----- util/mach/task_memory.cc | 77 +++++-------------- util/mach/task_memory.h | 65 ++-------------- util/process/process_memory_native.h | 4 + util/process/process_memory_test.cc | 108 +++++++++++++++++++++------ 9 files changed, 141 insertions(+), 168 deletions(-) diff --git a/test/multiprocess_exec.h b/test/multiprocess_exec.h index 84335b28..1168f800 100644 --- a/test/multiprocess_exec.h +++ b/test/multiprocess_exec.h @@ -122,7 +122,9 @@ class MultiprocessExec : public Multiprocess { //! //! This method is only valid during the body of MultiprocessParent(). //! - //! \return A platform-specific type representing the child process. + //! \return A platform-specific type representing the child process. This + //! method can fail on macOS because access to a child's task port + //! requires the task_for_pid entitlement. ProcessType ChildProcess(); protected: diff --git a/test/multiprocess_exec_posix.cc b/test/multiprocess_exec_posix.cc index c91b7f73..bf4263a2 100644 --- a/test/multiprocess_exec_posix.cc +++ b/test/multiprocess_exec_posix.cc @@ -29,6 +29,10 @@ #include #endif +#if defined(OS_MACOSX) +#include "util/mach/task_for_pid.h" +#endif + namespace crashpad { namespace test { @@ -149,7 +153,11 @@ void MultiprocessExec::MultiprocessChild() { } ProcessType MultiprocessExec::ChildProcess() { +#if defined(OS_MACOSX) + return TaskForPID(ChildPID()); +#else return ChildPID(); +#endif } } // namespace test diff --git a/test/process_type.cc b/test/process_type.cc index 9fd08dd9..94fd912d 100644 --- a/test/process_type.cc +++ b/test/process_type.cc @@ -16,7 +16,7 @@ #if defined(OS_FUCHSIA) #include -#elif defined(OS_POSIX) +#elif defined(OS_LINUX) || defined(OS_ANDROID) #include #endif @@ -26,10 +26,12 @@ namespace test { ProcessType GetSelfProcess() { #if defined(OS_FUCHSIA) return zx::process::self(); -#elif defined(OS_POSIX) +#elif defined(OS_LINUX) || defined(OS_ANDROID) return getpid(); #elif defined(OS_WIN) return GetCurrentProcess(); +#elif defined(OS_MACOSX) + return mach_task_self(); #endif } diff --git a/test/process_type.h b/test/process_type.h index 240c083d..dc99687a 100644 --- a/test/process_type.h +++ b/test/process_type.h @@ -19,10 +19,12 @@ #if defined(OS_FUCHSIA) #include -#elif defined(OS_POSIX) +#elif defined(OS_LINUX) || defined(OS_ANDROID) #include #elif defined(OS_WIN) #include +#elif defined(OS_MACOSX) +#include #endif namespace crashpad { @@ -30,11 +32,13 @@ namespace test { #if defined(OS_FUCHSIA) using ProcessType = zx::unowned_process; -#elif defined(OS_POSIX) || DOXYGEN +#elif defined(OS_LINUX) || defined(OS_ANDROID) || DOXYGEN //! \brief Alias for platform-specific type to represent a process. using ProcessType = pid_t; #elif defined(OS_WIN) using ProcessType = HANDLE; +#elif defined(OS_MACOSX) +using ProcessType = task_t; #else #error Port. #endif diff --git a/util/BUILD.gn b/util/BUILD.gn index c02ecd4e..e43c42f0 100644 --- a/util/BUILD.gn +++ b/util/BUILD.gn @@ -137,6 +137,11 @@ static_library("util") { "numeric/in_range_cast.h", "numeric/int128.h", "numeric/safe_assignment.h", + "process/process_memory.cc", + "process/process_memory.h", + "process/process_memory_native.h", + "process/process_memory_range.cc", + "process/process_memory_range.h", "stdlib/aligned_allocator.cc", "stdlib/aligned_allocator.h", "stdlib/map_insert.h", @@ -313,19 +318,6 @@ static_library("util") { ] } - if (crashpad_is_linux || crashpad_is_android || crashpad_is_fuchsia || - crashpad_is_win) { - sources += [ - "process/process_memory.cc", - "process/process_memory.h", - "process/process_memory_native.h", - - # TODO: Port to all platforms. - "process/process_memory_range.cc", - "process/process_memory_range.h", - ] - } - if (crashpad_is_win) { sources += [ "file/directory_reader_win.cc", @@ -562,6 +554,8 @@ source_set("util_test") { "numeric/checked_range_test.cc", "numeric/in_range_cast_test.cc", "numeric/int128_test.cc", + "process/process_memory_range_test.cc", + "process/process_memory_test.cc", "stdlib/aligned_allocator_test.cc", "stdlib/map_insert_test.cc", "stdlib/string_number_conversion_test.cc", @@ -633,15 +627,6 @@ source_set("util_test") { sources += [ "misc/capture_context_test_util_fuchsia.cc" ] } - if (crashpad_is_linux || crashpad_is_android || crashpad_is_fuchsia || - crashpad_is_win) { - sources += [ - # TODO: Port to all platforms. - "process/process_memory_range_test.cc", - "process/process_memory_test.cc", - ] - } - if (crashpad_is_win) { sources += [ "misc/capture_context_test_util_win.cc", diff --git a/util/mach/task_memory.cc b/util/mach/task_memory.cc index 14069ddf..eb8b1f57 100644 --- a/util/mach/task_memory.cc +++ b/util/mach/task_memory.cc @@ -73,21 +73,9 @@ bool TaskMemory::Initialize(task_t task) { return true; } -bool TaskMemory::Read(mach_vm_address_t address, size_t size, void* buffer) { - INITIALIZATION_STATE_DCHECK_VALID(initialized_); - - std::unique_ptr memory = ReadMapped(address, size); - if (!memory) { - return false; - } - - memcpy(buffer, memory->data(), size); - return true; -} - std::unique_ptr TaskMemory::ReadMapped( mach_vm_address_t address, - size_t size) { + size_t size) const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); if (size == 0) { @@ -113,58 +101,29 @@ std::unique_ptr TaskMemory::ReadMapped( new MappedMemory(region, region_size, address - region_address, size)); } -bool TaskMemory::ReadCString(mach_vm_address_t address, std::string* string) { +ssize_t TaskMemory::ReadUpTo(VMAddress address, + size_t size, + void* buffer) const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); + DCHECK_LE(size, (size_t)std::numeric_limits::max()); - return ReadCStringInternal(address, false, 0, string); -} + std::unique_ptr memory = ReadMapped(address, size); + if (!memory) { + // If we can not read the entire mapping, try to perform a short read of the + // first page instead. This is necessary to support ReadCString(). + size_t short_read = PAGE_SIZE - (address % PAGE_SIZE); + if (short_read >= size) + return -1; -bool TaskMemory::ReadCStringSizeLimited(mach_vm_address_t address, - mach_vm_size_t size, - std::string* string) { - INITIALIZATION_STATE_DCHECK_VALID(initialized_); + memory = ReadMapped(address, short_read); + if (!memory) + return -1; - return ReadCStringInternal(address, true, size, string); -} - -bool TaskMemory::ReadCStringInternal(mach_vm_address_t address, - bool has_size, - mach_vm_size_t size, - std::string* string) { - INITIALIZATION_STATE_DCHECK_VALID(initialized_); - - if (!has_size) { - size = PAGE_SIZE; + size = short_read; } - std::string local_string; - mach_vm_address_t read_address = address; - do { - mach_vm_size_t read_length = - std::min(size, PAGE_SIZE - (read_address % PAGE_SIZE)); - std::unique_ptr read_region = - ReadMapped(read_address, read_length); - if (!read_region) { - return false; - } - - const char* read_region_data = - reinterpret_cast(read_region->data()); - size_t read_region_data_length = strnlen(read_region_data, read_length); - local_string.append(read_region_data, read_region_data_length); - if (read_region_data_length < read_length) { - string->swap(local_string); - return true; - } - - if (has_size) { - size -= read_length; - } - read_address = mach_vm_trunc_page(read_address + read_length); - } while ((!has_size || size > 0) && read_address > address); - - LOG(WARNING) << base::StringPrintf("unterminated string at 0x%llx", address); - return false; + memcpy(buffer, memory->data(), size); + return static_cast(size); } } // namespace crashpad diff --git a/util/mach/task_memory.h b/util/mach/task_memory.h index 118ff0ba..8fc217a6 100644 --- a/util/mach/task_memory.h +++ b/util/mach/task_memory.h @@ -23,12 +23,14 @@ #include "base/mac/scoped_mach_vm.h" #include "base/macros.h" +#include "util/misc/address_types.h" #include "util/misc/initialization_state_dcheck.h" +#include "util/process/process_memory.h" namespace crashpad { //! \brief Accesses the memory of another Mach task. -class TaskMemory { +class TaskMemory : public ProcessMemory { public: //! \brief A memory region mapped from another Mach task. //! @@ -103,23 +105,6 @@ class TaskMemory { //! \return `true` on success, `false` on failure with a message logged. bool Initialize(task_t task); - //! \brief Copies memory from the target task into a caller-provided buffer in - //! the current task. - //! - //! \param[in] address The address, in the target task’s address space, of the - //! memory region to copy. - //! \param[in] size The size, in bytes, of the memory region to copy. \a - //! buffer must be at least this size. - //! \param[out] buffer The buffer into which the contents of the other task’s - //! memory will be copied. - //! - //! \return `true` on success, with \a buffer filled appropriately. `false` on - //! failure, with a warning logged. Failures can occur, for example, when - //! encountering unmapped or unreadable pages. - //! - //! \sa ReadMapped() - bool Read(mach_vm_address_t address, size_t size, void* buffer); - //! \brief Maps memory from the target task into the current task. //! //! This interface is an alternative to Read() that does not require the @@ -134,50 +119,10 @@ class TaskMemory { //! requested. On faliure, `nullptr`, with a warning logged. Failures can //! occur, for example, when encountering unmapped or unreadable pages. std::unique_ptr ReadMapped(mach_vm_address_t address, - size_t size); - - //! \brief Reads a `NUL`-terminated C string from the target task into a - //! string in the current task. - //! - //! The length of the string need not be known ahead of time. This method will - //! read contiguous memory until a `NUL` terminator is found. - //! - //! \param[in] address The address, in the target task’s address space, of the - //! string to copy. - //! \param[out] string The string read from the other task. - //! - //! \return `true` on success, with \a string set appropriately. `false` on - //! failure, with a warning logged. Failures can occur, for example, when - //! encountering unmapped or unreadable pages. - //! - //! \sa MappedMemory::ReadCString() - bool ReadCString(mach_vm_address_t address, std::string* string); - - //! \brief Reads a `NUL`-terminated C string from the target task into a - //! string in the current task. - //! - //! \param[in] address The address, in the target task’s address space, of the - //! string to copy. - //! \param[in] size The maximum number of bytes to read. The string is - //! required to be `NUL`-terminated within this many bytes. - //! \param[out] string The string read from the other task. - //! - //! \return `true` on success, with \a string set appropriately. `false` on - //! failure, with a warning logged. Failures can occur, for example, when - //! a `NUL` terminator is not found within \a size bytes, or when - //! encountering unmapped or unreadable pages. - //! - //! \sa MappedMemory::ReadCString() - bool ReadCStringSizeLimited(mach_vm_address_t address, - mach_vm_size_t size, - std::string* string); + size_t size) const; private: - // The common internal implementation shared by the ReadCString*() methods. - bool ReadCStringInternal(mach_vm_address_t address, - bool has_size, - mach_vm_size_t size, - std::string* string); + ssize_t ReadUpTo(VMAddress address, size_t size, void* buffer) const override; task_t task_; // weak InitializationStateDcheck initialized_; diff --git a/util/process/process_memory_native.h b/util/process/process_memory_native.h index e81208ee..79cf1e1d 100644 --- a/util/process/process_memory_native.h +++ b/util/process/process_memory_native.h @@ -20,6 +20,8 @@ #include "util/process/process_memory_linux.h" #elif defined(OS_WIN) #include "util/process/process_memory_win.h" +#elif defined(OS_MACOSX) +#include "util/mach/task_memory.h" #endif namespace crashpad { @@ -31,6 +33,8 @@ using ProcessMemoryNative = ProcessMemoryFuchsia; using ProcessMemoryNative = ProcessMemoryLinux; #elif defined(OS_WIN) using ProcessMemoryNative = ProcessMemoryWin; +#elif defined(OS_MACOSX) +using ProcessMemoryNative = TaskMemory; #else #error Port. #endif diff --git a/util/process/process_memory_test.cc b/util/process/process_memory_test.cc index ca1f4973..a824bf63 100644 --- a/util/process/process_memory_test.cc +++ b/util/process/process_memory_test.cc @@ -29,10 +29,75 @@ #include "util/misc/from_pointer_cast.h" #include "util/process/process_memory_native.h" +#if defined(OS_MACOSX) +#include "test/mac/mach_multiprocess.h" +#endif // defined(OS_MACOSX) + namespace crashpad { namespace test { namespace { +// On macOS the ProcessMemoryTests require accessing the child process' task +// port which requires root or a code signing entitlement. To account for this +// we implement an adaptor class that wraps MachMultiprocess on macOS, because +// it shares the child's task port, and makes it behave like MultiprocessExec. +#if defined(OS_MACOSX) +class MultiprocessAdaptor : public MachMultiprocess { + public: + void SetChildTestMainFunction(const std::string& function_name) { + test_function_ = function_name; + } + + ProcessType ChildProcess() { return ChildTask(); } + + // Helpers to get I/O handles in the child process + static FileHandle OutputHandle() { + CHECK_NE(write_pipe_handle_, -1); + return write_pipe_handle_; + } + + static FileHandle InputHandle() { + CHECK_NE(read_pipe_handle_, -1); + return read_pipe_handle_; + } + + private: + virtual void Parent() = 0; + + void MachMultiprocessParent() override { Parent(); } + + void MachMultiprocessChild() override { + read_pipe_handle_ = ReadPipeHandle(); + write_pipe_handle_ = WritePipeHandle(); + internal::CheckedInvokeMultiprocessChild(test_function_); + } + + std::string test_function_; + + static FileHandle read_pipe_handle_; + static FileHandle write_pipe_handle_; +}; + +FileHandle MultiprocessAdaptor::read_pipe_handle_ = -1; +FileHandle MultiprocessAdaptor::write_pipe_handle_ = -1; +#else +class MultiprocessAdaptor : public MultiprocessExec { + public: + static FileHandle OutputHandle() { + return StdioFileHandle(StdioStream::kStandardOutput); + } + + static FileHandle InputHandle() { + return StdioFileHandle(StdioStream::kStandardInput); + } + + private: + virtual void Parent() = 0; + + void MultiprocessParent() override { Parent(); } +}; +#endif // defined(OS_MACOSX) + void DoChildReadTestSetup(size_t* region_size, std::unique_ptr* region) { *region_size = 4 * base::GetPageSize(); @@ -46,17 +111,17 @@ CRASHPAD_CHILD_TEST_MAIN(ReadTestChild) { size_t region_size; std::unique_ptr region; DoChildReadTestSetup(®ion_size, ®ion); - FileHandle out = StdioFileHandle(StdioStream::kStandardOutput); + FileHandle out = MultiprocessAdaptor::OutputHandle(); CheckedWriteFile(out, ®ion_size, sizeof(region_size)); VMAddress address = FromPointerCast(region.get()); CheckedWriteFile(out, &address, sizeof(address)); - CheckedReadFileAtEOF(StdioFileHandle(StdioStream::kStandardInput)); + CheckedReadFileAtEOF(MultiprocessAdaptor::InputHandle()); return 0; } -class ReadTest : public MultiprocessExec { +class ReadTest : public MultiprocessAdaptor { public: - ReadTest() : MultiprocessExec() { + ReadTest() : MultiprocessAdaptor() { SetChildTestMainFunction("ReadTestChild"); } @@ -72,7 +137,7 @@ class ReadTest : public MultiprocessExec { void RunAgainstChild() { Run(); } private: - void MultiprocessParent() override { + void Parent() override { size_t region_size; VMAddress region; ASSERT_TRUE( @@ -183,23 +248,22 @@ CRASHPAD_CHILD_TEST_MAIN(ReadCStringTestChild) { &const_empty, &const_short, &local_empty, &local_short, &long_string); const auto write_address = [](const char* p) { VMAddress address = FromPointerCast(p); - CheckedWriteFile(StdioFileHandle(StdioStream::kStandardOutput), - &address, - sizeof(address)); + CheckedWriteFile( + MultiprocessAdaptor::OutputHandle(), &address, sizeof(address)); }; write_address(const_empty); write_address(const_short); write_address(local_empty); write_address(local_short); write_address(long_string.c_str()); - CheckedReadFileAtEOF(StdioFileHandle(StdioStream::kStandardInput)); + CheckedReadFileAtEOF(MultiprocessAdaptor::InputHandle()); return 0; } -class ReadCStringTest : public MultiprocessExec { +class ReadCStringTest : public MultiprocessAdaptor { public: ReadCStringTest(bool limit_size) - : MultiprocessExec(), limit_size_(limit_size) { + : MultiprocessAdaptor(), limit_size_(limit_size) { SetChildTestMainFunction("ReadCStringTestChild"); } @@ -221,7 +285,7 @@ class ReadCStringTest : public MultiprocessExec { void RunAgainstChild() { Run(); } private: - void MultiprocessParent() override { + void Parent() override { #define DECLARE_AND_READ_ADDRESS(name) \ VMAddress name; \ ASSERT_TRUE(ReadFileExactly(ReadPipeHandle(), &name, sizeof(name))); @@ -332,24 +396,24 @@ CRASHPAD_CHILD_TEST_MAIN(ReadUnmappedChildMain) { ScopedGuardedPage pages; VMAddress address = reinterpret_cast(pages.Pointer()); DoReadUnmappedChildMainSetup(pages.Pointer()); - FileHandle out = StdioFileHandle(StdioStream::kStandardOutput); + FileHandle out = MultiprocessAdaptor::OutputHandle(); CheckedWriteFile(out, &address, sizeof(address)); - CheckedReadFileAtEOF(StdioFileHandle(StdioStream::kStandardInput)); + CheckedReadFileAtEOF(MultiprocessAdaptor::InputHandle()); return 0; } // This test only supports running against a child process because // ScopedGuardedPage is not thread-safe. -class ReadUnmappedTest : public MultiprocessExec { +class ReadUnmappedTest : public MultiprocessAdaptor { public: - ReadUnmappedTest() : MultiprocessExec() { + ReadUnmappedTest() : MultiprocessAdaptor() { SetChildTestMainFunction("ReadUnmappedChildMain"); } void RunAgainstChild() { Run(); } private: - void MultiprocessParent() override { + void Parent() override { VMAddress address = 0; ASSERT_TRUE(ReadFileExactly(ReadPipeHandle(), &address, sizeof(address))); DoTest(ChildProcess(), address); @@ -450,28 +514,28 @@ CRASHPAD_CHILD_TEST_MAIN(ReadCStringUnmappedChildMain) { ScopedGuardedPage pages; std::vector strings; DoCStringUnmappedTestSetup(pages.Pointer(), &strings); - FileHandle out = StdioFileHandle(StdioStream::kStandardOutput); + FileHandle out = MultiprocessAdaptor::OutputHandle(); strings[0].Write(out); strings[1].Write(out); strings[2].Write(out); strings[3].Write(out); - CheckedReadFileAtEOF(StdioFileHandle(StdioStream::kStandardInput)); + CheckedReadFileAtEOF(MultiprocessAdaptor::InputHandle()); return 0; } // This test only supports running against a child process because // ScopedGuardedPage is not thread-safe. -class ReadCStringUnmappedTest : public MultiprocessExec { +class ReadCStringUnmappedTest : public MultiprocessAdaptor { public: ReadCStringUnmappedTest(bool limit_size) - : MultiprocessExec(), limit_size_(limit_size) { + : MultiprocessAdaptor(), limit_size_(limit_size) { SetChildTestMainFunction("ReadCStringUnmappedChildMain"); } void RunAgainstChild() { Run(); } private: - void MultiprocessParent() override { + void Parent() override { std::vector strings; strings.push_back(StringDataInChildProcess::Read(ReadPipeHandle())); strings.push_back(StringDataInChildProcess::Read(ReadPipeHandle()));