Skip to content
Permalink
Browse files

Fix synchronization issue at thread start

If a newly spawned thread called getThreadId or getThreadHandle before
the spawning thread finished saving the thread handle, then the
handle/id would be used uninitialized.  This would cause the threading
tests to fail since isCurrentThread would return false, and if Minetest
is built with C++11 support the std::thread object pointer would be
dereferenced while ininitialized, causing a segmentation fault.

This fixes the issue by using a mutex to force the spawned thread to
wait for the spawning thread to finish initializing the thread object.

An alternative way to handle this would be to also set the thread
handle/id in the started thread but this wouldn't work for C++11
builds because there's no way to get the partially constructed object.
  • Loading branch information...
ShadowNinja committed Jan 28, 2017
1 parent 79d752b commit c93f7f5ceaf255bc9314be022b7bebb55b1d0605
Showing with 27 additions and 2 deletions.
  1. +9 −0 src/threading/mutex.cpp
  2. +2 −0 src/threading/mutex.h
  3. +15 −0 src/threading/thread.cpp
  4. +1 −0 src/threading/thread.h
  5. +0 −2 src/unittest/test_threading.cpp
@@ -88,6 +88,15 @@ void Mutex::lock()
#endif
}

bool Mutex::try_lock()
{
#if USE_WIN_MUTEX
return TryEnterCriticalSection(&mutex) != 0;
#else
return pthread_mutex_trylock(&mutex) == 0;
#endif
}

void Mutex::unlock()
{
#if USE_WIN_MUTEX
@@ -56,6 +56,8 @@ class Mutex
void lock();
void unlock();

bool try_lock();

protected:
Mutex(bool recursive);
void init_mutex(bool recursive);
@@ -101,6 +101,11 @@ Thread::Thread(const std::string &name) :
Thread::~Thread()
{
kill();

// Make sure start finished mutex is unlocked before it's destroyed
m_start_finished_mutex.try_lock();
m_start_finished_mutex.unlock();

}


@@ -113,6 +118,9 @@ bool Thread::start()

m_request_stop = false;

// The mutex may already be locked if the thread is being restarted
m_start_finished_mutex.try_lock();

#if USE_CPP11_THREADS

try {
@@ -135,6 +143,9 @@ bool Thread::start()

#endif

// Allow spawned thread to continue
m_start_finished_mutex.unlock();

while (!m_running)
sleep_ms(1);

@@ -249,6 +260,10 @@ DWORD WINAPI Thread::threadProc(LPVOID param)
g_logger.registerThread(thr->m_name);
thr->m_running = true;

// Wait for the thread that started this one to finish initializing the
// thread handle so that getThreadId/getThreadHandle will work.
thr->m_start_finished_mutex.lock();

thr->m_retval = thr->run();

thr->m_running = false;
@@ -153,6 +153,7 @@ class Thread {
Atomic<bool> m_request_stop;
Atomic<bool> m_running;
Mutex m_mutex;
Mutex m_start_finished_mutex;

#if USE_CPP11_THREADS
std::thread *m_thread_obj;
@@ -39,9 +39,7 @@ static TestThreading g_test_instance;

void TestThreading::runTests(IGameDef *gamedef)
{
#if !(defined(__MACH__) && defined(__APPLE__))
TEST(testStartStopWait);
#endif
TEST(testThreadKill);
TEST(testAtomicSemaphoreThread);
}

0 comments on commit c93f7f5

Please sign in to comment.
You can’t perform that action at this time.