Opened 7 years ago

Closed 6 years ago

#10116 closed defect (fixed)

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

Reported by: robertmegone Owned by: sev-
Priority: blocker Component: Engine: SCUMM
Version: Keywords: semi-reproducible-ub
Cc: Game: Maniac Mansion

Description

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

Change History (18)

comment:1 by robertmegone, 7 years ago

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.

comment:2 by bluegr, 7 years ago

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?

comment:3 by robertmegone, 7 years ago

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

comment:4 by csnover, 6 years ago

Priority: normalblocker

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

comment:5 by csnover, 6 years ago

Keywords: MM C64 DEMO V0 removed

comment:6 by csnover, 6 years ago

Owner: set to csnover

comment:7 by csnover, 6 years ago

Keywords: reproducible added

comment:8 by csnover, 6 years ago

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.

Last edited 6 years ago by csnover (previous) (diff)

comment:9 by csnover, 6 years ago

Keywords: semi-reproducible-ub added; reproducible removed

comment:10 by csnover, 6 years ago

Owner: csnover removed

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

comment:11 by sev-, 6 years ago

Owner: set to sev-
Resolution: fixed
Status: newclosed

Fixed in a7182e2a6.

comment:12 by csnover, 6 years ago

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

comment:13 by sev-, 6 years ago

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

comment:14 by csnover, 6 years ago

Resolution: fixed
Status: closednew

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.

comment:15 by sev-, 6 years ago

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


comment:16 by csnover, 6 years ago

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!

comment:17 by sev-, 6 years ago

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

Version 0, edited 6 years ago by sev- (next)

comment:18 by csnover, 6 years ago

Resolution: fixed
Status: newclosed

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.

Note: See TracTickets for help on using tickets.