This change is addressing a particularly nasty issue where the stackwalker

doesn't see the correct thread stack memory.  Instead, it loads garbage
(from offset 0 of the minidump file - well that's not garbage, but it is
not the stack memory region either) and attempts to walk it.  A typical
symptom of this issue is when you get a single stack frame after
processing - the context frame - for which you don't need stack memory.

This issue is caused by an invalid RVA in the memory descriptor stored
inside the MINIDUMP_THREAD structure for the thread.  Luckily, the
invalid RVA is 0, and the start_of_memory_region appears to be correct,
so this issue can be easily detected and the correct memory region can be
loaded using an RVA specified in the MinidumpMemoryList.

I couldn't find a reasonable description on MSDN regarding
MINIDUMP_MEMORY_DESCRIPTOR.MINIDUMP_LOCATION_DESCRIPTOR having RVA of 0
except maybe for full dumps where the 64-bit version of the structure
(MINIDUMP_MEMORY_DESCRIPTOR64) is used and it has no RVA at all.  It has
a 64-bit DataSize which if interpreted as the 32-bit structure will very
likely result in 0 for the RVA:
  http://msdn.microsoft.com/en-us/library/windows/desktop/ms680384(v=vs.85).aspx 

Anyways, the dump that I looked at was not a full dump so 0 for RVA is a
bit puzzling (at least easily detectable):
...
Microsoft (R) Windows Debugger Version 6.2.9200.20512 X86
Copyright (c) Microsoft Corporation. All rights reserved.
...
User Mini Dump File: Only registers, stack and portions of memory are available
...
MINIDUMP_HEADER:
Version         A793 (62F0)
NumberOfStreams 11
Flags           160
                0020 MiniDumpWithUnloadedModules
                0040 MiniDumpWithIndirectlyReferencedMemory
                0100 MiniDumpWithProcessThreadData
Review URL: https://breakpad.appspot.com/606002

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

4 files changed, 76 insertions, 14 deletions

diff --git a/src/google_breakpad/processor/minidump.h b/src/google_breakpad/processor/minidump.h
index 8b2d0fe8..16b9d856 100644
--- a/src/google_breakpad/processor/minidump.h
+++ b/src/google_breakpad/processor/minidump.h
@@ -326,6 +326,11 @@ class MinidumpThread : public MinidumpObject {
   // Print a human-readable representation of the object to stdout.
   void Print();
 
+  // Returns the start address of the thread stack memory region.  Returns 0 if
+  // MinidumpThread is invalid.  Note that this method can be called even when
+  // the thread memory cannot be read and GetMemory returns NULL.
+  virtual uint64_t GetStartOfStackMemoryRange() const;
+
  protected:
   explicit MinidumpThread(Minidump* minidump);
 
@@ -586,13 +591,14 @@ class MinidumpMemoryList : public MinidumpStream {
 
   // Random access to memory regions.  Returns the region encompassing
   // the address identified by address.
-  MinidumpMemoryRegion* GetMemoryRegionForAddress(uint64_t address);
+  virtual MinidumpMemoryRegion* GetMemoryRegionForAddress(uint64_t address);
 
   // Print a human-readable representation of the object to stdout.
   void Print();
 
  private:
   friend class Minidump;
+  friend class MockMinidumpMemoryList;
 
   typedef vector<MDMemoryDescriptor>   MemoryDescriptors;
   typedef vector<MinidumpMemoryRegion> MemoryRegions;
@@ -932,7 +938,7 @@ class Minidump {
   // parameter).
   virtual MinidumpThreadList* GetThreadList();
   MinidumpModuleList* GetModuleList();
-  MinidumpMemoryList* GetMemoryList();
+  virtual MinidumpMemoryList* GetMemoryList();
   MinidumpException* GetException();
   MinidumpAssertion* GetAssertion();
   virtual MinidumpSystemInfo* GetSystemInfo();
diff --git a/src/processor/minidump.cc b/src/processor/minidump.cc
index 4ad82053..7ad736d5 100755
--- a/src/processor/minidump.cc
+++ b/src/processor/minidump.cc
@@ -1509,13 +1509,15 @@ bool MinidumpThread::Read() {
   }
 
   // Check for base + size overflow or undersize.
-  if (thread_.stack.memory.data_size == 0 ||
+  if (thread_.stack.memory.rva == 0 ||
+      thread_.stack.memory.data_size == 0 ||
       thread_.stack.memory.data_size > numeric_limits<uint64_t>::max() -
                                        thread_.stack.start_of_memory_range) {
     // This is ok, but log an error anyway.
     BPLOG(ERROR) << "MinidumpThread has a memory region problem, " <<
                     HexString(thread_.stack.start_of_memory_range) << "+" <<
-                    HexString(thread_.stack.memory.data_size);
+                    HexString(thread_.stack.memory.data_size) <<
+                    ", RVA 0x" << HexString(thread_.stack.memory.rva);
   } else {
     memory_ = new MinidumpMemoryRegion(minidump_);
     memory_->SetDescriptor(&thread_.stack);
@@ -1525,6 +1527,14 @@ bool MinidumpThread::Read() {
   return true;
 }
 
+uint64_t MinidumpThread::GetStartOfStackMemoryRange() const {
+  if (!valid_) {
+    BPLOG(ERROR) << "GetStartOfStackMemoryRange: Invalid MinidumpThread";
+    return 0;
+  }
+
+  return thread_.stack.start_of_memory_range;
+}
 
 MinidumpMemoryRegion* MinidumpThread::GetMemory() {
   if (!valid_) {
diff --git a/src/processor/minidump_processor.cc b/src/processor/minidump_processor.cc
index 232a19ed..a8742cd2 100644
--- a/src/processor/minidump_processor.cc
+++ b/src/processor/minidump_processor.cc
@@ -122,6 +122,12 @@ ProcessResult MinidumpProcessor::Process(
   if (module_list)
     process_state->modules_ = module_list->Copy();
 
+  MinidumpMemoryList *memory_list = dump->GetMemoryList();
+  if (memory_list) {
+    BPLOG(INFO) << "Found " << memory_list->region_count()
+                << " memory regions.";
+  }
+
   MinidumpThreadList *threads = dump->GetThreadList();
   if (!threads) {
     BPLOG(ERROR) << "Minidump " << dump->path() << " has no thread list";
@@ -208,7 +214,17 @@ ProcessResult MinidumpProcessor::Process(
       }
     }
 
+    // If the memory region for the stack cannot be read using the RVA stored
+    // in the memory descriptor inside MINIDUMP_THREAD, try to locate and use
+    // a memory region (containing the stack) from the minidump memory list.
     MinidumpMemoryRegion *thread_memory = thread->GetMemory();
+    if (!thread_memory && memory_list) {
+      uint64_t start_stack_memory_range = thread->GetStartOfStackMemoryRange();
+      if (start_stack_memory_range) {
+        thread_memory = memory_list->GetMemoryRegionForAddress(
+           start_stack_memory_range);
+      }
+    }
     if (!thread_memory) {
       BPLOG(ERROR) << "No memory region for " << thread_string;
     }
diff --git a/src/processor/minidump_processor_unittest.cc b/src/processor/minidump_processor_unittest.cc
index b80c04aa..d0d8b780 100644
--- a/src/processor/minidump_processor_unittest.cc
+++ b/src/processor/minidump_processor_unittest.cc
@@ -70,6 +70,7 @@ class MockMinidump : public Minidump {
   MOCK_METHOD0(GetException, MinidumpException*());
   MOCK_METHOD0(GetAssertion, MinidumpAssertion*());
   MOCK_METHOD0(GetModuleList, MinidumpModuleList*());
+  MOCK_METHOD0(GetMemoryList, MinidumpMemoryList*());
 };
 
 class MockMinidumpThreadList : public MinidumpThreadList {
@@ -80,6 +81,13 @@ class MockMinidumpThreadList : public MinidumpThreadList {
   MOCK_CONST_METHOD1(GetThreadAtIndex, MinidumpThread*(unsigned int));
 };
 
+class MockMinidumpMemoryList : public MinidumpMemoryList {
+ public:
+  MockMinidumpMemoryList() : MinidumpMemoryList(NULL) {}
+
+  MOCK_METHOD1(GetMemoryRegionForAddress, MinidumpMemoryRegion*(uint64_t));
+};
+
 class MockMinidumpThread : public MinidumpThread {
  public:
   MockMinidumpThread() : MinidumpThread(NULL) {}
@@ -87,6 +95,7 @@ class MockMinidumpThread : public MinidumpThread {
   MOCK_CONST_METHOD1(GetThreadID, bool(uint32_t*));
   MOCK_METHOD0(GetContext, MinidumpContext*());
   MOCK_METHOD0(GetMemory, MinidumpMemoryRegion*());
+  MOCK_CONST_METHOD0(GetStartOfStackMemoryRange, uint64_t());
 };
 
 // This is crappy, but MinidumpProcessor really does want a
@@ -131,6 +140,7 @@ using google_breakpad::MinidumpSystemInfo;
 using google_breakpad::MinidumpThreadList;
 using google_breakpad::MinidumpThread;
 using google_breakpad::MockMinidump;
+using google_breakpad::MockMinidumpMemoryList;
 using google_breakpad::MockMinidumpMemoryRegion;
 using google_breakpad::MockMinidumpThread;
 using google_breakpad::MockMinidumpThreadList;
@@ -271,7 +281,7 @@ void TestSymbolSupplier::FreeSymbolData(const CodeModule *module) {
 // MDRawSystemInfo fed to it.
 class TestMinidumpSystemInfo : public MinidumpSystemInfo {
  public:
-  TestMinidumpSystemInfo(MDRawSystemInfo info) :
+  explicit TestMinidumpSystemInfo(MDRawSystemInfo info) :
       MinidumpSystemInfo(NULL) {
     valid_ = true;
     system_info_ = info;
@@ -282,8 +292,9 @@ class TestMinidumpSystemInfo : public MinidumpSystemInfo {
 // A test minidump context, just returns the MDRawContextX86
 // fed to it.
 class TestMinidumpContext : public MinidumpContext {
-public:
-  TestMinidumpContext(const MDRawContextX86& context) : MinidumpContext(NULL) {
+ public:
+  explicit TestMinidumpContext(const MDRawContextX86& context) :
+      MinidumpContext(NULL) {
     valid_ = true;
     context_.x86 = new MDRawContextX86(context);
     context_flags_ = MD_CONTEXT_X86;
@@ -308,16 +319,17 @@ TEST_F(MinidumpProcessorTest, TestCorruptMinidumps) {
 
   MDRawHeader fakeHeader;
   fakeHeader.time_date_stamp = 0;
-  EXPECT_CALL(dump, header()).WillOnce(Return((MDRawHeader*)NULL)).
+  EXPECT_CALL(dump, header()).
+      WillOnce(Return(reinterpret_cast<MDRawHeader*>(NULL))).
       WillRepeatedly(Return(&fakeHeader));
 
   EXPECT_EQ(processor.Process(&dump, &state),
             google_breakpad::PROCESS_ERROR_NO_MINIDUMP_HEADER);
 
   EXPECT_CALL(dump, GetThreadList()).
-      WillOnce(Return((MinidumpThreadList*)NULL));
+      WillOnce(Return(reinterpret_cast<MinidumpThreadList*>(NULL)));
   EXPECT_CALL(dump, GetSystemInfo()).
-    WillRepeatedly(Return((MinidumpSystemInfo*)NULL));
+      WillRepeatedly(Return(reinterpret_cast<MinidumpSystemInfo*>(NULL)));
 
   EXPECT_EQ(processor.Process(&dump, &state),
             google_breakpad::PROCESS_ERROR_NO_THREAD_LIST);
@@ -470,13 +482,23 @@ TEST_F(MinidumpProcessorTest, TestThreadMissingMemory) {
   EXPECT_CALL(dump, GetThreadList()).
       WillOnce(Return(&thread_list));
 
+  MockMinidumpMemoryList memory_list;
+  EXPECT_CALL(dump, GetMemoryList()).
+      WillOnce(Return(&memory_list));
+
   // Return a thread missing stack memory.
   MockMinidumpThread no_memory_thread;
   EXPECT_CALL(no_memory_thread, GetThreadID(_)).
     WillRepeatedly(DoAll(SetArgumentPointee<0>(1),
                          Return(true)));
   EXPECT_CALL(no_memory_thread, GetMemory()).
-    WillRepeatedly(Return((MinidumpMemoryRegion*)NULL));
+    WillRepeatedly(Return(reinterpret_cast<MinidumpMemoryRegion*>(NULL)));
+
+  const uint64_t kTestStartOfMemoryRange = 0x1234;
+  EXPECT_CALL(no_memory_thread, GetStartOfStackMemoryRange()).
+    WillRepeatedly(Return(kTestStartOfMemoryRange));
+  EXPECT_CALL(memory_list, GetMemoryRegionForAddress(kTestStartOfMemoryRange)).
+    WillRepeatedly(Return(reinterpret_cast<MinidumpMemoryRegion*>(NULL)));
 
   MDRawContextX86 no_memory_thread_raw_context;
   memset(&no_memory_thread_raw_context, 0,
@@ -493,7 +515,7 @@ TEST_F(MinidumpProcessorTest, TestThreadMissingMemory) {
   EXPECT_CALL(thread_list, GetThreadAtIndex(0)).
     WillOnce(Return(&no_memory_thread));
 
-  MinidumpProcessor processor((SymbolSupplier*)NULL, NULL);
+  MinidumpProcessor processor(reinterpret_cast<SymbolSupplier*>(NULL), NULL);
   ProcessState state;
   EXPECT_EQ(processor.Process(&dump, &state),
             google_breakpad::PROCESS_OK);
@@ -526,25 +548,33 @@ TEST_F(MinidumpProcessorTest, TestThreadMissingContext) {
   EXPECT_CALL(dump, GetThreadList()).
       WillOnce(Return(&thread_list));
 
+  MockMinidumpMemoryList memory_list;
+  EXPECT_CALL(dump, GetMemoryList()).
+      WillOnce(Return(&memory_list));
+
   // Return a thread missing a thread context.
   MockMinidumpThread no_context_thread;
   EXPECT_CALL(no_context_thread, GetThreadID(_)).
     WillRepeatedly(DoAll(SetArgumentPointee<0>(1),
                          Return(true)));
   EXPECT_CALL(no_context_thread, GetContext()).
-    WillRepeatedly(Return((MinidumpContext*)NULL));
+    WillRepeatedly(Return(reinterpret_cast<MinidumpContext*>(NULL)));
 
   // The memory contents don't really matter here, since it won't be used.
   MockMinidumpMemoryRegion no_context_thread_memory(0x1234, "xxx");
   EXPECT_CALL(no_context_thread, GetMemory()).
     WillRepeatedly(Return(&no_context_thread_memory));
+  EXPECT_CALL(no_context_thread, GetStartOfStackMemoryRange()).
+    Times(0);
+  EXPECT_CALL(memory_list, GetMemoryRegionForAddress(_)).
+    Times(0);
 
   EXPECT_CALL(thread_list, thread_count()).
     WillRepeatedly(Return(1));
   EXPECT_CALL(thread_list, GetThreadAtIndex(0)).
     WillOnce(Return(&no_context_thread));
 
-  MinidumpProcessor processor((SymbolSupplier*)NULL, NULL);
+  MinidumpProcessor processor(reinterpret_cast<SymbolSupplier*>(NULL), NULL);
   ProcessState state;
   EXPECT_EQ(processor.Process(&dump, &state),
             google_breakpad::PROCESS_OK);