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.

2 files changed, 62 insertions, 2 deletions

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);
 
diff --git a/engines/sci/sound/audio32.h b/engines/sci/sound/audio32.h
index 82893fb5cf..416b81d865 100644
--- a/engines/sci/sound/audio32.h
+++ b/engines/sci/sound/audio32.h
@@ -225,6 +225,8 @@ public:
 	int16 findChannelById(const ResourceId resourceId, const reg_t soundNode = NULL_REG) const;
 
 private:
+	typedef Common::Array<Resource *> UnlockList;
+
 	/**
 	 * The audio channels.
 	 */
@@ -238,6 +240,23 @@ private:
 	uint8 _numActiveChannels;
 
 	/**
+	 * Whether or not we are in the audio thread.
+	 *
+	 * This flag is used instead of passing a parameter to
+	 * `freeUnusedChannels` because a parameter would
+	 * require forwarding through the public method `stop`,
+	 * and there is not currently any reason for this
+	 * implementation detail to be exposed.
+	 */
+	bool _inAudioThread;
+
+	/**
+	 * The list of resources from freed channels that need
+	 * to be unlocked from the main thread.
+	 */
+	UnlockList _resourcesToUnlock;
+
+	/**
 	 * Gets the audio channel at the given index.
 	 */
 	inline AudioChannel &getChannel(const int16 channelIndex) {
@@ -266,6 +285,12 @@ private:
 	 */
 	void freeChannel(const int16 channelIndex);
 
+	/**
+	 * Unlocks all resources that were freed by the audio
+	 * thread.
+	 */
+	void unlockResources();
+
 #pragma mark -
 #pragma mark Script compatibility
 public: