From 97483928ccc8b979466f504e08ecb67ed0f6f711 Mon Sep 17 00:00:00 2001 From: Tobias Sargeant Date: Tue, 21 Mar 2017 22:43:34 +0000 Subject: Don't generate minidump if crash thread doesn't ref principal mapping. If the crashing thread doesn't reference the principal mapping we can assume that not only is that thread uninteresting from a debugging perspective, the whole crash is uninteresting. In that case we should not generate a minidump at all. BUG=703599 Change-Id: Ia25bbb8adb79d04dcaf3992c3d2474f3b9b1f796 Reviewed-on: https://chromium-review.googlesource.com/457338 Reviewed-by: Robert Sesek --- .../linux/minidump_writer/minidump_writer.cc | 56 ++++++++++++++++--- .../minidump_writer/minidump_writer_unittest.cc | 62 +++++++++++++++++++--- 2 files changed, 105 insertions(+), 13 deletions(-) (limited to 'src') diff --git a/src/client/linux/minidump_writer/minidump_writer.cc b/src/client/linux/minidump_writer/minidump_writer.cc index 5c2a339e..d11ba6e5 100644 --- a/src/client/linux/minidump_writer/minidump_writer.cc +++ b/src/client/linux/minidump_writer/minidump_writer.cc @@ -147,6 +147,7 @@ class MinidumpWriter { skip_stacks_if_mapping_unreferenced_( skip_stacks_if_mapping_unreferenced), principal_mapping_address_(principal_mapping_address), + principal_mapping_(nullptr), sanitize_stacks_(sanitize_stacks) { // Assert there should be either a valid fd or a valid path, not both. assert(fd_ != -1 || minidump_path); @@ -157,12 +158,22 @@ class MinidumpWriter { if (!dumper_->Init()) return false; + if (!dumper_->ThreadsSuspend() || !dumper_->LateInit()) + return false; + + if (skip_stacks_if_mapping_unreferenced_) { + principal_mapping_ = + dumper_->FindMappingNoBias(principal_mapping_address_); + if (!CrashingThreadReferencesPrincipalMapping()) + return false; + } + if (fd_ != -1) minidump_writer_.SetFile(fd_); else if (!minidump_writer_.Open(path_)) return false; - return dumper_->ThreadsSuspend() && dumper_->LateInit(); + return true; } ~MinidumpWriter() { @@ -173,6 +184,38 @@ class MinidumpWriter { dumper_->ThreadsResume(); } + bool CrashingThreadReferencesPrincipalMapping() { + if (!ucontext_ || !principal_mapping_) + return false; + + const uintptr_t low_addr = + principal_mapping_->system_mapping_info.start_addr; + const uintptr_t high_addr = + principal_mapping_->system_mapping_info.end_addr; + + const uintptr_t stack_pointer = UContextReader::GetStackPointer(ucontext_); + const uintptr_t pc = UContextReader::GetInstructionPointer(ucontext_); + + if (pc >= low_addr && pc < high_addr) + return true; + + uint8_t* stack_copy; + const void* stack; + size_t stack_len; + + if (!dumper_->GetStackInfo(&stack, &stack_len, stack_pointer)) + return false; + + stack_copy = reinterpret_cast(Alloc(stack_len)); + dumper_->CopyFromProcess(stack_copy, GetCrashThread(), stack, stack_len); + + uintptr_t stack_pointer_offset = + stack_pointer - reinterpret_cast(stack); + + return dumper_->StackHasPointerToMapping( + stack_copy, stack_len, stack_pointer_offset, *principal_mapping_); + } + bool Dump() { // A minidump file contains a number of tagged streams. This is the number // of stream which we write. @@ -302,17 +345,15 @@ class MinidumpWriter { 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) { + 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 low_addr = principal_mapping_->system_mapping_info.start_addr; + uintptr_t high_addr = principal_mapping_->system_mapping_info.end_addr; if ((pc < low_addr || pc > high_addr) && !dumper_->StackHasPointerToMapping(*stack_copy, stack_len, stack_pointer_offset, - *principal_mapping)) { + *principal_mapping_)) { return true; } } @@ -1303,6 +1344,7 @@ class MinidumpWriter { // mapping containing principal_mapping_address_. bool skip_stacks_if_mapping_unreferenced_; uintptr_t principal_mapping_address_; + const MappingInfo* principal_mapping_; // If true, apply stack sanitization to stored stack data. bool sanitize_stacks_; }; diff --git a/src/client/linux/minidump_writer/minidump_writer_unittest.cc b/src/client/linux/minidump_writer/minidump_writer_unittest.cc index f3b78a22..8d7f232c 100644 --- a/src/client/linux/minidump_writer/minidump_writer_unittest.cc +++ b/src/client/linux/minidump_writer/minidump_writer_unittest.cc @@ -215,8 +215,9 @@ TEST(MinidumpWriterTest, MappingInfo) { close(fds[1]); } -// Test that stacks can be skipped while writing minidumps. -TEST(MinidumpWriterTest, StacksSkippedIfRequested) { +// Test that minidumping is skipped while writing minidumps if principal mapping +// is not referenced. +TEST(MinidumpWriterTest, MinidumpSkippedIfRequested) { int fds[2]; ASSERT_NE(-1, pipe(fds)); @@ -239,20 +240,69 @@ TEST(MinidumpWriterTest, StacksSkippedIfRequested) { 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), + // WriteMinidump to not write a minidump. + ASSERT_FALSE(WriteMinidump(templ.c_str(), child, &context, sizeof(context), true, static_cast(0x0102030405060708ull), false)); + close(fds[1]); +} + +// Test that minidumping is skipped while writing minidumps if principal mapping +// is not referenced. +TEST(MinidumpWriterTest, MinidumpStacksSkippedIfRequested) { + int fds[2]; + ASSERT_NE(-1, pipe(fds)); + + const pid_t child = fork(); + if (child == 0) { + close(fds[1]); + + // Create a thread that does not return, and only references libc (not the + // current executable). This thread should not be captured in the minidump. + pthread_t thread; + pthread_attr_t thread_attributes; + pthread_attr_init(&thread_attributes); + pthread_attr_setdetachstate(&thread_attributes, PTHREAD_CREATE_DETACHED); + sigset_t sigset; + sigemptyset(&sigset); + pthread_create(&thread, &thread_attributes, + reinterpret_cast(&sigsuspend), &sigset); + + char b; + IGNORE_RET(HANDLE_EINTR(read(fds[0], &b, sizeof(b)))); + close(fds[0]); + syscall(__NR_exit); + } + close(fds[0]); - // Read the minidump. And ensure that no thread memory was dumped. + 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, + reinterpret_cast(google_breakpad::WriteFile), false)); + + // Read the minidump. And ensure that thread memory was dumped only for the + // main thread. Minidump minidump(templ); ASSERT_TRUE(minidump.Read()); MinidumpThreadList *threads = minidump.GetThreadList(); + int threads_with_stacks = 0; for (unsigned int i = 0; i < threads->thread_count(); ++i) { MinidumpThread *thread = threads->GetThreadAtIndex(i); - ASSERT_TRUE(thread->GetMemory() == nullptr); + if (thread->GetMemory()) { + ++threads_with_stacks; + } } + ASSERT_EQUAL(1, threads_with_stacks); close(fds[1]); } -- cgit v1.2.1