[CODE] Script hook parameter reset

[samisalreadytaken]

Describe the bug

Static initialisation of ScriptHook_t variables happens after ScriptHook_t::AddParameter() is called inside BEGIN_ENT_SCRIPTDESC(), resetting ScriptHook_t::m_desc::m_Parameters which breaks scripts that expect parameters.

This is not an issue with non-entity hooks such as OnEntityCreated.

This can be observed in a debugger. Using PlayerRunCommand as that is the simplest hook with parameters that doesn't have any prerequisites.

(gdb) watch g_Hook_PlayerRunCommand.m_desc.m_Parameters.m_Size
Hardware watchpoint 3: g_Hook_PlayerRunCommand.m_desc.m_Parameters.m_Size
(gdb) c
Continuing.

Thread 1 "hl2_linux" hit Hardware watchpoint 3: g_Hook_PlayerRunCommand.m_desc.m_Parameters.m_Size

Old value = 0
New value = 1
CUtlVector<int, CUtlMemory<int, int> >::GrowVector (this=0xda558190 <g_Hook_PlayerRunCommand+16>, num=1) at /home/runner/work/source-sdk-2013/source-sdk-2013/sp/src/public/tier1/utlvector.h:698
698		ResetDbgInfo();
(gdb) i s
#0  CUtlVector<int, CUtlMemory<int, int> >::GrowVector (this=0xda558190 <g_Hook_PlayerRunCommand+16>, num=1)
    at /home/runner/work/source-sdk-2013/source-sdk-2013/sp/src/public/tier1/utlvector.h:698
#1  0xd9002c0a in CUtlVector<int, CUtlMemory<int, int> >::InsertBefore (
    this=0xda558190 <g_Hook_PlayerRunCommand+16>, elem=0, src=@0xffffafbc: 31)
    at /home/runner/work/source-sdk-2013/source-sdk-2013/sp/src/public/tier1/utlvector.h:858
#2  0xd9044684 in CUtlVector<int, CUtlMemory<int, int> >::AddToTail (this=0xda558190 <g_Hook_PlayerRunCommand+16>, 
    src=@0xffffafbc: 31) at /home/runner/work/source-sdk-2013/source-sdk-2013/sp/src/public/tier1/utlvector.h:837
#3  0xd9a4d438 in ScriptHook_t::AddParameter (type=<optimized out>, pszName=<optimized out>, 
    this=0xda558180 <g_Hook_PlayerRunCommand>)
    at /home/runner/work/source-sdk-2013/source-sdk-2013/sp/src/public/vscript/ivscript.h:1899
#4  GetScriptDesc<CBasePlayer> ()
    at /home/runner/work/source-sdk-2013/source-sdk-2013/sp/src/game/server/player.cpp:560
#5  0xd96df964 in GetScriptDesc<CHL2_Player> ()
    at /home/runner/work/source-sdk-2013/source-sdk-2013/sp/src/game/server/hl2/hl2_player.cpp:613
#6  0xd96e04d3 in __static_initialization_and_destruction_0 (__initialize_p=__initialize_p@entry=1, 
    __priority=__priority@entry=65535)
    at /home/runner/work/source-sdk-2013/source-sdk-2013/sp/src/game/server/hl2/hl2_player.cpp:613
#7  0xd96e0dd1 in _GLOBAL__sub_I_hl2_player.cpp(void) ()
    at /home/runner/work/source-sdk-2013/source-sdk-2013/sp/src/game/server/hl2/hl2_player.cpp:5068
(gdb) c
Continuing.
ConVarRef mat_dxlevel doesnt point to an existing ConVar

Thread 1 "hl2_linux" hit Hardware watchpoint 3: g_Hook_PlayerRunCommand.m_desc.m_Parameters.m_Size

Old value = 1
New value = 0
CUtlVector<int, CUtlMemory<int, int> >::CUtlVector (initSize=0, growSize=0, this=0xda558190 <g_Hook_PlayerRunCommand+16>) at /home/runner/work/source-sdk-2013/source-sdk-2013/sp/src/public/tier1/utlvector.h:556
556		ResetDbgInfo();
(gdb) i s
#0  CUtlVector<int, CUtlMemory<int, int> >::CUtlVector (initSize=0, growSize=0, 
    this=0xda558190 <g_Hook_PlayerRunCommand+16>)
    at /home/runner/work/source-sdk-2013/source-sdk-2013/sp/src/public/tier1/utlvector.h:556
#1  ScriptFuncDescriptor_t::ScriptFuncDescriptor_t (this=0xda558180 <g_Hook_PlayerRunCommand>)
    at /home/runner/work/source-sdk-2013/source-sdk-2013/sp/src/public/vscript/ivscript.h:251
#2  ScriptHook_t::ScriptHook_t (this=0xda558180 <g_Hook_PlayerRunCommand>)
    at /home/runner/work/source-sdk-2013/source-sdk-2013/sp/src/public/vscript/ivscript.h:1909
#3  0xd9a56ee8 in __static_initialization_and_destruction_0 (__initialize_p=__initialize_p@entry=1, 
    __priority=__priority@entry=65535)
    at /home/runner/work/source-sdk-2013/source-sdk-2013/sp/src/game/server/player.cpp:500
#4  0xd9a5738a in _GLOBAL__sub_I_player.cpp(void) ()
    at /home/runner/work/source-sdk-2013/source-sdk-2013/sp/src/game/server/player.cpp:10362

Steps to reproduce

To observe in game, create a hook with parameters. Legacy type or not does not matter.

Use FireBullets to test in MSVC, PlayerRunCommand works in there.

Hooks.Add( player.GetOrCreatePrivateScriptScope(), "PlayerRunCommand", function( ucmd )
{
}, "" );

player.GetOrCreatePrivateScriptScope().PlayerRunCommand <- function()
{
	command;
}

player.GetOrCreatePrivateScriptScope().FireBullets <- function()
{
	entity;
}

[z33ky]

I stumbled upon this limitation when trying out a mod, and I attempted to fix this:

diff --git a/sp/src/game/shared/baseentity_shared.h b/sp/src/game/shared/baseentity_shared.h
index 85a0ffd8..4891b0bc 100644
--- a/sp/src/game/shared/baseentity_shared.h
+++ b/sp/src/game/shared/baseentity_shared.h
@@ -255,7 +255,7 @@ inline HSCRIPT ToHScript(CBaseEntity* pEnt)
 	return (pEnt) ? pEnt->GetScriptInstance() : NULL;
 }

-template <> ScriptClassDesc_t* GetScriptDesc<CBaseEntity>(CBaseEntity*);
+template <> ScriptClassDesc_t* GetScriptDesc<CBaseEntity>(CBaseEntity*, bool);
 inline CBaseEntity* ToEnt(HSCRIPT hScript)
 {
 	return (hScript) ? (CBaseEntity*)g_pScriptVM->GetInstanceValue(hScript, GetScriptDescForClass(CBaseEntity)) : NULL;
diff --git a/sp/src/game/shared/vscript_shared.h b/sp/src/game/shared/vscript_shared.h
index 50834220..2145825b 100644
--- a/sp/src/game/shared/vscript_shared.h
+++ b/sp/src/game/shared/vscript_shared.h
@@ -26,7 +26,7 @@ inline bool VScriptRunScript( const char *pszScriptName, bool bWarnMissing = fal
 #define BEGIN_ENT_SCRIPTDESC_NAMED( className, baseClass, scriptName, description )	_IMPLEMENT_ENT_SCRIPTDESC_ACCESSOR( className ); BEGIN_SCRIPTDESC_NAMED( className, baseClass, scriptName, description )
 #define BEGIN_ENT_SCRIPTDESC_ROOT_NAMED( className, scriptName, description )		_IMPLEMENT_ENT_SCRIPTDESC_ACCESSOR( className ); BEGIN_SCRIPTDESC_ROOT_NAMED( className, scriptName, description )

-#define _IMPLEMENT_ENT_SCRIPTDESC_ACCESSOR( className )					template <> ScriptClassDesc_t * GetScriptDesc<className>( className * ); ScriptClassDesc_t *className::GetScriptDesc()  { return ::GetScriptDesc( this ); }		
+#define _IMPLEMENT_ENT_SCRIPTDESC_ACCESSOR( className )					template <> ScriptClassDesc_t * GetScriptDesc<className>( className *, bool ); ScriptClassDesc_t *className::GetScriptDesc()  { return ::GetScriptDesc( this ); }		

 // Only allow scripts to create entities during map initialization
 bool IsEntityCreationAllowedInScripts( void );
diff --git a/sp/src/public/vscript/ivscript.h b/sp/src/public/vscript/ivscript.h
index 58f981e0..3b6d0b02 100644
--- a/sp/src/public/vscript/ivscript.h
+++ b/sp/src/public/vscript/ivscript.h
@@ -697,33 +703,36 @@ static inline int ToConstantVariant(int value)
 //
 //-----------------------------------------------------------------------------

-#define ALLOW_SCRIPT_ACCESS() 																template <typename T> friend ScriptClassDesc_t *GetScriptDesc(T *);
+#define ALLOW_SCRIPT_ACCESS() 																template <typename T> friend ScriptClassDesc_t *GetScriptDesc(T *, bool);

 #define BEGIN_SCRIPTDESC( className, baseClass, description )								BEGIN_SCRIPTDESC_NAMED( className, baseClass, #className, description )
 #define BEGIN_SCRIPTDESC_ROOT( className, description )										BEGIN_SCRIPTDESC_ROOT_NAMED( className, #className, description )

 #define BEGIN_SCRIPTDESC_NAMED( className, baseClass, scriptName, description ) \
-	template <> ScriptClassDesc_t* GetScriptDesc<baseClass>(baseClass*); \
-	template <> ScriptClassDesc_t* GetScriptDesc<className>(className*); \
-	ScriptClassDesc_t & g_##className##_ScriptDesc = *GetScriptDesc<className>(nullptr); \
-	template <> ScriptClassDesc_t* GetScriptDesc<className>(className*) \
+	template <> ScriptClassDesc_t* GetScriptDesc<baseClass>(baseClass*, bool); \
+	template <> ScriptClassDesc_t* GetScriptDesc<className>(className*, bool); \
+	ScriptClassDesc_t & g_##className##_ScriptDesc = *GetScriptDesc<className>(nullptr, true); \
+	template <> ScriptClassDesc_t* GetScriptDesc<className>(className*, bool init) \
 	{ \
 		static ScriptClassDesc_t g_##className##_ScriptDesc; \
 		typedef className _className; \
 		ScriptClassDesc_t *pDesc = &g_##className##_ScriptDesc; \
-		if (pDesc->m_pszClassname) return pDesc; \
-		pDesc->m_pszDescription = description; \
-		ScriptInitClassDescNamed( pDesc, className, GetScriptDescForClass( baseClass ), scriptName ); \
-		ScriptClassDesc_t *pInstanceHelperBase = pDesc->m_pBaseDesc; \
-		while ( pInstanceHelperBase ) \
+		if (!pDesc->m_pszClassname) \
 		{ \
-			if ( pInstanceHelperBase->pHelper ) \
+			pDesc->m_pszDescription = description; \
+			ScriptInitClassDescNamed( pDesc, className, GetScriptDescForClass( baseClass ), scriptName ); \
+			ScriptClassDesc_t *pInstanceHelperBase = pDesc->m_pBaseDesc; \
+			while ( pInstanceHelperBase ) \
 			{ \
-				pDesc->pHelper = pInstanceHelperBase->pHelper; \
-				break; \
+				if ( pInstanceHelperBase->pHelper ) \
+				{ \
+					pDesc->pHelper = pInstanceHelperBase->pHelper; \
+					break; \
+				} \
+				pInstanceHelperBase = pInstanceHelperBase->m_pBaseDesc; \
 			} \
-			pInstanceHelperBase = pInstanceHelperBase->m_pBaseDesc; \
-		}
+		} \
+		if (!init) return pDesc;


 #define BEGIN_SCRIPTDESC_ROOT_NAMED( className, scriptName, description ) \
@@ -781,10 +790,10 @@ static inline int ToConstantVariant(int value)
 	do { ScriptMemberDesc_t *pBinding = &((pDesc)->m_Members[(pDesc)->m_Members.AddToTail()]); pBinding->m_pszScriptName = varName; pBinding->m_pszDescription = description; pBinding->m_ReturnType = returnType; } while (0);
 #endif

-template <typename T> ScriptClassDesc_t *GetScriptDesc(T *);
+template <typename T> ScriptClassDesc_t *GetScriptDesc(T *, bool = false);

 struct ScriptNoBase_t;
-template <> inline ScriptClassDesc_t *GetScriptDesc<ScriptNoBase_t>( ScriptNoBase_t *) { return NULL; }
+template <> inline ScriptClassDesc_t *GetScriptDesc<ScriptNoBase_t>( ScriptNoBase_t *, bool ) { return NULL; }

 #define GetScriptDescForClass( className ) GetScriptDesc( ( className *)NULL )

Within a TU, gcc orders the static constructors from first/top to bottom/last, which is fine as long as the ScriptHook_ts are put before the SCRIPTDESC() block.
The issue presents itself across TUs, like when GetScriptDesc<CBaseAnimating>() callsGetScriptDesc<CBaseEntity>(), which hasn't happened yet (and neither it's ScriptHook_t's).
To fix this I added a boolean parameter to GetScriptDesc() that is only set for the call from the own TU to initialize the ScriptClassDesc_t global, otherwise function initialization (DEFINE_SCRIPTFUNC/BEGIN_SCRIPTHOOK) is skipped (I guess deferring this should be fine), but a valid pointer is still returned (and will be fully initialized later - but the address won't change!) to properly setup the caller's pHelper.

Seems to work on my end. If you think it looks fine I can open a PR, but I'm not familiar with this system, perhaps deferring that part when initializing the subclasses' ScriptClassDesc_t is bad in subtle ways.

[Blixibon]

Nothing in this fix jumps out to me as problematic. @samisalreadytaken might have his own input, but please feel free to open a PR for this.

[samisalreadytaken]

The design of ScriptHook_t could also be changed to fix it, but this looks fine.

[samisalreadytaken]

This is an issue in msvc as well. None of the hooks from baseentity.cpp or baseanimating.cpp work, #320 fixes it. I don't know how this went unnoticed, maybe different compiler versions handle it differently.