PE: Add base relocation parser #444
Conversation
kkent030315
force-pushed
the
basereloc
branch
from
b2cb4bc to
a7d3726
Compare
|
@m4b Hi there, I'd like to get review and see if this worth merging. Thank you very much for your attention (as always)! |
There was a problem hiding this comment.
the only issue is have is the temproary empty &[] i mentioned in a comment. not sure if there is a better way.
only other thing is very minor nit about missing pub docs on the simpler items, but it's fine, the other documentation is excellent, thank you!
otherwise, this is great!
i'll sit on this a bit to think if there's a better design (or if you think of one) w.r.t. &[] otherwise i'll merge.
one option is to hide the TryFromCtx impl in some way, though I don't see that being feasible without annoying things.
src/pe/relocation.rs
Outdated
| if offset + dd.size as usize > bytes.len() { | ||
| return Err(error::Error::Malformed(format!( | ||
| "base reloc offset {:#x} and size {:#x} exceeds the bounds of the bytes size {:#x}", | ||
| offset, | ||
| dd.size, | ||
| bytes.len() | ||
| ))); | ||
| } | ||
|
|
||
| let bytes = &bytes[offset..offset + dd.size as usize]; |
There was a problem hiding this comment.
it doesn't matter now but i wonder if a better pattern here to use is something like:
let size = dd.size as usize;
let bytes = bytes[offset..].pread::<&[u8]>(size).map_err(|_| TheErrorWeWantToDisplay)?;it's just easy to copy-pasta the bounds error, and slice the wrong bounds that were different than what we checked, etc. anyway, not too important.
There was a problem hiding this comment.
Wow, didn't know that there's such a great expression. Thanks!
src/pe/relocation.rs
Outdated
| let mut offset = 0; | ||
| let rva = bytes.gread_with::<u32>(&mut offset, ctx)?; | ||
| let size = bytes.gread_with::<u32>(&mut offset, ctx)?; | ||
| let bytes = &[]; |
src/pe/relocation.rs
Outdated
| // "Block"s and "Word"s are entirely contiguous | ||
| self.offset += word_size; | ||
| Ok(RelocationBlock { | ||
| bytes: word_bytes, |
There was a problem hiding this comment.
i see, this gets rewritten later. hmmm, one thing i don't like is that temporarily, if someone were to use that raw TryFromCtx impl the bytes is empty, which feels "half-constructed" to me, which is generally a pattern i like to avoid.
however, not sure there is a better solution here, other than making it optional, which also feels a little laborious and still doesn't capture semantic meaning that well
There was a problem hiding this comment.
It's been quite a while since I wrote the code and I have no idea why did I chose to do this ugly.. anyway, I fixed that. It was trivial to just merge that construction into the TryFromCtx for RelocationBlock.
There was a problem hiding this comment.
This is super awesome PR thank you for revising and fixing the half constructed thing.
I’d merge except I think we don’t need repr(C) on the struct, and it’ll be a new pub item so good to get it right.
again this is awesome sorry took so long to get it merged but almost there :)
|
NB: backwards compatible |
It was a little bit surprise for me that goblin does not have a parser for base relocations in PE besides COFF relocations.
IMAGE_DIRECTORY_ENTRY_BASERELOCdirectory.