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

Accept populated COFF symbol headers #410

Merged
merged 1 commit into from
Dec 20, 2021

Conversation

daladim
Copy link
Contributor

@daladim daladim commented Dec 17, 2021

Some binaries (not sure what toolchain has generated them) may have pointers to COFF symbols that point to nothing.
Since COFF symbols tables are deprecated anyway, it could be better to ignore these symbols than returning an Err that will prevent the whole PE file from parsing.

I know you're not always happy to remove error conditions for the sake of parsing more binaries, so I'd like to have your opinion on such a "fix" :-)

Example of such binaries: git for Windows (at least some builds).

  • Download PortableGit-2.29.2.3-64-bit.7z.exe from their release page
  • Run it so that it extracts itself
  • The file at PortableGit\cmd\git.exe has populated pointer_to_symbol_table and number_of_symbols that point after the end of file. Hence, it cannot be parsed by object because of "Invalid COFF symbol table offset or size"

@daladim daladim force-pushed the accept_populated_coff_headers branch 2 times, most recently from 21a851c to d4b5b82 Compare December 17, 2021 15:33
@philipc
Copy link
Contributor

philipc commented Dec 18, 2021

I think this error should be ignored in PeFile::parse instead, since this is only deprecated for PE, not COFF. This also allows someone to still detect this error for PE if desired by parsing the symbol table themselves, outside of PeFile.

This kind of error handling is a reason why PeFile::parse should do as little as possible.

Some PE binaries may hade pointers to COFF symbols that point to nothing.
Since COFF symbols tables are deprecated in PE files anyway, they can be ignored.
@daladim daladim force-pushed the accept_populated_coff_headers branch from d4b5b82 to 9f457c4 Compare December 20, 2021 09:54
@daladim
Copy link
Contributor Author

daladim commented Dec 20, 2021

OK, I have pushed a new proposal, that intercepts the error in PeFile::parse instead.

Not sure why adding a #[derive(Default)] on top of struct SymbolTable failed with the error below, so I implemented Default myself.

Compiler error
error[E0599]: the method `unwrap_or_default` exists for enum `core::result::Result<coff::symbol::SymbolTable<'_, R>, read::Error>`, but its trait bounds were not satisfied
  --> src/read/pe/file.rs:60:39
   |
60 |                   symbols: coff_symbols.unwrap_or_default(),
   |                                         ^^^^^^^^^^^^^^^^^ method cannot be called on `core::result::Result<coff::symbol::SymbolTable<'_, R>, read::Error>` due to unsatisfied trait bounds
   | 
  ::: src/read/coff/symbol.rs:20:1
   |
20 | / pub struct SymbolTable<'data, R = &'data [u8]>
21 | | where
22 | |     R: ReadRef<'data>,
23 | | {
24 | |     symbols: &'data [pe::ImageSymbolBytes],
25 | |     strings: StringTable<'data, R>,
26 | | }
   | |_- doesn't satisfy `coff::symbol::SymbolTable<'_, R>: Default`
   |
   = note: the following trait bounds were not satisfied:
           `coff::symbol::SymbolTable<'_, R>: Default`

@philipc philipc merged commit f9c8b3f into gimli-rs:master Dec 20, 2021
@philipc
Copy link
Contributor

philipc commented Dec 20, 2021

Thanks!

derive can sometimes add unnecessary trait bounds. I didn't investigate, but I assume that is happening here.

mcbegamerxx954 pushed a commit to mcbegamerxx954/object that referenced this pull request Jun 15, 2024
Some PE binaries may have pointers to COFF symbols that are invalid.
Since COFF symbols tables are deprecated in PE files anyway, they can be ignored.
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.

2 participants