Fix parsing of PE32+ import tables #82
Conversation
src/pe/import.rs
Outdated
| @@ -26,41 +60,37 @@ impl<'a> HintNameTableEntry<'a> { | |||
| #[derive(Debug, Clone)] | |||
| pub enum SyntheticImportLookupTableEntry<'a> { | |||
| OrdinalNumber(u16), | |||
| HintNameTableRVA ((u32, HintNameTableEntry<'a>)), // [u8; 31] bitfield :/ | |||
| HintNameTableRVA ((usize, HintNameTableEntry<'a>)), // [u8; 31] bitfield :/ | |||
There was a problem hiding this comment.
Why is this changed to usize?
There was a problem hiding this comment.
Never mind, I see it now. :)
There was a problem hiding this comment.
As far as I can tell, it didn't need to change, and u32 seems more correct to me, since it's never more than 32 bits even for P32+.
src/pe/import.rs
Outdated
| @@ -26,41 +60,37 @@ impl<'a> HintNameTableEntry<'a> { | |||
| #[derive(Debug, Clone)] | |||
| pub enum SyntheticImportLookupTableEntry<'a> { | |||
| OrdinalNumber(u16), | |||
| HintNameTableRVA ((u32, HintNameTableEntry<'a>)), // [u8; 31] bitfield :/ | |||
| HintNameTableRVA ((usize, HintNameTableEntry<'a>)), // [u8; 31] bitfield :/ | |||
There was a problem hiding this comment.
As far as I can tell, it didn't need to change, and u32 seems more correct to me, since it's never more than 32 bits even for P32+.
src/pe/mod.rs
Outdated
| @@ -90,9 +90,16 @@ impl<'a> PE<'a> { | |||
| } | |||
| debug!("exports: {:#?}", exports); | |||
| if let &Some(import_table) = optional_header.data_directories.get_import_table() { | |||
| let id = import::ImportData::parse(bytes, &import_table, §ions)?; | |||
| let id = match is_64 { | |||
There was a problem hiding this comment.
An if statement seems more natural here?
src/pe/import.rs
Outdated
| pub type ImportAddressTable = Vec<u32>; | ||
|
|
||
| pub const SIZEOF_IMPORT_ADDRESS_TABLE_ENTRY: usize = 4; | ||
| pub type ImportAddressTable = Vec<usize>; |
There was a problem hiding this comment.
This should be u64, and the import address table parsing also needs fixing to read either u32 or u64 based on whether it is PE32+.
There was a problem hiding this comment.
Thank you for the comments. I changed ImportAddressTable to an enum that holds a Vec<u32> or Vec<u64>, and added a method in the Bitfield trait that converts Vec<T> to an ImportAddressTable. Not sure if this is the optimal solution though.
There was a problem hiding this comment.
For unified 32-bit/64-bit types, I think the rest of the crate tends to convert the fields to u64, so I think a plain Vec<u64> is better. You can add a method in the Bitfield trait to convert a value to u64.
src/pe/import.rs
Outdated
| pub const IMPORT_RVA_MASK_32: u32 = 0x8fff_ffff; | ||
| pub const IMPORT_RVA_MASK_64: u64 = 0x0000_0000_8fff_ffff; | ||
| pub const SIZEOF_IMPORT_ADDRESS_TABLE_ENTRY_32: usize = 4; | ||
| pub const SIZEOF_IMPORT_ADDRESS_TABLE_ENTRY_64: usize = 8; |
There was a problem hiding this comment.
Since this value is only used for the import lookup table (not the address table), I think it would be better to use 'lookup' in the name, even though both tables have the same entry size (and sometimes the same entry format).
src/pe/import.rs
Outdated
| } | ||
|
|
||
| #[derive(Debug)] | ||
| pub struct ImportLookupTableEntry<'a> { | ||
| pub bitfield: u32, |
There was a problem hiding this comment.
If we're going to delete bitfield (which I think is fine to do), then there doesn't seem much point having this struct at all.
I noticed that only the first imported symbol from each library is being parsed in 64-bit PEs. This is one way to fix it.