-
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
pe.import: Add get_imports_rva to SynthethicImportDirectoryEntry #278
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.
I haven’t had a chance to read the entirely of the thread prompting this, I’m afk intermittently for a bit, but this PR seems reasonable. Does it solve your major usecase ?
pub fn get_imports_rva<T: Bitfield<'a>>( | ||
&self, | ||
) -> error::Result<Vec<SyntheticImportAddressTableEntry<'a>>> { | ||
let mut out = Vec::<SyntheticImportAddressTableEntry>::new(); |
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.
I don’t see any fallibility here, so I’d drop the error return. Also I would attempt to get rid of allocation via returning a vec and try impl Iterator<Item = SyntheticImportAddressTableEntry<‘a>> + ‘a
(you may have to play with the lifetimes). Since it’s optional too you may have to do self.import_table.unwrap_or(&[]).iter().enumerate().flatmap
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.
Got it will implement it as an iterator
SyntheticImportLookupTableEntry::HintNameTableRVA((_, v)) => { | ||
let import_rva = (self.import_directory_entry.import_address_table_rva | ||
as u64) | ||
+ ((i * std::mem::size_of::<T>()) as u64); |
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.
You mentioned needing to know the size of the PE; this information is usually in the parsing context but I’m not sure if you have access to it here, haven’t looked at this code in a while. Or is the mem::size_of sufficient for this ?
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.
mem::size_of
is sufficient for this, but honestly passing a boolean called is_64bit
would be a cleaner option IMO.
@m4b Thanks for reviewing, I'll do the fixes and make sure the PR passes the tests. |
I've added an extra function called
get_imports_rva
To be honest I'm not sure about the correct interface so I'm leaving it like this and opening it to the discussion.
I've separated
get_imports_rva
according to the discussion in #171 , As saving this information was consideredwasteful
since it's easily calculated.The problem with this function is that it needs to know the bitness of the PE (whether it's 32bit or 64bit) to properly calculate the RVAs.
Also, I started working with rust a few days ago, so any tips or comments are more than welcome.
Thanks!