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

Support exports without offset #293

Merged
merged 2 commits into from
Dec 12, 2021
Merged

Support exports without offset #293

merged 2 commits into from
Dec 12, 2021

Conversation

dureuill
Copy link
Contributor

@dureuill dureuill commented Dec 2, 2021

This PR is a replacement for #292, which applies the change discussed there:

  • make offset an Option<_> in Export, and support when an Export has no offset.

Moreover, the correct commit of #292 (add missing subtraction) is retained.

Test

The same procedure as in #292 was followed.
This change does fix the issue #291 and the previously missing symbols are now returned for the tested binaries.

Thanks again for your time and consideration!

@dureuill
Copy link
Contributor Author

dureuill commented Dec 2, 2021

Ran cargo fmt and pushed again

Copy link
Owner

@m4b m4b left a comment

Choose a reason for hiding this comment

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

LGTM minus a comment might be nice for any future readers what's going on in that read_size computation; since this is a breaking change it won't go in until goblin 5.0, which I may wait for more breaking changes to be introduced

src/pe/utils.rs Show resolved Hide resolved
Copy link
Owner

@m4b m4b left a comment

Choose a reason for hiding this comment

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

LGTM, since this is a breaking change I'd like to roll it up potentially with other breaks

@m4b m4b merged commit 46b6a47 into m4b:master Dec 12, 2021
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.

2 participants