SCUMM: SIGSEGV in Scumm::ScummEngine_v5::saveLoadWithSerializer(Common::Serializer&)

[lephilousophe]

Version 2.7.1

Here is a crash report from Google Play console:

backtrace:
  #00  pc 0x0000000001a05138  /data/app/~~_TYEzaW_0VjS1b33x2EjvQ==/org.scummvm.scummvm-mOccEg7CYf8ujARyKMfNbA==/lib/arm64/libscummvm.so (Scumm::ScummEngine_v5::saveLoadWithSerializer(Common::Serializer&)+392)
  #01  pc 0x0000000001a00dc4  /data/app/~~_TYEzaW_0VjS1b33x2EjvQ==/org.scummvm.scummvm-mOccEg7CYf8ujARyKMfNbA==/lib/arm64/libscummvm.so (Scumm::ScummEngine::loadState(int, bool, Common::String&)+788)
  #02  pc 0x0000000001a00a90  /data/app/~~_TYEzaW_0VjS1b33x2EjvQ==/org.scummvm.scummvm-mOccEg7CYf8ujARyKMfNbA==/lib/arm64/libscummvm.so (Scumm::ScummEngine::loadState(int, bool)+36)
  #03  pc 0x0000000001a0cf44  /data/app/~~_TYEzaW_0VjS1b33x2EjvQ==/org.scummvm.scummvm-mOccEg7CYf8ujARyKMfNbA==/lib/arm64/libscummvm.so (Scumm::ScummEngine::go()+96)
  #04  pc 0x0000000001a10464  /data/app/~~_TYEzaW_0VjS1b33x2EjvQ==/org.scummvm.scummvm-mOccEg7CYf8ujARyKMfNbA==/lib/arm64/libscummvm.so (Scumm::ScummEngine::run()+128)
  #05  pc 0x00000000019e90f4  /data/app/~~_TYEzaW_0VjS1b33x2EjvQ==/org.scummvm.scummvm-mOccEg7CYf8ujARyKMfNbA==/lib/arm64/libscummvm.so (scummvm_main+7012)
  #06  pc 0x00000000019db3a8  /data/app/~~_TYEzaW_0VjS1b33x2EjvQ==/org.scummvm.scummvm-mOccEg7CYf8ujARyKMfNbA==/lib/arm64/libscummvm.so (JNI::main(_JNIEnv*, _jobject*, _jobjectArray*)+340)
  #07  pc 0x000000000001d260  /data/app/~~_TYEzaW_0VjS1b33x2EjvQ==/org.scummvm.scummvm-mOccEg7CYf8ujARyKMfNbA==/oat/arm64/base.odex (art_jni_trampoline+112)
  #08  pc 0x00000000000471d8  /data/app/~~_TYEzaW_0VjS1b33x2EjvQ==/org.scummvm.scummvm-mOccEg7CYf8ujARyKMfNbA==/oat/arm64/base.odex (org.scummvm.scummvm.ScummVM.run+520)
  #09  pc 0x00000000001bf1cc  /apex/com.android.art/javalib/arm64/boot.oat (java.lang.Thread.run+76)
  #10  pc 0x0000000000218964  /apex/com.android.art/lib64/libart.so (art_quick_invoke_stub+548)
  #11  pc 0x00000000002851f0  /apex/com.android.art/lib64/libart.so (art::ArtMethod::Invoke(art::Thread*, unsigned int*, unsigned int, art::JValue*, char const*)+184)
  #12  pc 0x0000000000628a24  /apex/com.android.art/lib64/libart.so (art::JValue art::InvokeVirtualOrInterfaceWithJValues<art::ArtMethod*>(art::ScopedObjectAccessAlreadyRunnable const&, _jobject*, art::ArtMethod*, jvalue const*)+460)
  #13  pc 0x0000000000678488  /apex/com.android.art/lib64/libart.so (art::Thread::CreateCallback(void*)+1184)
  #14  pc 0x00000000000b4ad8  /apex/com.android.runtime/lib64/bionic/libc.so (__pthread_start(void*)+264)
  #15  pc 0x0000000000052c08  /apex/com.android.runtime/lib64/bionic/libc.so (__start_thread+64)

From disassembly, it looks like the crash happens in engines/scumm/saveload.cpp at line 2031:

_16BitPalette[i] = get16BitColor(_currentPalette[i * 3 + 0], _currentPalette[i * 3 + 1], _currentPalette[i * 3 + 2]);

It seems _16BitPalette is not initialized correctly as it happens when storing the result of get16BitColor call.

[PushmePullyu]

I had a quick look at this:

Since we arrived at go() via run(), init() must have completed without errors (see Scumm::ScummEngine::run() in engines/scumm/scumm.h).

This means init() called Scumm::ScummEngine::resetScumm() (all the overridden resetScumm() methods eventually call this one), which has this code:

#ifdef USE_RGB_COLOR
	if (_game.features & GF_16BIT_COLOR
#ifndef DISABLE_TOWNS_DUAL_LAYER_MODE
		|| (_game.platform == Common::kPlatformFMTowns)
#endif
		)
		_16BitPalette = (uint16 *)calloc(512, sizeof(uint16));
#endif

_16BitPalette only ever gets free'd in ScummEngine::~ScummEngine().

According to the report the crash then occurs in engines/scumm/saveload.cpp:
in void ScummEngine_v5::saveLoadWithSerializer(Common::Serializer &s):

	// Regenerate 16bit palette after loading.
	// This avoids color issues when loading savegames that have been saved with a different ScummVM port
	// that uses a different 16bit color mode than the ScummVM port which is currently used.
#ifdef USE_RGB_COLOR
	if (_game.platform == Common::kPlatformPCEngine && s.isLoading()) {
		for (int i = 0; i < 256; ++i)
			_16BitPalette[i] = get16BitColor(_currentPalette[i * 3 + 0], _currentPalette[i * 3 + 1], _currentPalette[i * 3 + 2]);
	}
#endif

Apart from a genuine calloc() failure or the pointer getting clobbered somehow, the only way I can see this crashing is having platform set to Common::kPlatformPCEngine but feature GF_16BIT_COLOR unset (which would skip the allocation).

Since the only entry in the detection table that sets platform to kPlatformPCEngine is Loom PCE, which has GF_16BIT_COLOR, perhaps platform was overridden by the user.

I tested this by adding SOMI CD/DOS/English and then changing platform to PC-Engine in the ScummVM game options. Loading a save from the launcher with this leads to a backtrace matching the one reported.

If this is the cause of the crash, it still exists in current master (e510f0868ea2e2f7fdd724ed3c14104ca7da0851). The following diff fixes it (but running a game in such a configuration will of course still cause all kinds of problems):

--- a/engines/scumm/saveload.cpp
+++ b/engines/scumm/saveload.cpp
@@ -2069,7 +2069,7 @@ void ScummEngine_v5::saveLoadWithSerializer(Common::Serializer &s) {
        // This avoids color issues when loading savegames that have been saved with a different ScummVM port
        // that uses a different 16bit color mode than the ScummVM port which is currently used.
 #ifdef USE_RGB_COLOR
-       if (_game.platform == Common::kPlatformPCEngine && s.isLoading()) {
+       if (_game.platform == Common::kPlatformPCEngine && s.isLoading() && _game.features & GF_16BIT_COLOR) {
                for (int i = 0; i < 256; ++i)
                        _16BitPalette[i] = get16BitColor(_currentPalette[i * 3 + 0], _currentPalette[i * 3 + 1], _currentPalette[i * 3 + 2]);
        }

[AndywinXp]

I attempted a fix on the master branch. The issue is gone for me, let me know how it goes for you...

[lotharsm]

Any idea if https://bugs.scummvm.org/ticket/10427 is related to this and if we are lucky enough to have a fix for both reports at the same?

Just asking because both bugs are marked as blockers. 😁

[AndywinXp]

Heh, that one is one I could never ever ever reproduce... 😁

Still, I can confidently say that the resolution of this ticket is not related to the one you mentioned!

[lephilousophe]

This bug comes from Google Play, so let's assume it's fixed.