Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 1902783002: Make x86-64 frame pointer unwinding stricter (Closed)

Created:
3 years, 4 months ago by Ted Mielczarek
Modified:
3 years, 4 months ago
Reviewers:
Mark Mentovai
CC:
google-breakpad-dev_googlegroups.com
Base URL:
https://chromium.googlesource.com/breakpad/breakpad.git@master
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Make x86-64 frame pointer unwinding stricter 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 Committed: https://chromium.googlesource.com/breakpad/breakpad/+/ea2e22b3526754d5d60a405b6c8657f37a4d066a

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+169 lines, -51 lines) Patch
M src/processor/stackwalker_amd64.cc View 2 chunks +17 lines, -0 lines 0 comments Download
M src/processor/stackwalker_amd64_unittest.cc View 26 chunks +152 lines, -51 lines 0 comments Download

Messages

Total messages: 4 (1 generated)
Ted Mielczarek
3 years, 4 months ago (2016-04-19 14:21:13 UTC) #1
Mark Mentovai
LGTM
3 years, 4 months ago (2016-04-19 15:26:28 UTC) #2
Ted Mielczarek
3 years, 4 months ago (2016-04-19 19:20:14 UTC) #4
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as
ea2e22b3526754d5d60a405b6c8657f37a4d066a (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698