From 463128270953d03a4d1389ff8c976b042f31a14b Mon Sep 17 00:00:00 2001 From: David Anderson Date: Sun, 10 Jan 2010 16:58:17 -0800 Subject: [PATCH] Fixed reentrancy problems with iterators, forwards, and function removals (bug 4059, r=fyren). --- core/ConVarManager.cpp | 39 ++++++++++++++++++++++++++++++- core/ForwardSys.cpp | 17 ++++++++------ core/ForwardSys.h | 45 ++++++++++++++++++++++++++++++++++++ plugins/testsuite/bug4059.sp | 31 +++++++++++++++++++++++++ 4 files changed, 124 insertions(+), 8 deletions(-) create mode 100644 plugins/testsuite/bug4059.sp diff --git a/core/ConVarManager.cpp b/core/ConVarManager.cpp index 04aa6416..a0d30c70 100644 --- a/core/ConVarManager.cpp +++ b/core/ConVarManager.cpp @@ -58,6 +58,40 @@ const ParamType CONVARCHANGE_PARAMS[] = {Param_Cell, Param_String, Param_String} typedef List ConVarList; KTrie convar_cache; +class ConVarReentrancyGuard +{ + ConVar *cvar; + ConVarReentrancyGuard *up; +public: + static ConVarReentrancyGuard *chain; + + ConVarReentrancyGuard(ConVar *cvar) + : cvar(cvar), up(chain) + { + chain = this; + } + + ~ConVarReentrancyGuard() + { + assert(chain == this); + chain = up; + } + + static bool IsCvarInChain(ConVar *cvar) + { + ConVarReentrancyGuard *guard = chain; + while (guard != NULL) + { + if (guard->cvar == cvar) + return true; + guard = guard->up; + } + return false; + } +}; + +ConVarReentrancyGuard *ConVarReentrancyGuard::chain = NULL; + ConVarManager::ConVarManager() : m_ConVarType(0), m_bIsDLLQueryHooked(false), m_bIsVSPQueryHooked(false) { } @@ -536,7 +570,8 @@ void ConVarManager::UnhookConVarChange(ConVar *pConVar, IPluginFunction *pFuncti } /* If the forward now has 0 functions in it... */ - if (pForward->GetFunctionCount() == 0) + if (pForward->GetFunctionCount() == 0 && + !ConVarReentrancyGuard::IsCvarInChain(pConVar)) { /* Free this forward */ g_Forwards.ReleaseForward(pForward); @@ -647,6 +682,8 @@ void ConVarManager::OnConVarChanged(ConVar *pConVar, const char *oldValue) if (pForward != NULL) { + ConVarReentrancyGuard guard(pConVar); + /* Now call forwards in plugins that have hooked this */ pForward->PushCell(pInfo->handle); pForward->PushString(oldValue); diff --git a/core/ForwardSys.cpp b/core/ForwardSys.cpp index 16245222..09b2b9f6 100644 --- a/core/ForwardSys.cpp +++ b/core/ForwardSys.cpp @@ -255,6 +255,7 @@ CForward *CForward::CreateForward(const char *name, ExecType et, unsigned int nu } CForward *pForward = g_Forwards.ForwardMake(); + pForward->m_IterGuard = NULL; pForward->m_curparam = 0; pForward->m_ExecType = et; snprintf(pForward->m_name, FORWARDS_NAME_MAX, "%s", name ? name : ""); @@ -294,7 +295,7 @@ int CForward::Execute(cell_t *result, IForwardFilter *filter) cell_t high_result = 0; cell_t low_result = 0; int err; - unsigned int failed=0, success=0; + unsigned int success=0; unsigned int num_params = m_curparam; FwdParamInfo temp_info[SP_MAX_EXEC_PARAMS]; FwdParamInfo *param; @@ -304,7 +305,9 @@ int CForward::Execute(cell_t *result, IForwardFilter *filter) memcpy(temp_info, m_params, sizeof(m_params)); m_curparam = 0; - for (iter=m_functions.begin(); iter!=m_functions.end(); iter++) + FuncIteratorGuard guard(&m_IterGuard, &iter); + + while (iter != m_functions.end()) { func = (*iter); @@ -362,11 +365,7 @@ int CForward::Execute(cell_t *result, IForwardFilter *filter) } /* Call the function and deal with the return value. */ - if ((err=func->Execute(&cur_result)) != SP_ERROR_NONE) - { - failed++; - } - else + if ((err=func->Execute(&cur_result)) == SP_ERROR_NONE) { success++; switch (m_ExecType) @@ -406,6 +405,9 @@ int CForward::Execute(cell_t *result, IForwardFilter *filter) } } } + + if (!guard.Triggered()) + iter++; } done: @@ -685,6 +687,7 @@ bool CForward::RemoveFunction(IPluginFunction *func) { if ((*iter) == func) { + m_IterGuard->FixIteratorChain(iter); found = true; lst->erase(iter); break; diff --git a/core/ForwardSys.h b/core/ForwardSys.h index f602b780..798e1588 100644 --- a/core/ForwardSys.h +++ b/core/ForwardSys.h @@ -69,6 +69,50 @@ public: } }; +class FuncIteratorGuard +{ + bool triggered; + FuncIteratorGuard **pprev; + FuncIter *iter; + FuncIteratorGuard *next; +public: + FuncIteratorGuard(FuncIteratorGuard **pprev, FuncIter *iter) + : triggered(false), pprev(pprev), iter(iter), next(*pprev) + { + *pprev = this; + } + + ~FuncIteratorGuard() + { + *pprev = next; + } + + inline bool Triggered() + { + bool t = triggered; + triggered = false; + return t; + } + + /** + * This should not read from |this| before the NULL check, because FwdSys + * can call (NULL)->FixIteratorChain(). + */ + void FixIteratorChain(FuncIter &other) + { + FuncIteratorGuard *guard = this; + while (guard != NULL) + { + if (*guard->iter == other) + { + *(guard->iter) = ++(*(guard->iter)); + guard->triggered = true; + } + guard = guard->next; + } + } +}; + class CForward : public IChangeableForward { public: //ICallable @@ -111,6 +155,7 @@ protected: */ mutable List m_functions; mutable List m_paused; + FuncIteratorGuard *m_IterGuard; /* Type and name information */ FwdParamInfo m_params[SP_MAX_EXEC_PARAMS]; diff --git a/plugins/testsuite/bug4059.sp b/plugins/testsuite/bug4059.sp new file mode 100644 index 00000000..4d6e5d45 --- /dev/null +++ b/plugins/testsuite/bug4059.sp @@ -0,0 +1,31 @@ +#include + +public OnPluginStart() +{ + new Handle:hostname = FindConVar("hostname") + HookConVarChange(hostname, OnChange) + HookEvent("player_team", cb) + RegServerCmd("test_bug4059", Test_Bug) +} + +public Action:cb(Handle:event, const String:name[], bool:dontBroadcast) +{ + UnhookEvent(name, cb) + PrintToServer("whee") + HookEvent(name, cb) + return Plugin_Handled +} + +public OnChange(Handle:convar, const String:oldValue[], const String:newValue[]) +{ + PrintToServer("called: %x", convar) + UnhookConVarChange(convar, OnChange) + ResetConVar(convar) + HookConVarChange(convar, OnChange) +} + +public Action:Test_Bug(args) +{ + ServerCommand("hostname \"bug4059\"") +} +