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)

enhancement

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.
Blocks: 1378837
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
Assignee: ted → nobody

Mark as enhancement.

Type: task → enhancement

We're not going to get to this any time soon, so I'm bumping it down to P3.

Priority: P2 → P3

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

As it happens, this is being addressed in the rust-minidump codebase in this PR: https://github.com/rust-minidump/rust-minidump/pull/665

Yay! I'll tweak it to MOVED, then.

Thanks, Ted!

Resolution: WONTFIX → MOVED
You need to log in before you can comment on or make changes to this bug.