Fix Lua scripting synchronization

For several years now, the lua script lock has been completely broken. This commit fixes the main issue (creation of a temporary rather than scoped object), and fixes a subsequent deadlock issue caused by nested script API calls by adding support for recursive mutexes.
minetest · Nov 1, 2015 · 52e5b51 · 52e5b51
1 parent d198e42
commit 52e5b51
Show file tree

Hide file tree

Showing 5 changed files with 57 additions and 17 deletions.
diff --git a/src/script/cpp_api/s_base.cpp b/src/script/cpp_api/s_base.cpp
@@ -67,10 +67,11 @@ class ModNameStorer
 	ScriptApiBase
 */
 
-ScriptApiBase::ScriptApiBase()
+ScriptApiBase::ScriptApiBase() :
+	m_luastackmutex(true)
 {
 #ifdef SCRIPTAPI_LOCK_DEBUG
-	m_locked = false;
+	m_lock_recursion_count = 0;
 #endif
 
 	m_luastack = luaL_newstate();
@@ -157,9 +158,14 @@ void ScriptApiBase::loadScript(const std::string &script_path)
 // - runs the callbacks
 // - replaces the table and arguments with the return value,
 //     computed depending on mode
+// This function must only be called with scriptlock held (i.e. inside of a
+// code block with SCRIPTAPI_PRECHECKHEADER declared)
 void ScriptApiBase::runCallbacksRaw(int nargs,
 		RunCallbacksMode mode, const char *fxn)
 {
+#ifdef SCRIPTAPI_LOCK_DEBUG
+	assert(m_lock_recursion_count > 0);
+#endif
 	lua_State *L = getStack();
 	FATAL_ERROR_IF(lua_gettop(L) < nargs + 1, "Not enough arguments");
 

diff --git a/src/script/cpp_api/s_base.h b/src/script/cpp_api/s_base.h
@@ -28,6 +28,7 @@ extern "C" {
 }
 
 #include "irrlichttypes.h"
+#include "threads.h"
 #include "threading/mutex.h"
 #include "threading/mutex_auto_lock.h"
 #include "common/c_types.h"
@@ -111,7 +112,8 @@ class ScriptApiBase {
 	std::string     m_last_run_mod;
 	bool            m_secure;
 #ifdef SCRIPTAPI_LOCK_DEBUG
-	bool            m_locked;
+	int             m_lock_recursion_count;
+	threadid_t      m_owning_thread;
 #endif
 
 private:

diff --git a/src/script/cpp_api/s_internal.h b/src/script/cpp_api/s_internal.h
@@ -32,28 +32,50 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 
 #ifdef SCRIPTAPI_LOCK_DEBUG
 #include "debug.h" // assert()
+
 class LockChecker {
 public:
-	LockChecker(bool *variable) {
-		assert(*variable == false);
+	LockChecker(int *recursion_counter, threadid_t *owning_thread)
+	{
+		m_lock_recursion_counter = recursion_counter;
+		m_owning_thread          = owning_thread;
+		m_original_level         = *recursion_counter;
+
+		if (*m_lock_recursion_counter > 0)
+			assert(thr_is_current_thread(*m_owning_thread));
+		else
+			*m_owning_thread = thr_get_current_thread_id();
 
-		m_variable = variable;
-		*m_variable = true;
+		(*m_lock_recursion_counter)++;
 	}
-	~LockChecker() {
-		*m_variable = false;
+
+	~LockChecker()
+	{
+		assert(thr_is_current_thread(*m_owning_thread));
+		assert(*m_lock_recursion_counter > 0);
+
+		(*m_lock_recursion_counter)--;
+
+		assert(*m_lock_recursion_counter == m_original_level);
 	}
+
 private:
-	bool *m_variable;
+	int *m_lock_recursion_counter;
+	int m_original_level;
+	threadid_t *m_owning_thread;
 };
 
-#define SCRIPTAPI_LOCK_CHECK LockChecker(&(this->m_locked))
+#define SCRIPTAPI_LOCK_CHECK           \
+	LockChecker scriptlock_checker(    \
+		&this->m_lock_recursion_count, \
+		&this->m_owning_thread)
+
 #else
-#define SCRIPTAPI_LOCK_CHECK while(0)
+	#define SCRIPTAPI_LOCK_CHECK while(0)
 #endif
 
 #define SCRIPTAPI_PRECHECKHEADER                                               \
-		MutexAutoLock(this->m_luastackmutex);                                 \
+		MutexAutoLock scriptlock(this->m_luastackmutex);                       \
 		SCRIPTAPI_LOCK_CHECK;                                                  \
 		realityCheck();                                                        \
 		lua_State *L = getStack();                                             \

diff --git a/src/threading/mutex.cpp b/src/threading/mutex.cpp
@@ -34,15 +34,25 @@ DEALINGS IN THE SOFTWARE.
 
 #define UNUSED(expr) do { (void)(expr); } while (0)
 
-
-Mutex::Mutex()
+Mutex::Mutex(bool recursive)
 {
 #ifdef _WIN32
+	// Windows critical sections are recursive by default
+	UNUSED(recursive);
+
 	InitializeCriticalSection(&mutex);
 #else
-	int ret = pthread_mutex_init(&mutex, NULL);
+	pthread_mutexattr_t attr;
+	pthread_mutexattr_init(&attr);
+
+	if (recursive)
+		pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE);
+
+	int ret = pthread_mutex_init(&mutex, &attr);
 	assert(!ret);
 	UNUSED(ret);
+
+	pthread_mutexattr_destroy(&attr);
 #endif
 }
 

diff --git a/src/threading/mutex.h b/src/threading/mutex.h
@@ -49,7 +49,7 @@ DEALINGS IN THE SOFTWARE.
 class Mutex
 {
 public:
-	Mutex();
+	Mutex(bool recursive=false);
 	~Mutex();
 	void lock();
 	void unlock();