Don't bail if a thread's stack pointer is invalid

Currently, if a thread's stack pointer is not within a valid memory page,
the minidump writing will fail with an error.  This change allows an invalid
stack pointer by simply setting the memory size to zero in the minidump.
The processing code already checks for the size being zero, although it
currently just gives an error (see https://breakpad.appspot.com/413002/).

BUG=google-breakpad:499, chromium-os:34880
TEST=make check, manually ran minidump-2-core and core2md
Review URL: https://breakpad.appspot.com/478002

git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@1065 4c0a9323-5329-0410-9bdc-e9ce6186880e

7 files changed, 137 insertions, 36 deletions

diff --git a/src/client/linux/minidump_writer/linux_core_dumper.cc b/src/client/linux/minidump_writer/linux_core_dumper.cc
index 3e8c92fe..47cc26c0 100644
--- a/src/client/linux/minidump_writer/linux_core_dumper.cc
+++ b/src/client/linux/minidump_writer/linux_core_dumper.cc
@@ -102,9 +102,8 @@ bool LinuxCoreDumper::GetThreadInfoByIndex(size_t index, ThreadInfo* info) {
 #else
 #error "This code hasn't been ported to your platform yet."
 #endif
-
-  return GetStackInfo(&info->stack, &info->stack_len,
-                      reinterpret_cast<uintptr_t>(stack_pointer));
+  info->stack_pointer = reinterpret_cast<uintptr_t>(stack_pointer);
+  return true;
 }
 
 bool LinuxCoreDumper::IsPostMortem() const {
diff --git a/src/client/linux/minidump_writer/linux_core_dumper_unittest.cc b/src/client/linux/minidump_writer/linux_core_dumper_unittest.cc
index d04ef3e1..50b31938 100644
--- a/src/client/linux/minidump_writer/linux_core_dumper_unittest.cc
+++ b/src/client/linux/minidump_writer/linux_core_dumper_unittest.cc
@@ -105,6 +105,9 @@ TEST(LinuxCoreDumperTest, VerifyDumpWithMultipleThreads) {
   for (unsigned i = 0; i < kNumOfThreads; ++i) {
     ThreadInfo info;
     EXPECT_TRUE(dumper.GetThreadInfoByIndex(i, &info));
+    const void* stack;
+    size_t stack_len;
+    EXPECT_TRUE(dumper.GetStackInfo(&stack, &stack_len, info.stack_pointer));
     EXPECT_EQ(getpid(), info.ppid);
   }
 }
diff --git a/src/client/linux/minidump_writer/linux_dumper.h b/src/client/linux/minidump_writer/linux_dumper.h
index 2ed3e14d..a4a3ab5a 100644
--- a/src/client/linux/minidump_writer/linux_dumper.h
+++ b/src/client/linux/minidump_writer/linux_dumper.h
@@ -72,10 +72,7 @@ struct ThreadInfo {
   pid_t tgid;   // thread group id
   pid_t ppid;   // parent process
 
-  // Even on platforms where the stack grows down, the following will point to
-  // the smallest address in the stack.
-  const void* stack;  // pointer to the stack area
-  size_t stack_len;  // length of the stack to copy
+  uintptr_t stack_pointer;  // thread stack pointer
 
 
 #if defined(__i386) || defined(__x86_64)
diff --git a/src/client/linux/minidump_writer/linux_ptrace_dumper.cc b/src/client/linux/minidump_writer/linux_ptrace_dumper.cc
index 3863d79f..45d0f48f 100644
--- a/src/client/linux/minidump_writer/linux_ptrace_dumper.cc
+++ b/src/client/linux/minidump_writer/linux_ptrace_dumper.cc
@@ -218,9 +218,9 @@ bool LinuxPtraceDumper::GetThreadInfoByIndex(size_t index, ThreadInfo* info) {
 #else
 #error "This code hasn't been ported to your platform yet."
 #endif
+  info->stack_pointer = reinterpret_cast<uintptr_t>(stack_pointer);
 
-  return GetStackInfo(&info->stack, &info->stack_len,
-                      (uintptr_t) stack_pointer);
+  return true;
 }
 
 bool LinuxPtraceDumper::IsPostMortem() const {
diff --git a/src/client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc b/src/client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc
index ba3548c5..ee3593cd 100644
--- a/src/client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc
+++ b/src/client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc
@@ -230,6 +230,10 @@ TEST(LinuxPtraceDumperTest, VerifyStackReadWithMultipleThreads) {
   ThreadInfo one_thread;
   for (size_t i = 0; i < dumper.threads().size(); ++i) {
     EXPECT_TRUE(dumper.GetThreadInfoByIndex(i, &one_thread));
+    const void* stack;
+    size_t stack_len;
+    EXPECT_TRUE(dumper.GetStackInfo(&stack, &stack_len,
+        one_thread.stack_pointer));
     // In the helper program, we stored a pointer to the thread id in a
     // specific register. Check that we can recover its value.
 #if defined(__ARM_EABI__)
diff --git a/src/client/linux/minidump_writer/minidump_writer.cc b/src/client/linux/minidump_writer/minidump_writer.cc
index 02f6c3e2..43ad05b6 100644
--- a/src/client/linux/minidump_writer/minidump_writer.cc
+++ b/src/client/linux/minidump_writer/minidump_writer.cc
@@ -628,6 +628,31 @@ class MinidumpWriter {
 #endif
   }
 
+  bool FillThreadStack(MDRawThread* thread, uintptr_t stack_pointer,
+                       uint8_t** stack_copy) {
+    *stack_copy = NULL;
+    const void* stack;
+    size_t stack_len;
+    if (dumper_->GetStackInfo(&stack, &stack_len, stack_pointer)) {
+      UntypedMDRVA memory(&minidump_writer_);
+      if (!memory.Allocate(stack_len))
+        return false;
+      *stack_copy = reinterpret_cast<uint8_t*>(Alloc(stack_len));
+      dumper_->CopyFromProcess(*stack_copy, thread->thread_id, stack,
+                               stack_len);
+      memory.Copy(*stack_copy, stack_len);
+      thread->stack.start_of_memory_range =
+          reinterpret_cast<uintptr_t>(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;
+  }
+
   // Write information about the threads.
   bool WriteThreadListStream(MDRawDirectory* dirent) {
     const unsigned num_threads = dumper_->threads().size();
@@ -645,6 +670,7 @@ class MinidumpWriter {
       MDRawThread thread;
       my_memset(&thread, 0, sizeof(thread));
       thread.thread_id = dumper_->threads()[i];
+
       // We have a different source of information for the crashing thread. If
       // we used the actual state of the thread we would find it running in the
       // signal handler with the alternative stack, which would be deeply
@@ -652,20 +678,9 @@ class MinidumpWriter {
       if (static_cast<pid_t>(thread.thread_id) == GetCrashThread() &&
           ucontext_ &&
           !dumper_->IsPostMortem()) {
-        const void* stack;
-        size_t stack_len;
-        if (!dumper_->GetStackInfo(&stack, &stack_len, GetStackPointer()))
+        uint8_t* stack_copy;
+        if (!FillThreadStack(&thread, GetStackPointer(), &stack_copy))
           return false;
-        UntypedMDRVA memory(&minidump_writer_);
-        if (!memory.Allocate(stack_len))
-          return false;
-        uint8_t* stack_copy = reinterpret_cast<uint8_t*>(Alloc(stack_len));
-        dumper_->CopyFromProcess(stack_copy, thread.thread_id, stack,
-                                 stack_len);
-        memory.Copy(stack_copy, stack_len);
-        thread.stack.start_of_memory_range = (uintptr_t) (stack);
-        thread.stack.memory = memory.location();
-        memory_blocks_.push_back(thread.stack);
 
         // Copy 256 bytes around crashing instruction pointer to minidump.
         const size_t kIPMemorySize = 256;
@@ -715,30 +730,26 @@ class MinidumpWriter {
           return false;
         my_memset(cpu.get(), 0, sizeof(RawContextCPU));
         CPUFillFromUContext(cpu.get(), ucontext_, float_state_);
-        PopSeccompStackFrame(cpu.get(), thread, stack_copy);
+        if (stack_copy)
+          PopSeccompStackFrame(cpu.get(), thread, stack_copy);
         thread.thread_context = cpu.location();
         crashing_thread_context_ = cpu.location();
       } else {
         ThreadInfo info;
         if (!dumper_->GetThreadInfoByIndex(i, &info))
           return false;
-        UntypedMDRVA memory(&minidump_writer_);
-        if (!memory.Allocate(info.stack_len))
+
+        uint8_t* stack_copy;
+        if (!FillThreadStack(&thread, info.stack_pointer, &stack_copy))
           return false;
-        uint8_t* stack_copy = reinterpret_cast<uint8_t*>(Alloc(info.stack_len));
-        dumper_->CopyFromProcess(stack_copy, thread.thread_id, info.stack,
-                                 info.stack_len);
-        memory.Copy(stack_copy, info.stack_len);
-        thread.stack.start_of_memory_range = (uintptr_t)(info.stack);
-        thread.stack.memory = memory.location();
-        memory_blocks_.push_back(thread.stack);
 
         TypedMDRVA<RawContextCPU> cpu(&minidump_writer_);
         if (!cpu.Allocate())
           return false;
         my_memset(cpu.get(), 0, sizeof(RawContextCPU));
         CPUFillFromThreadInfo(cpu.get(), info);
-        PopSeccompStackFrame(cpu.get(), thread, stack_copy);
+        if (stack_copy)
+          PopSeccompStackFrame(cpu.get(), thread, stack_copy);
         thread.thread_context = cpu.location();
         if (dumper_->threads()[i] == GetCrashThread()) {
           crashing_thread_context_ = cpu.location();
@@ -916,9 +927,16 @@ class MinidumpWriter {
 
   bool WriteMemoryListStream(MDRawDirectory* dirent) {
     TypedMDRVA<uint32_t> list(&minidump_writer_);
-    if (!list.AllocateObjectAndArray(memory_blocks_.size(),
-                                     sizeof(MDMemoryDescriptor)))
-      return false;
+    if (memory_blocks_.size()) {
+      if (!list.AllocateObjectAndArray(memory_blocks_.size(),
+                                       sizeof(MDMemoryDescriptor)))
+        return false;
+    } else {
+      // Still create the memory list stream, although it will have zero
+      // memory blocks.
+      if (!list.Allocate())
+        return false;
+    }
 
     dirent->stream_type = MD_MEMORY_LIST_STREAM;
     dirent->location = list.location();
diff --git a/src/client/linux/minidump_writer/minidump_writer_unittest.cc b/src/client/linux/minidump_writer/minidump_writer_unittest.cc
index e0d0fa9c..68bde073 100644
--- a/src/client/linux/minidump_writer/minidump_writer_unittest.cc
+++ b/src/client/linux/minidump_writer/minidump_writer_unittest.cc
@@ -501,3 +501,83 @@ TEST(MinidumpWriterTest, AdditionalMemory) {
   delete[] memory;
   close(fds[1]);
 }
+
+// Test that an invalid thread stack pointer still results in a minidump.
+TEST(MinidumpWriterTest, InvalidStackPointer) {
+  int fds[2];
+  ASSERT_NE(-1, pipe(fds));
+
+  const pid_t child = fork();
+  if (child == 0) {
+    close(fds[1]);
+    char b;
+    HANDLE_EINTR(read(fds[0], &b, sizeof(b)));
+    close(fds[0]);
+    syscall(__NR_exit);
+  }
+  close(fds[0]);
+
+  ExceptionHandler::CrashContext context;
+
+  // This needs a valid context for minidump writing to work, but getting
+  // a useful one from the child is too much work, so just use one from
+  // the parent since the child is just a forked copy anyway.
+  ASSERT_EQ(0, getcontext(&context.context));
+  context.tid = child;
+
+  // Fake the child's stack pointer for its crashing thread.  NOTE: This must
+  // be an invalid memory address for the child process (stack or otherwise).
+#if defined(__i386)
+  // Try 1MB below the current stack.
+  uintptr_t invalid_stack_pointer =
+      reinterpret_cast<uintptr_t>(&context) - 1024*1024;
+  context.context.uc_mcontext.gregs[REG_ESP] = invalid_stack_pointer;
+#elif defined(__x86_64)
+  // Try 1MB below the current stack.
+  uintptr_t invalid_stack_pointer =
+      reinterpret_cast<uintptr_t>(&context) - 1024*1024;
+  context.context.uc_mcontext.gregs[REG_RSP] = invalid_stack_pointer;
+#elif defined(__ARM_EABI__)
+  // Try 1MB below the current stack.
+  uintptr_t invalid_stack_pointer =
+      reinterpret_cast<uintptr_t>(&context) - 1024*1024;
+  context.context.uc_mcontext.arm_sp = invalid_stack_pointer;
+#else
+# error "This code has not been ported to your platform yet."
+#endif
+
+  AutoTempDir temp_dir;
+  string templ = temp_dir.path() + "/minidump-writer-unittest";
+  // NOTE: In previous versions of Breakpad, WriteMinidump() would fail if
+  // presented with an invalid stack pointer.
+  ASSERT_TRUE(WriteMinidump(templ.c_str(), child, &context, sizeof(context)));
+
+  // Read the minidump. Ensure that the memory region is present
+  Minidump minidump(templ.c_str());
+  ASSERT_TRUE(minidump.Read());
+
+  // TODO(ted.mielczarek,mkrebs): Enable this part of the test once
+  // https://breakpad.appspot.com/413002/ is committed.
+#if 0
+  // Make sure there's a thread without a stack.  NOTE: It's okay if
+  // GetThreadList() shows the error: "ERROR: MinidumpThread has a memory
+  // region problem".
+  MinidumpThreadList* dump_thread_list = minidump.GetThreadList();
+  ASSERT_TRUE(dump_thread_list);
+  bool found_empty_stack = false;
+  for (int i = 0; i < dump_thread_list->thread_count(); i++) {
+    MinidumpThread* thread = dump_thread_list->GetThreadAtIndex(i);
+    ASSERT_TRUE(thread->thread() != NULL);
+    // When the stack size is zero bytes, GetMemory() returns NULL.
+    if (thread->GetMemory() == NULL) {
+      found_empty_stack = true;
+      break;
+    }
+  }
+  // NOTE: If you fail this, first make sure that "invalid_stack_pointer"
+  // above is indeed set to an invalid address.
+  ASSERT_TRUE(found_empty_stack);
+#endif
+
+  close(fds[1]);
+}