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.

diff --git a/src/threading/mutex.cpp b/src/threading/mutex.cpp
index 0908b5d3797f2219e1d8e8f09a7d8071edda7b8d..d864f12c5d3bc3539e6cffd58eaad654bceb3975 100644 (file)
--- a/src/threading/mutex.cpp
+++ b/src/threading/mutex.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

diff --git a/src/threading/mutex.h b/src/threading/mutex.h
index fb5c029fc0cd9a432a68c61944863c1e8322071d..6feb23c25d90ad89209292e2acb013165b39f680 100644 (file)
--- a/src/threading/mutex.h
+++ b/src/threading/mutex.h
@@ -56,6 +56,8 @@ public:
        void lock();
        void unlock();
 
+       bool try_lock();
+
 protected:
        Mutex(bool recursive);
        void init_mutex(bool recursive);

diff --git a/src/threading/thread.cpp b/src/threading/thread.cpp
index fbe4ba1f395a07214488c23ad0709057afa8dad0..4f4c9474afe0f1a5ddb90330960defd63bd68979 100644 (file)
--- a/src/threading/thread.cpp
+++ b/src/threading/thread.cpp
@@ -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;

diff --git a/src/threading/thread.h b/src/threading/thread.h
index 14a0e13ab66f14e88ab3ac72c3144bfeb1984053..4785d3e039477cbe29109e533443c35c5761ceb5 100644 (file)
--- a/src/threading/thread.h
+++ b/src/threading/thread.h
@@ -153,6 +153,7 @@ private:
        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;

diff --git a/src/unittest/test_threading.cpp b/src/unittest/test_threading.cpp
index cdbf9674e39c1aee1b6ad02652df781a2a989c1e..e1e1d366014c1e45654a03d42509b7d70ef4ee98 100644 (file)
--- a/src/unittest/test_threading.cpp
+++ b/src/unittest/test_threading.cpp
@@ -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);
 }