From 6ec206ccee670254f591c93a7421e16ad3124a66 Mon Sep 17 00:00:00 2001
From: Colin Snover
Date: Fri, 24 Jun 2016 14:45:41 -0500
Subject: SCI32: Fix race condition when freeing audio resources

It's not possible to call any ResourceManager methods from any
thread other than the main thread because it is not thread-safe.
---
 engines/sci/sound/audio32.cpp | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

(limited to 'engines/sci/sound/audio32.cpp')

diff --git a/engines/sci/sound/audio32.cpp b/engines/sci/sound/audio32.cpp
index 7958a6f9d7..1714d3da3c 100644
--- a/engines/sci/sound/audio32.cpp
+++ b/engines/sci/sound/audio32.cpp
@@ -107,6 +107,7 @@ Audio32::Audio32(ResourceManager *resMan) :
 	_mutex(),
 
 	_numActiveChannels(0),
+	_inAudioThread(false),
 
 	_maxAllowedSampleRate(44100),
 	_maxAllowedBitDepth(16),
@@ -236,10 +237,18 @@ int Audio32::writeAudioInternal(Audio::RewindableAudioStream *const sourceStream
 int Audio32::readBuffer(Audio::st_sample_t *buffer, const int numSamples) {
 	Common::StackLock lock(_mutex);
 
+	// ResourceManager is not thread-safe so we need to
+	// avoid calling into it from the audio thread, but at
+	// the same time we need to be able to clear out any
+	// finished channels on a regular basis
+	_inAudioThread = true;
+
 	// The system mixer should not try to get data when
 	// Audio32 is paused
 	assert(_pausedAtTick == 0 && _numActiveChannels > 0);
 
+	freeUnusedChannels();
+
 	// The caller of `readBuffer` is a rate converter,
 	// which reuses (without clearing) an intermediate
 	// buffer, so we need to zero the intermediate buffer
@@ -354,7 +363,7 @@ int Audio32::readBuffer(Audio::st_sample_t *buffer, const int numSamples) {
 		}
 	}
 
-	freeUnusedChannels();
+	_inAudioThread = false;
 
 	return maxSamplesWritten;
 }
@@ -440,6 +449,10 @@ void Audio32::freeUnusedChannels() {
 			}
 		}
 	}
+
+	if (!_inAudioThread) {
+		unlockResources();
+	}
 }
 
 void Audio32::freeChannel(const int16 channelIndex) {
@@ -450,7 +463,17 @@ void Audio32::freeChannel(const int16 channelIndex) {
 	// 4. Clear monitored memory buffer, if one existed
 	Common::StackLock lock(_mutex);
 	AudioChannel &channel = getChannel(channelIndex);
-	_resMan->unlockResource(channel.resource);
+
+	// We cannot unlock resources from the audio thread
+	// because ResourceManager is not thread-safe; instead,
+	// we just record that the resource needs unlocking and
+	// unlock it whenever we are on the main thread again
+	if (_inAudioThread) {
+		_resourcesToUnlock.push_back(channel.resource);
+	} else {
+		_resMan->unlockResource(channel.resource);
+	}
+
 	channel.resource = nullptr;
 	delete channel.stream;
 	channel.stream = nullptr;
@@ -464,6 +487,16 @@ void Audio32::freeChannel(const int16 channelIndex) {
 	}
 }
 
+void Audio32::unlockResources() {
+	Common::StackLock lock(_mutex);
+	assert(!_inAudioThread);
+
+	for (UnlockList::const_iterator it = _resourcesToUnlock.begin(); it != _resourcesToUnlock.end(); ++it) {
+		_resMan->unlockResource(*it);
+	}
+	_resourcesToUnlock.clear();
+}
+
 #pragma mark -
 #pragma mark Script compatibility
 
@@ -497,6 +530,8 @@ void Audio32::setNumOutputChannels(int16 numChannels) {
 uint16 Audio32::play(int16 channelIndex, const ResourceId resourceId, const bool autoPlay, const bool loop, const int16 volume, const reg_t soundNode, const bool monitor) {
 	Common::StackLock lock(_mutex);
 
+	freeUnusedChannels();
+
 	if (channelIndex != kNoExistingChannel) {
 		AudioChannel &channel = getChannel(channelIndex);
 
-- 
cgit v1.2.3