search

m4b / goblin

An impish, cross-platform binary parsing crate, written in Rust
MIT License
1.17k stars 156 forks source link

Adjust coff symbol offset to account for the strtable length field #321

Closed SquareMan closed 2 years ago

SquareMan commented 2 years ago

I took a crack at #318 myself after attempting to do some investigation. It seems to me that the best place to adjust the offset was here in the pe specific symbol code since strtab looks to be at a higher level of abstraction. Apologies if I've missed something important here.

m4b commented 2 years ago

@SquareMan thanks for taking a stab at this! So I'm not super happy about the regression, so I'd love to see tests showing:

  1. the regression is fixed
  2. the original issue which caused the regression is also fixed

If we can't show 2., i think i'm more likely to revert the cause of the regression, but I'm hoping your patch does 1. and 2. ?

And thanks again for taking the initiative here!

SquareMan commented 2 years ago

Regarding 2, the cause of the issue in my understanding is roughly as follows: a coff object's string table includes a 4 byte length of the table at the beginning. The string table pointer in the header points to the length field, and therefore symbol entries' name offsets are relative to the address of the length field (meaning an offset of 4 refers to the first string).

It seems to me that goblin's strtab keeps track of the offset of each string as it reads the table. Originally the coff parser wasn't accounting for the length field at the beginning of the table, and thus treating the length as strings. This meant that the symbol names' offsets could be used directly without adjustment, but there were garbage strings at the beginning. #318 fixed that issue but inadvertently caused this bug to crop up, since strtab's internal offsets no longer matched the offsets used in the coff file.

I believe that the test I added addresses both of these but let me know if you think I missed something. Perhaps this issue could be addressed in some other way entirely, I'm not sure.

m4b commented 2 years ago

Ok that makes sense, thanks for the detailed explanation! And thanks for adding the additional test! Let's fix CI and get this in asap if possible, I'd like to make a release with this so the regression is addressed

m4b commented 2 years ago

to fix ci using core::mem instead of std::mem should work

SquareMan commented 2 years ago

@m4b Happy to help out, looks like CI is taken care of.

m4b commented 2 years ago

I’m quite tired and likely won’t get to a crates release until little later next week, I want to do some manual testing also, hope that’s ok ?

m4b commented 2 years ago

I’ve also fixed the stupid settings that didn’t run CI if new contributor, which caused slower review / fix cycles (since the CI failure wouldn’t show up until I manually clicked run), doesn’t change anything now, but in future this should be less annoying to new contributors

and thanks again for this quick fix !

m4b commented 2 years ago

this is now published in 0.5.4, thanks for this fix!