From 049a1532e57b2631b6f228f078759323a13674d0 Mon Sep 17 00:00:00 2001 From: Tobias Sargeant Date: Thu, 2 Feb 2017 17:11:13 +0000 Subject: [PATCH] Wire up stack sanitization and skipping to WriteMinidump This makes the parameters stored in the MinidumpDescriptor structure functional for minidumps, analogously to how they are applied to microdumps. BUG=664460 Change-Id: I7578e7a1638cea8f0445b18d4bbdaf5e0a32d808 Reviewed-on: https://chromium-review.googlesource.com/435380 Reviewed-by: Robert Sesek --- src/client/linux/handler/exception_handler.cc | 21 ++- .../microdump_writer/microdump_writer.cc | 4 +- .../linux/minidump_writer/minidump_writer.cc | 131 ++++++++++++++---- .../linux/minidump_writer/minidump_writer.h | 30 +++- .../minidump_writer_unittest.cc | 86 +++++++++++- 5 files changed, 232 insertions(+), 40 deletions(-) diff --git a/src/client/linux/handler/exception_handler.cc b/src/client/linux/handler/exception_handler.cc index dd3cbc67..d372a10c 100644 --- a/src/client/linux/handler/exception_handler.cc +++ b/src/client/linux/handler/exception_handler.cc @@ -586,15 +586,20 @@ void ExceptionHandler::WaitForContinueSignal() { // Runs on the cloned process. bool ExceptionHandler::DoDump(pid_t crashing_process, const void* context, size_t context_size) { + const bool may_skip_dump = + minidump_descriptor_.skip_dump_if_principal_mapping_not_referenced(); + const uintptr_t principal_mapping_address = + minidump_descriptor_.address_within_principal_mapping(); + const bool sanitize_stacks = minidump_descriptor_.sanitize_stacks(); if (minidump_descriptor_.IsMicrodumpOnConsole()) { return google_breakpad::WriteMicrodump( crashing_process, context, context_size, mapping_list_, - minidump_descriptor_.skip_dump_if_principal_mapping_not_referenced(), - minidump_descriptor_.address_within_principal_mapping(), - minidump_descriptor_.sanitize_stacks(), + may_skip_dump, + principal_mapping_address, + sanitize_stacks, *minidump_descriptor_.microdump_extra_info()); } if (minidump_descriptor_.IsFD()) { @@ -604,7 +609,10 @@ bool ExceptionHandler::DoDump(pid_t crashing_process, const void* context, context, context_size, mapping_list_, - app_memory_list_); + app_memory_list_, + may_skip_dump, + principal_mapping_address, + sanitize_stacks); } return google_breakpad::WriteMinidump(minidump_descriptor_.path(), minidump_descriptor_.size_limit(), @@ -612,7 +620,10 @@ bool ExceptionHandler::DoDump(pid_t crashing_process, const void* context, context, context_size, mapping_list_, - app_memory_list_); + app_memory_list_, + may_skip_dump, + principal_mapping_address, + sanitize_stacks); } // static diff --git a/src/client/linux/microdump_writer/microdump_writer.cc b/src/client/linux/microdump_writer/microdump_writer.cc index 341d7f5c..065ca0b7 100644 --- a/src/client/linux/microdump_writer/microdump_writer.cc +++ b/src/client/linux/microdump_writer/microdump_writer.cc @@ -265,8 +265,8 @@ class MicrodumpWriter { dumper_->FindMappingNoBias(address_within_principal_mapping_); if (!principal_mapping) return CAPTURE_UNINTERESTING; - uintptr_t low_addr = principal_mapping->start_addr; - uintptr_t high_addr = principal_mapping->start_addr + principal_mapping->size; + uintptr_t low_addr = principal_mapping->system_mapping_info.start_addr; + uintptr_t high_addr = principal_mapping->system_mapping_info.end_addr; uintptr_t pc = UContextReader::GetInstructionPointer(ucontext_); if (low_addr <= pc && pc <= high_addr) return CAPTURE_OK; diff --git a/src/client/linux/minidump_writer/minidump_writer.cc b/src/client/linux/minidump_writer/minidump_writer.cc index 86009b9f..cdd0a1cc 100644 --- a/src/client/linux/minidump_writer/minidump_writer.cc +++ b/src/client/linux/minidump_writer/minidump_writer.cc @@ -129,6 +129,9 @@ class MinidumpWriter { const ExceptionHandler::CrashContext* context, const MappingList& mappings, const AppMemoryList& appmem, + bool skip_stacks_if_mapping_unreferenced, + uintptr_t principal_mapping_address, + bool sanitize_stacks, LinuxDumper* dumper) : fd_(minidump_fd), path_(minidump_path), @@ -140,7 +143,11 @@ class MinidumpWriter { minidump_size_limit_(-1), memory_blocks_(dumper_->allocator()), mapping_list_(mappings), - app_memory_list_(appmem) { + app_memory_list_(appmem), + skip_stacks_if_mapping_unreferenced_( + skip_stacks_if_mapping_unreferenced), + principal_mapping_address_(principal_mapping_address), + sanitize_stacks_(sanitize_stacks) { // Assert there should be either a valid fd or a valid path, not both. assert(fd_ != -1 || minidump_path); assert(fd_ == -1 || !minidump_path); @@ -270,8 +277,12 @@ class MinidumpWriter { *stack_copy = NULL; const void* stack; size_t stack_len; + + thread->stack.start_of_memory_range = stack_pointer; + thread->stack.memory.data_size = 0; + thread->stack.memory.rva = minidump_writer_.position(); + if (dumper_->GetStackInfo(&stack, &stack_len, stack_pointer)) { - UntypedMDRVA memory(&minidump_writer_); if (max_stack_len >= 0 && stack_len > static_cast(max_stack_len)) { stack_len = max_stack_len; @@ -284,20 +295,41 @@ class MinidumpWriter { } stack = reinterpret_cast(int_stack); } - if (!memory.Allocate(stack_len)) - return false; *stack_copy = reinterpret_cast(Alloc(stack_len)); dumper_->CopyFromProcess(*stack_copy, thread->thread_id, stack, stack_len); + + uintptr_t stack_pointer_offset = + stack_pointer - reinterpret_cast(stack); + if (skip_stacks_if_mapping_unreferenced_) { + const MappingInfo* principal_mapping = + dumper_->FindMappingNoBias(principal_mapping_address_); + if (!principal_mapping) { + return true; + } + uintptr_t low_addr = principal_mapping->system_mapping_info.start_addr; + uintptr_t high_addr = principal_mapping->system_mapping_info.end_addr; + uintptr_t pc = UContextReader::GetInstructionPointer(ucontext_); + if ((pc < low_addr || pc > high_addr) && + !dumper_->StackHasPointerToMapping(*stack_copy, stack_len, + stack_pointer_offset, + *principal_mapping)) { + return true; + } + } + + if (sanitize_stacks_) { + dumper_->SanitizeStackCopy(*stack_copy, stack_len, stack_pointer, + stack_pointer_offset); + } + + UntypedMDRVA memory(&minidump_writer_); + if (!memory.Allocate(stack_len)) + return false; memory.Copy(*stack_copy, stack_len); - thread->stack.start_of_memory_range = - reinterpret_cast(stack); + thread->stack.start_of_memory_range = reinterpret_cast(stack); thread->stack.memory = memory.location(); memory_blocks_.push_back(thread->stack); - } else { - thread->stack.start_of_memory_range = stack_pointer; - thread->stack.memory.data_size = 0; - thread->stack.memory.rva = minidump_writer_.position(); } return true; } @@ -1265,6 +1297,12 @@ class MinidumpWriter { // Additional memory regions to be included in the dump, // provided by the caller. const AppMemoryList& app_memory_list_; + // If set, skip recording any threads that do not reference the + // mapping containing principal_mapping_address_. + bool skip_stacks_if_mapping_unreferenced_; + uintptr_t principal_mapping_address_; + // If true, apply stack sanitization to stored stack data. + bool sanitize_stacks_; }; @@ -1274,7 +1312,10 @@ bool WriteMinidumpImpl(const char* minidump_path, pid_t crashing_process, const void* blob, size_t blob_size, const MappingList& mappings, - const AppMemoryList& appmem) { + const AppMemoryList& appmem, + bool skip_stacks_if_mapping_unreferenced, + uintptr_t principal_mapping_address, + bool sanitize_stacks) { LinuxPtraceDumper dumper(crashing_process); const ExceptionHandler::CrashContext* context = NULL; if (blob) { @@ -1287,7 +1328,8 @@ bool WriteMinidumpImpl(const char* minidump_path, dumper.set_crash_thread(context->tid); } MinidumpWriter writer(minidump_path, minidump_fd, context, mappings, - appmem, &dumper); + appmem, skip_stacks_if_mapping_unreferenced, + principal_mapping_address, sanitize_stacks, &dumper); // Set desired limit for file size of minidump (-1 means no limit). writer.set_minidump_size_limit(minidump_size_limit); if (!writer.Init()) @@ -1300,17 +1342,29 @@ bool WriteMinidumpImpl(const char* minidump_path, namespace google_breakpad { bool WriteMinidump(const char* minidump_path, pid_t crashing_process, - const void* blob, size_t blob_size) { + const void* blob, size_t blob_size, + bool skip_stacks_if_mapping_unreferenced, + uintptr_t principal_mapping_address, + bool sanitize_stacks) { return WriteMinidumpImpl(minidump_path, -1, -1, crashing_process, blob, blob_size, - MappingList(), AppMemoryList()); + MappingList(), AppMemoryList(), + skip_stacks_if_mapping_unreferenced, + principal_mapping_address, + sanitize_stacks); } bool WriteMinidump(int minidump_fd, pid_t crashing_process, - const void* blob, size_t blob_size) { + const void* blob, size_t blob_size, + bool skip_stacks_if_mapping_unreferenced, + uintptr_t principal_mapping_address, + bool sanitize_stacks) { return WriteMinidumpImpl(NULL, minidump_fd, -1, crashing_process, blob, blob_size, - MappingList(), AppMemoryList()); + MappingList(), AppMemoryList(), + skip_stacks_if_mapping_unreferenced, + principal_mapping_address, + sanitize_stacks); } bool WriteMinidump(const char* minidump_path, pid_t process, @@ -1320,7 +1374,7 @@ bool WriteMinidump(const char* minidump_path, pid_t process, dumper.set_crash_signal(MD_EXCEPTION_CODE_LIN_DUMP_REQUESTED); dumper.set_crash_thread(process_blamed_thread); MinidumpWriter writer(minidump_path, -1, NULL, MappingList(), - AppMemoryList(), &dumper); + AppMemoryList(), false, 0, false, &dumper); if (!writer.Init()) return false; return writer.Dump(); @@ -1329,46 +1383,71 @@ bool WriteMinidump(const char* minidump_path, pid_t process, bool WriteMinidump(const char* minidump_path, pid_t crashing_process, const void* blob, size_t blob_size, const MappingList& mappings, - const AppMemoryList& appmem) { + const AppMemoryList& appmem, + bool skip_stacks_if_mapping_unreferenced, + uintptr_t principal_mapping_address, + bool sanitize_stacks) { return WriteMinidumpImpl(minidump_path, -1, -1, crashing_process, blob, blob_size, - mappings, appmem); + mappings, appmem, + skip_stacks_if_mapping_unreferenced, + principal_mapping_address, + sanitize_stacks); } bool WriteMinidump(int minidump_fd, pid_t crashing_process, const void* blob, size_t blob_size, const MappingList& mappings, - const AppMemoryList& appmem) { + const AppMemoryList& appmem, + bool skip_stacks_if_mapping_unreferenced, + uintptr_t principal_mapping_address, + bool sanitize_stacks) { return WriteMinidumpImpl(NULL, minidump_fd, -1, crashing_process, blob, blob_size, - mappings, appmem); + mappings, appmem, + skip_stacks_if_mapping_unreferenced, + principal_mapping_address, + sanitize_stacks); } bool WriteMinidump(const char* minidump_path, off_t minidump_size_limit, pid_t crashing_process, const void* blob, size_t blob_size, const MappingList& mappings, - const AppMemoryList& appmem) { + const AppMemoryList& appmem, + bool skip_stacks_if_mapping_unreferenced, + uintptr_t principal_mapping_address, + bool sanitize_stacks) { return WriteMinidumpImpl(minidump_path, -1, minidump_size_limit, crashing_process, blob, blob_size, - mappings, appmem); + mappings, appmem, + skip_stacks_if_mapping_unreferenced, + principal_mapping_address, + sanitize_stacks); } bool WriteMinidump(int minidump_fd, off_t minidump_size_limit, pid_t crashing_process, const void* blob, size_t blob_size, const MappingList& mappings, - const AppMemoryList& appmem) { + const AppMemoryList& appmem, + bool skip_stacks_if_mapping_unreferenced, + uintptr_t principal_mapping_address, + bool sanitize_stacks) { return WriteMinidumpImpl(NULL, minidump_fd, minidump_size_limit, crashing_process, blob, blob_size, - mappings, appmem); + mappings, appmem, + skip_stacks_if_mapping_unreferenced, + principal_mapping_address, + sanitize_stacks); } bool WriteMinidump(const char* filename, const MappingList& mappings, const AppMemoryList& appmem, LinuxDumper* dumper) { - MinidumpWriter writer(filename, -1, NULL, mappings, appmem, dumper); + MinidumpWriter writer(filename, -1, NULL, mappings, appmem, + false, 0, false, dumper); if (!writer.Init()) return false; return writer.Dump(); diff --git a/src/client/linux/minidump_writer/minidump_writer.h b/src/client/linux/minidump_writer/minidump_writer.h index d13fb120..d1dc3312 100644 --- a/src/client/linux/minidump_writer/minidump_writer.h +++ b/src/client/linux/minidump_writer/minidump_writer.h @@ -78,10 +78,16 @@ typedef std::list AppMemoryList; // // Returns true iff successful. bool WriteMinidump(const char* minidump_path, pid_t crashing_process, - const void* blob, size_t blob_size); + const void* blob, size_t blob_size, + bool skip_stacks_if_mapping_unreferenced = false, + uintptr_t principal_mapping_address = 0, + bool sanitize_stacks = false); // Same as above but takes an open file descriptor instead of a path. bool WriteMinidump(int minidump_fd, pid_t crashing_process, - const void* blob, size_t blob_size); + const void* blob, size_t blob_size, + bool skip_stacks_if_mapping_unreferenced = false, + uintptr_t principal_mapping_address = 0, + bool sanitize_stacks = false); // Alternate form of WriteMinidump() that works with processes that // are not expected to have crashed. If |process_blamed_thread| is @@ -96,23 +102,35 @@ bool WriteMinidump(const char* minidump_path, pid_t process, bool WriteMinidump(const char* minidump_path, pid_t crashing_process, const void* blob, size_t blob_size, const MappingList& mappings, - const AppMemoryList& appdata); + const AppMemoryList& appdata, + bool skip_stacks_if_mapping_unreferenced = false, + uintptr_t principal_mapping_address = 0, + bool sanitize_stacks = false); bool WriteMinidump(int minidump_fd, pid_t crashing_process, const void* blob, size_t blob_size, const MappingList& mappings, - const AppMemoryList& appdata); + const AppMemoryList& appdata, + bool skip_stacks_if_mapping_unreferenced = false, + uintptr_t principal_mapping_address = 0, + bool sanitize_stacks = false); // These overloads also allow passing a file size limit for the minidump. bool WriteMinidump(const char* minidump_path, off_t minidump_size_limit, pid_t crashing_process, const void* blob, size_t blob_size, const MappingList& mappings, - const AppMemoryList& appdata); + const AppMemoryList& appdata, + bool skip_stacks_if_mapping_unreferenced = false, + uintptr_t principal_mapping_address = 0, + bool sanitize_stacks = false); bool WriteMinidump(int minidump_fd, off_t minidump_size_limit, pid_t crashing_process, const void* blob, size_t blob_size, const MappingList& mappings, - const AppMemoryList& appdata); + const AppMemoryList& appdata, + bool skip_stacks_if_mapping_unreferenced = false, + uintptr_t principal_mapping_address = 0, + bool sanitize_stacks = false); bool WriteMinidump(const char* filename, const MappingList& mappings, diff --git a/src/client/linux/minidump_writer/minidump_writer_unittest.cc b/src/client/linux/minidump_writer/minidump_writer_unittest.cc index 2e4749e7..b9d8cd0d 100644 --- a/src/client/linux/minidump_writer/minidump_writer_unittest.cc +++ b/src/client/linux/minidump_writer/minidump_writer_unittest.cc @@ -179,7 +179,7 @@ TEST(MinidumpWriterTest, MappingInfo) { memcpy(mapping.second, kModuleGUID, sizeof(MDGUID)); mappings.push_back(mapping); ASSERT_TRUE(WriteMinidump(templ.c_str(), child, &context, sizeof(context), - mappings, memory_list)); + mappings, memory_list, false, 0, false)); // Read the minidump. Load the module list, and ensure that // the mmap'ed |memory| is listed with the given module name @@ -215,6 +215,90 @@ TEST(MinidumpWriterTest, MappingInfo) { close(fds[1]); } +// Test that stacks can be skipped while writing minidumps. +TEST(MinidumpWriterTest, StacksSkippedIfRequested) { + int fds[2]; + ASSERT_NE(-1, pipe(fds)); + + const pid_t child = fork(); + if (child == 0) { + close(fds[1]); + char b; + IGNORE_RET(HANDLE_EINTR(read(fds[0], &b, sizeof(b)))); + close(fds[0]); + syscall(__NR_exit); + } + close(fds[0]); + + ExceptionHandler::CrashContext context; + memset(&context, 0, sizeof(context)); + ASSERT_EQ(0, getcontext(&context.context)); + context.tid = child; + + AutoTempDir temp_dir; + string templ = temp_dir.path() + kMDWriterUnitTestFileName; + + // pass an invalid principal mapping address, which will force + // WriteMinidump to not dump any thread stacks. + ASSERT_TRUE(WriteMinidump(templ.c_str(), child, &context, sizeof(context), + true, 0x0102030405060708, false)); + + // Read the minidump. And ensure that no thread memory was dumped. + Minidump minidump(templ); + ASSERT_TRUE(minidump.Read()); + + MinidumpThreadList *threads = minidump.GetThreadList(); + for (unsigned int i = 0; i < threads->thread_count(); ++i) { + MinidumpThread *thread = threads->GetThreadAtIndex(i); + ASSERT_TRUE(thread->GetMemory() == nullptr); + } + close(fds[1]); +} + +// Test that stacks can be sanitized while writing minidumps. +TEST(MinidumpWriterTest, StacksAreSanitizedIfRequested) { + int fds[2]; + ASSERT_NE(-1, pipe(fds)); + + const pid_t child = fork(); + if (child == 0) { + close(fds[1]); + char b; + IGNORE_RET(HANDLE_EINTR(read(fds[0], &b, sizeof(b)))); + close(fds[0]); + syscall(__NR_exit); + } + close(fds[0]); + + ExceptionHandler::CrashContext context; + memset(&context, 0, sizeof(context)); + ASSERT_EQ(0, getcontext(&context.context)); + context.tid = child; + + AutoTempDir temp_dir; + string templ = temp_dir.path() + kMDWriterUnitTestFileName; + // pass an invalid principal mapping address, which will force + // WriteMinidump to not dump any thread stacks. + ASSERT_TRUE(WriteMinidump(templ.c_str(), child, &context, sizeof(context), + false, 0, true)); + + // Read the minidump. And ensure that thread memory contains a defaced value. + Minidump minidump(templ); + ASSERT_TRUE(minidump.Read()); + + const uintptr_t defaced = 0X0DEFACED0DEFACEDull; + MinidumpThreadList *threads = minidump.GetThreadList(); + for (unsigned int i = 0; i < threads->thread_count(); ++i) { + MinidumpThread *thread = threads->GetThreadAtIndex(i); + MinidumpMemoryRegion *mem = thread->GetMemory(); + ASSERT_TRUE(mem != nullptr); + uint32_t sz = mem->GetSize(); + const uint8_t *data = mem->GetMemory(); + ASSERT_TRUE(memmem(data, sz, &defaced, sizeof(defaced)) != nullptr); + } + close(fds[1]); +} + // Test that a binary with a longer-than-usual build id note // makes its way all the way through to the minidump unscathed. // The linux_client_unittest is linked with an explicit --build-id