Skip to content
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.

Sign up for GitHub

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

Fixed possible access to unallocated memory in MPRESS unpacker #1

Merged
merged 1 commit into from
Aug 3, 2024
Merged

Fixed possible access to unallocated memory in MPRESS unpacker #1

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/unpackertool/plugins/mpress/mpress.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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);

while (pos < maxAddr)
{
std::uint32_t moveOffset = pos;
Expand Down
Loading