-
Notifications
You must be signed in to change notification settings - Fork 156
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
base: master
Are you sure you want to change the base?
Conversation
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.
it seems like there are two changes, one adding a new public function, and the other fixing a bug in the rva helper function, yes? need to understand why you need the new pub function, but the other fix probably is ok, though i would prefer a test showing how it fixes a bug
@@ -569,6 +569,29 @@ impl Header { | |||
optional_header, | |||
}) | |||
} | |||
|
|||
pub fn parse_without_dos(bytes: &[u8]) -> error::Result<Self> { |
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.
needs documentation if pub
; i also want to understand what's different here and why this can't be the original function for parsing?
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.
@m4b Hello! This bug occurs when parsing file AmdCleanupUtility https://www.amd.com/en/resources/support-articles/faqs/GPU-601.html. Should I add .exe
file to tests?
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.
need to understand why you need the new pub function
Do you mean error on DosHeader::parse(&bytes)?
should be handled inside original Header::parse
?
And try to parse without DosHeader in case of Error?
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.
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 parse
function? Ideally I'd prefer not to add another pub
function if possible; maybe it is not possible, I don't know
Sometimes I need to parse PE header without parsing DOS header + DOS stub
Seems that finding offset by rva is not implemented correctly and breaks down on some PE files. See 'cannot map exception_rva (0x5e000) into offset' when parsing PE32+ library #307