Use image loader when loading corrupted resources #3
Conversation
When the resource tree doesn't have expected depth, we use different path to load resources which didn't use loader but calculated offset from RVA on it's own resulting in underflows on integers and completely bogus offsets. Even though offsets are still kinda bogus, they make sense and are within the file itself after this change. Example file: f39f1ad9f97766618826d47c2d32529425e773659f5577fea06a82e21f13b064
|
Unable to locate .performanceTestingBot config file |
|
Processing PR updates... |
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information |
![gitginie[bot]](https://avatars.githubusercontent.com/in/712912?s=60&v=4){kind=small}
There was a problem hiding this comment.
@2lambda123
Thank you for your contribution to this repository! We appreciate your effort in opening pull request.
Happy coding!
|
Thanks @2lambda123 for opening this PR! For COLLABORATOR only :
|
Their most recently public accepted PR is: #1 |
|
Important Review skippedAuto reviews are limited to specific labels. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent update enhances the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
labels-and-badges
bot
added
NO JIRA
|
PR Details of @2lambda123 in avast-retdec :
|
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| auto resource = std::make_unique<Resource>(); | ||
| resource->setOffset(leaf->getOffsetToData() - rva + formatParser->getResourceDirectoryOffset()); | ||
| resource->setOffset(getImageLoader().getValidOffsetFromRva(leaf->getOffsetToData())); | ||
| resource->setSizeInFile(leaf->getSize()); | ||
| resource->load(this); | ||
| resourceTable->addResource(std::move(resource)); |
There was a problem hiding this comment.
The code does not handle potential exceptions that might be thrown by getValidOffsetFromRva, getSize, or load methods. If any of these methods throw an exception, it could lead to resource leaks or inconsistent state.
Recommended Solution:
Wrap the code in a try-catch block to handle potential exceptions and ensure that resources are properly managed in case of an error.
Reviewer's Guide by SourceryThis pull request addresses an issue with loading corrupted resources by changing the offset calculation method. Instead of manually calculating offsets, the code now uses the image loader's File-Level Changes
Tips
|
There was a problem hiding this comment.
@2lambda123
Thank you for your contribution to this repository! We appreciate your effort in closing pull request.
Happy coding!
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @2lambda123 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a brief inline comment explaining why
getValidOffsetFromRva()is used instead of the previous calculation method. - Please provide more details on how to use the example file mentioned (f39f1ad9f97766618826d47c2d32529425e773659f5577fea06a82e21f13b064) to reproduce the issue and verify the fix.
- The PR template checkboxes are not filled out. Please complete this information to clarify the nature and impact of your changes.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
PR summaryThis Pull Request addresses an issue with loading corrupted resources in the PE file format. Previously, when the resource tree did not have the expected depth, the code calculated offsets from RVA manually, leading to integer underflows and invalid offsets. The change ensures that the image loader is used to obtain valid offsets, which are within the file itself, even if they are still somewhat incorrect. This improves the robustness of the resource loading process for corrupted files. SuggestionConsider adding unit tests to verify the behavior of the new offset calculation method, especially with corrupted resource trees. This will help ensure that the changes work as expected and prevent regressions in the future. Disclaimer: This comment was entirely generated using AI. Be aware that the information provided may be incorrect. Current plan usage: 6.86% Have feedback or need help? |
When the resource tree doesn't have expected depth, we use different path to load resources which didn't use loader but calculated offset from RVA on it's own resulting in underflows on integers and completely bogus offsets. Even though offsets are still kinda bogus, they make sense and are within the file itself after this change.
Example file: f39f1ad9f97766618826d47c2d32529425e773659f5577fea06a82e21f13b064
Description
Related Issue
Types of changes
Checklist:
Summary by Sourcery
Use image loader for calculating offsets when loading corrupted resources to prevent integer underflows and ensure offsets are within the file.
Bug Fixes:
Summary by CodeRabbit