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.

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

PEGASUS: fix bug 11758. #2794

Open
wants to merge 1 commit into
base: master
from
Open

Conversation

@fedor4ever
Copy link
Contributor

@fedor4ever fedor4ever commented Feb 22, 2021

See - https://bugs.scummvm.org/ticket/11758
PEGASUS: colossal memory leak, wan't start.

See - https://bugs.scummvm.org/ticket/11758
PEGASUS: colossal memory leak, wan't start.
@sluicebox
Copy link
Member

@sluicebox sluicebox commented Feb 22, 2021

Could you rename this commit to conform to the guidelines? https://wiki.scummvm.org/index.php/Commit_Guidelines

The prefix should be COMMON since this doesn't touch the Pegasus engine. The summary should describe the change as opposed to just a bug number.

I'm confused if this reverts 1bb7386 or not. Do ISOBuster files still work? That commit commented out a line and added the comment "Files produced by ISOBuster are not padded, thus, compare with the actual size", this PR restores the line but leaves that comment, and that doesn't sound right.

My interest is so that our tester who uses ISOBuster can still test my Mac stuff =)

Copy link
Member

@criezy criezy left a comment

It seems to me that your change will completely break reading macbinary files (created by ISOBuster or not).

The cleaning in the code is good. But the logic for the two checks in isMacBinary() and loadMacBinary() needs to be changed (and also they are currently not consistent).

And as @sluicebox pointed out, the commit message is also incorrect. The prefix should be COMMON, and the first line should be more descriptive (so that we do not have to go to the bug tracker and read the bug to understand what the commit does`). So for example something like the following:

COMMON: Fix reading some non-macbinary files as macbinary files in MacResManager

This fixes bug 11758: PEGASUS: colossal memory leak, wan't start.
This bug occurred because the Inventory Panel Movie file was read as macbinary.
if (MBI_INFOHDR + dataSizePad + rsrcSize <= (uint32)stream.size()) {
resForkOffset = MBI_INFOHDR + dataSizePad;
}
if (MBI_INFOHDR + dataSizePad + rsrcSize == (uint32)stream.size())

This comment has been minimized.

@criezy

criezy Feb 22, 2021
Member

That test is incorrect because with that change only macbinary files created with ISOBuster will (or at least might) work.

A test that might work for both ISOBuster and standard macbinary files would be:

if (MBI_INFOHDR + dataSizePad + rsrcSize == (uint32)stream.size() || MBI_INFOHDR + dataSizePad + rsrcSizePad == (uint32)stream.size())

// Length check
if (MBI_INFOHDR + dataSizePad + rsrcSize <= (uint32)stream.size()) {
if (MBI_INFOHDR + dataSizePad + rsrcSizePad <= (uint32)stream.size()) {

This comment has been minimized.

@criezy

criezy Feb 22, 2021
Member

And that change is also incorrect because it means that macbinary created by ISOBuster will fail to load.
Combined with the previous issue I pointed above it means that the commit as it currently is will likely break all macbinary files. The check here should be the same check as in isMacBinary().

@criezy criezy requested a review from sev- Feb 22, 2021
@criezy
Copy link
Member

@criezy criezy commented Feb 22, 2021

I have also requested a review from @sev-
Since I am not familiar with ISOBuster and he wrote the code, it might be good for him to confirm that:
MBI_INFOHDR + dataSizePad + rsrcSize == (uint32)stream.size() is true for ISOBuster macbinary and thus that the following condition should work in all macbinary cases.

if (MBI_INFOHDR + dataSizePad + rsrcSize == (uint32)stream.size() || MBI_INFOHDR + dataSizePad + rsrcSizePad == (uint32)stream.size())
@fedor4ever
Copy link
Contributor Author

@fedor4ever fedor4ever commented Feb 23, 2021

Can you trace with debugger pegasus engine? Maybe we find better solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants