amxmodx icon indicating copy to clipboard operation
amxmodx copied to clipboard

Fix RequestFrame

Open DarthMan opened this issue 2 years ago • 6 comments

Hello. After some investigations regarding server crashes I discovered that the RequestFrame handles are not freed on map change. For example, if I use it inside the client_disconnected public forward it will cause memory leaks if the forward is called after pfnChangeLevel. It seems that the reason behind that is the fact the RequestFrame forwards are not freed at mapchange, like they are for most of the functions that return handles, such as register_forward. Manual deletion is also not possible, as the native doesn't return the handle that was created. Could this please be fixed in AMXX 1.10 ?

Thanks in advance! :)

DarthMan avatar Mar 10 '22 17:03 DarthMan

The root of the issue here seems to be that CSPForward stores a AMX * to identify the plugin to call the forward on, but that pointer is bound to the lifetime of the plugin - so if a plugin is unloaded then any CSPForward instances that still exist for it have a high chance of crashing when being executed.

asherkin avatar Apr 08 '22 16:04 asherkin

i think that the issue is due to CFrameActionMngr not being cleared when everything is (and should be) cleared, manually adding a clear method seems to fix this:

diff --git a/amxmodx/CFrameAction.h b/amxmodx/CFrameAction.h
index ee368b6..3dceb7b 100644
--- a/amxmodx/CFrameAction.h
+++ b/amxmodx/CFrameAction.h
@@ -53,6 +53,13 @@ public:
 		}
 	}
 
+	void clear()
+	{
+		// Can't call deque's clear, so i just copied ExecuteFrameCallbacks
+		int callbacksToRun = m_requestedFrames.length();
+		while (callbacksToRun--) m_requestedFrames.popFront();
+	}
+
 private:
 	ke::Deque<ke::AutoPtr<CFrameAction>> m_requestedFrames;
 
diff --git a/amxmodx/meta_api.cpp b/amxmodx/meta_api.cpp
index 81560a9..cd31a1e 100755
--- a/amxmodx/meta_api.cpp
+++ b/amxmodx/meta_api.cpp
@@ -393,6 +393,7 @@ int	C_Spawn(edict_t *pent)
 
 	}
 
+	g_frameActionMngr.clear();
 	g_forwards.clear();
 
 	g_log.MapChange();
@@ -771,6 +772,7 @@ void C_ServerDeactivate_Post()
 	g_forcegeneric.clear();
 	g_grenades.clear();
 	g_tasksMngr.clear();
+	g_frameActionMngr.clear();
 	g_forwards.clear();
 	g_logevents.clearLogEvents();
 	g_events.clearEvents();
@@ -1731,6 +1733,7 @@ C_DLLEXPORT	int	Meta_Detach(PLUG_LOADTIME now, PL_UNLOAD_REASON	reason)
 	modules_callPluginsUnloading();
 
 	g_auth.clear();
+	g_frameActionMngr.clear();
 	g_forwards.clear();
 	g_commands.clear();
 	g_forcemodels.clear();

i want to make a PR with this fix, but i still have doubts if i can call deque's clear method directly from there, as is not on am-deque.h

Th3-822 avatar Mar 11 '23 03:03 Th3-822

up?

wilianmaique avatar Nov 07 '23 14:11 wilianmaique

Up?

BRUN0R2 avatar Nov 09 '23 11:11 BRUN0R2

@Th3-822 calling m_requestedFrames.clear() should be fine. Either way, your fix looks good.

dvander avatar Dec 01 '23 06:12 dvander