-
Notifications
You must be signed in to change notification settings - Fork 940
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
Do not return entry point offset if it's not backed up by disk data #975
Conversation
This is a good fix - I think exactly what @metthal wanted in #962. So few things to finalize this:
|
I've changed some unsigned long long types to std::uint64_t, but god a little bit carried away by the number of changes that were necessary 😅 I should probably remove the b2c3089 commit right? |
…doesn't give warning about invalid entry point due to the memory-only entry points
I've removed the warning in case that the I've also changed the anomaly flow, that if the address (instead of offset like before) isn't part of any section, then the warning I've added an anomaly for the offset outside of the physical file when the address is part of an actual section:
Which makes sense to me But I've noticed, the Current code looks like this: bool getEpAddress(std::uint64_t & epAddress) const
{
std::uint64_t imageBase = peFile->imageLoader().getImageBase();
std::uint32_t entryPoint = peFile->imageLoader().getOptionalHeader().AddressOfEntryPoint;
// Do not report zero entry point on DLLs
epAddress = imageBase + entryPoint;
return (entryPoint != 0 || isDll() == false);
} The last thing is I'm not sure about the difference between the warnings and anomalies. The warnings seem to be specific for |
…ger. Separate the ignore of invalid offset just for PE.
Maybe actually it is not useful to check for EP address validity there, I think we would like to export the information anyway and the anomalies already check and export information that the EP is outside of the mapped section. But that brings me to a failing regression test (bugs/invalid-address-of-entry-point-2 test_invalid_entry_point) that expects the warning, but that information is already part of anomalies. Maybe remove the test case for the PE files that solve this more accurately with the anomalies, or should I keep the warnings there? |
If the test no longer makes sense then please remove it. The point here is to not return EP offset if it can't be mapped back from any address and if this is the case for that file then that test can be removed. If this is ready to be merged please let us know. |
I feel like this could be finished |
src/pelib/ImageLoader.cpp
Outdated
@@ -483,6 +484,50 @@ uint32_t PeLib::ImageLoader::getFileOffsetFromRva(uint32_t rva) const | |||
return rva; | |||
} | |||
|
|||
// similar to getFileOffsetFromRva, but the offset is within the real file and not memory image | |||
uint32_t PeLib::ImageLoader::getValidOffsetFromRva(uint32_t rva) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same review notes as I given you in #982.
lets run TC tests |
@HoundThe At the moment, there are merge conflicts. |
The idea to fix #962
I am not completely sure about the correct solution to this, I have created a simple fix, but this has few problems that need to be addressed if this is the way to go: