diff options
author | Leonard Mosescu <mosescu@chromium.org> | 2017-07-11 12:26:50 -0700 |
---|---|---|
committer | Leonard Mosescu <mosescu@chromium.org> | 2017-07-12 17:53:15 +0000 |
commit | 01431c2f61aa2af1804f1e139da9bc7c4afa9e7b (patch) | |
tree | d26ce2a6e690edd61f73488ecce56bc04a53be60 /src/processor/stackwalker_amd64.cc | |
parent | A couple of minor fixes (diff) | |
download | breakpad-01431c2f61aa2af1804f1e139da9bc7c4afa9e7b.tar.xz |
Handle very large stack traces
The main motivation for this change is to handle very large stack
traces, normally the result of infinite recursion. This part is
actually fairly simple, relaxing a few self-imposed limits on how
many frames we can unwind and the max size for stack memory.
Relaxing these limits requires stricter and more consistent checks for
stack unwinding. There are a number of unwinding invariants that apply
to all the platforms:
1. stack pointer (and frame pointer) must be within the stack memory
(frame pointer, if preset, must point to the right frame too)
2. unwinding must monotonically increase SP
(except for the first frame unwind, this must be a strict increase)
3. Instruction pointer (return address) must point to a valid location
4. stack pointer (and frame pointer) must be appropriately aligned
This change is focused on 2), which is enough to guarantee that the
unwinding doesn't get stuck in an infinite loop.
1) is implicitly validated part of accessing the stack memory
(explicit checks might be nice though).
4) is ABI specific and while it may be valuable in catching suspicious
frames is not in the scope of this change.
3) is also an interesting check but thanks to just-in-time compilation
it's more complex than just calling
StackWalker::InstructionAddressSeemsValid()
and we don't want to drop parts of the callstack due to an overly
conservative check.
Bug: chromium:735989
Change-Id: I9aaba77c7fd028942d77c87d51b5e6f94e136ddd
Reviewed-on: https://chromium-review.googlesource.com/563771
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Ivan Penkov <ivanpe@chromium.org>
Diffstat (limited to 'src/processor/stackwalker_amd64.cc')
-rw-r--r-- | src/processor/stackwalker_amd64.cc | 30 |
1 files changed, 5 insertions, 25 deletions
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; } |