AGS: Buffer overflow in AdLib driver at the start of Black Cauldron

[criezy]

When starting Black Cauldron, when it reaches the title screen after the Disney logo, get a buffer overflow. It does not matter if you skip or not the Disney logo sound with Esc. Either way the bugger overflow occurs.

Here is the address-sanitiser report:

=================================================================
==90047==ERROR: AddressSanitizer: global-buffer-overflow on address 0x000105353c58 at pc 0x0001050c4e0c bp 0x00016c0aa990 sp 0x00016c0aa988
READ of size 1 at 0x000105353c58 thread T8
    #0 0x1050c4e08 in MidiDriver_ADLIB::adlibPlayNote(int, int) adlib.cpp:1920
    #1 0x1050c7a5c in MidiDriver_ADLIB::adlibNoteOnEx(int, unsigned char, int) adlib.cpp:2282
    #2 0x1050c6bb0 in MidiDriver_ADLIB::mcKeyOn(AdLibVoice*, AdLibInstrument const*, unsigned char, unsigned char, AdLibInstrument const*, unsigned char) adlib.cpp:2078
    #3 0x1050bb0c8 in MidiDriver_ADLIB::partKeyOn(AdLibPart*, AdLibInstrument const*, unsigned char, unsigned char, AdLibInstrument const*, unsigned char) adlib.cpp:1956
    #4 0x1050bafe4 in AdLibPart::noteOn(unsigned char, unsigned char) adlib.cpp:1052
    #5 0x1050c1a7c in MidiDriver_ADLIB::send(signed char, unsigned int) adlib.cpp:1508
    #6 0x1050bacc4 in AdLibPart::send(unsigned int) adlib.cpp:1038
    #7 0x1042cbcd0 in AGS::Music::sendToChannel(unsigned char, unsigned int) music.cpp:63
    #8 0x1050f0a00 in Audio::MidiPlayer::send(unsigned int) midiplayer.cpp:111
    #9 0x1050e9040 in MidiParser::sendToDriver(unsigned int) midiparser.cpp:88
    #10 0x1050e7dec in MidiParser_SMF::sendToDriver(unsigned int) midiparser_smf.cpp:402
    #11 0x1050e9e38 in MidiParser::sendToDriver(unsigned char, unsigned char, unsigned char) midiparser.h:341
    #12 0x1050ec0a4 in MidiParser::processEvent(EventInfo const&, bool) midiparser.cpp:311
    #13 0x1050eaca8 in MidiParser::onTimer() midiparser.cpp:240
    #14 0x1050f11f0 in Audio::MidiPlayer::onTimer() midiplayer.cpp:157
    #15 0x1050f1008 in Audio::MidiPlayer::timerCallback(void*) midiplayer.cpp:147
    #16 0x1050c0fa8 in MidiDriver_ADLIB::onTimer() adlib.cpp:1628
 #17 0x1050ca928 in Common::Functor0Mem<void, MidiDriver_ADLIB>::operator()() const func.h:398
    #18 0x1050d840c in OPL::EmulatedOPL::readBuffer(short*, int) fmopl.cpp:337
    #19 0x1051b8d4c in Audio::CopyRateConverter<false, false>::flow(Audio::AudioStream&, short*, unsigned int, unsigned short, unsigned short) rate.cpp:315
    #20 0x1050f3824 in Audio::Channel::mix(short*, unsigned int) mixer.cpp:618
    #21 0x1050f3170 in Audio::MixerImpl::mixCallback(unsigned char*, unsigned int) mixer.cpp:293
    #22 0x104ced148 in SdlMixerManager::callbackHandler(unsigned char*, int) sdl-mixer.cpp:189
    #23 0x104ced050 in SdlMixerManager::sdlCallback(void*, unsigned char*, int) sdl-mixer.cpp:196
0x000105353c58 is located 8 bytes to the left of global variable 'Audio::MSADPCMAdaptationTable' defined in 'audio/decoders/adpcm.cpp:368:18' (0x105353c60) of size 64
0x000105353c58 is located 48 bytes to the right of global variable 'Audio::s_xaTable' defined in 'audio/decoders/adpcm.cpp:151:18' (0x105353c00) of size 40

The issue is not recent. I went as far back as commit 5e8e40d (Feb 7) and it still occurs.

[criezy]

And debugger output:

frame #5: 0x0000000100f18e0c scummvm`MidiDriver_ADLIB::adlibPlayNote(this=0x000000011a8ca100, channel=1, note=0) at adlib.cpp:1920:29
   1917		}
   1918	
   1919		i = (notex << 3) + ((note >> 4) & 0x7);
-> 1920		adlibWrite(channel + 0xA0, g_noteFrequencies[i]);
   1921		adlibWrite(channel + 0xB0, oct | 0x20);
   1922	}
   1923	
(lldb) p i
(int) $1 = 2040
(lldb) p sizeof(g_noteFrequencies)
(unsigned long) $2 = 144
(lldb) p note
(int) $3 = 0
(lldb) p notex
(byte) $4 = '?'
(lldb) p (int)notex
(int) $5 = 255

[criezy]

The issue occurs when trying to play the music1.mid file. The file plays without error in VLC.

Changing the preferred audio device to use FluidSynth or Apple DLS Synth instead of the default also plays this properly. So the issue might be in the MidiDriver_ADLIB itself and not in AGS or the game data.

Maybe we just need to add a sanity check:

audio/adlib.cpp

@@ -1917 +1917 @@
        }
 
        i = (notex << 3) + ((note >> 4) & 0x7);
-       adlibWrite(channel + 0xA0, g_noteFrequencies[i]);
-       adlibWrite(channel + 0xB0, oct | 0x20);
+       if (i >= 0 && i < sizeof(g_noteFrequencies)) {
+               adlibWrite(channel + 0xA0, g_noteFrequencies[i]);
+               adlibWrite(channel + 0xB0, oct | 0x20);
+       }
 }

Or should the case where note == 0 be handled before we get there, such as in AdLibPart::noteOn()?

[criezy]

The game to test can be found at ​https://www.adventuregamestudio.co.uk/site/games/game/451-black-cauldron-remake/

[dreammaster]

I've done multiple tests recently, and not been able to replicate any crash. Given the number of changes to the support code over time, and the complete replacement of the AGS codebase, it may be that the problem has simply been fixed at some point. I'm closing this for now, and we can re-open it again in the future if it's able to be replicated again.

[criezy]

I just tested again, and I confirm that it works for me now as well.