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

Public utils + more examples #116

Merged
merged 11 commits into from
Dec 12, 2018
Merged

Public utils + more examples #116

merged 11 commits into from
Dec 12, 2018

Conversation

Pzixel
Copy link
Contributor

@Pzixel Pzixel commented Dec 9, 2018

Fixes #115

examples/dotnet_pe_analysis.rs Outdated Show resolved Hide resolved
@Pzixel
Copy link
Contributor Author

Pzixel commented Dec 9, 2018

Also renamed CLR -> CLI to match documentation

image

Copy link
Owner

@m4b m4b left a comment

Choose a reason for hiding this comment

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

This looks good to me modulo the dev dep, thanks for the PR!

Generic case is possible but you’ll need a TryFromCtx bound. Probably not worth the trouble for an example :)

Cargo.toml Outdated Show resolved Hide resolved
@kgv
Copy link

kgv commented Dec 10, 2018

@Pzixel

Also renamed CLR -> CLI to match documentation

See official microsoft pe format documentation it names CLR

@Pzixel
Copy link
Contributor Author

Pzixel commented Dec 10, 2018

@kgv hmm, looks like there is some mess with them. However, I based o ECMA335 (see link, page 280) which sould be more precise source.

@kgv
Copy link

kgv commented Dec 10, 2018

@Pzixel
Microsoft documentation has always been a mess.
However, ESMA-335 primarily concerns .NET specification. PE format is not only .NET.

In my opinion, to make changes that are consistent with the documentation and ESMA-335, you should send an issue to https://github.com/MicrosoftDocs, and then, when it is approved, change it in this repo.

@Pzixel
Copy link
Contributor Author

Pzixel commented Dec 10, 2018

Yep, but ECMA-335 conserns CLI specification (not .Net as you said) and this header is specifically CLI header, so it must be the most competent in this question.

However, you are right that docs should be updated. I'm creating an issue. However, I don't see corresponding repository for it.

@kgv
Copy link

kgv commented Dec 10, 2018

Check it: https://github.com/MicrosoftDocs/feedback

@m4b
Copy link
Owner

m4b commented Dec 10, 2018

So what is the resolution (if any) for CLR/CLI data directory naming ? I would prefer to not make a breaking change and I used the original documentation that @kgv linked. We can always update it in a later PR if need be. And thanks for both your investigations here!

@Pzixel
Copy link
Contributor Author

Pzixel commented Dec 10, 2018

@m4b I created a PR in MS documentation repo, however, it keeps silent. It looks like it takes 2-3 days until someone react on it.

I can revert this change if you wish, OTOH I have strong feeling that CLI specification is more compenent in this question.

@Pzixel
Copy link
Contributor Author

Pzixel commented Dec 10, 2018

I reverted rename change.

P.S. I wonder why you keep rust 1.20. It doesn't make sense to me since rust is pretty well backward compatible.

@Pzixel
Copy link
Contributor Author

Pzixel commented Dec 11, 2018

Ping?

Copy link
Owner

@m4b m4b left a comment

Choose a reason for hiding this comment

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

Modulo comments I think this is ready to go. I will probably squash the commits since many of them aren’t informative or are fixes

src/pe/utils.rs Outdated Show resolved Hide resolved
src/pe/utils.rs Outdated
@@ -74,3 +76,24 @@ pub fn try_name<'a>(bytes: &'a [u8], rva: usize, sections: &[section_table::Sect
}
}
}

pub fn get_data<'a, T>(bytes: &'a [u8], sections: &[section_table::SectionTable], directory: &DataDirectory, file_alignment: u32) -> Result<T, DataDirectoryConversionError>
Copy link
Owner

Choose a reason for hiding this comment

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

This seems like it belongs as a method in DataDirectories ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, because it belongs to SectionTable as well.. Utils is the good place for this imo

src/pe/utils.rs Outdated Show resolved Hide resolved
@Pzixel
Copy link
Contributor Author

Pzixel commented Dec 11, 2018

Pong?

Copy link
Owner

@m4b m4b left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for this @Pzixel !

@m4b m4b merged commit ab252b0 into m4b:master Dec 12, 2018
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.

3 participants