From 0510e34cbfe1b920b968c33e83101bed49c47aeb Mon Sep 17 00:00:00 2001 From: "ted.mielczarek@gmail.com" Date: Mon, 19 Aug 2013 18:31:51 +0000 Subject: [PATCH] Allow setting a limit on the number of frames to be recovered by stack scanning. Patch by Julian Seward R=ted at https://bugzilla.mozilla.org/show_bug.cgi?id=894264 git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@1206 4c0a9323-5329-0410-9bdc-e9ce6186880e --- src/google_breakpad/processor/stackwalker.h | 17 +++++- src/processor/stackwalker.cc | 21 ++++++- src/processor/stackwalker_amd64.cc | 5 +- src/processor/stackwalker_amd64.h | 3 +- src/processor/stackwalker_amd64_unittest.cc | 66 +++++++++++++++++++++ src/processor/stackwalker_arm.cc | 5 +- src/processor/stackwalker_arm.h | 3 +- src/processor/stackwalker_arm_unittest.cc | 59 ++++++++++++++++++ src/processor/stackwalker_ppc.cc | 3 +- src/processor/stackwalker_ppc.h | 3 +- src/processor/stackwalker_ppc64.cc | 3 +- src/processor/stackwalker_ppc64.h | 3 +- src/processor/stackwalker_sparc.cc | 3 +- src/processor/stackwalker_sparc.h | 3 +- src/processor/stackwalker_x86.cc | 29 +++++---- src/processor/stackwalker_x86.h | 9 ++- src/processor/stackwalker_x86_unittest.cc | 56 +++++++++++++++++ 17 files changed, 262 insertions(+), 29 deletions(-) diff --git a/src/google_breakpad/processor/stackwalker.h b/src/google_breakpad/processor/stackwalker.h index 9229237c..81ef6557 100644 --- a/src/google_breakpad/processor/stackwalker.h +++ b/src/google_breakpad/processor/stackwalker.h @@ -97,6 +97,10 @@ class Stackwalker { } static uint32_t max_frames() { return max_frames_; } + static void set_max_frames_scanned(uint32_t max_frames_scanned) { + max_frames_scanned_ = max_frames_scanned; + } + protected: // system_info identifies the operating system, NULL or empty if unknown. // memory identifies a MemoryRegion that provides the stack memory @@ -203,8 +207,11 @@ class Stackwalker { // return NULL on failure or when there are no more caller frames (when // the end of the stack has been reached). GetCallerFrame allocates a new // StackFrame (or StackFrame subclass), ownership of which is taken by - // the caller. - virtual StackFrame* GetCallerFrame(const CallStack* stack) = 0; + // the caller. |stack_scan_allowed| controls whether stack scanning is + // an allowable frame-recovery method, since it is desirable to be able to + // disable stack scanning in performance-critical use cases. + virtual StackFrame* GetCallerFrame(const CallStack* stack, + bool stack_scan_allowed) = 0; // The maximum number of frames Stackwalker will walk through. // This defaults to 1024 to prevent infinite loops. @@ -214,6 +221,12 @@ class Stackwalker { // it affects whether or not an error message is printed in the case // where an unwind got stopped by the limit. static bool max_frames_set_; + + // The maximum number of stack-scanned and otherwise untrustworthy + // frames allowed. Stack-scanning can be expensive, so the option to + // disable or limit it is helpful in cases where unwind performance is + // important. This defaults to 1024, the same as max_frames_. + static uint32_t max_frames_scanned_; }; } // namespace google_breakpad diff --git a/src/processor/stackwalker.cc b/src/processor/stackwalker.cc index 00358b26..c8438ccf 100644 --- a/src/processor/stackwalker.cc +++ b/src/processor/stackwalker.cc @@ -57,9 +57,12 @@ namespace google_breakpad { const int Stackwalker::kRASearchWords = 30; + uint32_t Stackwalker::max_frames_ = 1024; bool Stackwalker::max_frames_set_ = false; +uint32_t Stackwalker::max_frames_scanned_ = 1024; + Stackwalker::Stackwalker(const SystemInfo* system_info, MemoryRegion* memory, const CodeModules* modules, @@ -115,6 +118,10 @@ bool Stackwalker::Walk( // Begin with the context frame, and keep getting callers until there are // no more. + // Keep track of the number of scanned or otherwise dubious frames seen + // so far, as the caller may have set a limit. + uint32_t scanned_frames = 0; + // Take ownership of the pointer returned by GetContextFrame. scoped_ptr frame(GetContextFrame()); @@ -147,6 +154,17 @@ bool Stackwalker::Walk( break; } + // Keep track of the number of dubious frames so far. + switch (frame.get()->trust) { + case StackFrame::FRAME_TRUST_NONE: + case StackFrame::FRAME_TRUST_SCAN: + case StackFrame::FRAME_TRUST_CFI_SCAN: + scanned_frames++; + break; + default: + break; + } + // Add the frame to the call stack. Relinquish the ownership claim // over the frame, because the stack now owns it. stack->frames_.push_back(frame.release()); @@ -159,7 +177,8 @@ bool Stackwalker::Walk( } // Get the next frame and take ownership. - frame.reset(GetCallerFrame(stack)); + bool stack_scan_allowed = scanned_frames < max_frames_scanned_; + frame.reset(GetCallerFrame(stack, stack_scan_allowed)); } return true; diff --git a/src/processor/stackwalker_amd64.cc b/src/processor/stackwalker_amd64.cc index 1cf9132e..b2ffdb89 100644 --- a/src/processor/stackwalker_amd64.cc +++ b/src/processor/stackwalker_amd64.cc @@ -197,7 +197,8 @@ StackFrameAMD64* StackwalkerAMD64::GetCallerByStackScan( return frame; } -StackFrame* StackwalkerAMD64::GetCallerFrame(const CallStack* stack) { +StackFrame* StackwalkerAMD64::GetCallerFrame(const CallStack* stack, + bool stack_scan_allowed) { if (!memory_ || !stack) { BPLOG(ERROR) << "Can't get caller frame without memory or stack"; return NULL; @@ -215,7 +216,7 @@ StackFrame* StackwalkerAMD64::GetCallerFrame(const CallStack* stack) { // If CFI failed, or there wasn't CFI available, fall back // to stack scanning. - if (!new_frame.get()) { + if (stack_scan_allowed && !new_frame.get()) { new_frame.reset(GetCallerByStackScan(frames)); } diff --git a/src/processor/stackwalker_amd64.h b/src/processor/stackwalker_amd64.h index 3f1eaf77..acdd2c2f 100644 --- a/src/processor/stackwalker_amd64.h +++ b/src/processor/stackwalker_amd64.h @@ -69,7 +69,8 @@ class StackwalkerAMD64 : public Stackwalker { // Implementation of Stackwalker, using amd64 context (stack pointer in %rsp, // stack base in %rbp) and stack conventions (saved stack pointer at 0(%rbp)) virtual StackFrame* GetContextFrame(); - virtual StackFrame* GetCallerFrame(const CallStack* stack); + virtual StackFrame* GetCallerFrame(const CallStack* stack, + bool stack_scan_allowed); // Use cfi_frame_info (derived from STACK CFI records) to construct // the frame that called frames.back(). The caller takes ownership diff --git a/src/processor/stackwalker_amd64_unittest.cc b/src/processor/stackwalker_amd64_unittest.cc index 169316c6..31489106 100644 --- a/src/processor/stackwalker_amd64_unittest.cc +++ b/src/processor/stackwalker_amd64_unittest.cc @@ -53,6 +53,7 @@ using google_breakpad::CodeModule; using google_breakpad::StackFrameSymbolizer; using google_breakpad::StackFrame; using google_breakpad::StackFrameAMD64; +using google_breakpad::Stackwalker; using google_breakpad::StackwalkerAMD64; using google_breakpad::SystemInfo; using google_breakpad::test_assembler::kLittleEndian; @@ -95,6 +96,9 @@ class StackwalkerAMD64Fixture { // Avoid GMOCK WARNING "Uninteresting mock function call - returning // directly" for FreeSymbolData(). EXPECT_CALL(supplier, FreeSymbolData(_)).Times(AnyNumber()); + + // Reset max_frames_scanned since it's static. + Stackwalker::set_max_frames_scanned(1024); } // Set the Breakpad symbol information that supplier should return for @@ -366,6 +370,68 @@ TEST_F(GetCallerFrame, ScanWithFunctionSymbols) { EXPECT_EQ(0x50000000b0000100ULL, frame1->function_base); } +// Test that set_max_frames_scanned prevents using stack scanning +// to find caller frames. +TEST_F(GetCallerFrame, ScanningNotAllowed) { + // When the stack walker resorts to scanning the stack, + // only addresses located within loaded modules are + // considered valid return addresses. + stack_section.start() = 0x8000000080000000ULL; + uint64_t return_address1 = 0x50000000b0000100ULL; + uint64_t return_address2 = 0x50000000b0000900ULL; + Label frame1_sp, frame2_sp, frame1_rbp; + stack_section + // frame 0 + .Append(16, 0) // space + + .D64(0x40000000b0000000ULL) // junk that's not + .D64(0x50000000d0000000ULL) // a return address + + .D64(return_address1) // actual return address + // frame 1 + .Mark(&frame1_sp) + .Append(16, 0) // space + + .D64(0x40000000b0000000ULL) // more junk + .D64(0x50000000d0000000ULL) + + .Mark(&frame1_rbp) + .D64(stack_section.start()) // This is in the right place to be + // a saved rbp, but it's bogus, so + // we shouldn't report it. + + .D64(return_address2) // actual return address + // frame 2 + .Mark(&frame2_sp) + .Append(32, 0); // end of stack + + RegionFromSection(); + + raw_context.rip = 0x40000000c0000200ULL; + raw_context.rbp = frame1_rbp.Value(); + raw_context.rsp = stack_section.start().Value(); + + StackFrameSymbolizer frame_symbolizer(&supplier, &resolver); + StackwalkerAMD64 walker(&system_info, &raw_context, &stack_region, &modules, + &frame_symbolizer); + Stackwalker::set_max_frames_scanned(0); + + vector modules_without_symbols; + vector modules_with_corrupt_symbols; + ASSERT_TRUE(walker.Walk(&call_stack, &modules_without_symbols, + &modules_with_corrupt_symbols)); + ASSERT_EQ(1U, modules_without_symbols.size()); + ASSERT_EQ("module1", modules_without_symbols[0]->debug_file()); + ASSERT_EQ(0U, modules_with_corrupt_symbols.size()); + frames = call_stack.frames(); + ASSERT_EQ(1U, frames->size()); + + StackFrameAMD64 *frame0 = static_cast(frames->at(0)); + EXPECT_EQ(StackFrame::FRAME_TRUST_CONTEXT, frame0->trust); + ASSERT_EQ(StackFrameAMD64::CONTEXT_VALID_ALL, frame0->context_validity); + EXPECT_EQ(0, memcmp(&raw_context, &frame0->context, sizeof(raw_context))); +} + TEST_F(GetCallerFrame, CallerPushedRBP) { // Functions typically push their %rbp upon entry and set %rbp pointing // there. If stackwalking finds a plausible address for the next frame's diff --git a/src/processor/stackwalker_arm.cc b/src/processor/stackwalker_arm.cc index b114fa17..e4fc5869 100644 --- a/src/processor/stackwalker_arm.cc +++ b/src/processor/stackwalker_arm.cc @@ -237,7 +237,8 @@ StackFrameARM* StackwalkerARM::GetCallerByFramePointer( return frame; } -StackFrame* StackwalkerARM::GetCallerFrame(const CallStack* stack) { +StackFrame* StackwalkerARM::GetCallerFrame(const CallStack* stack, + bool stack_scan_allowed) { if (!memory_ || !stack) { BPLOG(ERROR) << "Can't get caller frame without memory or stack"; return NULL; @@ -259,7 +260,7 @@ StackFrame* StackwalkerARM::GetCallerFrame(const CallStack* stack) { frame.reset(GetCallerByFramePointer(frames)); // If everuthing failed, fall back to stack scanning. - if (!frame.get()) + if (stack_scan_allowed && !frame.get()) frame.reset(GetCallerByStackScan(frames)); // If nothing worked, tell the caller. diff --git a/src/processor/stackwalker_arm.h b/src/processor/stackwalker_arm.h index eb480156..9081a40c 100644 --- a/src/processor/stackwalker_arm.h +++ b/src/processor/stackwalker_arm.h @@ -69,7 +69,8 @@ class StackwalkerARM : public Stackwalker { private: // Implementation of Stackwalker, using arm context and stack conventions. virtual StackFrame* GetContextFrame(); - virtual StackFrame* GetCallerFrame(const CallStack* stack); + virtual StackFrame* GetCallerFrame(const CallStack* stack, + bool stack_scan_allowed); // Use cfi_frame_info (derived from STACK CFI records) to construct // the frame that called frames.back(). The caller takes ownership diff --git a/src/processor/stackwalker_arm_unittest.cc b/src/processor/stackwalker_arm_unittest.cc index b50f4e99..c73322e6 100644 --- a/src/processor/stackwalker_arm_unittest.cc +++ b/src/processor/stackwalker_arm_unittest.cc @@ -54,6 +54,7 @@ using google_breakpad::CodeModule; using google_breakpad::StackFrameSymbolizer; using google_breakpad::StackFrame; using google_breakpad::StackFrameARM; +using google_breakpad::Stackwalker; using google_breakpad::StackwalkerARM; using google_breakpad::SystemInfo; using google_breakpad::WindowsFrameInfo; @@ -97,6 +98,9 @@ class StackwalkerARMFixture { // Avoid GMOCK WARNING "Uninteresting mock function call - returning // directly" for FreeSymbolData(). EXPECT_CALL(supplier, FreeSymbolData(_)).Times(AnyNumber()); + + // Reset max_frames_scanned since it's static. + Stackwalker::set_max_frames_scanned(1024); } // Set the Breakpad symbol information that supplier should return for @@ -406,6 +410,61 @@ TEST_F(GetCallerFrame, ScanFirstFrame) { EXPECT_EQ(frame1_sp.Value(), frame1->context.iregs[MD_CONTEXT_ARM_REG_SP]); } +// Test that set_max_frames_scanned prevents using stack scanning +// to find caller frames. +TEST_F(GetCallerFrame, ScanningNotAllowed) { + // When the stack walker resorts to scanning the stack, + // only addresses located within loaded modules are + // considered valid return addresses. + stack_section.start() = 0x80000000; + uint32_t return_address1 = 0x50000100; + uint32_t return_address2 = 0x50000900; + Label frame1_sp, frame2_sp; + stack_section + // frame 0 + .Append(16, 0) // space + + .D32(0x40090000) // junk that's not + .D32(0x60000000) // a return address + + .D32(return_address1) // actual return address + // frame 1 + .Mark(&frame1_sp) + .Append(16, 0) // space + + .D32(0xF0000000) // more junk + .D32(0x0000000D) + + .D32(return_address2) // actual return address + // frame 2 + .Mark(&frame2_sp) + .Append(32, 0); // end of stack + RegionFromSection(); + + raw_context.iregs[MD_CONTEXT_ARM_REG_PC] = 0x40005510; + raw_context.iregs[MD_CONTEXT_ARM_REG_SP] = stack_section.start().Value(); + + StackFrameSymbolizer frame_symbolizer(&supplier, &resolver); + StackwalkerARM walker(&system_info, &raw_context, -1, &stack_region, &modules, + &frame_symbolizer); + Stackwalker::set_max_frames_scanned(0); + + vector modules_without_symbols; + vector modules_with_corrupt_symbols; + ASSERT_TRUE(walker.Walk(&call_stack, &modules_without_symbols, + &modules_with_corrupt_symbols)); + ASSERT_EQ(1U, modules_without_symbols.size()); + ASSERT_EQ("module1", modules_without_symbols[0]->debug_file()); + ASSERT_EQ(0U, modules_with_corrupt_symbols.size()); + frames = call_stack.frames(); + ASSERT_EQ(1U, frames->size()); + + StackFrameARM *frame0 = static_cast(frames->at(0)); + EXPECT_EQ(StackFrame::FRAME_TRUST_CONTEXT, frame0->trust); + ASSERT_EQ(StackFrameARM::CONTEXT_VALID_ALL, frame0->context_validity); + EXPECT_EQ(0, memcmp(&raw_context, &frame0->context, sizeof(raw_context))); +} + struct CFIFixture: public StackwalkerARMFixture { CFIFixture() { // Provide a bunch of STACK CFI records; we'll walk to the caller diff --git a/src/processor/stackwalker_ppc.cc b/src/processor/stackwalker_ppc.cc index 29015dff..7e208844 100644 --- a/src/processor/stackwalker_ppc.cc +++ b/src/processor/stackwalker_ppc.cc @@ -81,7 +81,8 @@ StackFrame* StackwalkerPPC::GetContextFrame() { } -StackFrame* StackwalkerPPC::GetCallerFrame(const CallStack* stack) { +StackFrame* StackwalkerPPC::GetCallerFrame(const CallStack* stack, + bool stack_scan_allowed) { if (!memory_ || !stack) { BPLOG(ERROR) << "Can't get caller frame without memory or stack"; return NULL; diff --git a/src/processor/stackwalker_ppc.h b/src/processor/stackwalker_ppc.h index 0c989578..012e5c32 100644 --- a/src/processor/stackwalker_ppc.h +++ b/src/processor/stackwalker_ppc.h @@ -64,7 +64,8 @@ class StackwalkerPPC : public Stackwalker { // saved program counter in %srr0) and stack conventions (saved stack // pointer at 0(%r1), return address at 8(0(%r1)). virtual StackFrame* GetContextFrame(); - virtual StackFrame* GetCallerFrame(const CallStack* stack); + virtual StackFrame* GetCallerFrame(const CallStack* stack, + bool stack_scan_allowed); // Stores the CPU context corresponding to the innermost stack frame to // be returned by GetContextFrame. diff --git a/src/processor/stackwalker_ppc64.cc b/src/processor/stackwalker_ppc64.cc index fa49ce39..51c71fe5 100644 --- a/src/processor/stackwalker_ppc64.cc +++ b/src/processor/stackwalker_ppc64.cc @@ -72,7 +72,8 @@ StackFrame* StackwalkerPPC64::GetContextFrame() { } -StackFrame* StackwalkerPPC64::GetCallerFrame(const CallStack* stack) { +StackFrame* StackwalkerPPC64::GetCallerFrame(const CallStack* stack, + bool stack_scan_allowed) { if (!memory_ || !stack) { BPLOG(ERROR) << "Can't get caller frame without memory or stack"; return NULL; diff --git a/src/processor/stackwalker_ppc64.h b/src/processor/stackwalker_ppc64.h index 6579db70..a406343a 100644 --- a/src/processor/stackwalker_ppc64.h +++ b/src/processor/stackwalker_ppc64.h @@ -62,7 +62,8 @@ class StackwalkerPPC64 : public Stackwalker { // saved program counter in %srr0) and stack conventions (saved stack // pointer at 0(%r1), return address at 8(0(%r1)). virtual StackFrame* GetContextFrame(); - virtual StackFrame* GetCallerFrame(const CallStack* stack); + virtual StackFrame* GetCallerFrame(const CallStack* stack, + bool stack_scan_allowed); // Stores the CPU context corresponding to the innermost stack frame to // be returned by GetContextFrame. diff --git a/src/processor/stackwalker_sparc.cc b/src/processor/stackwalker_sparc.cc index 7c3c5200..ff2ea75a 100644 --- a/src/processor/stackwalker_sparc.cc +++ b/src/processor/stackwalker_sparc.cc @@ -72,7 +72,8 @@ StackFrame* StackwalkerSPARC::GetContextFrame() { } -StackFrame* StackwalkerSPARC::GetCallerFrame(const CallStack* stack) { +StackFrame* StackwalkerSPARC::GetCallerFrame(const CallStack* stack, + bool stack_scan_allowed) { if (!memory_ || !stack) { BPLOG(ERROR) << "Can't get caller frame without memory or stack"; return NULL; diff --git a/src/processor/stackwalker_sparc.h b/src/processor/stackwalker_sparc.h index 53fbc843..e8f2a388 100644 --- a/src/processor/stackwalker_sparc.h +++ b/src/processor/stackwalker_sparc.h @@ -63,7 +63,8 @@ class StackwalkerSPARC : public Stackwalker { // Implementation of Stackwalker, using sparc context (%fp, %sp, %pc) and // stack conventions virtual StackFrame* GetContextFrame(); - virtual StackFrame* GetCallerFrame(const CallStack* stack); + virtual StackFrame* GetCallerFrame(const CallStack* stack, + bool stack_scan_allowed); // Stores the CPU context corresponding to the innermost stack frame to // be returned by GetContextFrame. diff --git a/src/processor/stackwalker_x86.cc b/src/processor/stackwalker_x86.cc index ab592b58..0d8b3edc 100644 --- a/src/processor/stackwalker_x86.cc +++ b/src/processor/stackwalker_x86.cc @@ -137,7 +137,8 @@ StackFrame* StackwalkerX86::GetContextFrame() { StackFrameX86* StackwalkerX86::GetCallerByWindowsFrameInfo( const vector &frames, - WindowsFrameInfo* last_frame_info) { + WindowsFrameInfo* last_frame_info, + bool stack_scan_allowed) { StackFrame::FrameTrust trust = StackFrame::FRAME_TRUST_NONE; StackFrameX86* last_frame = static_cast(frames.back()); @@ -345,8 +346,9 @@ StackFrameX86* StackwalkerX86::GetCallerByWindowsFrameInfo( // frame pointer. uint32_t location_start = last_frame->context.esp; uint32_t location, eip; - if (!ScanForReturnAddress(location_start, &location, &eip, - frames.size() == 1 /* is_context_frame */)) { + if (!stack_scan_allowed + || !ScanForReturnAddress(location_start, &location, &eip, + frames.size() == 1 /* is_context_frame */)) { // if we can't find an instruction pointer even with stack scanning, // give up. return NULL; @@ -388,8 +390,9 @@ StackFrameX86* StackwalkerX86::GetCallerByWindowsFrameInfo( // looking one 32-bit word above that location. uint32_t location_start = dictionary[".raSearchStart"] + 4; uint32_t location; - if (ScanForReturnAddress(location_start, &location, &eip, - frames.size() == 1 /* is_context_frame */)) { + if (stack_scan_allowed + && ScanForReturnAddress(location_start, &location, &eip, + frames.size() == 1 /* is_context_frame */)) { // This is a better return address that what program string // evaluation found. Use it, and set %esp to the location above the // one where the return address was found. @@ -497,7 +500,8 @@ StackFrameX86* StackwalkerX86::GetCallerByCFIFrameInfo( } StackFrameX86* StackwalkerX86::GetCallerByEBPAtBase( - const vector &frames) { + const vector &frames, + bool stack_scan_allowed) { StackFrame::FrameTrust trust; StackFrameX86* last_frame = static_cast(frames.back()); uint32_t last_esp = last_frame->context.esp; @@ -538,8 +542,9 @@ StackFrameX86* StackwalkerX86::GetCallerByEBPAtBase( // return address. This can happen if last_frame is executing code // for a module for which we don't have symbols, and that module // is compiled without a frame pointer. - if (!ScanForReturnAddress(last_esp, &caller_esp, &caller_eip, - frames.size() == 1 /* is_context_frame */)) { + if (!stack_scan_allowed + || !ScanForReturnAddress(last_esp, &caller_esp, &caller_eip, + frames.size() == 1 /* is_context_frame */)) { // if we can't find an instruction pointer even with stack scanning, // give up. return NULL; @@ -581,7 +586,8 @@ StackFrameX86* StackwalkerX86::GetCallerByEBPAtBase( return frame; } -StackFrame* StackwalkerX86::GetCallerFrame(const CallStack* stack) { +StackFrame* StackwalkerX86::GetCallerFrame(const CallStack* stack, + bool stack_scan_allowed) { if (!memory_ || !stack) { BPLOG(ERROR) << "Can't get caller frame without memory or stack"; return NULL; @@ -595,7 +601,8 @@ StackFrame* StackwalkerX86::GetCallerFrame(const CallStack* stack) { WindowsFrameInfo* windows_frame_info = frame_symbolizer_->FindWindowsFrameInfo(last_frame); if (windows_frame_info) - new_frame.reset(GetCallerByWindowsFrameInfo(frames, windows_frame_info)); + new_frame.reset(GetCallerByWindowsFrameInfo(frames, windows_frame_info, + stack_scan_allowed)); // If the resolver has DWARF CFI information, use that. if (!new_frame.get()) { @@ -607,7 +614,7 @@ StackFrame* StackwalkerX86::GetCallerFrame(const CallStack* stack) { // Otherwise, hope that the program was using a traditional frame structure. if (!new_frame.get()) - new_frame.reset(GetCallerByEBPAtBase(frames)); + new_frame.reset(GetCallerByEBPAtBase(frames, stack_scan_allowed)); // If nothing worked, tell the caller. if (!new_frame.get()) diff --git a/src/processor/stackwalker_x86.h b/src/processor/stackwalker_x86.h index 45e9709b..0659a13b 100644 --- a/src/processor/stackwalker_x86.h +++ b/src/processor/stackwalker_x86.h @@ -74,14 +74,16 @@ class StackwalkerX86 : public Stackwalker { // alternate conventions as guided by any WindowsFrameInfo available for the // code in question.). virtual StackFrame* GetContextFrame(); - virtual StackFrame* GetCallerFrame(const CallStack* stack); + virtual StackFrame* GetCallerFrame(const CallStack* stack, + bool stack_scan_allowed); // Use windows_frame_info (derived from STACK WIN and FUNC records) // to construct the frame that called frames.back(). The caller // takes ownership of the returned frame. Return NULL on failure. StackFrameX86* GetCallerByWindowsFrameInfo( const vector &frames, - WindowsFrameInfo* windows_frame_info); + WindowsFrameInfo* windows_frame_info, + bool stack_scan_allowed); // Use cfi_frame_info (derived from STACK CFI records) to construct // the frame that called frames.back(). The caller takes ownership @@ -94,7 +96,8 @@ class StackwalkerX86 : public Stackwalker { // %ebp points to the saved %ebp --- construct the frame that called // frames.back(). The caller takes ownership of the returned frame. // Return NULL on failure. - StackFrameX86* GetCallerByEBPAtBase(const vector &frames); + StackFrameX86* GetCallerByEBPAtBase(const vector &frames, + bool stack_scan_allowed); // Stores the CPU context corresponding to the innermost stack frame to // be returned by GetContextFrame. diff --git a/src/processor/stackwalker_x86_unittest.cc b/src/processor/stackwalker_x86_unittest.cc index 03df3210..690f5f31 100644 --- a/src/processor/stackwalker_x86_unittest.cc +++ b/src/processor/stackwalker_x86_unittest.cc @@ -53,6 +53,7 @@ using google_breakpad::CodeModule; using google_breakpad::StackFrameSymbolizer; using google_breakpad::StackFrame; using google_breakpad::StackFrameX86; +using google_breakpad::Stackwalker; using google_breakpad::StackwalkerX86; using google_breakpad::SystemInfo; using google_breakpad::WindowsFrameInfo; @@ -104,6 +105,9 @@ class StackwalkerX86Fixture { // Avoid GMOCK WARNING "Uninteresting mock function call - returning // directly" for FreeSymbolData(). EXPECT_CALL(supplier, FreeSymbolData(_)).Times(AnyNumber()); + + // Reset max_frames_scanned since it's static. + Stackwalker::set_max_frames_scanned(1024); } // Set the Breakpad symbol information that supplier should return for @@ -419,6 +423,58 @@ TEST_F(GetCallerFrame, TraditionalScanLongWay) { } } +// Test that set_max_frames_scanned prevents using stack scanning +// to find caller frames. +TEST_F(GetCallerFrame, ScanningNotAllowed) { + stack_section.start() = 0x80000000; + Label frame1_ebp; + stack_section + // frame 0 + .D32(0xf065dc76) // locals area: + .D32(0x46ee2167) // garbage that doesn't look like + .D32(0xbab023ec) // a return address + .D32(frame1_ebp) // saved %ebp (%ebp fails to point here, forcing scan) + .D32(0x4000129d) // return address + // frame 1 + .Append(8, 0) // space + .Mark(&frame1_ebp) // %ebp points here + .D32(0) // saved %ebp (stack end) + .D32(0); // return address (stack end) + + RegionFromSection(); + raw_context.eip = 0x4000f49d; + raw_context.esp = stack_section.start().Value(); + // Make the frame pointer bogus, to make the stackwalker scan the stack + // for something that looks like a return address. + raw_context.ebp = 0xd43eed6e; + + StackFrameSymbolizer frame_symbolizer(&supplier, &resolver); + StackwalkerX86 walker(&system_info, &raw_context, &stack_region, &modules, + &frame_symbolizer); + Stackwalker::set_max_frames_scanned(0); + + vector modules_without_symbols; + vector modules_with_corrupt_symbols; + ASSERT_TRUE(walker.Walk(&call_stack, &modules_without_symbols, + &modules_with_corrupt_symbols)); + ASSERT_EQ(1U, modules_without_symbols.size()); + ASSERT_EQ("module1", modules_without_symbols[0]->debug_file()); + ASSERT_EQ(0U, modules_with_corrupt_symbols.size()); + frames = call_stack.frames(); + ASSERT_EQ(1U, frames->size()); + + { // To avoid reusing locals by mistake + StackFrameX86 *frame0 = static_cast(frames->at(0)); + EXPECT_EQ(StackFrame::FRAME_TRUST_CONTEXT, frame0->trust); + ASSERT_EQ(StackFrameX86::CONTEXT_VALID_ALL, frame0->context_validity); + EXPECT_EQ(0x4000f49dU, frame0->instruction); + EXPECT_EQ(0x4000f49dU, frame0->context.eip); + EXPECT_EQ(stack_section.start().Value(), frame0->context.esp); + EXPECT_EQ(0xd43eed6eU, frame0->context.ebp); + EXPECT_EQ(NULL, frame0->windows_frame_info); + } +} + // Use Windows frame data (a "STACK WIN 4" record, from a // FrameTypeFrameData DIA record) to walk a stack frame. TEST_F(GetCallerFrame, WindowsFrameData) {