From 3501693188b3d9376953b61603d47311a1cff1cb Mon Sep 17 00:00:00 2001 From: Perttu Ahola Date: Mon, 27 Oct 2014 16:55:08 +0200 Subject: [PATCH] server/state: Check that modules are accessed according to dependencies to avoid deadlocks --- doc/conventions.txt | 5 +++ doc/todo.txt | 2 - src/interface/server.h | 4 +- src/server/state.cpp | 87 +++++++++++++++++++++++++++++++++++++----- 4 files changed, 85 insertions(+), 13 deletions(-) diff --git a/doc/conventions.txt b/doc/conventions.txt index c046e88..1a8ea89 100644 --- a/doc/conventions.txt +++ b/doc/conventions.txt @@ -37,6 +37,11 @@ Function naming: Do not use assert(); throw anonymous exceptions instead: - if(fail) throw Exception("Thing failed"); +In normal operation, zero exceptions should be thrown. Unless any potential bugs +are occurring, you should be able to play the game fine if you set a breakpoint +on a GCC Linux build to __cxa_throw, which will stop the program immediately +when an exception occurs. + Naming: - "type": Numeric id representing a type - "name": A string; generally works as an identifier but not necessarily diff --git a/doc/todo.txt b/doc/todo.txt index cecb339..8fb5585 100644 --- a/doc/todo.txt +++ b/doc/todo.txt @@ -68,7 +68,5 @@ Buildat TODO - Make some kind of an ace of spades clone (or less of a clone); something that requires the game to control the creation and deletion of the world anyway, and implement the required interfaces -- Check that modules are accessed in order of dependency to effectively maintain - recursive locking - Fix everything in /games/ - Use builtin/main_context diff --git a/src/interface/server.h b/src/interface/server.h index 46d9374..27e54e6 100644 --- a/src/interface/server.h +++ b/src/interface/server.h @@ -65,8 +65,10 @@ namespace interface virtual ss_ get_module_path(const ss_ &module_name) = 0; virtual bool has_module(const ss_ &module_name) = 0; virtual sv_ get_loaded_modules() = 0; - virtual bool access_module(const ss_ &module_name, + virtual bool access_module(const ss_ &module_name, // Always returns true std::function cb) = 0; + /*virtual bool access_module_optional(const ss_ &module_name, TODO + std::function cb) = 0;*/ virtual void sub_event(struct Module *module, const Event::Type &type) = 0; virtual void emit_event(Event event) = 0; diff --git a/src/server/state.cpp b/src/server/state.cpp index 1c0d895..b5266f4 100644 --- a/src/server/state.cpp +++ b/src/server/state.cpp @@ -50,6 +50,7 @@ struct ModuleThread: public interface::ThreadedThing struct ModuleContainer { + interface::Server *server; interface::ThreadLocalKey *thread_local_key; // Stores mc* up_ module; interface::ModuleInfo info; @@ -69,9 +70,11 @@ struct ModuleContainer // post() when direct_cb becomes free, wait() for that to happen interface::Semaphore direct_cb_free_sem; - ModuleContainer(interface::ThreadLocalKey *thread_local_key = NULL, + ModuleContainer(interface::Server *server = nullptr, + interface::ThreadLocalKey *thread_local_key = NULL, interface::Module *module = NULL, const interface::ModuleInfo &info = interface::ModuleInfo()): + server(server), thread_local_key(thread_local_key), module(module), info(info) @@ -196,6 +199,7 @@ void ModuleThread::run(interface::Thread *thread) cs(mc->info.name)); } catch(std::exception &e){ log_w(MODULE, "direct_cb() failed: %s", e.what()); + mc->server->shutdown(1, mc->info.name+" failed: "+e.what()); } } { @@ -215,6 +219,7 @@ void ModuleThread::run(interface::Thread *thread) mc->module->event(event.type, event.p.get()); } catch(std::exception &e){ log_w(MODULE, "module->event() failed: %s", e.what()); + mc->server->shutdown(1, mc->info.name+" failed: "+e.what()); } } } @@ -495,7 +500,7 @@ struct CState: public State, public interface::Server m_module_info[info.name] = info; mc = sp_(new ModuleContainer( - &m_thread_local_mc_key, m, info)); + this, &m_thread_local_mc_key, m, info)); m_modules[info.name] = mc; m_module_load_order.push_back(info.name); } @@ -531,7 +536,7 @@ struct CState: public State, public interface::Server } } mc = sp_(new ModuleContainer( - &m_thread_local_mc_key, m, info)); + this, &m_thread_local_mc_key, m, info)); m_modules[info.name] = mc; m_module_load_order.push_back(info.name); } @@ -707,18 +712,73 @@ struct CState: public State, public interface::Server return result; } + // Call with m_modules_mutex locked + bool is_dependency_u(ModuleContainer *mc_should_be_dependent, + ModuleContainer *mc_should_be_dependency) + { + const ss_ &search_dep_name = mc_should_be_dependency->info.name; + const interface::ModuleInfo &info = mc_should_be_dependent->info; + // Breadth-first + for(const interface::ModuleDependency &dep : info.meta.dependencies){ + /*log_t(MODULE, "is_dependency_u(): \"%s\" has dependency \"%s\"; " + "searching for \"%s\"", + cs(info.name), cs(dep.module), cs(search_dep_name));*/ + if(dep.module == search_dep_name) + return true; + } + for(const interface::ModuleDependency &dep : info.meta.dependencies){ + auto it = m_modules.find(dep.module); + if(it == m_modules.end()) + continue; + ModuleContainer *mc_dependency = it->second.get(); + bool is = is_dependency_u(mc_dependency, mc_should_be_dependency); + if(is) + return true; + } + /*log_t(MODULE, "is_dependency_u(): \"%s\" does not depend on \"%s\"", + cs(info.name), cs(search_dep_name));*/ + return false; + } + + // Throws on invalid access + // Call with m_modules_mutex locked + void check_valid_access_u( + ModuleContainer *target_mc, + ModuleContainer *caller_mc + ){ + const ss_ &target_name = target_mc->info.name; + const ss_ &caller_name = caller_mc->info.name; + + // Access is invalid if target is caller + if(caller_mc == target_mc) + throw Exception("Cannot access \""+target_name+"\" from \""+ + caller_name+"\": Accessing itself is disallowed"); + + // Access is invalid if target is being accessed by somebody who is + // being accessed by caller, directly or indirectly + if(!is_dependency_u(caller_mc, target_mc)) + throw Exception("Cannot access \""+target_name+"\" from \""+ + caller_name+"\": Not a dependency"); + + // Access is valid + } + bool access_module(const ss_ &module_name, std::function cb) { sp_ mc; + ss_ caller_module_name; { - // This prevents module from being deleted while a reference is - // being copied interface::MutexScope ms(m_modules_mutex); + auto it = m_modules.find(module_name); if(it == m_modules.end()) - return false; + throw Exception("access_module(): Module \""+module_name+ + "\" not found"); mc = it->second; + if(!mc) + throw Exception("access_module(): Module \""+module_name+ + "\" container is null"); // Get container of current module ModuleContainer *caller_mc = @@ -726,15 +786,22 @@ struct CState: public State, public interface::Server if(caller_mc){ log_t(MODULE, "access_module(\"%s\"): Called by \"%s\"", cs(mc->info.name), cs(caller_mc->info.name)); + + // Throws exception if not valid. + // If accessing a module from a nested access_module(), this + // function is called from the thread of the nested module, + // effectively taking into account the lock hierarchy. + check_valid_access_u(mc.get(), caller_mc); + + // Access OK; copy the caller's name so we can use it + caller_module_name = caller_mc->info.name; } else { log_t(MODULE, "access_module(\"%s\"): Called by something else" " than a module", cs(mc->info.name)); } - - // TODO: Check that the module being accessed is a direct or - // indirect dependency of the module accessing it, and not the - // module itself } + + // Execute callback in module thread mc->execute_direct_cb(cb); return true; }