fix: Skip trailing garbage frames after _start
#514
Conversation
Our lack of support for `DW_CFA_undefined` can lead to conditions where we read a garbage return address via CFI. We observed this specifically for the `_start` function as generated for some linux compiler/libc combinations. This works around this by explicitly skipping a trailing unmapped frame following `_start`.
|
|
||
| for (index, mut frame) in thread.frames.into_iter().enumerate() { | ||
| while let Some((index, mut frame)) = frames_iter.next() { |
There was a problem hiding this comment.
I was sure that clippy would complain about this being equivalent to for (I realize that for doesn't work here due to borrowing).
There was a problem hiding this comment.
It did complain (as expected), but putting in the .peek() made it realize that we do need a mutable frames_iter to be around.
There was a problem hiding this comment.
Clippy really is quite amazing.
| // Certain linux compilers/libc combinations create a `DW_CFA_undefined: RIP` DWARF | ||
| // rule to say that `_start` has no return address. We do not support this and will |
There was a problem hiding this comment.
out of curiosity, is this undocumented behaviour? i'm wondering if it'd be useful to directly link to or reference a spec or source that mentions this.
There was a problem hiding this comment.
I can do that, it is stated here explicitly: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/x86_64/start.S;h=1b3e36826b8a477474cee24d1c931429fbdf6d8f;hb=HEAD#l59
| @@ -1316,6 +1312,23 @@ fn symbolicate_stacktrace( | |||
| continue; | |||
| } | |||
|
|
|||
| // Certain linux compilers/libc combinations create a `DW_CFA_undefined: RIP` DWARF | |||
| // rule to say that `_start` has no return address. We do not support this and will | |||
| // thus use the previous rule for RIP, which might look up the register value on the | |||
There was a problem hiding this comment.
just to be clear, what does the "previous rule" refer to in this case?
There was a problem hiding this comment.
CFI generated a table of rules, the previous one basically is the previous row. you can see an example here: https://github.com/getsentry/breakpad/blob/master/docs/symbol_files.md#stack-cfi-records
| && frames_iter.peek().is_none() | ||
| && frames.last().map_or(false, is_start) | ||
| { | ||
| continue; |
There was a problem hiding this comment.
do you suppose it'd be useful to eventually track these garbage frames (and other skipped frames) with metrics or logging eventually in the future?
There was a problem hiding this comment.
I don’t think this makes particular sense. We had a discussion recently about measuring with intent, and not just throw a metric on every little thing. I’m not sure how such a metric can drive us in a direction to improve this or not.
Our lack of support for
DW_CFA_undefinedcan lead to conditions where we read a garbage return address via CFI. We observed this specifically for the_startfunction as generated for some linux compiler/libc combinations.This works around this by explicitly skipping a trailing unmapped frame following
_start.Fixes https://getsentry.atlassian.net/browse/ISSUE-1247