Fixed possible access to unallocated memory in MPRESS unpacker #1
Conversation
If the size of data is less than 0x1000 then we can possibly underflow unsigned int and access unallocated data.
|
Thanks @2lambda123 for opening this PR! For COLLABORATOR only :
|
|
Unable to locate .performanceTestingBot config file |
![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!
|
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 |
Their most recently public accepted PR is: 2lambda123/Accenture-sfmc-devtools#104 |
|
Processing PR updates... |
|
First PR by @2lambda123 PR Details of @2lambda123 in avast-retdec :
|
|
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 changes enhance the robustness of 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 (
|
Reviewer's Guide by SourceryThis pull request addresses a potential issue in the MPRESS unpacker where accessing unallocated memory could occur if the buffer size is less than 0x1000. The fix involves modifying the calculation of the maxAddr variable to use std::max, ensuring that the result does not underflow. Tips
|
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| @@ -234,7 +234,7 @@ std::uint32_t MpressPlugin::getFixStub() | |||
| void MpressPlugin::fixJumpsAndCalls(DynamicBuffer& buffer) | |||
| { | |||
| std::uint32_t pos = 0; | |||
| std::uint32_t maxAddr = buffer.getRealDataSize() - 0x1000; | |||
| std::uint32_t maxAddr = std::max(0, static_cast<std::int32_t>(buffer.getRealDataSize()) - 0x1000); | |||
There was a problem hiding this comment.
The calculation of maxAddr is problematic. The use of std::max(0, static_cast<std::int32_t>(buffer.getRealDataSize()) - 0x1000) is incorrect because std::max expects both arguments to be of the same type. Here, the first argument is an int and the second is an std::int32_t. This can lead to unexpected behavior or even bugs.
Recommended Solution: Ensure both arguments to std::max are of the same type, preferably std::int32_t in this context.
labels-and-badges
bot
added
NO JIRA
![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 and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 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.
| @@ -234,7 +234,7 @@ std::uint32_t MpressPlugin::getFixStub() | |||
| void MpressPlugin::fixJumpsAndCalls(DynamicBuffer& buffer) | |||
| { | |||
| std::uint32_t pos = 0; | |||
| std::uint32_t maxAddr = buffer.getRealDataSize() - 0x1000; | |||
| std::uint32_t maxAddr = std::max(0, static_cast<std::int32_t>(buffer.getRealDataSize()) - 0x1000); | |||
There was a problem hiding this comment.
suggestion: Consider defining 0x1000 as a named constant for improved readability and maintainability.
Using a named constant would make the code more self-documenting and easier to update if this value needs to change in the future.
| std::uint32_t maxAddr = std::max(0, static_cast<std::int32_t>(buffer.getRealDataSize()) - 0x1000); | |
| constexpr std::uint32_t PAGE_SIZE = 0x1000; | |
| std::uint32_t maxAddr = std::max(0, static_cast<std::int32_t>(buffer.getRealDataSize()) - PAGE_SIZE); |
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!
PR summaryThis Pull Request addresses a potential bug in the MPRESS unpacker where accessing unallocated memory could occur if the data size is less than 0x1000. The fix involves ensuring that the calculation of SuggestionConsider adding a unit test to verify that the function behaves correctly when Disclaimer: This comment was entirely generated using AI. Be aware that the information provided may be incorrect. Current plan usage: 6.55% Have feedback or need help? |
If the size of data is less than 0x1000 then we can possibly underflow unsigned int and access unallocated data.
Description
Related Issue
Types of changes
Checklist:
Summary by Sourcery
Fix potential memory access issue in the MPRESS unpacker by adjusting the calculation of the maximum address to prevent underflow.
Bug Fixes:
Summary by CodeRabbit