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 <rsesek@chromium.org>

2 files changed, 105 insertions, 13 deletions

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<uint8_t*>(Alloc(stack_len));
+    dumper_->CopyFromProcess(stack_copy, GetCrashThread(), stack, stack_len);
+
+    uintptr_t stack_pointer_offset =
+        stack_pointer - reinterpret_cast<uintptr_t>(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<uintptr_t>(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<uintptr_t>(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<void* (*)(void*)>(&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<uintptr_t>(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]);
 }