pe-parse
pe-parse copied to clipboard
trailofbits
Stop at non-conforming Debug Directory entry
Debug directory is not necessary for program execution. Sometimes toolchains put there data not conforming to any standards. It is still possible to parse the rest of the file, no need to fail parsing with an error.
@woodruffw , may I ask to review and hopefully merge this change? It is a simple change, but it allows parsing of some files on which pe-parse fails currently.
I believe this change is straightforward, but if there are any concerns that prevent merging it, I would be glad to address them.
Sorry for the delay @batuzovk -- this change is indeed straightforward, but I need to do a more in-depth read to make sure it doesn't break any of our larger invariants about expecting sections to be present: this codebase hasn't seen active development in a while, so I'll need to refamiliarize myself before I'm comfortable merging this 🙂
(Specifically, my concern is that there may be places where parsed_pe_internals.debugdirs is assumed to be nonempty. I don't think that's the case, but I'll need to manually confirm that.)
Actually, looks like I was wrong about the above: it should be safe to never populate debugdirs, since all uses iterate over it and have sensible behavior when it's empty (e.g. not firing an invalid callback).
BTW, do you happen to have an example of a PE with a malformed debug entry? It'd be useful to have a backstop/regression test here. Nonblocker, however.
There is an example of a file with a malformed debug entry in issue #190 When dump-pe is run on System.Text.Json.dll from the attached archive, it fails without this change, but prints information correctly with it.
I see some CI checks failing. Some aren't related to my changes, but lint seem to be unhappy about the length of comment lines. Should I reformat those or will CI do it itself?
I see some CI checks failing. Some aren't related to my changes, but lint seem to be unhappy about the length of comment lines. Should I reformat those or will CI do it itself?
Yeah, if you could reformat, that would be great.
(Don't worry about the unrelated failures -- that's a known deprecation thing. I'll merge regardless of those, and fix the build with a follow-up.)
Comment formatting was updated. Also, rebased on new master (no changes, clean rebase).