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

Be incredibly lenient when parsing ELFs #434

Merged
merged 6 commits into from Sep 28, 2021

Conversation

relaxolotl
Copy link
Contributor

This is a proof of concept PR to demonstrate an excessive solution that could be used to sidestep problems like the one mentioned in getsentry/sentry-dart#591, where only a portion of an ELF is malformed but the rest of it can be perfectly parsed. The ELF parser has been changed so that malformed ELFs could still be partially parsed out instead of eagerly failing at the first error.

The PR basically copy-pastes nearly everything from goblin's ELF parser, and replaces all short-circuited errors with a macro that returns the ELF object with everything successfully parsed up until the failure. A good second iteration on this would be to merely skip unparseable pieces but not short-circuit on the first failure, allowing us to, for example parse out dynamic symbols even if regular symbol parsing fails.

An additional field as been added to all object types to indicate whether it's malformed or not, in order to provide some feedback in the event that an ELF is malformed and cannot be fully parsed. Ideally this is properly used in other objects.

An ideal alternative to this would probably be to extend goblin's repertoire of lazy_parse helpers, following the idea mentioned in m4b/goblin#254 (comment) instead. Following that path would allow us to properly lazily parse the file instead of just copying line-for-line what Elf::parse() is doing.

@Swatinem Swatinem marked this pull request as ready for review September 28, 2021 07:42
@Swatinem Swatinem requested a review from a team as a code owner September 28, 2021 07:42
@Swatinem
Copy link
Member

Fixed test failures (snapshot updates, reading the interpreter correctly).

Should be good to go, if we really want to get this hotfixed.

@relaxolotl
Copy link
Contributor Author

Updated some docs, and made successful parsing of section headers a requirement to return a partially parsed ELF.

There are some follow-up actions that ideally should be made to refine this hotfix:

  • Ensure that all of the well-formed sections that can be parsed are parsed when there is a malformed section, ie make an active attempt to provide as many debug file features (symbol tables, unwinding, etc) as we can
  • Ensure that well-formed ELFs with zero usable information behave the same as before even with the previous bullet point
  • Unit tests to double check that the object API isn't broken in unexpected ways due to this partial parse

I'll also try to pursue the goal mentioned in the last paragraph of the description:

extend goblin's repertoire of lazy_parse helpers, following the idea mentioned in m4b/goblin#254 (comment)

@relaxolotl relaxolotl merged commit 9d308ef into master Sep 28, 2021
@relaxolotl relaxolotl deleted the ref/very-lenient-elf-parsing branch September 28, 2021 18:19
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.

None yet

3 participants