-
Notifications
You must be signed in to change notification settings - Fork 195
fix(pe): only map file-backed sections to offsets #527
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,9 +51,17 @@ fn section_read_size(section: §ion_table::SectionTable, file_alignment: u32) | |
| // { | ||
| // readsize = min(readsize, (virtsize + 0xfff) & ~0xfff); | ||
| // } | ||
| // | ||
| // Only bytes that are actually backed by the file may be mapped into offsets. | ||
| // Sections with `SizeOfRawData == 0` are virtual-only when parsing an on-disk PE, | ||
| // even if they have a non-zero `VirtualSize`. | ||
|
|
||
| let file_alignment = file_alignment as usize; | ||
| let size_of_raw_data = section.size_of_raw_data as usize; | ||
| if size_of_raw_data == 0 { | ||
| return 0; | ||
| } | ||
|
|
||
| let file_alignment = file_alignment as usize; | ||
| let virtual_size = section.virtual_size as usize; | ||
| let read_size = { | ||
| let read_size = | ||
|
|
@@ -65,8 +73,6 @@ fn section_read_size(section: §ion_table::SectionTable, file_alignment: u32) | |
|
|
||
| if virtual_size == 0 { | ||
| read_size | ||
| } else if read_size == 0 { | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only question I have is regressions in other cases that might happen because this check was removed; to be fair, I can't remember why it was added, and there's no comment explaining the logic here, so if this fixes a big bug for you, and all our tests pass, I'm biased towards just merging.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The actual fix is the early exit on
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check was added in #396. It was added to fix parsing of the file in https://github.com/ideeockus/goblin/blob/f504af9de4c86bdf697ceb6876f2683c0226886f/tests/bins/pe/exception_rva_mapping/amdcleanuputility.exe.upx_packed, which was giving the same error as #307. That error reappears with this PR regardless of whether this check is removed or not (the earlier early return is enough to cause it). I think the change in this PR is fine, and #396 should be fixed differently (use the permissive parsing option). |
||
| virtual_size | ||
| } else { | ||
| cmp::min(read_size, round_size(virtual_size)) | ||
| } | ||
|
|
@@ -262,3 +268,45 @@ where | |
| debug_assert!(align != 0u8.into(), "Align must be non-zero"); | ||
| (value + align - 1u8.into()) & !(align - 1u8.into()) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::{find_offset, section_read_size}; | ||
| use crate::pe::options::ParseOptions; | ||
| use crate::pe::section_table::SectionTable; | ||
|
|
||
| fn section( | ||
| virtual_address: u32, | ||
| virtual_size: u32, | ||
| size_of_raw_data: u32, | ||
| pointer_to_raw_data: u32, | ||
| ) -> SectionTable { | ||
| SectionTable { | ||
| virtual_address, | ||
| virtual_size, | ||
| size_of_raw_data, | ||
| pointer_to_raw_data, | ||
| ..SectionTable::default() | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn virtual_only_sections_are_not_file_backed() { | ||
| let section = section(0x1fb000, 0x28590, 0, 0); | ||
| assert_eq!(section_read_size(§ion, 0x200), 0); | ||
| assert_eq!( | ||
| find_offset(0x215580, &[section], 0x200, &ParseOptions::default()), | ||
| None, | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn file_backed_sections_still_map_rvas() { | ||
| let section = section(0x1000, 0x600, 0x400, 0x200); | ||
| assert_eq!(section_read_size(§ion, 0x200), 0x400); | ||
| assert_eq!( | ||
| find_offset(0x1123, &[section], 0x200, &ParseOptions::default()), | ||
| Some(0x323), | ||
| ); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kkent030315 could you review changes in this file? I generally defer to you on PE stuff :)