MM C64 DEMO: Attempting to display the original save/load screen causes a crash.

[robertmegone]

Playing the Maniac Mansion C64 demo, I experienced a crash when trying to press ALT+F5 to display the original save/load screen.

I don't know if the demo version has a save/load screen but it seems safe to assume that it doesn't.

The console spits out this error when the game crashes:
../../src-master/src/common/array.h:164: failed assertion `idx < _size'

But I think the safest thing to do is simply add an additional condition to exclude the demo from the line:

if (_game.id == GID_MANIAC && _game.version == 0) {

Which is currently around line 488 in engines/scumm/input.cpp

[robertmegone]

Pressing Shift+F1 in an emulator using the full game opens the save/load screen but it does nothing in the demo so I assume it just doesn't have a save/load screen.

[bluegr]

I tried pressing alt-F5 in the demo, but nothing happens, i.e. there's no dialog shown, and there's no crash. Perhaps you got a local patch that causes this behavior?

[robertmegone]

I have no local modifications. Are you using a Mac? On my Mac I have to press Fn+Alt+F5.

[csnover]

Raising all identified crasher, hang, and memory violation bugs which I could not fully triage myself to blocker priority for the next release.

[csnover]

I seem to be unable to trigger the specific crash mentioned by the OP, but since the SCUMM save code triggers undefined behaviour, it makes sense that it would do different things to different people.

This is not legal C++:

#define OFFS(type,item) ((uint32)(((ptrdiff_t)(&((type *)42)->type::item))-42))

// ...later...

OFFS(ScummEngine, _gameMD5[0])

ScummEngine is not a standard layout type, so this triggers UB in C++14 and earlier, and implementation-defined behaviour in C++17 and later.

It is frustrating to me that this was known UB and compilers actively were warning about it and instead of eliminating the UB 42 was added to defeat the heuristic of the compiler.

This code happens to also crash Clang 5’s UndefinedBehaviorSanitizer code, which can’t deal with someone trying to get the offset of a member in a class with a vtable like this.

So in conclusion, this save code needs to be fixed to be C++-compliant, and I don’t know enough about this engine right now to know how to do that successfully.

In the meantime it would be helpful to learn from robertmegone a specific set of reproduction steps to get the array assertion error since I want to make sure I have not just unmasked a separate issue.

[csnover]

Removing myself as owner for the moment to allow someone with more SCUMM engine knowledge to maybe take a crack at resolving this.

[sev-]

Fixed in a7182e2a6.

[csnover]

sev-, did you reproduce this crash as the OP mentioned it? Did you see comment 8?

[sev-]

No, I did not reproduce that crash. The demo is not supposed to have the save screen in general.

[csnover]

OK, then the patches that were just committed to not call the save code don’t do anything, and don’t need to be there. There was already no action when hitting the save key so long as the UB didn’t trigger a failure. The UB which I discussed in comment 8 has not been fixed by those patches.

[sev-]

and that OFFS() code existed since

commit 061f9c1289fac34d45887a8cd6ffd627446dcd7f
Author: Ludvig Strigeus <strigeus@…>
Date: Wed Oct 10 10:02:33 2001 +0000

alternative mouse cursor
basic save&load

svn-id: r3416

[csnover]

I’m sorry, I don’t understand. Could you please explain to me, like I am a small child, these two things:

1. Since the only reproducible issue on this ticket is the UB, why does it matter that the UB has existed since 2001?
2. Why should the recently added patch, which was added without first reproducing any issue, be kept?

Thank you!

[sev-]

What made you link that UB to the crash in the first place? Do you have any non-theoretic evidence that the UB could, in fact, lead to the unexpected or unintended results? This code has been tested on dozens of platforms and millions of devices. It still works well, and if it was broken, no saves would work in SCUMM engine reliably.

The demo does not have the relevant save screen anyway, and it is pointless to attempt to run it. Thus, the patch makes sense in any case.

I cannot explain it to you as you are not a small child, and even if you were, it is too long to describe the programming from the scratch.

[csnover]

We had a discussion, here is the outcome.

sev- went back a few years in history and was still not able to reproduce the OP’s assertion failure, and feels 95% certain that the OP’s issue was due to invalid data. Since the MM C64 demo has no save screen anyway, he felt it was appropriate to make a change to return early from the hotkey handler.

Since the OP’s issue was not reproduced I still think that the patch committed has equal probability of (1) doing nothing (since the game already doesn’t show a save screen or crash normally), (2) masking a different bug with out-of-bounds reads that exists elsewhere in SCUMM engine, which can be conditionally triggered from the MM C64 demo, or (3) fixing the OP’s problem with the MM C64 demo as reported. Due to concerns over (1) and (2) I would prefer having made no code change in this regard and simply say that this ticket is worksforme/outdated, so that there is the opportunity for a new reproduction in future, but that is my opinion.

Since the UB was causing a reproducible crash with Clang 5’s UBSan that was the angle I approached this ticket with, but it is not the OP’s crash so shouldn’t remain here. I will open a separate ticket to track the problem of the UB-hack save code which crashes Clang 5’s UBSan at runtime, and that problem is not a release blocker for 2.0.