Suyu/breakpad
1
0
Fork 0
mirror of https://git.suyu.dev/suyu/breakpad.git synced 2025-12-27 17:55:29 +01:00
Code Issues Projects Releases Packages Wiki Activity Actions

Make x86-64 frame pointer unwinding stricter

Browse source 
The x86-64 frame pointer-based unwind method will accept values
that aren't valid for the frame pointer register and the return address.
This fixes it to reject non-8-byte-aligned frame pointers, as
well as non-canonical addresses for the return address it finds.

A colleague of mine asked me why Breakpad gave a bad stack
for a crash in our crash-stats system:
https://crash-stats.mozilla.com/report/index/a472c842-2c7b-4ca7-a267-478cf2160405

Digging in, it turns out that the function in frame 0 is a leaf function,
so MSVC doesn't generate an entry in the unwind table for it, so
dump_syms doesn't produce a STACK CFI entry for it in the symbol file.
The stackwalker tries frame pointer unwinding, and %rbp is set to a
value that sort-of works, so it produces a garbage frame 1 and then
is lost. Either of the two checks in this patch would have stopped
the stackwalker from using the frame pointer.

It's possible we could do something smarter on the dump_syms side,
like enumerating all functions and outputing some default STACK CFI rule
for those that don't have unwind info, but that wouldn't fix crashes
from existing builds without re-dumping symbols for them. In any event,
these checks should always pass for valid frame pointer-using functions.

R=mark@chromium.org
BUG=https://bugzilla.mozilla.org/show_bug.cgi?id=1263001

Review URL: https://codereview.chromium.org/1902783002 .
This commit is contained in:
Ted Mielczarek 2016-04-19 15:20:09 -04:00
parent d48fa9d3a4
commit ea2e22b352
2 changed files with 169 additions and 51 deletions
Download patch file Download diff file Expand all files Collapse all files

17
src/processor/stackwalker_amd64.cc
View file

@ -164,6 +164,12 @@ bool StackwalkerAMD64::IsEndOfStack(uint64_t caller_rip, uint64_t caller_rsp,
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) {
return ptr > 0x7FFFFFFFFFFF && ptr < 0xFFFF800000000000;
}
StackFrameAMD64* StackwalkerAMD64::GetCallerByFramePointerRecovery(
const vector<StackFrame*>& frames) {
StackFrameAMD64* last_frame = static_cast<StackFrameAMD64*>(frames.back());
@ -186,11 +192,22 @@ StackFrameAMD64* StackwalkerAMD64::GetCallerByFramePointerRecovery(
// %caller_rip = *(%callee_rbp + 8)
// %caller_rbp = *(%callee_rbp)
// If rbp is not 8-byte aligned it can't be a frame pointer.
if (last_rbp % 8 != 0) {
return NULL;
}
uint64_t caller_rip, caller_rbp;
if (memory_->GetMemoryAtAddress(last_rbp + 8, &caller_rip) &&
memory_->GetMemoryAtAddress(last_rbp, &caller_rbp)) {
uint64_t caller_rsp = last_rbp + 16;
// If the recovered rip is not a canonical address it can't be
// the return address, so rbp must not have been a frame pointer.
if (is_non_canonical(caller_rip)) {
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) {