From 1e11da712ba8359cb65b0a171c7ce83de6a17879 Mon Sep 17 00:00:00 2001
From: Thierry Crozat <criezy@scummvm.org>
Date: Sat, 7 Jul 2018 01:03:04 +0100
Subject: [PATCH] COMMON: Add mutex to protect access to the String memory pool

This fixes a crash due to concurrent access to the global MemoryPool
used by the String class when String objects are used simultaneously
from several threads (as is for example the case when enabling the
cloud features).

See bug #10524: Thread safety issue with MemoryPool
---
 common/str.cpp    | 30 ++++++++++++++++++++++++++++++
 common/str.h      |  2 ++
 common/system.cpp |  1 +
 3 files changed, 33 insertions(+)

diff --git a/common/str.cpp b/common/str.cpp
index f60d7d5e93c..8ed652f8471 100644
--- a/common/str.cpp
+++ b/common/str.cpp
@@ -25,10 +25,36 @@
 #include "common/memorypool.h"
 #include "common/str.h"
 #include "common/util.h"
+#include "common/mutex.h"
 
 namespace Common {
 
 MemoryPool *g_refCountPool = nullptr; // FIXME: This is never freed right now
+MutexRef g_refCountPoolMutex = nullptr;
+
+void lockMemoryPoolMutex() {
+	// The Mutex class can only be used once g_system is set and initialized,
+	// but we may use the String class earlier than that (it is for example
+	// used in the OSystem_POSIX constructor). However in those early stages
+	// we can hope we don't have multiple threads either.
+	if (!g_system || !g_system->backendInitialized())
+		return;
+	if (!g_refCountPoolMutex)
+		g_refCountPoolMutex = g_system->createMutex();
+	g_system->lockMutex(g_refCountPoolMutex);
+}
+
+void unlockMemoryPoolMutex() {
+	if (g_refCountPoolMutex)
+		g_system->unlockMutex(g_refCountPoolMutex);
+}
+
+void String::releaseMemoryPoolMutex() {
+	if (g_refCountPoolMutex){
+		g_system->deleteMutex(g_refCountPoolMutex);
+		g_refCountPoolMutex = nullptr;
+	}
+}
 
 static uint32 computeCapacity(uint32 len) {
 	// By default, for the capacity we use the next multiple of 32
@@ -173,12 +199,14 @@ void String::ensureCapacity(uint32 new_size, bool keep_old) {
 void String::incRefCount() const {
 	assert(!isStorageIntern());
 	if (_extern._refCount == nullptr) {
+		lockMemoryPoolMutex();
 		if (g_refCountPool == nullptr) {
 			g_refCountPool = new MemoryPool(sizeof(int));
 			assert(g_refCountPool);
 		}
 
 		_extern._refCount = (int *)g_refCountPool->allocChunk();
+		unlockMemoryPoolMutex();
 		*_extern._refCount = 2;
 	} else {
 		++(*_extern._refCount);
@@ -196,8 +224,10 @@ void String::decRefCount(int *oldRefCount) {
 		// The ref count reached zero, so we free the string storage
 		// and the ref count storage.
 		if (oldRefCount) {
+			lockMemoryPoolMutex();
 			assert(g_refCountPool);
 			g_refCountPool->freeChunk(oldRefCount);
+			unlockMemoryPoolMutex();
 		}
 		delete[] _str;
 
diff --git a/common/str.h b/common/str.h
index 5ed14b6942e..704114b68e5 100644
--- a/common/str.h
+++ b/common/str.h
@@ -47,6 +47,8 @@ class String {
 public:
 	static const uint32 npos = 0xFFFFFFFF;
 
+	static void releaseMemoryPoolMutex();
+
 	typedef char          value_type;
 	/**
 	 * Unsigned version of the underlying type. This can be used to cast
diff --git a/common/system.cpp b/common/system.cpp
index 97c8f8520bb..c8023c8ec29 100644
--- a/common/system.cpp
+++ b/common/system.cpp
@@ -105,6 +105,7 @@ void OSystem::initBackend() {
 
 void OSystem::destroy() {
 	_backendInitialized = false;
+	Common::String::releaseMemoryPoolMutex();
 	delete this;
 }