Merge r1065: 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
Original Review URL: https://breakpad.appspot.com/478002

* To make this build, there is an additional #include <ucontext.h>.
Review URL: https://breakpad.appspot.com/485002

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

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 {

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);
   }
 }

src/client/linux/minidump_writer/linux_dumper.h

@@ -69,10 +69,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)

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 {

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__)

src/client/linux/minidump_writer/minidump_writer.cc

@@ -62,9 +62,6 @@
 
 #include <algorithm>
 
-#include "client/minidump_file_writer.h"
-#include "google_breakpad/common/minidump_format.h"
-
 #include "client/linux/handler/exception_handler.h"
 #include "client/linux/minidump_writer/line_reader.h"
 #include "client/linux/minidump_writer/linux_dumper.h"
@@ -658,6 +655,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();
@@ -675,26 +697,16 @@ 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
       // unhelpful.
       if (static_cast<pid_t>(thread.thread_id) == GetCrashThread() &&
           !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;
@@ -744,30 +756,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()) {
           assert(dumper_->IsPostMortem());
@@ -916,9 +924,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();
@@ -1035,7 +1050,7 @@ class MinidumpWriter {
     debug.get()->ldbase = (void*)debug_entry.r_ldbase;
     debug.get()->dynamic = (void*)&_DYNAMIC;
 
-    char *dso_debug_data = new char[dynamic_length];
+    char* dso_debug_data = new char[dynamic_length];
     dumper_->CopyFromProcess(dso_debug_data, GetCrashThread(), &_DYNAMIC,
                              dynamic_length);
     debug.CopyIndexAfterObject(0, dso_debug_data, dynamic_length);
@@ -1150,8 +1165,14 @@ class MinidumpWriter {
             if (space_ptr != value)
               continue;
 
+            // skip past the colon and all the spaces that follow
+            do {
+              value++;
+            } while (my_isspace(*value));
+
             uintptr_t val;
-            my_read_decimal_ptr(&val, ++value);
+            if (my_read_decimal_ptr(&val, value) == value)
+              continue;
             entry->value = static_cast<int>(val);
             entry->found = true;
           }
@@ -1163,7 +1184,7 @@ class MinidumpWriter {
           if (!value)
             goto popline;
 
-          // skip ':" and all the spaces that follows
+          // skip past the colon and all the spaces that follow
           do {
             value++;
           } while (my_isspace(*value));

src/client/linux/minidump_writer/minidump_writer_unittest.cc

@@ -32,6 +32,7 @@
 #include <sys/stat.h>
 #include <sys/syscall.h>
 #include <sys/types.h>
+#include <ucontext.h>
 #include <unistd.h>
 
 #include <string>
@@ -411,3 +412,83 @@ TEST(MinidumpWriterTest, DeletedBinary) {
   module_identifier += "0";
   EXPECT_EQ(module_identifier, module->debug_identifier());
 }
+
+// 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]);
+}