Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for parsing breakpad sym files containing INLINE_ORIGIN and INLINE records #605

Merged
merged 3 commits into from
Jun 28, 2022

Conversation

mstange
Copy link
Contributor

@mstange mstange commented Jun 25, 2022

Fixes #432.

Please review commits individually.

@mstange mstange requested a review from a team June 25, 2022 23:02
@codecov-commenter
Copy link

Codecov Report

Merging #605 (e9925e2) into master (33fd1b0) will increase coverage by 0.63%.
The diff coverage is 96.91%.

@@            Coverage Diff             @@
##           master     #605      +/-   ##
==========================================
+ Coverage   68.24%   68.88%   +0.63%     
==========================================
  Files          83       83              
  Lines       16434    16827     +393     
==========================================
+ Hits        11216    11591     +375     
- Misses       5218     5236      +18     

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very nice!

There is a bunch of asserts and unwraps, I wonder how resilient this is to malformed breakpad files? Is it worth the effort to run this through a fuzzer? Or proptest that programmatically generates inlinee and line records?

@Swatinem
Copy link
Member

Also, "quash and merge" is the only thing available in this repo, so if you want to retain at least the first commit separately, you would need to open that as its own PR.

Copy link
Contributor

@loewenheim loewenheim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. What's your reasoning for adding stuff to shared.rs? AFAICT it's only used for Breakpad processing.

symbolic-debuginfo/src/breakpad.rs Outdated Show resolved Hide resolved
@loewenheim
Copy link
Contributor

There is a bunch of asserts and unwraps, I wonder how resilient this is to malformed breakpad files? Is it worth the effort to run this through a fuzzer? Or proptest that programmatically generates inlinee and line records?

I'm pretty sure all of these are actually infallible, irrespective of whether the Breakpad file is well-formed. They only unwrap if it has been checked immediately before that an element exists. Some of this stuff could arguably be rewritten in terms of matching, but I'm not sure it's worth the hassle.

@loewenheim
Copy link
Contributor

Also I opened #609 to use saturating_add in Function::end_address, so you can use that method if you like. We can also do it as follow-up.

@Swatinem
Copy link
Member

Fair enough, I looked again at the FunctionBuilder, and all the unwrap/assert look to be unreachable. I wonder if the compiler is smart enough to optimize all that away as well.

I also agree that the FunctionBuilder should maybe move into its own file, so we can get rid of a bunch of cfg switches.

@mstange
Copy link
Contributor Author

mstange commented Jun 27, 2022

Thanks for the fast reviews! I'll open a new PR for just the first commit, as Arpad suggested.

Creating a new file for FunctionBuilder sounds good, I'll do that. I didn't put it into breakpad.rs because #607 adds another use of it in the DWARF parser.

@mstange
Copy link
Contributor Author

mstange commented Jun 27, 2022

I've addressed the review comments.

@mstange
Copy link
Contributor Author

mstange commented Jun 27, 2022

I wonder how resilient this is to malformed breakpad files? Is it worth the effort to run this through a fuzzer?

It might be worth it, just for added confidence. But I'm not too concerned about the unwraps and asserts, as you already said I think they're all unreachable.

There's one unchecked addition which could overflow on malicious input on 32 bit platforms (and panic debug builds), and that's the one in flush_depth:

    pub fn flush_depth(&mut self, depth: u32) {
        while self.stack.len() > depth as usize + 1 {

I've chosen to not care about it, but I can fix it if you'd like me to.

Fixes getsentry#432.

The breakpad format represents lines and inlinees in the same way as
DWARF: Line records decribe locations in the "innermost" function on the
inline stack at that address, and inlinees have "call" locations.
So a similar conversion to a tree structure is performed as in the DWARF
parser.

The new test based on crash.inlines.sym isn't perfect yet; the .sym file
contains some inconsistent pieces of information for inline functions
with multiple address ranges.
Once the underlying bug has been fixed (getsentry#603), I intend to create a
correct .sym file and the test will be better.
@Swatinem Swatinem merged commit 7bf7275 into getsentry:master Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for INLINE and INLINE_ORIGIN entries when parsing .sym files
4 participants