AUDIO: SCUMM: buffer-overflow in MidiDriver_ADLIB::adlibPlayNote (ASAN)

[dwatteau]

This is on macOS 12.6 (arm64) with clang++ 14.0 and an --enable-asan build. HEAD is 20c1dbb50d14.

This looks similar to previous issue #12258, which was closed.

1. Start the Passport to Adventure demo (​https://downloads.scummvm.org/frs/demos/scumm/pass-dos-en.zip) with default settings (so Adlib output is used here).
2. Pick the Monkey Island game.
3. Press Esc when the title appears, and open the SCUMM bar door.

When ASAN is enabled, the following crash systematically happens:

=================================================================
==7999==ERROR: AddressSanitizer: global-buffer-overflow on address 0x000102380b1f at pc 0x000101eac944 bp 0x00016f21fb60 sp 0x00016f21fb58
READ of size 1 at 0x000102380b1f thread T7
    #0 0x101eac940 in MidiDriver_ADLIB::adlibPlayNote(int, int) adlib.cpp:1932
    #1 0x101ea4158 in MidiDriver_ADLIB::adlibSetParam(int, unsigned char, int, bool) adlib.cpp:1814
    #2 0x101eaab5c in MidiDriver_ADLIB::mcIncStuff(AdLibVoice*, Struct10*, Struct11*) adlib.cpp:1727
    #3 0x101ea89e4 in MidiDriver_ADLIB::onTimer() adlib.cpp:1662
    #4 0x1011b8514 in Common::Functor0Mem<void, Scumm::ScummEngine_v70he>::operator()() const func.h:397
    #5 0x101ed9318 in OPL::EmulatedOPL::readBuffer(short*, int) fmopl.cpp:358
    #6 0x101f32b6c in Audio::CopyRateConverter<false, false>::flow(Audio::AudioStream&, short*, unsigned int, unsigned short, unsigned short) rate.cpp:314
    #7 0x101f12414 in Audio::Channel::mix(short*, unsigned int) mixer.cpp:648
    #8 0x101f11cf4 in Audio::MixerImpl::mixCallback(unsigned char*, unsigned int) mixer.cpp:301
    #9 0x101b75e08 in SdlMixerManager::callbackHandler(unsigned char*, int) sdl-mixer.cpp:184
    #10 0x101b75d0c in SdlMixerManager::sdlCallback(void*, unsigned char*, int) sdl-mixer.cpp:191
    #11 0x1036afb9c in outputCallback+0x168 (libSDL2-2.0.0.dylib:arm64+0xc7b9c)
    #12 0x1b8ed71d4 in ClientAudioQueue::CallOutputCallback(AudioQueueBuffer*)+0x120 (AudioToolbox:arm64e+0x421d4)
    #13 0x1b8ec1374 in ClientAudioQueue::FetchAndDeliverPendingCallbacks(unsigned int)+0x328 (AudioToolbox:arm64e+0x2c374)
    #14 0x1b8ec0fb8 in _XCallbackNotificationsAvailable+0xd4 (AudioToolbox:arm64e+0x2bfb8)
    #15 0x1b7d9b188 in mshMIGPerform+0x104 (libAudioToolboxUtility.dylib:arm64e+0xf188)
    #16 0x1ab80d814 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE1_PERFORM_FUNCTION__+0x38 (CoreFoundation:arm64e+0x85814)
    #17 0x1ab80d6d0 in __CFRunLoopDoSource1+0x258 (CoreFoundation:arm64e+0x856d0)
    #18 0x1ab80bb64 in __CFRunLoopRun+0x940 (CoreFoundation:arm64e+0x83b64)
    #19 0x1ab80aa80 in CFRunLoopRunSpecific+0x254 (CoreFoundation:arm64e+0x82a80)
    #20 0x1036af754 in audioqueue_thread+0x47c (libSDL2-2.0.0.dylib:arm64+0xc7754)
    #21 0x10364ab60 in SDL_RunThread+0x2c (libSDL2-2.0.0.dylib:arm64+0x62b60)
    #22 0x1036a2350 in RunThread+0x8 (libSDL2-2.0.0.dylib:arm64+0xba350)
    #23 0x1ab740268 in _pthread_start+0x90 (libsystem_pthread.dylib:arm64e+0x7268)
    #24 0x1ab73b088 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x2088)

0x000102380b1f is located 33 bytes to the left of global variable 'Audio::milesMT32SysExInitReverb' defined in 'audio/miles_midi.cpp:46:12' (0x102380b40) of size 3
0x000102380b1f is located 1 bytes to the left of global variable 'Audio::milesMT32SysExPartialReserveTable' defined in 'audio/miles_midi.cpp:42:12' (0x102380b20) of size 9
0x000102380b1f is located 22 bytes to the right of global variable 'Audio::milesMT32SysExChansSetup' defined in 'audio/miles_midi.cpp:38:12' (0x102380b00) of size 9
SUMMARY: AddressSanitizer: global-buffer-overflow adlib.cpp:1932 in MidiDriver_ADLIB::adlibPlayNote(int, int)
Shadow bytes around the buggy address:
  0x007020490110: 00 00 00 00 f9 f9 f9 f9 00 01 f9 f9 04 f9 f9 f9
  0x007020490120: 00 01 f9 f9 00 05 f9 f9 05 f9 f9 f9 00 00 00 00
  0x007020490130: 00 00 00 00 01 f9 f9 f9 02 f9 f9 f9 02 f9 f9 f9
  0x007020490140: 00 00 00 00 00 00 00 00 00 00 f9 f9 00 00 00 00
  0x007020490150: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x007020490160: 00 01 f9[f9]00 01 f9 f9 03 f9 f9 f9 00 00 00 00
  0x007020490170: 00 00 00 00 00 00 00 00 00 00 00 00 01 f9 f9 f9
  0x007020490180: 01 f9 f9 f9 01 f9 f9 f9 01 f9 f9 f9 00 f9 f9 f9
  0x007020490190: 00 f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 00
  0x0070204901a0: 00 00 00 00 f9 f9 f9 f9 01 f9 f9 f9 04 f9 f9 f9
  0x0070204901b0: 07 f9 f9 f9 00 04 f9 f9 00 01 f9 f9 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
Thread T7 created by T0 here:
    #0 0x103d18c5c in wrap_pthread_create+0x54 (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x38c5c)
    #1 0x1036a2308 in SDL_SYS_CreateThread+0x98 (libSDL2-2.0.0.dylib:arm64+0xba308)
    #2 0x10364ac34 in SDL_CreateThreadWithStackSize_REAL+0x6c (libSDL2-2.0.0.dylib:arm64+0x62c34)
    #3 0x1036aee64 in COREAUDIO_OpenDevice+0x23c (libSDL2-2.0.0.dylib:arm64+0xc6e64)
    #4 0x1035f3244 in open_audio_device+0x5cc (libSDL2-2.0.0.dylib:arm64+0xb244)
    #5 0x1035f2c10 in SDL_OpenAudio_REAL+0x70 (libSDL2-2.0.0.dylib:arm64+0xac10)
    #6 0x101b747d8 in SdlMixerManager::init() sdl-mixer.cpp:72
    #7 0x100fca95c in OSystem_SDL::initBackend() sdl.cpp:284
    #8 0x100fdff64 in OSystem_POSIX::initBackend() posix.cpp:92
    #9 0x100fe4c2c in OSystem_MacOSX::initBackend() macosx.cpp:121
    #10 0x100fec7c4 in scummvm_main main.cpp:501
    #11 0x100fe3ea0 in main macosx-main.cpp:44
    #12 0x1032d5088 in start+0x204 (dyld:arm64e+0x5088)

==7999==ABORTING
Abort trap: 6

Here's what's happening when the following note is hit:

// channel: 2, note: 112, note2: -4, oct: 0, notex: 255, old: 0, i: 2047!
adlibWrite(channel + 0xA0, g_noteFrequencies[i]);

but g_noteFrequencies[] only has 144 elements.

[dwatteau]

I wonder if this isn't just a matter of changing the note2 variable from int to byte, in MidiDriver_ADLIB::adlibPlayNote().

But I'll let someone with more knowledge about audio and Adlib code check if that's the case.

[dwatteau]

Also, that particular sound (= opening the SCUMM bar door) seems a bit off. In DOSbox or DREAMM (in Adlib mode), you'll hear two "clicks" when opening the door; but only a single one in ScummVM (even with various opl_driver values).

[athrxx]

Thanks, I have fixed it. It seems to be a bug in the original driver, too (it isn't supposed to eat note2 values >= 128).

But maybe the values that get passed from adlibSetParam are wrong, too. I haven't checked that.

[athrxx]

I have just quickly tested with Dreamm. Weirdly, sometimes the door opens with two clicks and sometimes with just one. So I assume that the original driver indeed reads some random value from memory (at would-be table offset 255) and that value isn't even constant. The door closing sound is inconsistent, too.

[dwatteau]

Thanks for the fix!

As for the random extra sounds, we don't really know if this was intended. I had never noticed this before!

Well, closing this issue, since the original problem has been fixed.

[athrxx]

Thanks, the research regarding our AdLib sound effects quality will go on. I was just kind of busy with fixing the Sam & Max MT-32 sound, but I'll get back to it...