From 559229dea167621f49e145ecb161ac5e6193cca2 Mon Sep 17 00:00:00 2001
From: Gleb Mazovetskiy <glex.spb@gmail.com>
Date: Sun, 2 May 2021 13:49:58 +0100
Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9E=20=20Duplicate=20sounds:=20Fix=20d?=
 =?UTF-8?q?ata=20race?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This fixes a data race by deleting the sound exactly when it finishes
playing.

Previously, a data race could happen in
`CleanupFinishedDuplicateSounds`, where the sound would be destroyed
while Aulib was reading from its buffer.
---
 Source/effects.cpp         |  1 -
 Source/sound.cpp           | 28 +++++++++++++++++-----------
 Source/sound.h             |  1 -
 Source/sound_stubs.cpp     |  1 -
 Source/utils/soundsample.h |  5 +++++
 5 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/Source/effects.cpp b/Source/effects.cpp
index f951bb389..686f83440 100644
--- a/Source/effects.cpp
+++ b/Source/effects.cpp
@@ -1295,7 +1295,6 @@ void sound_update()
 	}
 
 	stream_update();
-	CleanupFinishedDuplicateSounds();
 }
 
 void effects_cleanup_sfx()
diff --git a/Source/sound.cpp b/Source/sound.cpp
index ae0b9e6e2..460f90f5a 100644
--- a/Source/sound.cpp
+++ b/Source/sound.cpp
@@ -6,6 +6,7 @@
 #include "sound.h"
 
 #include <cstdint>
+#include <list>
 #include <memory>
 
 #include <aulib.h>
@@ -19,6 +20,7 @@
 #include "storm/storm_sdl_rw.h"
 #include "storm/storm.h"
 #include "utils/log.hpp"
+#include "utils/sdl_mutex.h"
 #include "utils/stdcompat/optional.hpp"
 #include "utils/stdcompat/shared_ptr_array.hpp"
 #include "utils/stubs.h"
@@ -65,13 +67,24 @@ void CleanupMusic()
 #endif
 }
 
-std::vector<std::unique_ptr<SoundSample>> duplicateSounds;
+std::list<std::unique_ptr<SoundSample>> duplicateSounds;
+SDLMutexUniquePtr duplicateSoundsMutex;
+
 SoundSample *DuplicateSound(const SoundSample &sound) {
 	auto duplicate = std::make_unique<SoundSample>();
 	if (duplicate->DuplicateFrom(sound) != 0)
 		return nullptr;
 	auto *result = duplicate.get();
-	duplicateSounds.push_back(std::move(duplicate));
+	{
+		SDLMutexLockGuard lock(duplicateSoundsMutex.get());
+		duplicateSounds.push_back(std::move(duplicate));
+		auto it = duplicateSounds.end();
+		--it;
+		result->SetFinishCallback([it](Aulib::Stream &stream) {
+			SDLMutexLockGuard lock(duplicateSoundsMutex.get());
+			duplicateSounds.erase(it);
+		});
+	}
 	return result;
 }
 
@@ -120,15 +133,6 @@ void ClearDuplicateSounds() {
 	duplicateSounds.clear();
 }
 
-void CleanupFinishedDuplicateSounds()
-{
-	duplicateSounds.erase(
-	    std::remove_if(duplicateSounds.begin(), duplicateSounds.end(), [](const auto &sound) {
-		    return !sound->IsPlaying();
-	    }),
-	    duplicateSounds.end());
-}
-
 void snd_play_snd(TSnd *pSnd, int lVolume, int lPan)
 {
 	DWORD tc;
@@ -211,12 +215,14 @@ void snd_init()
 	LogVerbose(LogCategory::Audio, "Aulib sampleRate={} channels={} frameSize={} format={:#x}",
 	    Aulib::sampleRate(), Aulib::channelCount(), Aulib::frameSize(), Aulib::sampleFormat());
 
+	duplicateSoundsMutex = SDLMutexUniquePtr { SDL_CreateMutex() };
 	gbSndInited = true;
 }
 
 void snd_deinit() {
 	if (gbSndInited) {
 		Aulib::quit();
+		duplicateSoundsMutex = nullptr;
 	}
 
 	gbSndInited = false;
diff --git a/Source/sound.h b/Source/sound.h
index 6d3b53d48..8c5140789 100644
--- a/Source/sound.h
+++ b/Source/sound.h
@@ -48,7 +48,6 @@ struct TSnd {
 
 extern bool gbSndInited;
 void ClearDuplicateSounds();
-void CleanupFinishedDuplicateSounds();
 void snd_stop_snd(TSnd *pSnd);
 void snd_play_snd(TSnd *pSnd, int lVolume, int lPan);
 std::unique_ptr<TSnd> sound_file_load(const char *path, bool stream = false);
diff --git a/Source/sound_stubs.cpp b/Source/sound_stubs.cpp
index ae1dd99b6..a35057c6a 100644
--- a/Source/sound_stubs.cpp
+++ b/Source/sound_stubs.cpp
@@ -11,7 +11,6 @@ bool gbSoundOn;
 // AllowShortFunctionsOnASingleLine: None
 // clang-format off
 void ClearDuplicateSounds() { }
-void CleanupFinishedDuplicateSounds() { }
 void snd_stop_snd(TSnd *pSnd) { }
 void snd_play_snd(TSnd *pSnd, int lVolume, int lPan) { }
 std::unique_ptr<TSnd> sound_file_load(const char *path, bool stream) { return nullptr; }
diff --git a/Source/utils/soundsample.h b/Source/utils/soundsample.h
index aa6622502..583e2d667 100644
--- a/Source/utils/soundsample.h
+++ b/Source/utils/soundsample.h
@@ -23,6 +23,11 @@ public:
 	void Stop();
 	int SetChunkStream(std::string filePath);
 
+	void SetFinishCallback(Aulib::Stream::Callback &&callback)
+	{
+		stream_->setFinishCallback(std::forward<Aulib::Stream::Callback>(callback));
+	}
+
 	/**
 	 * @brief Sets the sample's WAV, FLAC, or Ogg/Vorbis data.
 	 * @param fileData Buffer containing the data