Closed
Bug 1378836
Opened 7 years ago
Closed 2 years ago
Make stackwalker detect whether crash address is likely wrong and output something
Categories
(Socorro :: Processor, enhancement, P3)
Socorro
Processor
Tracking
(Not tracked)
RESOLVED
MOVED
People
(Reporter: ted, Unassigned)
References
Details
In bug 974420 and bug 1002564 we realized that for certain kinds of crashes the crash address is incorrect.
Specifically, on x86-64, when a non-canonical address is referenced (https://en.wikipedia.org/wiki/X86-64#Canonical_form_addresses) the CPU raises a General Protection Fault that does not contain the faulting address, so operating systems put misleading data into the error structures. On Windows the crash address shows up as `0xFFFFFFFFFFFFFFFF`, on Linux and OS X it shows up as `0`.
We hit this a lot because our poison pattern is intentionally non-canonical, because there's no way for it to accidentally be located in usable memory. Unfortunately this means that a lot of crashes that are actually security-sensitive do not appear as such when viewed on crash-stats.
I have a patch against Breakpad that can find the actual faulting address much of the time, but it's very large (it requires a disassembler to disassemble the faulting instruction):
https://github.com/luser/breakpad/commit/c8c0bbcdba178b74db171c21d2c6f4a59efd0131
We can probably at least make this less bad by having our stackwalker replicate the heuristic from that patch:
https://github.com/luser/breakpad/commit/c8c0bbcdba178b74db171c21d2c6f4a59efd0131#diff-c95300c103ae8070d8bcee88ba40fe69R716
...and generate something in its output like `"crash_address_likely_wrong": true`. We could then highlight the crash address in the report/index view so people would know that it's not reliable.
Reporter | ||
Updated•7 years ago
|
Reporter | ||
Comment 1•7 years ago
|
||
Comment 2•7 years ago
|
||
Upping this to a P2 because as Ted says "i would like to have that functionality somehow--people get really confused by it".
In the meanwhile, we closed out PR 3853. It'd be good to try another approach, but I'm not sure what that would entail.
Priority: -- → P2
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Updated•6 years ago
|
Assignee: ted → nobody
Comment 4•5 years ago
|
||
We're not going to get to this any time soon, so I'm bumping it down to P3.
Priority: P2 → P3
Comment 5•2 years ago
|
||
We have a new stackwalker and it's got feature requests and all that in its own tracker here:
https://github.com/rust-minidump/rust-minidump/issues
If this is something that the new stackwalker doesn't do and we still want to do it, we should write up a new issue there. In the meantime, I'm going to mark this as WONTFIX because we no longer work on the old stackwalker.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
Reporter | ||
Comment 6•2 years ago
|
||
As it happens, this is being addressed in the rust-minidump codebase in this PR: https://github.com/rust-minidump/rust-minidump/pull/665
You need to log in
before you can comment on or make changes to this bug.
Description
•