PE: parse rich header and refactor DOS stub parser #406
Conversation
|
I'm a bit tired, will fix the CI at the rest of today or tomorrow. |
There was a problem hiding this comment.
unfortunately there are some breaking changes here, but some seem unavoidable; primarily I'd like to see DosStub have it's parse method move into an impl DosStub and the &[u8] be reverted back to the DosStub type that was there before. otherwise it's looking ok!
|
@kkent030315 gentle ping; would definitely like to see this go in, but I had requested some changes :) |
NOTE: all `cargo test` passed
|
@m4b Hey, sorry for being shadow. I had do deal with the my life, you know.. anyways, a little bit changes have been applied to fix the code indeed, and the tests The current codebase in this PR is what I am used/using as an backend parser helper for my own binary rewriting framework that can partiolize the any PE64 binary into object-by-object (aka struct-by-struct, byte-by-byte etc) and this rich header code works perfectly with the 1000 unique PE64 binaries over the world (regardless of compiler toolchains, including but not limited to MSVC(link), Clang(LLD), MinGW-GCC, NOTE: Only MSVC linker inserts rich header) as well, so it's quite stable for now I believe, while there are something to deal with the coding style you pointed though.
I'm going to deal with another my PRs ASAP. I'd like to contribute more on goblin, as theres too many things to get rid of, and some insufficient features. |
|
I'm sorry I knew there was another PR I needed to check before releasing 9.0, I will give this a review, but probably won't be till weekend. Please feel free to ping me then if you haven't gotten any feedback. And thanks for your contributions! :) |
|
Also I hope everything in your life is good; don't feel need to push yourself either, your life comes first :) |
|
@m4b Thanks for taking attention on this! I think theres lots of things to discuess here. But I think breaking changes are inevitable for this PR, perhaps how about introducing this feature on 0.10? |
There was a problem hiding this comment.
Ok this is the last time, I think we're almost done here; as I mention in another comment, tl;dr I did not realize the heart of the issue is that the DosStub is variable and bounded by pe_pointer_offset; your changes look correct, assuming that statement is correct :) that being said, we should go back to your original intuition, as I mention, and the bytes should not be allocated/owned, but slices, so we have to pass a lifetime param to Header, etc. This is fine, this pattern is common in other parts of goblin, e.g., mach-o has a lot of that.
For bonus points, we can keep Copy, i'm pretty sure, on all the structs, if you do impl Iterator<Item=Result<RichMetadata>> for a method on RichHeader to avoid having a Vec inside of it.
After that, I Think this is ready to go, though do note it is a breaking change, a pretty good one, but I think it's worthwhile.
Oh and you have the honor of deleting the comment about adding RichHeader to the Header field, since you implemented it :)
|
@m4b Thank you very much for the review! I think it's almost perfect now. Would you able to take a look into the fixes? |
|
Maybe something to discuss here #406 (comment):
I can definitely do that work if we should. |
|
Self review is done. Looks perfect for me right now. |
|
@kkent030315 apologies I wanted to merge the parse_without_dos patch since it was before this, and was waiting a while; so you'll have to rebase, and add a rich_header parameter to the I appreciate your patience! |
|
So I just tried doing this rebase, and it's very annoying/tedious due to all the individual commits; i suggest squashing your patch down to a single commit first, and then rebasing on master to make it easiest. |
|
How about now? I resolved all conflicts wtih the upstream and should be able to merge with squash without problems. |
As of current implementation of rich header parser is always individual regardless of DOS stub, since it takes hardcoded offset of end of DOS stub, so rich header parser still works even if DOS stub is default (no rich header) by #[test]
fn parse_without_dos() {
let header = Header::parse_without_dos(&BORLAND_PE32_VALID_NO_RICH_HEADER).unwrap();
assert_eq!(header.dos_stub, DosStub::default());
assert_eq!(header.rich_header.is_none(), true);
// DOS stub is default but rich parser still works
let header = Header::parse_without_dos(&CORRECT_RICH_HEADER).unwrap();
assert_eq!(header.dos_stub, DosStub::default());
assert_eq!(header.rich_header.is_some(), true);
}I don't know if we should not parse rich header when |
|
@ideeockus do you have any opinion on the rich header question? |
| pub struct DosStub(pub [u8; 0x40]); | ||
| impl Default for DosStub { | ||
| pub struct DosStub<'a> { | ||
| pub data: &'a [u8], |
There was a problem hiding this comment.
I'm going to change this to private and have a method bytes() -> &'a [u8] in a separate patch; if we ever want to switch this to something like Cow<'a, [u8]> and have an owned version to allow Header<'a> will be easier
|
this was an epic PR @kkent030315 thanks for your patience! |
This PR adds parsing of Rich headers, as someone opened issue #400.
goblin::pe::header::RichHeadergoblin::pe::header::RichMetadataparsing if present, with success/fail/corrupted tests.I took the constant bytes in the test code from mthiesen/link-patcher (MIT). If this can potentially be license incompliance, I am happy to make own specimens for testing.