Fixed reentrancy problems with iterators, forwards, and function removals (bug

4059, r=fyren, a13=blocking).

core/ConVarManager.cpp

@@ -58,6 +58,40 @@ const ParamType CONVARCHANGE_PARAMS[] = {Param_Cell, Param_String, Param_String}
 typedef List<const ConVar *> ConVarList;
 KTrie<ConVarInfo *> 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);

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)
+		if ((err=func->Execute(&cur_result)) == SP_ERROR_NONE)
-		{
-			failed++;
-		}
-		else
 		{
 			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;

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<IPluginFunction *> m_functions;
 	mutable List<IPluginFunction *> m_paused;
+	FuncIteratorGuard *m_IterGuard;
 
 	/* Type and name information */
 	FwdParamInfo m_params[SP_MAX_EXEC_PARAMS];

plugins/testsuite/bug4059.sp

@@ -0,0 +1,31 @@
+#include <sourcemod>
+
+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\"")
+}
+