From c685fe1153193853cff23e83d6d2b2c577aaa5f6 Mon Sep 17 00:00:00 2001 From: Ivan Penkov Date: Tue, 1 Mar 2022 21:01:34 +0000 Subject: [PATCH] Better identification of context frames. Since the introduction of inlined frames, it is not sufficient to check the stack trace length (== 1) in order to identify context frames. Updating all location that were depending on this assumption to check for frame trust level instead. Change-Id: I98f966889367c2270c268b8e78b67418c89c50f1 Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3499020 Reviewed-by: Mark Mentovai --- src/processor/stackwalker_amd64.cc | 12 ++++++++---- src/processor/stackwalker_arm.cc | 6 ++++-- src/processor/stackwalker_arm64.cc | 6 ++++-- src/processor/stackwalker_mips.cc | 3 ++- src/processor/stackwalker_ppc.cc | 7 +++---- src/processor/stackwalker_ppc64.cc | 7 +++---- src/processor/stackwalker_sparc.cc | 7 +++---- src/processor/stackwalker_x86.cc | 27 +++++++++++++++------------ 8 files changed, 42 insertions(+), 33 deletions(-) diff --git a/src/processor/stackwalker_amd64.cc b/src/processor/stackwalker_amd64.cc index 0fc22a4f..b7628ac3 100644 --- a/src/processor/stackwalker_amd64.cc +++ b/src/processor/stackwalker_amd64.cc @@ -223,7 +223,7 @@ StackFrameAMD64* StackwalkerAMD64::GetCallerByFramePointerRecovery( StackFrameAMD64* StackwalkerAMD64::GetCallerBySimulatingReturn( const vector& frames) { - assert(frames.size() == 1); + assert(frames.back()->trust == StackFrame::FRAME_TRUST_CONTEXT); StackFrameAMD64* last_frame = static_cast(frames.back()); uint64_t last_rsp = last_frame->context.rsp; uint64_t caller_rip_address, caller_rip; @@ -257,7 +257,8 @@ StackFrameAMD64* StackwalkerAMD64::GetCallerByStackScan( uint64_t caller_rip_address, caller_rip; if (!ScanForReturnAddress(last_rsp, &caller_rip_address, &caller_rip, - frames.size() == 1 /* is_context_frame */)) { + /*is_context_frame=*/last_frame->trust == + StackFrame::FRAME_TRUST_CONTEXT)) { // No plausible return address was found. return NULL; } @@ -322,7 +323,8 @@ StackFrame* StackwalkerAMD64::GetCallerFrame(const CallStack* stack, // According to https://reviews.llvm.org/D24748, LLVM doesn't generate unwind // info for such functions. According to MSDN, leaf functions can be unwound // simply by simulating a return. - if (!new_frame.get() && stack->frames()->size() == 1 && + if (!new_frame.get() && + last_frame->trust == StackFrame::FRAME_TRUST_CONTEXT && system_info_->os_short == "windows") { new_frame.reset(GetCallerBySimulatingReturn(frames)); } @@ -356,7 +358,9 @@ StackFrame* StackwalkerAMD64::GetCallerFrame(const CallStack* 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)) { + last_frame->context.rsp, + /*first_unwind=*/last_frame->trust == + StackFrame::FRAME_TRUST_CONTEXT)) { return NULL; } diff --git a/src/processor/stackwalker_arm.cc b/src/processor/stackwalker_arm.cc index f6f2c9bf..7890cbe3 100644 --- a/src/processor/stackwalker_arm.cc +++ b/src/processor/stackwalker_arm.cc @@ -168,7 +168,8 @@ StackFrameARM* StackwalkerARM::GetCallerByStackScan( uint32_t caller_sp, caller_pc; if (!ScanForReturnAddress(last_sp, &caller_sp, &caller_pc, - frames.size() == 1 /* is_context_frame */)) { + /*is_context_frame=*/last_frame->trust == + StackFrame::FRAME_TRUST_CONTEXT)) { // No plausible return address was found. return NULL; } @@ -276,7 +277,8 @@ StackFrame* StackwalkerARM::GetCallerFrame(const CallStack* stack, 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)) { + /*first_unwind=*/last_frame->trust == + StackFrame::FRAME_TRUST_CONTEXT)) { return NULL; } diff --git a/src/processor/stackwalker_arm64.cc b/src/processor/stackwalker_arm64.cc index 9acf8188..74410c9a 100644 --- a/src/processor/stackwalker_arm64.cc +++ b/src/processor/stackwalker_arm64.cc @@ -181,7 +181,8 @@ StackFrameARM64* StackwalkerARM64::GetCallerByStackScan( uint64_t caller_sp, caller_pc; if (!ScanForReturnAddress(last_sp, &caller_sp, &caller_pc, - frames.size() == 1 /* is_context_frame */)) { + /*is_context_frame=*/last_frame->trust == + StackFrame::FRAME_TRUST_CONTEXT)) { // No plausible return address was found. return NULL; } @@ -320,7 +321,8 @@ StackFrame* StackwalkerARM64::GetCallerFrame(const CallStack* stack, 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)) { + /*first_unwind=*/last_frame->trust == + StackFrame::FRAME_TRUST_CONTEXT)) { return NULL; } diff --git a/src/processor/stackwalker_mips.cc b/src/processor/stackwalker_mips.cc index c33ecdbe..c4ee21f3 100644 --- a/src/processor/stackwalker_mips.cc +++ b/src/processor/stackwalker_mips.cc @@ -276,7 +276,8 @@ StackFrame* StackwalkerMIPS::GetCallerFrame(const CallStack* stack, 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)) { + /*first_unwind=*/last_frame->trust == + StackFrame::FRAME_TRUST_CONTEXT)) { return NULL; } diff --git a/src/processor/stackwalker_ppc.cc b/src/processor/stackwalker_ppc.cc index 1e34c383..b5dab899 100644 --- a/src/processor/stackwalker_ppc.cc +++ b/src/processor/stackwalker_ppc.cc @@ -132,10 +132,9 @@ StackFrame* StackwalkerPPC::GetCallerFrame(const CallStack* stack, 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)) { + if (TerminateWalk(instruction, stack_pointer, last_frame->context.gpr[1], + /*first_unwind=*/last_frame->trust == + StackFrame::FRAME_TRUST_CONTEXT)) { return NULL; } diff --git a/src/processor/stackwalker_ppc64.cc b/src/processor/stackwalker_ppc64.cc index fb2bac3c..f7b24a2b 100644 --- a/src/processor/stackwalker_ppc64.cc +++ b/src/processor/stackwalker_ppc64.cc @@ -123,10 +123,9 @@ StackFrame* StackwalkerPPC64::GetCallerFrame(const CallStack* stack, 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)) { + if (TerminateWalk(instruction, stack_pointer, last_frame->context.gpr[1], + /*is_context_frame=*/last_frame->trust == + StackFrame::FRAME_TRUST_CONTEXT)) { return NULL; } diff --git a/src/processor/stackwalker_sparc.cc b/src/processor/stackwalker_sparc.cc index 4de838af..df58570d 100644 --- a/src/processor/stackwalker_sparc.cc +++ b/src/processor/stackwalker_sparc.cc @@ -112,10 +112,9 @@ StackFrame* StackwalkerSPARC::GetCallerFrame(const CallStack* stack, } // 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)) { + if (TerminateWalk(instruction, stack_pointer, last_frame->context.g_r[14], + /*is_context_frame=*/last_frame->trust == + StackFrame::FRAME_TRUST_CONTEXT)) { return NULL; } diff --git a/src/processor/stackwalker_x86.cc b/src/processor/stackwalker_x86.cc index c11a04d5..41acaae4 100644 --- a/src/processor/stackwalker_x86.cc +++ b/src/processor/stackwalker_x86.cc @@ -394,9 +394,10 @@ StackFrameX86* StackwalkerX86::GetCallerByWindowsFrameInfo( // frame pointer. uint32_t location_start = last_frame->context.esp; uint32_t location, eip; - if (!stack_scan_allowed - || !ScanForReturnAddress(location_start, &location, &eip, - frames.size() == 1 /* is_context_frame */)) { + if (!stack_scan_allowed || + !ScanForReturnAddress(location_start, &location, &eip, + /*is_context_frame=*/last_frame->trust == + StackFrame::FRAME_TRUST_CONTEXT)) { // if we can't find an instruction pointer even with stack scanning, // give up. return NULL; @@ -438,9 +439,10 @@ StackFrameX86* StackwalkerX86::GetCallerByWindowsFrameInfo( // looking one 32-bit word above that location. uint32_t location_start = dictionary[".raSearchStart"] + 4; uint32_t location; - if (stack_scan_allowed - && ScanForReturnAddress(location_start, &location, &eip, - frames.size() == 1 /* is_context_frame */)) { + if (stack_scan_allowed && + ScanForReturnAddress(location_start, &location, &eip, + /*is_context_frame=*/last_frame->trust == + StackFrame::FRAME_TRUST_CONTEXT)) { // 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. @@ -596,9 +598,10 @@ 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 (!stack_scan_allowed - || !ScanForReturnAddress(last_esp, &caller_esp, &caller_eip, - frames.size() == 1 /* is_context_frame */)) { + if (!stack_scan_allowed || + !ScanForReturnAddress(last_esp, &caller_esp, &caller_eip, + /*is_context_frame=*/last_frame->trust == + StackFrame::FRAME_TRUST_CONTEXT)) { // if we can't find an instruction pointer even with stack scanning, // give up. return NULL; @@ -678,10 +681,10 @@ StackFrame* StackwalkerX86::GetCallerFrame(const CallStack* stack, return NULL; // Should we terminate the stack walk? (end-of-stack or broken invariant) - if (TerminateWalk(new_frame->context.eip, - new_frame->context.esp, + if (TerminateWalk(new_frame->context.eip, new_frame->context.esp, last_frame->context.esp, - frames.size() == 1)) { + /*first_unwind=*/last_frame->trust == + StackFrame::FRAME_TRUST_CONTEXT)) { return NULL; }