SCUMM: OSX crash with misread local Macintosh resource forks

[dwatteau]

Both an Engine-SCUMM and Port-OSX issue, I'd say...

On branch-2-8 on OSX PPC. The original Indy3 and Monkey1 Macintosh binaries have been copied "as-is" from the HFS disks to the local HFS+ filesystem, so the resource forks are still there. But, strangely, upgrading from ScummVM 2.7.1 to ScummVM 2.8.0pre, it looks like the resource fork is not properly read by ScummVM on OSX anymore.

The original Macintosh releases of Monkey1 and Indy3 now trigger crashes on this port, which didn't happen on 2.7.1. 2.8.0 now makes use of the original Macintosh binaries (for original GUI and improved sound), but (AFAICS) the engine doesn't check for the proper format of those resources, so if invalid content is read, the engine crashes.

GDB logs below.

Another way of reproducing the crash on a different system is to emulate a bad extraction of the resource fork (such as a blind cp call from Linux reading the HFS disk), which typically gives you zero-byte files:

$ echo -n > "/path/to/INDY3-MAC/Indy"
$ echo -n > "/path/to/MONKEY1-MAC/Monkey Island"

Of course, users should follow our guide about preserving the resource forks, but the crash is probably "not nice" (we could maybe at least check for a 0-byte file?). Moreover, on OSX, reading the local resource fork used to work (without any need to macbinary-encode it).

[dwatteau]

Actually, the crash when reading an original resource fork (which is not macbinary-encoded, but just copied as-is to the local HFS+/APFS volume) is there on modern macOS, too.

[eriktorbjorn]

I suppose we could test the MD5 sum of the resource fork, but then we'd have to know exactly which versions exist and I have no idea.

We could check that all the expected resources exist, I guess. Or maybe it's enough to just check that there is a resource fork in the first place. Something like this perhaps, though I haven't tested it:

diff --git a/engines/scumm/scumm.cpp b/engines/scumm/scumm.cpp
index a9de92a7c71..767e7b201cf 100644
--- a/engines/scumm/scumm.cpp
+++ b/engines/scumm/scumm.cpp
@@ -1230,6 +1230,16 @@ Common::Error ScummEngine::init() {
                        }
                }
 
+               if (!macResourceFile.empty()) {
+                       if (!resource.open(macResourceFile))
+                               return Common::Error(Common::kReadingFailed, Common::U32String::format(_("Could not open Macintosh resource file %s"), macResourceFile.c_str()));
+
+                       if (!resource.hasResFork())
+                               return Common::Error(Common::kReadingFailed, Common::U32String::format(_("Could not find resource fork in Macintosh resource file %s"), macResourceFile.c_str()));
+
+                       resource.close();
+               }
+
                if (!_macScreen && _renderMode == Common::kRenderMacintoshBW)

I have no idea about the regression, though.

[eriktorbjorn]

There was a commit to macresman.cpp back in July that claims to "fix MacResManager native resource forks". It mentions an "archive refactor", so perhaps things broke around that time?

[dwatteau]

Thanks.

As for handling that case in the SCUMM engine, I see that Monkey2 already successfully triggers this code in engines/scumm/imuse/drivers/mac_m68k.cpp:196:

void IMuseDriver_MacM68k::loadAllInstruments() {
        Common::MacResManager resource;
        if (resource.open("iMUSE Setups")) {
                if (!resource.hasResFork()) {
                        error("MacM68kDriver::loadAllInstruments: \"iMUSE Setups\" loaded, but no resource fork present");
                }

        // ...

Then, I did some bisects with Monkey2 Macintosh (running on an HFS+ or APFS volume with the native resource fork), because this game was already loading resource forks before 2.8.0, so it was easier to find a regression outside of the engine.

Git bisect gives me commit 9892bedc61d8fee94587f80a589960ef6876ce02 ("COMMON: Add createReadStreamForAltStream to open Mac resource fork and metadata streams") as the first commit introducing the issue.

[eriktorbjorn]

Does the code I posted work to flag the error to the user? I haven't tested it, but if it does I might as well commit it. Even if it does nothing about the Mac regression.

[dwatteau]

@eriktorbjorn: Yes, your diff properly flags that case to the user.

The underlying resource fork regression with commit 9892bedc61d8fee94587f80a589960ef6876ce02 is still there, but for the SCUMM part itself, I think that diff can be committed.

Thanks.

[elasota]

I don't have a Mac to test this with, but I am not sure what would be causing the behavior differences here. If the res fork was copied correctly, then it should be opened in POSIXFilesystemNode::createReadStreamForAltStream now. That code path was tested before it was merged, so I'm not sure what the problem is.

[eriktorbjorn]

Ok, I've committed my check for if the Mac executables have a resource fork. (Again, this is not a fix for the regression. I have no ideas about that one.)

[elasota]

I think the problem is that FSDirectoryFile can open an alternate stream, but FSDirectory can not.

PR'd an attempt to fix: ​https://github.com/scummvm/scummvm/pull/5549

[bluegr]

The PR has been merged, so this can be closed now