diff options
Diffstat (limited to 'src/processor')
-rw-r--r-- | src/processor/microdump_processor_unittest.cc | 4 | ||||
-rw-r--r-- | src/processor/minidump.cc | 2 | ||||
-rw-r--r-- | src/processor/stackwalker.cc | 41 | ||||
-rw-r--r-- | src/processor/stackwalker_amd64.cc | 30 | ||||
-rw-r--r-- | src/processor/stackwalker_amd64.h | 8 | ||||
-rw-r--r-- | src/processor/stackwalker_arm.cc | 16 | ||||
-rw-r--r-- | src/processor/stackwalker_arm64.cc | 15 | ||||
-rw-r--r-- | src/processor/stackwalker_mips.cc | 15 | ||||
-rw-r--r-- | src/processor/stackwalker_ppc.cc | 8 | ||||
-rw-r--r-- | src/processor/stackwalker_ppc64.cc | 8 | ||||
-rw-r--r-- | src/processor/stackwalker_sparc.cc | 8 | ||||
-rw-r--r-- | src/processor/stackwalker_x86.cc | 14 |
12 files changed, 91 insertions, 78 deletions
diff --git a/src/processor/microdump_processor_unittest.cc b/src/processor/microdump_processor_unittest.cc index af897f7d..238b048c 100644 --- a/src/processor/microdump_processor_unittest.cc +++ b/src/processor/microdump_processor_unittest.cc @@ -132,7 +132,7 @@ TEST_F(MicrodumpProcessorTest, TestProcess_MissingSymbols) { ASSERT_EQ("arm64", state.system_info()->cpu); ASSERT_EQ("OS 64 VERSION INFO", state.system_info()->os_version); ASSERT_EQ(1U, state.threads()->size()); - ASSERT_EQ(12U, state.threads()->at(0)->frames()->size()); + ASSERT_EQ(11U, state.threads()->at(0)->frames()->size()); ASSERT_EQ("", state.threads()->at(0)->frames()->at(0)->function_name); @@ -205,7 +205,7 @@ TEST_F(MicrodumpProcessorTest, TestProcessX86) { ASSERT_EQ("x86", state.system_info()->cpu); ASSERT_EQ("asus/WW_Z00A/Z00A:5.0/LRX21V/2.19.40.22_20150627_5104_user:user/" "release-keys", state.system_info()->os_version); - ASSERT_EQ(56U, state.threads()->at(0)->frames()->size()); + ASSERT_EQ(17U, state.threads()->at(0)->frames()->size()); ASSERT_EQ("libc.so", state.threads()->at(0)->frames()->at(0)->module->debug_file()); // TODO(mmandlis): Get symbols for the test X86 microdump and test function diff --git a/src/processor/minidump.cc b/src/processor/minidump.cc index 430d49d9..d2d69202 100644 --- a/src/processor/minidump.cc +++ b/src/processor/minidump.cc @@ -1195,7 +1195,7 @@ bool MinidumpContext::CheckAgainstSystemInfo(uint32_t context_cpu_type) { // -uint32_t MinidumpMemoryRegion::max_bytes_ = 2 * 1024 * 1024; // 2MB +uint32_t MinidumpMemoryRegion::max_bytes_ = 64 * 1024 * 1024; // 64MB MinidumpMemoryRegion::MinidumpMemoryRegion(Minidump* minidump) diff --git a/src/processor/stackwalker.cc b/src/processor/stackwalker.cc index c85ce5e8..4988ef1e 100644 --- a/src/processor/stackwalker.cc +++ b/src/processor/stackwalker.cc @@ -60,10 +60,15 @@ namespace google_breakpad { const int Stackwalker::kRASearchWords = 40; -uint32_t Stackwalker::max_frames_ = 1024; +// This default is just a sanity check: a large enough value +// that allow capturing unbounded recursion traces, yet provide a +// guardrail against stack walking bugs. The stack walking invariants +// guarantee that the unwinding process is strictly monotonic and +// practically bounded by the size of the stack memory range. +uint32_t Stackwalker::max_frames_ = 1 << 20; // 1M bool Stackwalker::max_frames_set_ = false; -uint32_t Stackwalker::max_frames_scanned_ = 1024; +uint32_t Stackwalker::max_frames_scanned_ = 1 << 14; // 16k Stackwalker::Stackwalker(const SystemInfo* system_info, MemoryRegion* memory, @@ -234,7 +239,7 @@ Stackwalker* Stackwalker::StackwalkerForCPU( context->GetContextSPARC(), memory, modules, frame_symbolizer); break; - + case MD_CONTEXT_MIPS: case MD_CONTEXT_MIPS64: cpu_stackwalker = new StackwalkerMIPS(system_info, @@ -253,7 +258,7 @@ Stackwalker* Stackwalker::StackwalkerForCPU( frame_symbolizer); break; } - + case MD_CONTEXT_ARM64: cpu_stackwalker = new StackwalkerARM64(system_info, context->GetContextARM64(), @@ -271,7 +276,33 @@ Stackwalker* Stackwalker::StackwalkerForCPU( return cpu_stackwalker; } -bool Stackwalker::InstructionAddressSeemsValid(uint64_t address) { +// CONSIDER: check stack alignment? +bool Stackwalker::TerminateWalk(uint64_t caller_ip, + uint64_t caller_sp, + uint64_t callee_sp, + bool first_unwind) const { + // Treat an instruction address less than 4k as end-of-stack. + // (using InstructionAddressSeemsValid() here is very tempting, + // but we need to handle JITted code) + if (caller_ip < (1 << 12)) { + return true; + } + + // NOTE: The stack address range is implicitly checked + // when the stack memory is accessed. + + // The stack pointer should monotonically increase. For first unwind + // we allow caller_sp == callee_sp to account for architectures where + // the return address is stored in a register (so it's possible to have + // leaf functions which don't move the stack pointer) + if (first_unwind ? (caller_sp < callee_sp) : (caller_sp <= callee_sp)) { + return true; + } + + return false; +} + +bool Stackwalker::InstructionAddressSeemsValid(uint64_t address) const { StackFrame frame; frame.instruction = address; StackFrameSymbolizer::SymbolizerResult symbolizer_result = diff --git a/src/processor/stackwalker_amd64.cc b/src/processor/stackwalker_amd64.cc index d1333248..d5ac6c65 100644 --- a/src/processor/stackwalker_amd64.cc +++ b/src/processor/stackwalker_amd64.cc @@ -147,23 +147,6 @@ StackFrameAMD64* StackwalkerAMD64::GetCallerByCFIFrameInfo( return frame.release(); } -bool StackwalkerAMD64::IsEndOfStack(uint64_t caller_rip, uint64_t caller_rsp, - uint64_t callee_rsp) { - // Treat an instruction address of 0 as end-of-stack. - if (caller_rip == 0) { - return true; - } - - // If the new stack pointer is at a lower address than the old, then - // that's clearly incorrect. Treat this as end-of-stack to enforce - // progress and avoid infinite loops. - if (caller_rsp < callee_rsp) { - return true; - } - - return false; -} - // Returns true if `ptr` is not in x86-64 canonical form. // https://en.wikipedia.org/wiki/X86-64#Virtual_address_space_details static bool is_non_canonical(uint64_t ptr) { @@ -173,7 +156,6 @@ static bool is_non_canonical(uint64_t ptr) { StackFrameAMD64* StackwalkerAMD64::GetCallerByFramePointerRecovery( const vector<StackFrame*>& frames) { StackFrameAMD64* last_frame = static_cast<StackFrameAMD64*>(frames.back()); - uint64_t last_rsp = last_frame->context.rsp; uint64_t last_rbp = last_frame->context.rbp; // Assume the presence of a frame pointer. This is not mandated by the @@ -208,10 +190,8 @@ StackFrameAMD64* StackwalkerAMD64::GetCallerByFramePointerRecovery( return NULL; } - // Simple sanity check that the stack is growing downwards as expected. - if (IsEndOfStack(caller_rip, caller_rsp, last_rsp) || - caller_rbp < last_rbp) { - // Reached end-of-stack or stack is not growing downwards. + // Check that rbp is within the right frame + if (caller_rsp <= last_rbp || caller_rbp < caller_rsp) { return NULL; } @@ -327,9 +307,9 @@ StackFrame* StackwalkerAMD64::GetCallerFrame(const CallStack* stack, new_frame->context.rbp = static_cast<uint32_t>(new_frame->context.rbp); } - if (IsEndOfStack(new_frame->context.rip, new_frame->context.rsp, - last_frame->context.rsp)) { - // Reached end-of-stack. + // Should we terminate the stack walk? (end-of-stack or broken invariant) + if (TerminateWalk(new_frame->context.rip, new_frame->context.rsp, + last_frame->context.rsp, frames.size() == 1)) { return NULL; } diff --git a/src/processor/stackwalker_amd64.h b/src/processor/stackwalker_amd64.h index 67c45510..8f3dbd52 100644 --- a/src/processor/stackwalker_amd64.h +++ b/src/processor/stackwalker_amd64.h @@ -78,14 +78,6 @@ class StackwalkerAMD64 : public Stackwalker { StackFrameAMD64* GetCallerByCFIFrameInfo(const vector<StackFrame*> &frames, CFIFrameInfo* cfi_frame_info); - // Checks whether end-of-stack is reached. An instruction address of 0 is an - // end-of-stack marker. If the stack pointer of the caller is at a lower - // address than the stack pointer of the callee, then that's clearly incorrect - // and it is treated as end-of-stack to enforce progress and avoid infinite - // loops. - bool IsEndOfStack(uint64_t caller_rip, uint64_t caller_rsp, - uint64_t callee_rsp); - // Assumes a traditional frame layout where the frame pointer has not been // omitted. The expectation is that caller's %rbp is pushed to the stack // after the return address of the callee, and that the callee's %rsp can diff --git a/src/processor/stackwalker_arm.cc b/src/processor/stackwalker_arm.cc index e4fc5869..dabb4fd2 100644 --- a/src/processor/stackwalker_arm.cc +++ b/src/processor/stackwalker_arm.cc @@ -267,17 +267,13 @@ StackFrame* StackwalkerARM::GetCallerFrame(const CallStack* stack, if (!frame.get()) return NULL; - - // An instruction address of zero marks the end of the stack. - if (frame->context.iregs[MD_CONTEXT_ARM_REG_PC] == 0) - return NULL; - - // If the new stack pointer is at a lower address than the old, then - // that's clearly incorrect. Treat this as end-of-stack to enforce - // progress and avoid infinite loops. - if (frame->context.iregs[MD_CONTEXT_ARM_REG_SP] - < last_frame->context.iregs[MD_CONTEXT_ARM_REG_SP]) + // Should we terminate the stack walk? (end-of-stack or broken invariant) + if (TerminateWalk(frame->context.iregs[MD_CONTEXT_ARM_REG_PC], + frame->context.iregs[MD_CONTEXT_ARM_REG_SP], + last_frame->context.iregs[MD_CONTEXT_ARM_REG_SP], + frames.size() == 1)) { return NULL; + } // The new frame's context's PC is the return address, which is one // instruction past the instruction that caused us to arrive at the diff --git a/src/processor/stackwalker_arm64.cc b/src/processor/stackwalker_arm64.cc index 31119a97..f9660112 100644 --- a/src/processor/stackwalker_arm64.cc +++ b/src/processor/stackwalker_arm64.cc @@ -252,16 +252,13 @@ StackFrame* StackwalkerARM64::GetCallerFrame(const CallStack* stack, if (!frame.get()) return NULL; - // An instruction address of zero marks the end of the stack. - if (frame->context.iregs[MD_CONTEXT_ARM64_REG_PC] == 0) - return NULL; - - // If the new stack pointer is at a lower address than the old, then - // that's clearly incorrect. Treat this as end-of-stack to enforce - // progress and avoid infinite loops. - if (frame->context.iregs[MD_CONTEXT_ARM64_REG_SP] - < last_frame->context.iregs[MD_CONTEXT_ARM64_REG_SP]) + // Should we terminate the stack walk? (end-of-stack or broken invariant) + if (TerminateWalk(frame->context.iregs[MD_CONTEXT_ARM64_REG_PC], + frame->context.iregs[MD_CONTEXT_ARM64_REG_SP], + last_frame->context.iregs[MD_CONTEXT_ARM64_REG_SP], + frames.size() == 1)) { return NULL; + } // The new frame's context's PC is the return address, which is one // instruction past the instruction that caused us to arrive at the callee. diff --git a/src/processor/stackwalker_mips.cc b/src/processor/stackwalker_mips.cc index 9a81b46e..c33ecdbe 100644 --- a/src/processor/stackwalker_mips.cc +++ b/src/processor/stackwalker_mips.cc @@ -272,16 +272,11 @@ StackFrame* StackwalkerMIPS::GetCallerFrame(const CallStack* stack, return NULL; } - // Treat an instruction address of 0 as end-of-stack. - if (new_frame->context.epc == 0) { - return NULL; - } - - // If the new stack pointer is at a lower address than the old, then - // that's clearly incorrect. Treat this as end-of-stack to enforce - // progress and avoid infinite loops. - if (new_frame->context.iregs[MD_CONTEXT_MIPS_REG_SP] < - last_frame->context.iregs[MD_CONTEXT_MIPS_REG_SP]) { + // Should we terminate the stack walk? (end-of-stack or broken invariant) + if (TerminateWalk(new_frame->context.epc, + new_frame->context.iregs[MD_CONTEXT_MIPS_REG_SP], + last_frame->context.iregs[MD_CONTEXT_MIPS_REG_SP], + frames.size() == 1)) { return NULL; } diff --git a/src/processor/stackwalker_ppc.cc b/src/processor/stackwalker_ppc.cc index 7e208844..c9ab144f 100644 --- a/src/processor/stackwalker_ppc.cc +++ b/src/processor/stackwalker_ppc.cc @@ -130,6 +130,14 @@ StackFrame* StackwalkerPPC::GetCallerFrame(const CallStack* stack, StackFramePPC::CONTEXT_VALID_GPR1; frame->trust = StackFrame::FRAME_TRUST_FP; + // Should we terminate the stack walk? (end-of-stack or broken invariant) + if (TerminateWalk(instruction, + stack_pointer, + last_frame->context.gpr[1], + stack->frames()->size() == 1)) { + return NULL; + } + // frame->context.srr0 is the return address, which is one instruction // past the branch that caused us to arrive at the callee. Set // frame_ppc->instruction to four less than that. Since all ppc diff --git a/src/processor/stackwalker_ppc64.cc b/src/processor/stackwalker_ppc64.cc index 51c71fe5..4fd9e740 100644 --- a/src/processor/stackwalker_ppc64.cc +++ b/src/processor/stackwalker_ppc64.cc @@ -121,6 +121,14 @@ StackFrame* StackwalkerPPC64::GetCallerFrame(const CallStack* stack, StackFramePPC64::CONTEXT_VALID_GPR1; frame->trust = StackFrame::FRAME_TRUST_FP; + // Should we terminate the stack walk? (end-of-stack or broken invariant) + if (TerminateWalk(instruction, + stack_pointer, + last_frame->context.gpr[1], + stack->frames()->size() == 1)) { + return NULL; + } + // frame->context.srr0 is the return address, which is one instruction // past the branch that caused us to arrive at the callee. Set // frame_ppc64->instruction to eight less than that. Since all ppc64 diff --git a/src/processor/stackwalker_sparc.cc b/src/processor/stackwalker_sparc.cc index ff2ea75a..4de838af 100644 --- a/src/processor/stackwalker_sparc.cc +++ b/src/processor/stackwalker_sparc.cc @@ -111,6 +111,14 @@ StackFrame* StackwalkerSPARC::GetCallerFrame(const CallStack* stack, return NULL; } + // Should we terminate the stack walk? (end-of-stack or broken invariant) + if (TerminateWalk(instruction, + stack_pointer, + last_frame->context.g_r[14], + stack->frames()->size() == 1)) { + return NULL; + } + StackFrameSPARC* frame = new StackFrameSPARC(); frame->context = last_frame->context; diff --git a/src/processor/stackwalker_x86.cc b/src/processor/stackwalker_x86.cc index 46e26cf9..ed2b383d 100644 --- a/src/processor/stackwalker_x86.cc +++ b/src/processor/stackwalker_x86.cc @@ -659,15 +659,13 @@ StackFrame* StackwalkerX86::GetCallerFrame(const CallStack* stack, if (!new_frame.get()) return NULL; - // Treat an instruction address of 0 as end-of-stack. - if (new_frame->context.eip == 0) - return NULL; - - // If the new stack pointer is at a lower address than the old, then - // that's clearly incorrect. Treat this as end-of-stack to enforce - // progress and avoid infinite loops. - if (new_frame->context.esp <= last_frame->context.esp) + // Should we terminate the stack walk? (end-of-stack or broken invariant) + if (TerminateWalk(new_frame->context.eip, + new_frame->context.esp, + last_frame->context.esp, + frames.size() == 1)) { return NULL; + } // new_frame->context.eip is the return address, which is the instruction // after the CALL that caused us to arrive at the callee. Set |