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

Added PE utils for sections and directories #420

Merged
merged 3 commits into from
Jan 21, 2022

Conversation

daladim
Copy link
Contributor

@daladim daladim commented Jan 10, 2022

Hello,

Here are a few utility functions that I'm currently using in my own projects, but that I think everyone could benefit from, so this could make sense to upstream them :-)

@daladim daladim changed the title Added section and dir utils Added PE utils for sections and directories Jan 10, 2022
@daladim daladim force-pushed the added_section_and_dir_utils branch 2 times, most recently from aeca852 to 7e8ee05 Compare January 10, 2022 14:11
@philipc
Copy link
Contributor

philipc commented Jan 11, 2022

Thanks for the PR! I've reimplemented ImageDataDirectory::file_offset_range in #421 for better consistency with the existing APIs. Did you still need Section::contains_rva and SectionTable::section_at or were they only to support ImageDataDirectory::file_offset_range?

@daladim
Copy link
Contributor Author

daladim commented Jan 11, 2022

Thanks for this other PR, that's indeed probably more suitable as what I proposed :-)

Regarding Section::contains_rva and SectionTable::section_at, I happen to use SectionTable::section_at in my projects (but not contains_rva, at least not directly).
I'd not be surprised that other people as well could need both of them one day (in case they want to do some kind of manual parsing or displaying of PE files themselves). Thus, I think that would make sense to still provide them in the public API, but that's up to you to decide (and if you want me to, I can add a commit on top your MR to implement them).

@philipc
Copy link
Contributor

philipc commented Jan 11, 2022

They're still fine to add. You can probably update this PR, since I won't merge the other yet. Maybe rename section_at to something pe_section_containing or something like that, and rename contains_rva to pe_contains_rva (since these types are used for both PE and COFF, but these functions only apply to PE).

src/read/pe/section.rs Outdated Show resolved Hide resolved
@daladim
Copy link
Contributor Author

daladim commented Jan 19, 2022

Sorry for the delay, I was quite busy last week.
Since I saw you've merged #421, I have rebased on 0.28.3, and I have added Section::contains_rva and SectionTable::section_at, which is quite a trivial change now.

src/read/pe/section.rs Outdated Show resolved Hide resolved
Co-authored-by: Philip Craig <philipjcraig@gmail.com>
Copy link
Contributor

@philipc philipc left a comment

Choose a reason for hiding this comment

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

Thanks!

@philipc philipc merged commit 927b511 into gimli-rs:master Jan 21, 2022
mcbegamerxx954 pushed a commit to mcbegamerxx954/object that referenced this pull request Jun 15, 2024
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

2 participants