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

Calculate number of read bytes for sections using file alignment #101

Merged
merged 1 commit into from
Aug 23, 2018

Conversation

tathanhdinh
Copy link
Contributor

@tathanhdinh tathanhdinh commented Aug 9, 2018

Hello all,

The pointer_to_raw_data of each section should be rounded down to a multiple of 512. Moreover, the valid address range of each section should be calculated using also size_of_raw_data and file_alignment, e.g. quine.exe has virtual_size (of its unique section) is zero, but it is a valid PE.

To make the review becomes easier, I split the previous PR (which is closed), into parts.

Many thanks for any comments.

@tathanhdinh tathanhdinh force-pushed the section_map_size branch 2 times, most recently from ac50988 to fae9100 Compare August 9, 2018 17:10
@m4b
Copy link
Owner

m4b commented Aug 10, 2018

heh; i assumed the update branch button would rebase your commits on top; but it does not... what a pain in the ass

@m4b
Copy link
Owner

m4b commented Aug 20, 2018

I think this looks good to me, but I don't really have the bandwidth at the moment to fully review this. @philipc @willglynn (or anyone else) do you have any comments here? I'm inclined to just merge since it seems more or less fine.

src/pe/utils.rs Outdated
read_size
} else {
raw_data_round_size
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is clearer to do: cmp::min(read_size, round_size(size_of_raw_data)) (and same for virtual size).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@philipc thanks for your comments. It's is indeed clearer with cmp::min

@philipc
Copy link
Collaborator

philipc commented Aug 20, 2018

Looks fine, assuming the linked stackoverflow answer is correct (which is not something I can judge, this is all undocumented, in fact it's disallowed by the documentation...). From what I've read, this only applies to PE executables, not COFF objects, so this will need changing if we reuse this code for COFF.

@philipc
Copy link
Collaborator

philipc commented Aug 22, 2018

@tathanhdinh Can you rebase this to remove the extra 3 commits?

@tathanhdinh
Copy link
Contributor Author

@philipc Thanks for the remarks, the rebase has been done.

Copy link
Collaborator

@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 for your patience!

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