Permalink
Browse files

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
  • Loading branch information...
criezy committed Jul 7, 2018
1 parent c39dcc5 commit 1e11da712ba8359cb65b0a171c7ce83de6a17879
Showing with 33 additions and 0 deletions.
  1. +30 −0 common/str.cpp
  2. +2 −0 common/str.h
  3. +1 −0 common/system.cpp
View
@@ -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;
View
@@ -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
View
@@ -105,6 +105,7 @@ void OSystem::initBackend() {
void OSystem::destroy() {
_backendInitialized = false;
Common::String::releaseMemoryPoolMutex();
delete this;
}

0 comments on commit 1e11da7

Please sign in to comment.