Wrong %ebp after skipping a frame for which the instruction pointer is not in a known module.

http://breakpad.appspot.com/494002/



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

2 files changed, 269 insertions, 25 deletions

diff --git a/src/processor/stackwalker_x86.cc b/src/processor/stackwalker_x86.cc
index 4ee5beed..6a4569a2 100644
--- a/src/processor/stackwalker_x86.cc
+++ b/src/processor/stackwalker_x86.cc
@@ -386,31 +386,41 @@ StackFrameX86* StackwalkerX86::GetCallerByWindowsFrameInfo(
       }
     }
 
-    // When trying to recover the previous value of the frame pointer (%ebp),
-    // start looking at the lowest possible address in the saved-register
-    // area, and look at the entire saved register area, increased by the
-    // size of |offset| to account for additional data that may be on the
-    // stack.  The scan is performed from the highest possible address to
-    // the lowest, because we expect that the function's prolog would have
-    // saved %ebp early.
-    u_int32_t ebp = dictionary["$ebp"];
-    u_int32_t value;  // throwaway variable to check pointer validity
-    if (recover_ebp && !memory_->GetMemoryAtAddress(ebp, &value)) {
-      int fp_search_bytes = last_frame_info->saved_register_size + offset;
-      u_int32_t location_end = last_frame->context.esp +
-                               last_frame_callee_parameter_size;
-
-      for (u_int32_t location = location_end + fp_search_bytes;
-           location >= location_end;
-           location -= 4) {
-        if (!memory_->GetMemoryAtAddress(location, &ebp))
-          break;
-
-        if (memory_->GetMemoryAtAddress(ebp, &value)) {
-          // The candidate value is a pointer to the same memory region
-          // (the stack).  Prefer it as a recovered %ebp result.
-          dictionary["$ebp"] = ebp;
-          break;
+    if (recover_ebp) {
+      // When trying to recover the previous value of the frame pointer (%ebp),
+      // start looking at the lowest possible address in the saved-register
+      // area, and look at the entire saved register area, increased by the
+      // size of |offset| to account for additional data that may be on the
+      // stack.  The scan is performed from the highest possible address to
+      // the lowest, because the expectation is that the function's prolog
+      // would have saved %ebp early.
+      u_int32_t ebp = dictionary["$ebp"];
+
+      // When a scan for return address is used, it is possible to skip one or
+      // more frames (when return address is not in a known module).  One
+      // indication for skipped frames is when the value of %ebp is lower than
+      // the location of the return address on the stack
+      bool has_skipped_frames =
+        (trust != StackFrame::FRAME_TRUST_CFI && ebp <= raSearchStart + offset);
+
+      u_int32_t value;  // throwaway variable to check pointer validity
+      if (has_skipped_frames || !memory_->GetMemoryAtAddress(ebp, &value)) {
+        int fp_search_bytes = last_frame_info->saved_register_size + offset;
+        u_int32_t location_end = last_frame->context.esp +
+                                 last_frame_callee_parameter_size;
+
+        for (u_int32_t location = location_end + fp_search_bytes;
+             location >= location_end;
+             location -= 4) {
+          if (!memory_->GetMemoryAtAddress(location, &ebp))
+            break;
+
+          if (memory_->GetMemoryAtAddress(ebp, &value)) {
+            // The candidate value is a pointer to the same memory region
+            // (the stack).  Prefer it as a recovered %ebp result.
+            dictionary["$ebp"] = ebp;
+            break;
+          }
         }
       }
     }
diff --git a/src/processor/stackwalker_x86_unittest.cc b/src/processor/stackwalker_x86_unittest.cc
index 23efe87d..bf76e881 100644
--- a/src/processor/stackwalker_x86_unittest.cc
+++ b/src/processor/stackwalker_x86_unittest.cc
@@ -1064,6 +1064,240 @@ TEST_F(GetCallerFrame, WindowsFPOSystemCall) {
   }
 }
 
+// Scan the stack for a better return address and potentially skip frames
+// when the calculated return address is not in a known module.
+// Note, that the span of this scan is somewhat arbitrarily limited to 30
+// search words (pointers):
+//     const int kRASearchWords = 30;
+// This means that frames can be skipped only when their size is relatively
+// small: smaller than kRASearchWords * sizeof(InstructionType)
+TEST_F(GetCallerFrame, ReturnAddressIsNotInKnownModule) {
+  MockCodeModule msvcrt_dll(0x77be0000, 0x58000, "msvcrt.dll", "version1");
+  SetModuleSymbols(&msvcrt_dll,  // msvcrt.dll
+                   "PUBLIC 38180 0 wcsstr\n"
+                   "STACK WIN 4 38180 61 10 0 8 0 0 0 1 $T0 $ebp = $eip $T0 "
+                   "4 + ^ = $ebp $T0 ^ = $esp $T0 8 + = $L $T0 .cbSavedRegs "
+                   "- = $P $T0 4 + .cbParams + =\n");
+
+  MockCodeModule kernel32_dll(0x7c800000, 0x103000, "kernel32.dll", "version1");
+  SetModuleSymbols(&kernel32_dll,  // kernel32.dll
+                   "PUBLIC efda 8 FindNextFileW\n"
+                   "STACK WIN 4 efda 1bb c 0 8 8 3c 0 1 $T0 $ebp = $eip $T0 "
+                   "4 + ^ = $ebp $T0 ^ = $esp $T0 8 + = $L $T0 .cbSavedRegs "
+                   "- = $P $T0 4 + .cbParams + =\n");
+
+  MockCodeModule chrome_dll(0x1c30000, 0x28C8000, "chrome.dll", "version1");
+  SetModuleSymbols(&chrome_dll,  // chrome.dll
+                   "FUNC e3cff 4af 0 file_util::FileEnumerator::Next()\n"
+                   "e3cff 1a 711 2505\n"
+                   "STACK WIN 4 e3cff 4af 20 0 4 c 94 0 1 $T1 .raSearch = "
+                   "$T0  $T1 4 - 8 @ = $ebp $T1 4 - ^ = $eip $T1 ^ = $esp "
+                   "$T1 4 + = $20 $T0 152 - ^ =  $23 $T0 156 - ^ =  $24 "
+                   "$T0 160 - ^ =\n");
+
+  // Create some modules with some stock debugging information.
+  MockCodeModules local_modules;
+  local_modules.Add(&msvcrt_dll);
+  local_modules.Add(&kernel32_dll);
+  local_modules.Add(&chrome_dll);
+
+  Label frame0_esp;
+  Label frame0_ebp;
+  Label frame1_ebp;
+  Label frame2_ebp;
+  Label frame3_ebp;
+
+  stack_section.start() = 0x0932f2d0;
+  stack_section
+    .Mark(&frame0_esp)
+    .D32(0x0764e000)
+    .D32(0x0764e068)
+    .Mark(&frame0_ebp)
+    .D32(frame1_ebp)    // Child EBP
+    .D32(0x001767a0)    // return address of frame 0
+                        // Not in known module
+    .D32(0x0764e0c6)
+    .D32(0x001bb1b8)
+    .D32(0x0764e068)
+    .D32(0x00000003)
+    .D32(0x0764e068)
+    .D32(0x00000003)
+    .D32(0x07578828)
+    .D32(0x0764e000)
+    .D32(0x00000000)
+    .D32(0x001c0010)
+    .D32(0x0764e0c6)
+    .Mark(&frame1_ebp)
+    .D32(frame2_ebp)    // Child EBP
+    .D32(0x7c80f10f)    // return address of frame 1
+                        // inside kernel32!FindNextFileW
+    .D32(0x000008f8)
+    .D32(0x00000000)
+    .D32(0x00000000)
+    .D32(0x00000000)
+    .D32(0x0932f34c)
+    .D32(0x0764e000)
+    .D32(0x00001000)
+    .D32(0x00000000)
+    .D32(0x00000001)
+    .D32(0x00000000)
+    .D32(0x00000000)
+    .D32(0x0932f6a8)
+    .D32(0x00000000)
+    .D32(0x0932f6d8)
+    .D32(0x00000000)
+    .D32(0x000000d6)
+    .D32(0x0764e000)
+    .D32(0x7ff9a000)
+    .D32(0x0932f3fc)
+    .D32(0x00000001)
+    .D32(0x00000001)
+    .D32(0x07578828)
+    .D32(0x0000002e)
+    .D32(0x0932f340)
+    .D32(0x0932eef4)
+    .D32(0x0932ffdc)
+    .D32(0x7c839ad8)
+    .D32(0x7c80f0d8)
+    .D32(0x00000000)
+    .Mark(&frame2_ebp)
+    .D32(frame3_ebp)    // Child EBP
+    .D32(0x01d13f91)    // return address of frame 2
+                        // inside chrome_dll!file_util::FileEnumerator::Next
+    .D32(0x07578828)
+    .D32(0x0932f6ac)
+    .D32(0x0932f9c4)
+    .D32(0x0932f9b4)
+    .D32(0x00000000)
+    .D32(0x00000003)
+    .D32(0x0932f978)
+    .D32(0x01094330)
+    .D32(0x00000000)
+    .D32(0x00000001)
+    .D32(0x01094330)
+    .D32(0x00000000)
+    .D32(0x00000000)
+    .D32(0x07f30000)
+    .D32(0x01c3ba17)
+    .D32(0x08bab840)
+    .D32(0x07f31580)
+    .D32(0x00000000)
+    .D32(0x00000007)
+    .D32(0x0932f940)
+    .D32(0x0000002e)
+    .D32(0x0932f40c)
+    .D32(0x01d13b53)
+    .D32(0x0932f958)
+    .D32(0x00000001)
+    .D32(0x00000007)
+    .D32(0x0932f940)
+    .D32(0x0000002e)
+    .D32(0x00000000)
+    .D32(0x0932f6ac)
+    .D32(0x01e13ef0)
+    .D32(0x00000001)
+    .D32(0x00000007)
+    .D32(0x0932f958)
+    .D32(0x08bab840)
+    .D32(0x0932f9b4)
+    .D32(0x00000000)
+    .D32(0x0932f9b4)
+    .D32(0x000000a7)
+    .D32(0x000000a7)
+    .D32(0x0932f998)
+    .D32(0x579627a2)
+    .Mark(&frame3_ebp)
+    .D32(0)             // saved %ebp (stack end)
+    .D32(0);            // saved %eip (stack end)
+
+  RegionFromSection();
+  raw_context.eip = 0x77c181cd;  // inside msvcrt!wcsstr
+  raw_context.esp = frame0_esp.Value();
+  raw_context.ebp = frame0_ebp.Value();
+  // sanity
+  ASSERT_TRUE(raw_context.esp == stack_section.start().Value());
+  ASSERT_TRUE(raw_context.ebp == stack_section.start().Value() + 8);
+
+  StackFrameSymbolizer frame_symbolizer(&supplier, &resolver);
+  StackwalkerX86 walker(&system_info, &raw_context, &stack_region,
+                        &local_modules, &frame_symbolizer);
+  ASSERT_TRUE(walker.Walk(&call_stack));
+  frames = call_stack.frames();
+
+  ASSERT_EQ(3U, frames->size());
+
+  {  // To avoid reusing locals by mistake
+    StackFrameX86 *frame0 = static_cast<StackFrameX86 *>(frames->at(0));
+    EXPECT_EQ(StackFrame::FRAME_TRUST_CONTEXT, frame0->trust);
+    ASSERT_EQ(StackFrameX86::CONTEXT_VALID_ALL, frame0->context_validity);
+    EXPECT_EQ(0x77c181cdU, frame0->instruction);
+    EXPECT_EQ(0x77c181cdU, frame0->context.eip);
+    EXPECT_EQ(frame0_esp.Value(), frame0->context.esp);
+    EXPECT_EQ(frame0_ebp.Value(), frame0->context.ebp);
+    EXPECT_EQ(&msvcrt_dll, frame0->module);
+    EXPECT_EQ("wcsstr", frame0->function_name);
+    ASSERT_TRUE(frame0->windows_frame_info != NULL);
+    EXPECT_EQ(WindowsFrameInfo::VALID_ALL, frame0->windows_frame_info->valid);
+    EXPECT_EQ(WindowsFrameInfo::STACK_INFO_FRAME_DATA,
+              frame0->windows_frame_info->type_);
+    EXPECT_EQ("$T0 $ebp = $eip $T0 "
+              "4 + ^ = $ebp $T0 ^ = $esp $T0 8 + = $L $T0 .cbSavedRegs "
+              "- = $P $T0 4 + .cbParams + =",
+              frame0->windows_frame_info->program_string);
+    // It has program string, so allocates_base_pointer is not expected
+    EXPECT_FALSE(frame0->windows_frame_info->allocates_base_pointer);
+  }
+
+  {  // To avoid reusing locals by mistake
+    StackFrameX86 *frame1 = static_cast<StackFrameX86 *>(frames->at(1));
+    EXPECT_EQ(StackFrame::FRAME_TRUST_CFI_SCAN, frame1->trust);
+    ASSERT_EQ((StackFrameX86::CONTEXT_VALID_EIP |
+               StackFrameX86::CONTEXT_VALID_ESP |
+               StackFrameX86::CONTEXT_VALID_EBP),
+              frame1->context_validity);
+    EXPECT_EQ(0x7c80f10fU, frame1->instruction + 1);
+    EXPECT_EQ(0x7c80f10fU, frame1->context.eip);
+    // frame 1 was skipped, so intead of frame1_ebp compare with frame2_ebp.
+    EXPECT_EQ(frame2_ebp.Value(), frame1->context.ebp);
+    EXPECT_EQ(&kernel32_dll, frame1->module);
+    EXPECT_EQ("FindNextFileW", frame1->function_name);
+    ASSERT_TRUE(frame1->windows_frame_info != NULL);
+    EXPECT_EQ(WindowsFrameInfo::VALID_ALL, frame1->windows_frame_info->valid);
+    EXPECT_EQ(WindowsFrameInfo::STACK_INFO_FRAME_DATA,
+              frame1->windows_frame_info->type_);
+    EXPECT_EQ("$T0 $ebp = $eip $T0 "
+              "4 + ^ = $ebp $T0 ^ = $esp $T0 8 + = $L $T0 .cbSavedRegs "
+              "- = $P $T0 4 + .cbParams + =",
+              frame1->windows_frame_info->program_string);
+    EXPECT_FALSE(frame1->windows_frame_info->allocates_base_pointer);
+  }
+
+  {  // To avoid reusing locals by mistake
+    StackFrameX86 *frame2 = static_cast<StackFrameX86 *>(frames->at(2));
+    EXPECT_EQ(StackFrame::FRAME_TRUST_CFI, frame2->trust);
+    ASSERT_EQ((StackFrameX86::CONTEXT_VALID_EIP |
+               StackFrameX86::CONTEXT_VALID_ESP |
+               StackFrameX86::CONTEXT_VALID_EBP),
+              frame2->context_validity);
+    EXPECT_EQ(0x01d13f91U, frame2->instruction + 1);
+    EXPECT_EQ(0x01d13f91U, frame2->context.eip);
+    // frame 1 was skipped, so intead of frame2_ebp compare with frame3_ebp.
+    EXPECT_EQ(frame3_ebp.Value(), frame2->context.ebp);
+    EXPECT_EQ(&chrome_dll, frame2->module);
+    EXPECT_EQ("file_util::FileEnumerator::Next()", frame2->function_name);
+    ASSERT_TRUE(frame2->windows_frame_info != NULL);
+    EXPECT_EQ(WindowsFrameInfo::VALID_ALL, frame2->windows_frame_info->valid);
+    EXPECT_EQ(WindowsFrameInfo::STACK_INFO_FRAME_DATA,
+              frame2->windows_frame_info->type_);
+    EXPECT_EQ("$T1 .raSearch = "
+              "$T0  $T1 4 - 8 @ = $ebp $T1 4 - ^ = $eip $T1 ^ = $esp "
+              "$T1 4 + = $20 $T0 152 - ^ =  $23 $T0 156 - ^ =  $24 "
+              "$T0 160 - ^ =",
+              frame2->windows_frame_info->program_string);
+    EXPECT_FALSE(frame2->windows_frame_info->allocates_base_pointer);
+  }
+}
+
 struct CFIFixture: public StackwalkerX86Fixture {
   CFIFixture() {
     // Provide a bunch of STACK CFI records; individual tests walk to the