SCUMM: Use after free error in DOTT intro

[criezy]

With latest master code (b43d66b7f2) I am getting a crash during the Day of the Tentacle intro on macOS when enabling address sanitiser.

The error is:

==80587==ERROR: AddressSanitizer: heap-use-after-free on address 0x00010b0a04a8 at pc 0x000103d7a900 bp 0x00016d3937c0 sp 0x00016d3937b8
WRITE of size 4 at 0x00010b0a04a8 thread T8
    #0 0x103d7a8fc in MidiParser::processEvent(EventInfo const&, bool) midiparser.cpp:286
    #1 0x103d79c7c in MidiParser::onTimer() midiparser.cpp:240
    #2 0x1031b4a60 in Scumm::Player::onTimer() imuse_player.cpp:863
    #3 0x103197c7c in Scumm::IMuseInternal::sequencer_timers(MidiDriver*) imuse.cpp:1021
    #4 0x1031976ec in Scumm::IMuseInternal::on_timer(MidiDriver*) imuse.cpp:353
    #5 0x1031a6c68 in Scumm::IMuseInternal::midiTimerCallback(void*) imuse.cpp:1701
    #6 0x103e5890c in MidiDriver_Emulated::readBuffer(short*, int) emumidi.h:106
    #7 0x103d8d338 in Audio::CopyRateConverter<true, false>::flow(Audio::AudioStream&, short*, unsigned int, unsigned short, unsigned short) rate.cpp:315
    #8 0x103d80270 in Audio::Channel::mix(short*, unsigned int) mixer.cpp:618
    #9 0x103d7fbe8 in Audio::MixerImpl::mixCallback(unsigned char*, unsigned int) mixer.cpp:293
    #10 0x10386a440 in SdlMixerManager::callbackHandler(unsigned char*, int) sdl-mixer.cpp:189
    #11 0x10386a348 in SdlMixerManager::sdlCallback(void*, unsigned char*, int) sdl-mixer.cpp:196

0x00010b0a04a8 is located 552 bytes inside of 1632-byte region [0x00010b0a0280,0x00010b0a08e0)
freed by thread T8 here:
    #0 0x1064bcf14 in wrap__ZdlPv+0x74 (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x4cf14)
    #1 0x103d6e1f0 in MidiParser_SMF::~MidiParser_SMF() midiparser_smf.cpp:63
    #2 0x1031af6c4 in Scumm::Player::clear() imuse_player.cpp:154
    #3 0x1031a14f8 in Scumm::IMuseInternal::stopSound_internal(int) imuse.cpp:722
    #4 0x10319a7b0 in Scumm::IMuseInternal::doCommand_internal(int, int*) imuse.cpp:799
    #5 0x1031a267c in Scumm::IMuseInternal::doCommand_internal(int, int, int, int, int, int, int, int) imuse.cpp:761
    #6 0x1031a5318 in Scumm::IMuseInternal::handle_marker(unsigned int, unsigned char) imuse.cpp:1040
    #7 0x1031bb450 in Scumm::sysexHandler_Scumm(Scumm::Player*, unsigned char const*, unsigned short) sysex_scumm.cpp:185
    #8 0x1031b1580 in Scumm::Player::sysEx(unsigned char const*, unsigned short) imuse_player.cpp:422
    #9 0x103d6c424 in MidiDriver_BASE::sysExNoDelay(unsigned char const*, unsigned short) mididrv.h:213
    #10 0x103d7a774 in MidiParser::processEvent(EventInfo const&, bool) midiparser.cpp:280
    #11 0x103d79c7c in MidiParser::onTimer() midiparser.cpp:240
    #12 0x1031b4a60 in Scumm::Player::onTimer() imuse_player.cpp:863
    #13 0x103197c7c in Scumm::IMuseInternal::sequencer_timers(MidiDriver*) imuse.cpp:1021

previously allocated by thread T0 here:
    #0 0x1064bcafc in wrap__Znwm+0x74 (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x4cafc)
    #1 0x103d72160 in MidiParser::createParser_SMF(signed char) midiparser_smf.cpp:416
    #2 0x1031aed3c in Scumm::Player::start_seq_sound(int, bool) imuse_player.cpp:194
    #3 0x1031ae048 in Scumm::Player::startSound(int, MidiDriver*) imuse_player.cpp:122
    #4 0x1031a0c48 in Scumm::IMuseInternal::startSound_internal(int, int) imuse.cpp:715
    #5 0x1031a1250 in Scumm::IMuseInternal::startSound(int) imuse.cpp:557
    #6 0x1033d2100 in Scumm::Sound::playSound(int) sound.cpp:422
    #7 0x1033cf4fc in Scumm::Sound::processSoundQueues() sound.cpp:157
    #8 0x1033cdee8 in Scumm::Sound::processSound() sound.cpp:144
    #9 0x1033d7174 in Scumm::Sound::soundKludge(int*, int) sound.cpp:868
    #10 0x103375130 in Scumm::ScummEngine_v6::o6_soundKludge() script_v6.cpp:2250
    #11 0x103171d30 in Common::Functor0Mem<void, Scumm::ScummEngine_v70he>::operator()() const func.h:398
    #12 0x1033899e4 in Scumm::ScummEngine::executeOpcode(unsigned char) script.cpp:493
    #13 0x103389538 in Scumm::ScummEngine::executeScript() script.cpp:486
    #14 0x10338f9f8 in Scumm::ScummEngine::runAllScripts() script.cpp:920
    #15 0x1033c11cc in Scumm::ScummEngine::scummLoop(int) scumm.cpp:2541
    #16 0x1033bf28c in Scumm::ScummEngine::go() scumm.cpp:2395
    #17 0x1033c8c38 in Scumm::ScummEngine::run() scumm.h:335

This is with the original Mac CD version of DOTT.
You need to let the intro run for a while before the error occurs.

The issue has been present for at least a month (I tested f039bdb083, which has the issue). I will try to go further back to get a better idea when it was introduced.

[criezy]

The issue seems to occur always at the same place (or at least around the same place), about 2 minutes and 35 seconds in (about 10 seconds after Bernard says "And now I know that I must go back to the mention", then you get the LucasArts Entertainment Company Presents screen, and the start of the next scene where they get to the car and you hear the car exhaust noise, and the issue is right after that).

I have now gone back to d099ed504 (from late May) and it also contains the issue.

[criezy]

The bug was already present at the end of October last year (commit 7b7bea9fe5).

I have also tried running with Thread Sanitizer, and while it reports a number of issues right at the start, there is no issue at the point where Address Sanitizer reports the use after free error (which is hopefully good news - personally I hate when I have to debug race conditions).

[criezy]

This particular issue was introduced in 0af83d3faa (KYRA/MIDI: Fix minor MT-32/GM issues).

However even before that commit there was already a use after free error, just not in the same place (one level above in MidiParser::onTimer()).

The core of the issue is that on line 280 of MidiParser::processEvent() the call to _driver->sysExNoDelay() results in a Scumm::IMuseInternal::stopSound_internal() that deletes the MidiDriver. So any code in the MiDiDriver that gets executed after that line and accesses a member variable results in a use after free error.

I have however no idea how that could be fixed. Could Scumm::IMuseInternal::stopSound_internal() delay the deletion of the MidiDriver for example?

[eriktorbjorn]

Would it help if _sysExDelay was updated before the call to sysExNoDelay(), or is there code that depends on it not being updated until after?

[criezy]

Replying to eriktorbjorn:

Would it help if _sysExDelay was updated before the call to sysExNoDelay(), or is there code that depends on it not being updated until after?

This might be difficult to do since _sysExDelay is set using the value returned by sysExNoDelay().

And that would not be sufficient to fix the issue. The use-after-free issue would be fixed for _sysExDelay, but another one would occur in onTimer() after it returns from the processEvent() call. That one could probably easily be fixed by returning false in `processEvent() for that case.

Maybe one way to fix it would be to change the code so that sysExNoDelay() returns a special value (such as (uint)-1) that indicates we do not want to update the _sysExDelay variable and should return false.

But I am grasping at straws here as I am unfamiliar with the MIDI code and don't know what the implications of each change would be.

[athrxx]

Just to make sure I am seeing the same things as you guys: Scumm::IMuseInternal::stopSound_internal() will call player->clear() and that one will delete the _parser (not the MidiDriver object, since that would be the player itself).

It seems quite feasable to make a few changes that will omit the deletion of the parser. I can make an attempt at this if this is what you're talking about...

[athrxx]

e.g. something like this:
​https://github.com/athrxx/scummvm/tree/imuse-fix-12776

(haven't done any serious testing though)

[criezy]

Yes, that was a typo in my third comment. The MidiParser gets deleted, not the MidiDriver.

[orgads]

Fixed in 0876537f063d89aebfee1fb74e5816fd235e359a