-
Notifications
You must be signed in to change notification settings - Fork 163
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
Fix parsing PE with zero raw_data_size of section #396
Changes from 3 commits
06cf965
088b6d6
ac439f7
2a5534c
802d4cf
f504af9
0874939
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 |
---|---|---|
|
@@ -543,6 +543,7 @@ pub struct Header { | |
} | ||
|
||
impl Header { | ||
/// Parses PE header from the given bytes | ||
pub fn parse(bytes: &[u8]) -> error::Result<Self> { | ||
let dos_header = DosHeader::parse(&bytes)?; | ||
let dos_stub = bytes.pread(DOS_STUB_OFFSET as usize).map_err(|_| { | ||
|
@@ -569,6 +570,30 @@ impl Header { | |
optional_header, | ||
}) | ||
} | ||
|
||
/// Parses PE header from the given bytes that not contains DOS stub, default DosHeader will be used | ||
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. grammar: Parses PE header from the given bytes, a default DosHeader and DosStub are generated, and any malformed header or stub is ignored. 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. thanks for suggestion, fixed |
||
pub fn parse_without_dos(bytes: &[u8]) -> error::Result<Self> { | ||
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. needs documentation if 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. @m4b Hello! This bug occurs when parsing file AmdCleanupUtility https://www.amd.com/en/resources/support-articles/faqs/GPU-601.html. Should I add 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.
Do you mean error on 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. What I'm asking is why is a new function being added, when does the user call it, and why can't we handle this transparently inside of the regular 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. we can handle this transparently inside of the regular
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. I just added docstrings for this two functions, is it ok now? 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. ok thank you for clarifying when this happens, and why to use; i've suggested grammar for the documentation of |
||
let dos_header = DosHeader::default(); | ||
let mut offset = dos_header.pe_pointer as usize; | ||
let signature = bytes.gread_with(&mut offset, scroll::LE).map_err(|_| { | ||
error::Error::Malformed(format!("cannot parse PE signature (offset {:#x})", offset)) | ||
})?; | ||
let coff_header = CoffHeader::parse(bytes, &mut offset)?; | ||
let optional_header = if coff_header.size_of_optional_header > 0 { | ||
let opt_hdr = bytes.pread::<optional_header::OptionalHeader>(offset)?; | ||
Some(opt_hdr) | ||
} else { | ||
None | ||
}; | ||
|
||
Ok(Header { | ||
dos_header, | ||
dos_stub: DosStub::default(), | ||
signature, | ||
coff_header, | ||
optional_header, | ||
}) | ||
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. I should have said this before, but all of this is basically identical, it might be nice to make a private function: fn parse_impl(bytes: &[u8]), header: DosHeader, stub: DosStub) -> Self {
// highlighted section
} and then the two pub functions call this respectively (your new function supply Default to the header and stub args) |
||
} | ||
} | ||
|
||
impl ctx::TryIntoCtx<scroll::Endian> for Header { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,6 +64,8 @@ fn section_read_size(section: §ion_table::SectionTable, file_alignment: u32) | |
|
||
if virtual_size == 0 { | ||
read_size | ||
} else if size_of_raw_data == 0 { | ||
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. i'm a little worried about this; this function is used all over goblin, can you add a comment why this check is done, and why it returns the virtual_size in this case? also there are no tests added to verify anything here 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. analogically to I think this is not normal, but can happen on different types of modified files, so parser should handle this. 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. also added test + sample for this case: took file from https://www.amd.com/en/resources/support-articles/faqs/GPU-601.html + packed with upx. is it ok if i add this file to tests? 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. I have no idea what the license of that binary is 😅 so probably let's not add it for now; also, it seems to parse just fine for me without this patch? e.g.:
has no issue, neither does 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. Parse Ok, test removed from PR |
||
virtual_size | ||
} else { | ||
cmp::min(read_size, round_size(virtual_size)) | ||
} | ||
|
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.
thank you for adding documentation here; can you add the part about how we will fail if dos stub or header is wrong?
e.g.: