SCUMM: INDY3 (MAC): ASAN crash in Player_V2Base::next_freqs() in Castle Brunwald

[dwatteau]

Building yesterday's Git HEAD with --enable-optimizations --enable-debug --enable-asan on macOS with clang++ 14.0.0.

This is the Macintosh 16-color version of Indy3.

With ASAN enabled, the game always crashes when arriving at Castle Brunwald:

==12520==ERROR: AddressSanitizer: global-buffer-overflow on address 0x00011029f722 at pc 0x00010f799d18 bp 0x700004034650 sp 0x700004034648
READ of size 1 at 0x00011029f722 thread T6
    #0 0x10f799d17 in Scumm::Player_V2Base::next_freqs(Scumm::Player_V2Base::ChannelInfo*) player_v2base.cpp:607
    #1 0x10f799f39 in Scumm::Player_V2Base::nextTick() player_v2base.cpp:649
    #2 0x10f7747ea in Scumm::Player_V2::readBuffer(short*, int) player_v2.cpp:174
    #3 0x1100c8585 in Audio::CopyRateConverter<true, false>::flow(Audio::AudioStream&, short*, unsigned int, unsigned short, unsigned short) rate.cpp:314
    #4 0x1100be10d in Audio::Channel::mix(short*, unsigned int) mixer.cpp:648
    #5 0x1100bdd7c in Audio::MixerImpl::mixCallback(unsigned char*, unsigned int) mixer.cpp:301
    #6 0x111157c43 in outputCallback+0x1ac (libSDL2-2.0.0.dylib:x86_64+0xe2c43)
    #7 0x7ff80e7b1fe7 in ClientAudioQueue::CallOutputCallback(AudioQueueBuffer*)+0x11d (AudioToolbox:x86_64+0x45fe7)
    #8 0x7ff80e79aa03 in ClientAudioQueue::FetchAndDeliverPendingCallbacks(unsigned int)+0x33b (AudioToolbox:x86_64+0x2ea03)
    #9 0x7ff80e79a64d in _XCallbackNotificationsAvailable+0xa3 (AudioToolbox:x86_64+0x2e64d)
    #10 0x7ff80d6fea8d in mshMIGPerform+0xeb (libAudioToolboxUtility.dylib:x86_64+0xea8d)
    #11 0x7ff800e3a923 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE1_PERFORM_FUNCTION__+0x28 (CoreFoundation:x86_64h+0x80923)
    #12 0x7ff800e3a803 in __CFRunLoopDoSource1+0x26a (CoreFoundation:x86_64h+0x80803)
    #13 0x7ff800e38e6a in __CFRunLoopRun+0x96e (CoreFoundation:x86_64h+0x7ee6a)
    #14 0x7ff800e37e3b in CFRunLoopRunSpecific+0x231 (CoreFoundation:x86_64h+0x7de3b)
    #15 0x11115773c in audioqueue_thread+0x43e (libSDL2-2.0.0.dylib:x86_64+0xe273c)
    #16 0x1110db986 in SDL_RunThread+0x2b (libSDL2-2.0.0.dylib:x86_64+0x66986)
    #17 0x11114a7f2 in RunThread+0x8 (libSDL2-2.0.0.dylib:x86_64+0xd57f2)
    #18 0x7ff800d734e0 in _pthread_start+0x7c (libsystem_pthread.dylib:x86_64+0x64e0)
    #19 0x7ff800d6ef6a in thread_start+0xe (libsystem_pthread.dylib:x86_64+0x1f6a)

Full ASAN log attached.

How to reproduce:

1. Build with ASAN.
2. Load the attached savegame, and wait for Indy and Elsa to arrive in front of Castle Brunwald.

[dwatteau]

save just before Castle Brunwald in indy3-ega-mac

[dwatteau]

ASAN log

[eriktorbjorn]

I don't have the time to recompile ScummVM right now, so I hoped I would be able to capture it in Valgrind. Unfortunately not, but it did flag this:

==23698== Conditional jump or move depends on uninitialised value(s)
==23698==    at 0xB54D00: Scumm::ScummEngine::getIntegralTime(double) (scumm.cpp:2353)
==23698==    by 0xB54A7C: Scumm::ScummEngine::waitForTimer(int) (scumm.cpp:2302)
==23698==    by 0xB549D1: Scumm::ScummEngine::go() (scumm.cpp:2286)
==23698==    by 0xA48D45: Scumm::ScummEngine::run() (scumm.h:510)
==23698==    by 0x9FC7D0: runGame(Plugin const*, Plugin const*, OSystem&, Common::String const&) (main.cpp:318)
==23698==    by 0x9FE03E: scummvm_main (main.cpp:619)
==23698==    by 0x9F9AA8: main (posix-main.cpp:44)
==23698== 

Plus a few others, but they were just other ways of calling getIntegralTime().

This isn't directly related, of course.

[eriktorbjorn]

Right above, it's trying to limit the value of channel->d.freqmod_offset, but as far as I can see it's still larger than channel->d.freqmod_modulo. That seems bad?

[eriktorbjorn]

It's tempting to just change the "if (channel->d.freqmod_offset > channel->d.freqmod_modulo)" to a while. Or simply a modulo operation. Though apparently freqmod_modulo can be 0?

Does anyone know if this issue affects the DOS EGA version as well?

[dwatteau]

I'm not able to reproduce this particular crash at Castle Brunwald in my EGA (French) version.

However, I see a different audio ASAN issue in the DOS EGA game, once I'm done using the mallet on the boxing bell. It also happens in the DOS VGA version, so I'm attaching the ASAN log for it, since this release is easier to track down.

[dwatteau]

Different ASAN issue in Indy3 DOS VGA, after hitting the boxing bell with the mallet

[sev-]

It would be nice to get it fixed before the 2.8.0 release.

[eriktorbjorn]

When it reaches this line, right at the ASAN crash in Mac Last Crusade:

	channel->d.freq =
		(int)(freqmod_table[channel->d.freqmod_table + (channel->d.freqmod_offset >> 4)])
		* (int)channel->d.freqmod_multiplier / 256
		+ channel->d.base_freq;

the value of freqmod_table is 512 and freqmod_offset is 12328. That means it's accessing freqmod_table[1282], which is outside the array. The naive fix would of course be to clip the index to the size of the array, but maybe there's something more insidious going on here?

I couldn't reproduce the Indy 3 VGA ASAN crash, so maybe that's been fixed already?

[eriktorbjorn]

The highest index I recorded when not using ASAN was where freqmod_table was 512 and freqmod_offset was 65424, giving an offset of 4601. That's not just a little outside.

From what I understand, freqmod_table is set from from freqmod_offsets[], while freqmod_offset starts out at 0, is modified by freqmod_incr, and capped by freqmod_modulo.

In this case, freqmod_incr is 300 and freqmod_modulo is 32.

Which means that there is no chance that freqmod_modulo will be able to keep up:

	channel->d.freqmod_offset += channel->d.freqmod_incr;
	if (channel->d.freqmod_offset > channel->d.freqmod_modulo)
		channel->d.freqmod_offset -= channel->d.freqmod_modulo;

Perhaps this should be changed to

	channel->d.freqmod_offset += channel->d.freqmod_incr;
	while (channel->d.freqmod_offset > channel->d.freqmod_modulo)
		channel->d.freqmod_offset -= channel->d.freqmod_modulo;

or simply

	channel->d.freqmod_offset = (channel->d.freqmod_offset + channel->d.freqmod_incr) % channel->d.freqmod_modulo;

instead?

Edit: Fixed the second code snippet

[bluegr]

Replying to eriktorbjorn:

Which means that there is no chance that freqmod_modulo will be able to keep up:

	channel->d.freqmod_offset += channel->d.freqmod_incr;
	if (channel->d.freqmod_offset > channel->d.freqmod_modulo)
		channel->d.freqmod_offset -= channel->d.freqmod_modulo;

Perhaps this should be changed to

	channel->d.freqmod_offset += channel->d.freqmod_incr;
	if (channel->d.freqmod_offset > channel->d.freqmod_modulo)
		channel->d.freqmod_offset -= channel->d.freqmod_modulo;

Is it me, or is there no difference between these two code snippets?

[eriktorbjorn]

Is it me, or is there no difference between these two code snippets?

No, you're right. I messed up when copying/pasting. I meant for the second code snippet to read "while (...)" instead of "if (...)". I've updated my comment accordingly.

[eriktorbjorn]

One danger I can see in my proposed solution would be if channel->d.freqmod_modulo is 0. (It's unsigned, so it can't be negative.) But if that happens, then surely something is seriously out of whack?

[eriktorbjorn]

I've submitted a pull request for this: ​https://github.com/scummvm/scummvm/pull/5459

[eriktorbjorn]

It's been several days since I submitted the pull request. Plenty of times for people to object, so I've merged it.

I can no longer reproduce the error. If you can, let me know. If you can reproduce the VGA version crash, please file a separate bug report about that one.