Reference vtable hooks are lost after a map change when Ham Sandwich unloads its hooks

[isdpteam]

Description

On current master (ec67f02d3353d6bf5a41eedcec1033aa7af308d6),
WeaponMod reference vtable hooks can be displaced during a map change when Ham
Sandwich hooks the same reference class.

On the first map, a custom weapon has the correct first-person model and works
normally. After changelevel, the module and weapon plugins are still reported
as running and the custom weapon can retain other behavior, but its v_model
is missing/incorrect.

There are no WeaponMod or AMXX errors when this happens.

This is related to, but different from, #11. Commit
aca9b17db5c10ca7691ea80a5715fca7fd5ad50e fixed the lifetime of the
reference_weapon / reference_ammobox strings returned by
IGameConfig::GetKeyValue(). The issue described here is the lifetime and
ownership of the reference entity vtable hooks themselves.

Environment used to reproduce

• Linux i386 HLDS 48/1.1.2.2/Stdio, build 4419
• ReHLDS API v3.15
• AMX Mod X / Ham Sandwich 1.10.0.5479
• BugfixedHL-Rebased / BugfixedAPI 1.13.2
• WeaponMod current master ec67f02d3353d6bf5a41eedcec1033aa7af308d6
• Custom WeaponMod weapon plugins loaded

Reproduction

1. Start HLDS with WeaponMod, Ham Sandwich, and at least one custom WeaponMod
   weapon.
2. Equip the custom weapon on the first map and verify that its first-person
   model is correct.
3. Run changelevel <map> (the same or another map).
4. Equip the custom weapon again.

Observed

The first-person model is missing/incorrect after the map change while
WeaponMod, Ham Sandwich, and the weapon plugins remain loaded.

Expected

Reference hooks and all custom weapon behavior, including Deploy and the
first-person model, should continue to work after every map change.

Root cause

WeaponMod installs g_CrowbarHooks and g_AmmoBoxRefHooks only once, guarded
by m_bWeaponRefHooked and m_bAmmoBoxRefHooked. Its existing
CItems::ServerDeactivate() clears per-map item data but does not release these
reference hooks or reset the flags.

Ham Sandwich's Hook constructor saves the current vtable entry in
Hook::func and installs a trampoline. During AMXX
OnPluginsUnloaded() on map teardown, Hook::~Hook() unconditionally writes
the saved pointer back to the slot. This can overwrite WeaponMod's handler.

Because WeaponMod still considers the reference arrays hooked, normal weapon
registration on the next map does not install them again.

Temporary instrumentation showed only these two weapon_crowbar entries being
displaced in this setup:

AddToPlayer, offset 59
Deploy,      offset 63

The offsets above are diagnostic observations only. The fix does not hardcode
them and continues to use the existing gamedata offset lookup.

Why blindly rehooking from OnPluginsLoaded() is unsafe

An initial diagnostic workaround checked the top slot in OnPluginsLoaded()
and reinstalled WeaponMod when it was different. It fixed the reproduced client
behavior, but it is not safe as a final solution.

If Ham already wraps WeaponMod, replacing WeaponMod's saved underlying address
with Ham's current trampoline creates this cycle:

initial:        W -> original
Ham installs:  H -> W -> original
blind rehook:  W -> H -> W

The saved Ham trampoline can also become dangling when Ham frees it on the next
plugin unload.

Proposed fix

Release and reset WeaponMod's reference hooks at the end of map teardown, after
AMXX/Ham has removed its per-map hooks. Then allow the existing weapon and
ammobox registration paths to build a fresh chain on the next map.

The patch does the following:

1. Stores the exact vtable slot used by each VirtualHookData during the normal
   initial installation.

2. Adds an ownership-aware release operation:

   void ReleaseHookVirtual(VirtualHookData* hook)
   {
       if (hook == NULL || !hook->done)
           return;
   
       if (hook->slot != NULL && *hook->slot == hook->handler)
           *hook->slot = hook->address;
   
       hook->address = NULL;
       hook->slot = NULL;
       hook->done = false;
   }

3. Calls that operation for both reference arrays from WeaponMod's
   ServerDeactivate_Post().

4. Resets m_bWeaponRefHooked and m_bAmmoBoxRefHooked so normal next-map
   registration installs fresh hooks.

5. Does not overwrite a slot when WeaponMod no longer owns the top of the
   chain.

WeaponMod is attached as a Metamod plugin by the already-loaded AMXX core, so
AMXX's ServerDeactivate_Post() (including Ham's OnPluginsUnloaded() cleanup)
runs before WeaponMod's own post callback in this lifecycle.

This permits either valid next-map ordering without recursion:

Ham first:        W -> H -> original
WeaponMod first:  H -> W -> original

The change has no per-frame work, no forced pev_viewmodel, no hardcoded
offsets, and no changes to custom weapon behavior.

Patch scope

The full patch below is relative to current master commit
ec67f02d3353d6bf5a41eedcec1033aa7af308d6:

8 files changed, 47 insertions(+), 4 deletions(-)

Files:

src/sdk/moduleconfig.h
src/wpnmod_hooks.h
src/wpnmod_items.cpp
src/wpnmod_items.h
src/wpnmod_main.cpp
src/wpnmod_parse.cpp
src/wpnmod_vtable.cpp
src/wpnmod_vtable.h

Patch SHA-256:

fca46fdb907272508d8867446d482c6cfd8a6c1942e8af18a928f56674c0ec80

Validation

• git apply --check: passed against the exact base commit
• git diff --check: passed
• Linux i386 RelWithDebInfo build with WARNINGS_ARE_ERRORS=ON: passed with
  no warnings/errors
• Module loaded successfully with all Metamod plugins and AMXX modules running
• 15 consecutive map changes across boot_camp, crossfire, and stalkyard:
  same HLDS PID, zero automatic restarts, zero new core files, and zero new
  WeaponMod/AMXX errors
• The diagnostic rehook build was client A/B tested and prevented the observed
  post-map-change v_model failure, confirming that displaced reference hooks
  are involved

The final ownership-aware patch has completed server-side lifecycle/stability
testing. A final client-side visual check of this exact patch is still pending;
I am not treating the server-only test as proof of rendered v_model behavior.

Full upstream patch

diff --git a/src/sdk/moduleconfig.h b/src/sdk/moduleconfig.h
index 79cc4d3..40f2c2a 100644
--- a/src/sdk/moduleconfig.h
+++ b/src/sdk/moduleconfig.h
@@ -201,7 +201,7 @@
 // #define FN_ClientCommand_Post				ClientCommand_Post
 // #define FN_ClientUserInfoChanged_Post			ClientUserInfoChanged_Post
 #define FN_ServerActivate_Post					ServerActivate_Post
-// #define FN_ServerDeactivate_Post				ServerDeactivate_Post
+#define FN_ServerDeactivate_Post				ServerDeactivate_Post
 // #define FN_PlayerPreThink_Post					PlayerPreThink_Post
 // #define FN_PlayerPostThink_Post				PlayerPostThink_Post
 // #define FN_StartFrame_Post					StartFrame_Post
diff --git a/src/wpnmod_hooks.h b/src/wpnmod_hooks.h
index a70e56c..9ff2e75 100644
--- a/src/wpnmod_hooks.h
+++ b/src/wpnmod_hooks.h
@@ -165,17 +165,17 @@ void Hooks_InitReferenceEntities();
 
 	#define VHOOK(classname, offset, call)				\
 	{													\
-		classname, offset, (void*)call, NULL, NULL,		\
+		classname, offset, (void*)call, NULL, NULL, NULL,	\
 	}													\
 
 	#define VHOOK_WEAPON_REF(call)										\
 	{																	\
-		nullptr, VO_##call, (void*)Weapon_##call, NULL, NULL,			\
+		nullptr, VO_##call, (void*)Weapon_##call, NULL, NULL, NULL,	\
 	}																	\
 
 	#define VHOOK_AMMOBOX_REF(call)											\
 	{																		\
-		nullptr, VO_##call, (void*)AmmoBox_##call, NULL, NULL,				\
+		nullptr, VO_##call, (void*)AmmoBox_##call, NULL, NULL, NULL,		\
 	}																		\
 
 	enum AmmoBoxRefHooks
diff --git a/src/wpnmod_items.cpp b/src/wpnmod_items.cpp
index c204fc1..bf6df1a 100644
--- a/src/wpnmod_items.cpp
+++ b/src/wpnmod_items.cpp
@@ -173,6 +173,25 @@ void CItems::ServerDeactivate(void)
 	}
 }
 
+void CItems::ReleaseReferenceHooks(void)
+{
+	if (m_bWeaponRefHooked)
+	{
+		for (int k = 0; k < WeaponRefHook_End; k++)
+			ReleaseHookVirtual(&g_CrowbarHooks[k]);
+
+		m_bWeaponRefHooked = false;
+	}
+
+	if (m_bAmmoBoxRefHooked)
+	{
+		for (int k = 0; k < AmmoBoxRefHook_End; k++)
+			ReleaseHookVirtual(&g_AmmoBoxRefHooks[k]);
+
+		m_bAmmoBoxRefHooked = false;
+	}
+}
+
 int CItems::Ammobox_Register(const char *name)
 {
 	CAmmoBoxInfo *p = new CAmmoBoxInfo;
diff --git a/src/wpnmod_items.h b/src/wpnmod_items.h
index 871fc72..4210ab9 100644
--- a/src/wpnmod_items.h
+++ b/src/wpnmod_items.h
@@ -184,6 +184,7 @@ public:
 	void FreeWeaponSlots			(void);
 	bool CheckSlots					(int iWeaponID);
 	void ServerDeactivate			(void);
+	void ReleaseReferenceHooks		(void);
 
 	bool m_bWeaponRefHooked;
 	bool m_bAmmoBoxRefHooked;
diff --git a/src/wpnmod_main.cpp b/src/wpnmod_main.cpp
index 1c0b1a6..be9ff39 100644
--- a/src/wpnmod_main.cpp
+++ b/src/wpnmod_main.cpp
@@ -119,6 +119,12 @@ void ServerDeactivate()
 	RETURN_META(MRES_IGNORED);
 }
 
+void ServerDeactivate_Post()
+{
+	g_Items.ReleaseReferenceHooks();
+	RETURN_META(MRES_IGNORED);
+}
+
 void OnAmxxDetach(void)
 {
 	g_Memory.UnsetHooks();
diff --git a/src/wpnmod_parse.cpp b/src/wpnmod_parse.cpp
index 271eec4..e21e75f 100644
--- a/src/wpnmod_parse.cpp
+++ b/src/wpnmod_parse.cpp
@@ -123,6 +123,7 @@ void OnParseBlockedItems(std::string dummy, std::string BlockedItem)
 
 	p->done = false;
 	p->handler = NULL;
+	p->slot = NULL;
 	p->address = NULL;
 	p->classname = STRING(ALLOC_STRING(BlockedItem.c_str()));
 
diff --git a/src/wpnmod_vtable.cpp b/src/wpnmod_vtable.cpp
index 85e8633..fc370c4 100644
--- a/src/wpnmod_vtable.cpp
+++ b/src/wpnmod_vtable.cpp
@@ -175,6 +175,19 @@ void UnsetHookVirtual(VirtualHookData* hook)
 	}
 }
 
+void ReleaseHookVirtual(VirtualHookData* hook)
+{
+	if (hook == NULL || !hook->done)
+		return;
+
+	if (hook->slot != NULL && *hook->slot == hook->handler)
+		*hook->slot = hook->address;
+
+	hook->address = NULL;
+	hook->slot = NULL;
+	hook->done = false;
+}
+
 bool HandleHookVirtual(VirtualHookData* hook, bool bRevert)
 {
 	edict_t* pEdict = CREATE_ENTITY();
@@ -201,6 +214,7 @@ bool HandleHookVirtual(VirtualHookData* hook, bool bRevert)
 	if (!bRevert)
 	{
 		hook->address = (void*)ivtable[offset];
+		hook->slot = (void**)&ivtable[offset];
 	}
 
 	#ifdef __linux__
diff --git a/src/wpnmod_vtable.h b/src/wpnmod_vtable.h
index bf663b0..b4af83e 100644
--- a/src/wpnmod_vtable.h
+++ b/src/wpnmod_vtable.h
@@ -90,6 +90,7 @@ struct VirtualHookData
 	int		offset;
 	void*	handler;
 	void*	address;
+	void**	slot;
 	bool		done;
 };
 
@@ -105,6 +106,7 @@ extern void SetVTableOffsetBase	(int iOffset);
 
 extern void SetHookVirtual		(VirtualHookData* hook);
 extern void UnsetHookVirtual	(VirtualHookData* hook);
+extern void ReleaseHookVirtual	(VirtualHookData* hook);
 extern bool HandleHookVirtual	(VirtualHookData* hook, bool revert);
 
 inline int GET_VTABLE_OFFSET(int x)