mapbase-source / source-sdk-2013

This is Mapbase's public GitHub repo. It contains all of the code, but not the assets.
https://www.moddb.com/mods/mapbase
Other
232 stars 140 forks source link

[CODE] Script hook parameter reset #244

Open samisalreadytaken opened 1 year ago

samisalreadytaken commented 1 year ago

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 commented 3 months ago

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 commented 2 months ago

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 commented 2 months ago

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

samisalreadytaken commented 1 week ago

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.