Stop at non-conforming Debug Directory entry #199
Conversation
|
|
|
@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'll try to set aside some time for a review in the coming days. |
|
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 |
|
Actually, looks like I was wrong about the above: it should be safe to never populate |
pe-parser-library/src/parse.cpp
Outdated
| return false; | ||
| break; | ||
| } | ||
|
|
||
| // | ||
| // Get the section for the data | ||
| // | ||
| section dataSec; | ||
| if (!getSecForVA(p->internal->secs, rawData, dataSec)) { | ||
| return false; | ||
| break; | ||
| } | ||
|
|
||
| debugent ent; | ||
|
|
||
| auto dataofft = static_cast<std::uint32_t>(rawData - dataSec.sectionBase); | ||
| if (dataofft + curEnt.SizeOfData > dataSec.sectionData->bufLen) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Minor nitpick: I'd be good to have comments on these breaks explaining that we silently ignore invalid Debug Directory entries, rather than failing on them.
There was a problem hiding this comment.
I've added comments and update the code in the review.
|
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? |
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.) |
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.
|
Comment formatting was updated. Also, rebased on new master (no changes, clean rebase). |
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.