Permalink
Browse files

LASTEXPRESS: fix race condition in sound code

SoundEntry::play() calls StreamedSound::setFilterId(),
StreamSound::setFilterId() requires the underlying reference to be alive.
SoundQueue::handleTimer() checks that the stream is still alive
by calling SoundEntry::isFinished(). However, if the stream is finalized
just between calls to SoundEntry::isFinished() and SoundEntry::play(),
the sound mixer frees the stream leading to use-after-free in setFilterId().

Turn off the automatical disposing, delete the stream in SoundEntry::~SoundEntry().
  • Loading branch information...
grechnik committed Oct 15, 2018
1 parent 45641f3 commit 8162309212bbc287632fc375d1740a64733019fb
Showing with 11 additions and 5 deletions.
  1. +10 −4 engines/lastexpress/data/snd.cpp
  2. +1 −1 engines/lastexpress/data/snd.h
@@ -446,8 +446,9 @@ LastExpress_ADPCMStream *SimpleSound::makeDecoder(Common::SeekableReadStream *in
return new LastExpress_ADPCMStream(in, DisposeAfterUse::YES, size, _blockSize, filterId);
}
void SimpleSound::play(Audio::AudioStream *as) {
g_system->getMixer()->playStream(Audio::Mixer::kPlainSoundType, &_handle, as);
void SimpleSound::play(Audio::AudioStream *as, DisposeAfterUse::Flag autofreeStream) {
g_system->getMixer()->playStream(Audio::Mixer::kPlainSoundType, &_handle, as,
-1, Audio::Mixer::kMaxChannelVolume, 0, autofreeStream);
}
//////////////////////////////////////////////////////////////////////////
@@ -456,6 +457,7 @@ void SimpleSound::play(Audio::AudioStream *as) {
StreamedSound::StreamedSound() : _as(NULL), _loaded(false) {}
StreamedSound::~StreamedSound() {
delete _as;
_as = NULL;
}
@@ -467,11 +469,15 @@ bool StreamedSound::load(Common::SeekableReadStream *stream, int32 filterId) {
loadHeader(stream);
if (_as) {
stop();
delete _as;
}
// Start decoding the input stream
_as = makeDecoder(stream, _size, filterId);
// Start playing the decoded audio stream
play(_as);
play(_as, DisposeAfterUse::NO);
_loaded = true;
@@ -501,7 +507,7 @@ AppendableSound::AppendableSound() : SimpleSound() {
_finished = false;
// Start playing the decoded audio stream
play(_as);
play(_as, DisposeAfterUse::YES);
// Initialize the block size
// TODO: get it as an argument?
@@ -62,7 +62,7 @@ class SimpleSound {
protected:
void loadHeader(Common::SeekableReadStream *in);
LastExpress_ADPCMStream *makeDecoder(Common::SeekableReadStream *in, uint32 size, int32 filterId = -1) const;
void play(Audio::AudioStream *as);
void play(Audio::AudioStream *as, DisposeAfterUse::Flag autofreeStream);
uint32 _size; ///< data size
///< - NIS: size of all blocks, including those located in the matching LNK file

0 comments on commit 8162309

Please sign in to comment.