MM: XEEN: crash in carnage hand animation

[yarolig]

Game crashes while showing carnage hand attack or hit animation.

You can see this crash in mm4 endscene (say "showtime" to the mirror) or in the Darzog's Tower (save file attached).

See also https://bugs.scummvm.org/ticket/14431#comment:2

I had a dirty workaround:

diff --git a/engines/mm/shared/xeen/sprites.cpp b/engines/mm/shared/xeen/sprites.cpp
index 97fa01aa11b..ce5c377fc3c 100644
--- a/engines/mm/shared/xeen/sprites.cpp
+++ b/engines/mm/shared/xeen/sprites.cpp
@@ -213,6 +213,12 @@ void SpriteDrawer::draw(XSurface &dest, uint16 offset, const Common::Point &pt,
        _destBottom = (byte *)dest.getBasePtr(clipRect.right, clipRect.bottom - 1);
        _pitch = dest.pitch;

+       // WORKAROUND for carnage hand
+       if (_filesize == 5526) {
+               if (offset > _filesize || offset == 86 || offset == 3584)
+                       return;
+       }
+
        // Get cell header
        Common::MemoryReadStream f(_data, _filesize);
        f.seek(offset);

And now it less dirty:

diff --git a/engines/mm/shared/xeen/sprites.cpp b/engines/mm/shared/xeen/sprites.cpp
index 97fa01aa11b..859818ddb66 100644
--- a/engines/mm/shared/xeen/sprites.cpp
+++ b/engines/mm/shared/xeen/sprites.cpp
@@ -80,7 +80,7 @@ SpriteResource &SpriteResource::operator=(const SpriteResource &src) {
 void SpriteResource::load(const Common::String &filename) {
        _filename = filename;
        Common::File f;
-       if (f.open(filename)) {
+       if (g_engine->getGameID() == GType_MightAndMagic1 && f.open(filename)) {
                load(f);
        } else {
                File f2(filename);

I'm posting a pull request if you are ok with that ​https://github.com/scummvm/scummvm/pull/5084

The animation was ok in branch 2-7.

Backtrace:

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007ffff6a2d859 in __GI_abort () at abort.c:79
#2  0x00007ffff6a2d729 in __assert_fail_base (fmt=0x7ffff6bc3588 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x55555653e408 "_pos <= _size", file=0x55555653e365 "common/stream.cpp", line=132, function=<optimized out>)
    at assert.c:92
#3  0x00007ffff6a3efd6 in __GI___assert_fail (assertion=0x55555653e408 "_pos <= _size", file=0x55555653e365 "common/stream.cpp", line=132, function=0x55555653e3d0 "virtual bool Common::MemoryReadStream::seek(int64, int)")
    at assert.c:101
#4  0x00005555561e2582 in Common::MemoryReadStream::seek(long, int) (this=0x7fffffffb400, offs=39457, whence=0) at common/stream.cpp:132
#5  0x0000555555a1a5a0 in MM::Shared::Xeen::SpriteDrawer::draw(MM::Shared::Xeen::XSurface&, unsigned short, Common::Point const&, Common::Rect const&, unsigned int, int) (this=0x555557b2c750,
    dest=..., offset=39457, pt=..., clipRect=..., flags=8192, scale=8) at engines/mm/shared/xeen/sprites.cpp:218
#6  0x0000555555a19fde in MM::Shared::Xeen::SpriteResource::draw(MM::Shared::Xeen::XSurface&, int, Common::Point const&, Common::Rect const&, unsigned int, int) const
    (this=0x555557b1eb08, dest=..., frame=3, destPos=..., bounds=..., flags=8192, scale=8) at engines/mm/shared/xeen/sprites.cpp:153
#7  0x0000555555a0bd2b in MM::Xeen::SpriteResource::draw(MM::Xeen::Window&, int, Common::Point const&, unsigned int, int) (this=0x555557b1eb08, dest=..., frame=3, destPos=..., flags=8192, scale=8) at engines/mm/xeen/sprites.cpp:41
#8  0x0000555555a0e7d1 in MM::Xeen::Window::drawList(MM::Xeen::DrawStruct*, int) (this=0x555557c63348, items=0x555557b8e928, count=170) at engines/mm/xeen/window.cpp:262
#9  0x00005555559d831c in MM::Xeen::IndoorDrawList::draw() (this=0x555557b8d8a8) at engines/mm/xeen/interface_scene.cpp:389
#10 0x00005555559e84ed in MM::Xeen::InterfaceScene::drawIndoors() (this=0x555557b8c680) at engines/mm/xeen/interface_scene.cpp:4406
#11 0x00005555559da053 in MM::Xeen::InterfaceScene::drawIndoorsScene() (this=0x555557b8c680) at engines/mm/xeen/interface_scene.cpp:701
#12 0x00005555559d86cc in MM::Xeen::InterfaceScene::drawScene() (this=0x555557b8c680) at engines/mm/xeen/interface_scene.cpp:449
#13 0x00005555559ccb5e in MM::Xeen::Interface::draw3d(bool, bool) (this=0x555557b8c630, updateFlag=true, pauseFlag=true) at engines/mm/xeen/interface.cpp:1341
#14 0x0000555555acdf57 in MM::Xeen::Combat::attack2(int, MM::Xeen::RangeType) (this=0x555557b5d230, damage=6, rangeType=MM::Xeen::RT_HIT) at engines/mm/xeen/combat.cpp:1482
#15 0x0000555555acd562 in MM::Xeen::Combat::attack(MM::Xeen::Character&, MM::Xeen::RangeType) (this=0x555557b5d230, c=..., rangeType=MM::Xeen::RT_GROUP) at engines/mm/xeen/combat.cpp:1293
#16 0x0000555555acfb15 in MM::Xeen::Combat::rangedAttack(MM::Xeen::PowType) (this=0x555557b5d230, powNum=MM::Xeen::POW_ARROW) at engines/mm/xeen/combat.cpp:1979
#17 0x0000555555ad00f3 in MM::Xeen::Combat::shootRangedWeapon() (this=0x555557b5d230) at engines/mm/xeen/combat.cpp:2112
#18 0x00005555559ca9a9 in MM::Xeen::Interface::perform() (this=0x555557b8c630) at engines/mm/xeen/interface.cpp:684
#19 0x0000555555a10bd1 in MM::Xeen::XeenEngine::gameLoop() (this=0x555557b588a0) at engines/mm/xeen/xeen.cpp:285
#20 0x0000555555a109fe in MM::Xeen::XeenEngine::play() (this=0x555557b588a0) at engines/mm/xeen/xeen.cpp:254
#21 0x0000555555a10729 in MM::Xeen::XeenEngine::playGame() (this=0x555557b588a0) at engines/mm/xeen/xeen.cpp:210
#22 0x0000555555a104c8 in MM::Xeen::XeenEngine::outerGameLoop() (this=0x555557b588a0) at engines/mm/xeen/xeen.cpp:165
#23 0x0000555555a103ac in MM::Xeen::XeenEngine::run() (this=0x555557b588a0) at engines/mm/xeen/xeen.cpp:140
#24 0x000055555595ca1f in runGame(Plugin const*, Plugin const*, OSystem&, Common::String const&) (plugin=0x555557033b20, enginePlugin=0x555556ef59d0, system=..., debugLevels=...) at base/main.cpp:318
#25 0x000055555595ebbb in scummvm_main(int, char const* const*) (argc=1, argv=0x7fffffffdf08) at base/main.cpp:758
#26 0x0000555555959aac in main(int, char**) (argc=1, argv=0x7fffffffdf08) at backends/platform/sdl/posix/posix-main.cpp:44

[PushmePullyu]

Tested on Linux x86_64 with master 5a75fd3963d7698fa7bdd8f14f05dbcf897ca3d8, GOG version of World of Xeen:

I can reproduce the crash using the provided save.

It seems there are several cases where "SpriteResource::load(Common::SeekableReadStream&)" loads incorrect values into "_index[i]._offset1" and "_offset2". At least I assume so since some offsets are larger than "_filesize".
Those I observed so far happen with "xeenpic.dat" and "049.att". "count" also seems wrong for these.
The other files (all?) have the high octet of "count" as 0, so maybe it is actually just 8 bits?
An incorrect "count" also causes attempts to read from the stream after EOS was reached, but this is never checked. This means that some of the offsets are uninitialized.

xeenpic.dat:
size: 53
count: 1032
_index[0]._offset1: 1025
_index[0]._offset2: 268

049.att:
size: 5526
count: 7777
_index[0]._offset1: 8480
_index[0]._offset2: 21402

[yarolig]

I see there is two ways to open a file here. One using Common::File, and one with its subclass MM::Shared::Xeen::File.

In branch 2-7 everything was opened with Xeen::File.

For some reason "049.att" is opened with Common::File and it causes a crash. This hack will make the game work:

void SpriteResource::load(const Common::String &filename) {
	_filename = filename;
	Common::File f;
	if (filename != "049.att" && f.open(filename)) {
		load(f);
	} else {
		File f2(filename);
		load(f2);
	}
}

I guess there is a bad "049.att" somewhere...

I have not yet figured out how to inspect archives or how to log file access.

[PushmePullyu]

Found the cause: it's commit 2ca3831777e091d74510e9c0244b88458d0e363d

[yarolig]

looks like a Common::File calls CCArchive::createReadStreamForMember("049.att."), which calls BaseCCArchive::getHeaderEntry which tries to convert file name to resource id.

In case of file name "049.att." it is converted to existing resource with id=55679 and file size 5526.

The dot "." was added in Common:: File::Open

	if ((stream = archive.createReadStreamForMember(filename))) {
		debug(8, "Opening hashed: %s", filename.toString().c_str());
	} else if ((stream = archive.createReadStreamForMember(filename.append(".")))) {
		// WORKAROUND: Bug #2548: "SIMON1: Game Detection fails"
		// sometimes instead of "GAMEPC" we get "GAMEPC." (note trailing dot)
		debug(8, "Opening hashed: %s.", filename.toString().c_str());
	}

I guess that resource loading by ID is a MM1 thing, so my best workaround now is:

void SpriteResource::load(const Common::String &filename) {
	_filename = filename;
	Common::File f;

	if (g_engine->getGameID() == GType_MightAndMagic1 && f.open(filename)) {
		load(f);
	} else {
		File f2(filename);
		load(f2);
	}
}

[yarolig]

Replying to PushmePullyu:

Found the cause: it's commit 2ca3831777e091d74510e9c0244b88458d0e363d

Yes. But the more important cause is loading resources by ID that is hash(filename). That is what BaseCCArchive is doing. This logic is for MM1 and it appears when engines for MM1 and XEEN was combined. I do not know the right way to force using hashes for MM1 only.

Unfortunately the bug might appear in other places when file gets opened.

[yarolig]

I read ​https://xeen.fandom.com/wiki/CC_File_Format

MM4-5 also uses hash(file name) as resource IDs.
So now I don't know what is the right fix and why my last patch is working.