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.
This commit is contained in:
kwolekr 2015-10-31 16:31:43 -04:00
parent d198e420ec
commit 52e5b513ed
5 changed files with 57 additions and 17 deletions

View file

@ -67,10 +67,11 @@ class ModNameStorer
ScriptApiBase ScriptApiBase
*/ */
ScriptApiBase::ScriptApiBase() ScriptApiBase::ScriptApiBase() :
m_luastackmutex(true)
{ {
#ifdef SCRIPTAPI_LOCK_DEBUG #ifdef SCRIPTAPI_LOCK_DEBUG
m_locked = false; m_lock_recursion_count = 0;
#endif #endif
m_luastack = luaL_newstate(); m_luastack = luaL_newstate();
@ -157,9 +158,14 @@ void ScriptApiBase::loadScript(const std::string &script_path)
// - runs the callbacks // - runs the callbacks
// - replaces the table and arguments with the return value, // - replaces the table and arguments with the return value,
// computed depending on mode // 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, void ScriptApiBase::runCallbacksRaw(int nargs,
RunCallbacksMode mode, const char *fxn) RunCallbacksMode mode, const char *fxn)
{ {
#ifdef SCRIPTAPI_LOCK_DEBUG
assert(m_lock_recursion_count > 0);
#endif
lua_State *L = getStack(); lua_State *L = getStack();
FATAL_ERROR_IF(lua_gettop(L) < nargs + 1, "Not enough arguments"); FATAL_ERROR_IF(lua_gettop(L) < nargs + 1, "Not enough arguments");

View file

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

View file

@ -32,28 +32,50 @@ with this program; if not, write to the Free Software Foundation, Inc.,
#ifdef SCRIPTAPI_LOCK_DEBUG #ifdef SCRIPTAPI_LOCK_DEBUG
#include "debug.h" // assert() #include "debug.h" // assert()
class LockChecker { class LockChecker {
public: public:
LockChecker(bool *variable) { LockChecker(int *recursion_counter, threadid_t *owning_thread)
assert(*variable == false); {
m_lock_recursion_counter = recursion_counter;
m_owning_thread = owning_thread;
m_original_level = *recursion_counter;
m_variable = variable; if (*m_lock_recursion_counter > 0)
*m_variable = true; assert(thr_is_current_thread(*m_owning_thread));
else
*m_owning_thread = thr_get_current_thread_id();
(*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: 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 #else
#define SCRIPTAPI_LOCK_CHECK while(0) #define SCRIPTAPI_LOCK_CHECK while(0)
#endif #endif
#define SCRIPTAPI_PRECHECKHEADER \ #define SCRIPTAPI_PRECHECKHEADER \
MutexAutoLock(this->m_luastackmutex); \ MutexAutoLock scriptlock(this->m_luastackmutex); \
SCRIPTAPI_LOCK_CHECK; \ SCRIPTAPI_LOCK_CHECK; \
realityCheck(); \ realityCheck(); \
lua_State *L = getStack(); \ lua_State *L = getStack(); \

View file

@ -34,15 +34,25 @@ DEALINGS IN THE SOFTWARE.
#define UNUSED(expr) do { (void)(expr); } while (0) #define UNUSED(expr) do { (void)(expr); } while (0)
Mutex::Mutex(bool recursive)
Mutex::Mutex()
{ {
#ifdef _WIN32 #ifdef _WIN32
// Windows critical sections are recursive by default
UNUSED(recursive);
InitializeCriticalSection(&mutex); InitializeCriticalSection(&mutex);
#else #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); assert(!ret);
UNUSED(ret); UNUSED(ret);
pthread_mutexattr_destroy(&attr);
#endif #endif
} }

View file

@ -49,7 +49,7 @@ DEALINGS IN THE SOFTWARE.
class Mutex class Mutex
{ {
public: public:
Mutex(); Mutex(bool recursive=false);
~Mutex(); ~Mutex();
void lock(); void lock();
void unlock(); void unlock();