Opened 6 months ago

Closed 6 months ago

#14753 closed defect (fixed)

SCUMM: OSX crash with misread local Macintosh resource forks

Reported by: dwatteau Owned by: elasota
Priority: normal Component: Engine: SCUMM
Version: Keywords:
Cc: Game:

Description (last modified by 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).

Attachments (2)

indy3-mac.txt (15.5 KB ) - added by dwatteau 6 months ago.
monkey1-mac.txt (6.4 KB ) - added by dwatteau 6 months ago.

Download all attachments as: .zip

Change History (13)

by dwatteau, 6 months ago

Attachment: indy3-mac.txt added

by dwatteau, 6 months ago

Attachment: monkey1-mac.txt added

comment:1 by dwatteau, 6 months ago

Component: --Unset--Engine: SCUMM
Description: modified (diff)
Summary: SCUMM: OSX PPC crash when loading Macintosh resource forksSCUMM: OSX PPC crash with misread local Macintosh resource forks

comment:2 by dwatteau, 6 months ago

Description: modified (diff)
Summary: SCUMM: OSX PPC crash with misread local Macintosh resource forksSCUMM: OSX crash with misread local Macintosh resource forks

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.

comment:3 by eriktorbjorn, 6 months ago

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.

comment:4 by eriktorbjorn, 6 months ago

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?

comment:5 by dwatteau, 6 months ago

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.

comment:6 by eriktorbjorn, 6 months ago

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.

Last edited 6 months ago by eriktorbjorn (previous) (diff)

comment:7 by dwatteau, 6 months ago

@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.

comment:8 by elasota, 6 months ago

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.

comment:9 by eriktorbjorn, 6 months ago

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.)

comment:10 by elasota, 6 months ago

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

comment:11 by bluegr, 6 months ago

Owner: set to elasota
Resolution: fixed
Status: newclosed

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

Note: See TracTickets for help on using tickets.