From c9dd3574709039f284fd147b5923d246f29aac80 Mon Sep 17 00:00:00 2001 From: Alexander 'z33ky' Hirsch <1zeeky@gmail.com> Date: Thu, 5 Sep 2024 23:24:06 +0200 Subject: [PATCH 01/11] Prevent return of dangling Vector/QAngle to VScript When a Vector or QAngle rvalue reference is returned from a VScript function, the constructed ScriptVariant_t must not store the pointer to it since it is, per convention, a temporary reference. Only do that for lvalue-references, but do a copy when constructing from or assigning a rvalue reference. --- sp/src/public/vscript/ivscript.h | 6 ++++++ sp/src/public/vscript/vscript_templates.h | 4 ---- sp/src/vscript/vscript_squirrel.cpp | 3 +-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/sp/src/public/vscript/ivscript.h b/sp/src/public/vscript/ivscript.h index 58f981e0..88f4771a 100644 --- a/sp/src/public/vscript/ivscript.h +++ b/sp/src/public/vscript/ivscript.h @@ -396,6 +396,9 @@ struct ScriptVariant_t #ifdef MAPBASE_VSCRIPT ScriptVariant_t( const QAngle &val, bool bCopy = false ) : m_flags( 0 ), m_type( FIELD_VECTOR ) { if ( !bCopy ) { m_pAngle = &val; } else { m_pAngle = new QAngle( val ); m_flags |= SV_FREE; } } ScriptVariant_t( const QAngle *val, bool bCopy = false ) : m_flags( 0 ), m_type( FIELD_VECTOR ) { if ( !bCopy ) { m_pAngle = val; } else { m_pAngle = new QAngle( *val ); m_flags |= SV_FREE; } } + + ScriptVariant_t( Vector &&val ) : ScriptVariant_t( val, true ) { } + ScriptVariant_t( QAngle &&val ) : ScriptVariant_t( val, true ) { } #endif bool IsNull() const { return (m_type == FIELD_VOID ); } @@ -423,6 +426,9 @@ struct ScriptVariant_t #ifdef MAPBASE_VSCRIPT void operator=( const QAngle &vec ) { m_type = FIELD_VECTOR; m_pAngle = &vec; } void operator=( const QAngle *vec ) { m_type = FIELD_VECTOR; m_pAngle = vec; } + + void operator=( Vector &&vec ) { m_type = FIELD_VECTOR; m_pVector = new Vector( vec ); m_flags |= SV_FREE; } + void operator=( QAngle &&vec ) { m_type = FIELD_VECTOR; m_pAngle = new QAngle( vec ); m_flags |= SV_FREE; } #endif void Free() { if ( ( m_flags & SV_FREE ) && ( m_type == FIELD_HSCRIPT || m_type == FIELD_VECTOR || m_type == FIELD_CSTRING ) ) delete m_pszString; } // Generally only needed for return results diff --git a/sp/src/public/vscript/vscript_templates.h b/sp/src/public/vscript/vscript_templates.h index 2d7058a3..af020fe6 100644 --- a/sp/src/public/vscript/vscript_templates.h +++ b/sp/src/public/vscript/vscript_templates.h @@ -337,8 +337,6 @@ inline FUNCPTR_TYPE ScriptConvertFuncPtrFromVoid( void *p ) return false; \ } \ *pReturn = ((FUNC_TYPE)pFunction)( SCRIPT_BINDING_ARGS_##N ); \ - if ( pReturn->m_type == FIELD_VECTOR ) \ - pReturn->m_pVector = new Vector(*pReturn->m_pVector); \ return true; \ } \ }; \ @@ -377,8 +375,6 @@ inline FUNCPTR_TYPE ScriptConvertFuncPtrFromVoid( void *p ) return false; \ } \ *pReturn = (((OBJECT_TYPE_PTR)(pContext))->*ScriptConvertFuncPtrFromVoid(pFunction))( SCRIPT_BINDING_ARGS_##N ); \ - if ( pReturn->m_type == FIELD_VECTOR ) \ - pReturn->m_pVector = new Vector(*pReturn->m_pVector); \ return true; \ } \ }; \ diff --git a/sp/src/vscript/vscript_squirrel.cpp b/sp/src/vscript/vscript_squirrel.cpp index 5efb2314..aff21075 100644 --- a/sp/src/vscript/vscript_squirrel.cpp +++ b/sp/src/vscript/vscript_squirrel.cpp @@ -1415,8 +1415,7 @@ SQInteger function_stub(HSQUIRRELVM vm) PushVariant(vm, retval); - if (retval.m_type == FIELD_VECTOR) - delete retval.m_pVector; + retval.Free(); return pFunc->m_desc.m_ReturnType != FIELD_VOID; } From 80f19601eefcf138aeeea750aea13cd72f64be5d Mon Sep 17 00:00:00 2001 From: Alexander 'z33ky' Hirsch <1zeeky@gmail.com> Date: Fri, 6 Sep 2024 23:25:27 +0200 Subject: [PATCH 02/11] "Fix" ScriptVariant_t::Free() Type FIELD_HSCRIPT is removed, it never sets SV_FREE. All dynamic memory is now free`d via free(). For strdup(), this would be the conformant way in standard C++. It matters not in Source, since both use the same global allocator of the engine, but it is tidier. Vector and QAngle are now allocated via malloc() instead of new to provide symmetry. --- sp/src/public/vscript/ivscript.h | 112 +++++++++++++++++++++++----- sp/src/vscript/vscript_squirrel.cpp | 5 +- 2 files changed, 98 insertions(+), 19 deletions(-) diff --git a/sp/src/public/vscript/ivscript.h b/sp/src/public/vscript/ivscript.h index 88f4771a..c14b099e 100644 --- a/sp/src/public/vscript/ivscript.h +++ b/sp/src/public/vscript/ivscript.h @@ -389,13 +389,49 @@ struct ScriptVariant_t ScriptVariant_t( bool val ) : m_flags( 0 ), m_type( FIELD_BOOLEAN ) { m_bool = val; } ScriptVariant_t( HSCRIPT val ) : m_flags( 0 ), m_type( FIELD_HSCRIPT ) { m_hScript = val; } - ScriptVariant_t( const Vector &val, bool bCopy = false ) : m_flags( 0 ), m_type( FIELD_VECTOR ) { if ( !bCopy ) { m_pVector = &val; } else { m_pVector = new Vector( val ); m_flags |= SV_FREE; } } - ScriptVariant_t( const Vector *val, bool bCopy = false ) : m_flags( 0 ), m_type( FIELD_VECTOR ) { if ( !bCopy ) { m_pVector = val; } else { m_pVector = new Vector( *val ); m_flags |= SV_FREE; } } - ScriptVariant_t( const char *val , bool bCopy = false ) : m_flags( 0 ), m_type( FIELD_CSTRING ) { if ( !bCopy ) { m_pszString = val; } else { m_pszString = strdup( val ); m_flags |= SV_FREE; } } + ScriptVariant_t( const Vector &val, bool bCopy = false ) : m_flags( 0 ), m_type( FIELD_VECTOR ) + { + if ( !bCopy ) + { + m_pVector = &val; + } + else + { + m_pVector = (Vector*)malloc( sizeof( Vector ) ); + new ( (Vector*)m_pVector ) Vector( val ); + m_flags |= SV_FREE; + } + } + ScriptVariant_t( const Vector *val, bool bCopy = false ) : ScriptVariant_t( *val ) { } + + ScriptVariant_t( const char *val , bool bCopy = false ) : m_flags( 0 ), m_type( FIELD_CSTRING ) + { + if ( !bCopy ) + { + m_pszString = val; + } + else + { + m_pszString = strdup( val ); + m_flags |= SV_FREE; + } + } #ifdef MAPBASE_VSCRIPT - ScriptVariant_t( const QAngle &val, bool bCopy = false ) : m_flags( 0 ), m_type( FIELD_VECTOR ) { if ( !bCopy ) { m_pAngle = &val; } else { m_pAngle = new QAngle( val ); m_flags |= SV_FREE; } } - ScriptVariant_t( const QAngle *val, bool bCopy = false ) : m_flags( 0 ), m_type( FIELD_VECTOR ) { if ( !bCopy ) { m_pAngle = val; } else { m_pAngle = new QAngle( *val ); m_flags |= SV_FREE; } } + ScriptVariant_t( const QAngle &val, bool bCopy = false ) : m_flags( 0 ), m_type( FIELD_VECTOR ) + { + if ( !bCopy ) + { + m_pAngle = &val; + } + else + { + m_pAngle = (QAngle*)malloc( sizeof( QAngle ) ); + new ( (QAngle*)m_pAngle ) QAngle( val ); + m_flags |= SV_FREE; + } + } + ScriptVariant_t( const QAngle *val, bool bCopy = false ) : ScriptVariant_t( *val ) { } ScriptVariant_t( Vector &&val ) : ScriptVariant_t( val, true ) { } ScriptVariant_t( QAngle &&val ) : ScriptVariant_t( val, true ) { } @@ -427,11 +463,34 @@ struct ScriptVariant_t void operator=( const QAngle &vec ) { m_type = FIELD_VECTOR; m_pAngle = &vec; } void operator=( const QAngle *vec ) { m_type = FIELD_VECTOR; m_pAngle = vec; } - void operator=( Vector &&vec ) { m_type = FIELD_VECTOR; m_pVector = new Vector( vec ); m_flags |= SV_FREE; } - void operator=( QAngle &&vec ) { m_type = FIELD_VECTOR; m_pAngle = new QAngle( vec ); m_flags |= SV_FREE; } + void operator=( Vector &&vec ) + { + m_type = FIELD_VECTOR; + m_pVector = (Vector*)malloc( sizeof( Vector ) ); + new ( (Vector*)m_pVector ) Vector( vec ); + m_flags |= SV_FREE; + } + + void operator=( QAngle &&ang ) + { + m_type = FIELD_VECTOR; + m_pAngle = (QAngle*)malloc( sizeof( QAngle ) ); + new ( (QAngle*)m_pAngle ) QAngle( ang ); + m_flags |= SV_FREE; + } #endif - void Free() { if ( ( m_flags & SV_FREE ) && ( m_type == FIELD_HSCRIPT || m_type == FIELD_VECTOR || m_type == FIELD_CSTRING ) ) delete m_pszString; } // Generally only needed for return results + void Free() + { + // Generally only needed for return results + if ( ! ( m_flags & SV_FREE ) ) + { + return; + } + + AssertMsg( m_type == FIELD_CSTRING || m_type == FIELD_VECTOR, "Don't know how to free script variant of type %d", m_type ); + free( (void*)m_pszString ); + } template T Get() @@ -532,24 +591,36 @@ struct ScriptVariant_t bool AssignTo( ScriptVariant_t *pDest ) { pDest->m_type = m_type; - if ( m_type == FIELD_VECTOR ) + if ( m_flags & SV_FREE ) { - pDest->m_pVector = new Vector; - ((Vector *)(pDest->m_pVector))->Init( m_pVector->x, m_pVector->y, m_pVector->z ); - pDest->m_flags |= SV_FREE; - } - else if ( m_type == FIELD_CSTRING ) - { - pDest->m_pszString = strdup( m_pszString ); - pDest->m_flags |= SV_FREE; + if ( m_type == FIELD_VECTOR ) + { + pDest->m_pVector = (Vector*)malloc( sizeof( Vector ) ); + pDest->EmplaceAllocedVector( *m_pVector ); + m_flags |= SV_FREE; + } + else if ( m_type == FIELD_CSTRING ) + { + pDest->m_pszString = strdup( m_pszString ); + pDest->m_flags |= SV_FREE; + } + else + { + Assert( false ); + pDest->m_int = m_int; + pDest->m_flags &= ~SV_FREE; + } } else { pDest->m_int = m_int; + pDest->m_flags &= ~SV_FREE; } return false; } + void EmplaceAllocedVector( const Vector &vec ); + union { int m_int; @@ -571,6 +642,13 @@ struct ScriptVariant_t private: }; +#include "tier0/memdbgoff.h" +inline void ScriptVariant_t::EmplaceAllocedVector( const Vector &vec ) +{ + new ( (Vector*)m_pVector ) Vector( vec ); +} +#include "tier0/memdbgon.h" + #define SCRIPT_VARIANT_NULL ScriptVariant_t() #ifdef MAPBASE_VSCRIPT diff --git a/sp/src/vscript/vscript_squirrel.cpp b/sp/src/vscript/vscript_squirrel.cpp index aff21075..be95daf9 100644 --- a/sp/src/vscript/vscript_squirrel.cpp +++ b/sp/src/vscript/vscript_squirrel.cpp @@ -1247,7 +1247,7 @@ bool getVariant(HSQUIRRELVM vm, SQInteger idx, ScriptVariant_t& variant) { return false; } - char* buffer = new char[size + 1]; + char* buffer = (char*)malloc(size + 1); V_memcpy(buffer, val, size); buffer[size] = 0; variant = buffer; @@ -1262,7 +1262,8 @@ bool getVariant(HSQUIRRELVM vm, SQInteger idx, ScriptVariant_t& variant) tag == TYPETAG_VECTOR && SQ_SUCCEEDED(sq_getinstanceup(vm, idx, (SQUserPointer*)&v, TYPETAG_VECTOR))) { - variant = new Vector(*v); + variant = (Vector*)malloc(sizeof(Vector)); + variant.EmplaceAllocedVector(*v); variant.m_flags |= SV_FREE; return true; } From 879b9e8d25776fb24693348a3aec68ba637714c6 Mon Sep 17 00:00:00 2001 From: Alexander 'z33ky' Hirsch <1zeeky@gmail.com> Date: Tue, 12 Nov 2024 23:53:30 +0100 Subject: [PATCH 03/11] Fix dubious ScriptVariant_t Free()'s When assigning a null value in getVariant(), make sure a previous SV_FREE is not carried over. In SqurrelVM::ReleaseValue(), make sure SV_FREE is not kept after Free()ing a value. --- sp/src/vscript/vscript_squirrel.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sp/src/vscript/vscript_squirrel.cpp b/sp/src/vscript/vscript_squirrel.cpp index be95daf9..b9ba659c 100644 --- a/sp/src/vscript/vscript_squirrel.cpp +++ b/sp/src/vscript/vscript_squirrel.cpp @@ -1205,6 +1205,7 @@ bool getVariant(HSQUIRRELVM vm, SQInteger idx, ScriptVariant_t& variant) { case OT_NULL: { + variant.m_flags = 0; // TODO: Should this be (HSCRIPT)nullptr variant.m_type = FIELD_VOID; return true; @@ -3074,6 +3075,7 @@ void SquirrelVM::ReleaseValue(ScriptVariant_t& value) // Let's prevent this being called again and giving some UB value.m_type = FIELD_VOID; + value.m_flags = 0; } bool SquirrelVM::ClearValue(HSCRIPT hScope, const char* pszKey) From 99858e63f7ff125edbda1fb0d14d7c4682517b86 Mon Sep 17 00:00:00 2001 From: Alexander 'z33ky' Hirsch <1zeeky@gmail.com> Date: Sat, 7 Sep 2024 22:50:28 +0200 Subject: [PATCH 04/11] Optimize squirrel function_stub() Vector/QAngle return value Instead of temporary allocating dynamic memory, which subsequently is copied into another dynamically allocated piece of memory, provide a temporary buffer on the stack. The script value inserted into the VM still needs to allocate dynamically though... A few sites are adapted to not create a ScriptVariant_t from a temporary Vector to avoid dynamic allocation there too. cf. mapbase-source/source-sdk-2013#321 --- sp/src/game/server/hl2/proto_sniper.cpp | 15 ++- .../shared/mapbase/vscript_consts_shared.cpp | 3 +- sp/src/public/vscript/ivscript.h | 107 ++++++------------ sp/src/public/vscript/vscript_templates.h | 104 +++++++++++++++-- sp/src/vscript/vscript_squirrel.cpp | 7 +- 5 files changed, 145 insertions(+), 91 deletions(-) diff --git a/sp/src/game/server/hl2/proto_sniper.cpp b/sp/src/game/server/hl2/proto_sniper.cpp index 4315e35c..cd7c309e 100644 --- a/sp/src/game/server/hl2/proto_sniper.cpp +++ b/sp/src/game/server/hl2/proto_sniper.cpp @@ -2645,16 +2645,19 @@ Vector CProtoSniper::DesiredBodyTarget( CBaseEntity *pTarget ) { // By default, aim for the center Vector vecTarget = pTarget->WorldSpaceCenter(); + const Vector vecBulletOrigin = GetBulletOrigin(); #ifdef MAPBASE_VSCRIPT - if (m_ScriptScope.IsInitialized() && g_Hook_GetActualShootPosition.CanRunInScope(m_ScriptScope)) + if ( m_ScriptScope.IsInitialized() && g_Hook_GetActualShootPosition.CanRunInScope( m_ScriptScope ) ) { ScriptVariant_t functionReturn; - ScriptVariant_t args[] = { GetBulletOrigin(), ToHScript( pTarget ) }; - if (g_Hook_GetActualShootPosition.Call( m_ScriptScope, &functionReturn, args )) + ScriptVariant_t args[] = { vecBulletOrigin, ToHScript( pTarget ) }; + if ( g_Hook_GetActualShootPosition.Call( m_ScriptScope, &functionReturn, args ) ) { - if (functionReturn.m_type == FIELD_VECTOR && functionReturn.m_pVector->LengthSqr() != 0.0f) + if ( functionReturn.m_type == FIELD_VECTOR && functionReturn.m_pVector->LengthSqr() != 0.0f ) + { return *functionReturn.m_pVector; + } } } #endif @@ -2682,12 +2685,12 @@ Vector CProtoSniper::DesiredBodyTarget( CBaseEntity *pTarget ) { if( flTimeSinceLastMiss > 0.0f && flTimeSinceLastMiss < 4.0f && hl2_episodic.GetBool() ) { - vecTarget = pTarget->BodyTarget( GetBulletOrigin(), false ); + vecTarget = pTarget->BodyTarget( vecBulletOrigin, false ); } else { // Shoot zombies in the headcrab - vecTarget = pTarget->HeadTarget( GetBulletOrigin() ); + vecTarget = pTarget->HeadTarget( vecBulletOrigin ); } } else if( pTarget->Classify() == CLASS_ANTLION ) diff --git a/sp/src/game/shared/mapbase/vscript_consts_shared.cpp b/sp/src/game/shared/mapbase/vscript_consts_shared.cpp index 2ced7086..8b6fbd96 100644 --- a/sp/src/game/shared/mapbase/vscript_consts_shared.cpp +++ b/sp/src/game/shared/mapbase/vscript_consts_shared.cpp @@ -346,7 +346,8 @@ void RegisterSharedScriptConstants() ScriptRegisterConstant( g_pScriptVM, ROPE_NO_GRAVITY, "Disable gravity on this rope. (for use in rope flags)" ); ScriptRegisterConstant( g_pScriptVM, ROPE_NUMFLAGS, "The number of rope flags recognized by the game." ); - ScriptRegisterConstantNamed( g_pScriptVM, Vector( ROPE_GRAVITY ), "ROPE_GRAVITY", "Default rope gravity vector." ); + static Vector vecRopeGravity( ROPE_GRAVITY ); + ScriptRegisterConstantNamed( g_pScriptVM, vecRopeGravity, "ROPE_GRAVITY", "Default rope gravity vector." ); // // Sounds diff --git a/sp/src/public/vscript/ivscript.h b/sp/src/public/vscript/ivscript.h index c14b099e..c9a24e4b 100644 --- a/sp/src/public/vscript/ivscript.h +++ b/sp/src/public/vscript/ivscript.h @@ -300,7 +300,8 @@ enum ScriptFuncBindingFlags_t SF_MEMBER_FUNC = 0x01, }; -typedef bool (*ScriptBindingFunc_t)( void *pFunction, void *pContext, ScriptVariant_t *pArguments, int nArguments, ScriptVariant_t *pReturn ); +union ScriptVariantTemporaryStorage_t; +typedef bool (*ScriptBindingFunc_t)( void *pFunction, void *pContext, ScriptVariant_t *pArguments, int nArguments, ScriptVariant_t *pReturn, ScriptVariantTemporaryStorage_t &temporaryReturnStorage ); struct ScriptFunctionBinding_t { @@ -389,52 +390,17 @@ struct ScriptVariant_t ScriptVariant_t( bool val ) : m_flags( 0 ), m_type( FIELD_BOOLEAN ) { m_bool = val; } ScriptVariant_t( HSCRIPT val ) : m_flags( 0 ), m_type( FIELD_HSCRIPT ) { m_hScript = val; } - ScriptVariant_t( const Vector &val, bool bCopy = false ) : m_flags( 0 ), m_type( FIELD_VECTOR ) - { - if ( !bCopy ) - { - m_pVector = &val; - } - else - { - m_pVector = (Vector*)malloc( sizeof( Vector ) ); - new ( (Vector*)m_pVector ) Vector( val ); - m_flags |= SV_FREE; - } - } - ScriptVariant_t( const Vector *val, bool bCopy = false ) : ScriptVariant_t( *val ) { } + ScriptVariant_t( const Vector &val ) : m_flags( 0 ), m_type( FIELD_VECTOR ) { m_pVector = &val; } + ScriptVariant_t( const Vector *val ) : ScriptVariant_t( *val ) { } - ScriptVariant_t( const char *val , bool bCopy = false ) : m_flags( 0 ), m_type( FIELD_CSTRING ) - { - if ( !bCopy ) - { - m_pszString = val; - } - else - { - m_pszString = strdup( val ); - m_flags |= SV_FREE; - } - } + ScriptVariant_t( const char *val ) : m_flags( 0 ), m_type( FIELD_CSTRING ) { m_pszString = val; } #ifdef MAPBASE_VSCRIPT - ScriptVariant_t( const QAngle &val, bool bCopy = false ) : m_flags( 0 ), m_type( FIELD_VECTOR ) - { - if ( !bCopy ) - { - m_pAngle = &val; - } - else - { - m_pAngle = (QAngle*)malloc( sizeof( QAngle ) ); - new ( (QAngle*)m_pAngle ) QAngle( val ); - m_flags |= SV_FREE; - } - } - ScriptVariant_t( const QAngle *val, bool bCopy = false ) : ScriptVariant_t( *val ) { } + ScriptVariant_t( const QAngle &val ) : m_flags( 0 ), m_type( FIELD_VECTOR ) { m_pAngle = &val; } + ScriptVariant_t( const QAngle *val ) : ScriptVariant_t( *val ) { } - ScriptVariant_t( Vector &&val ) : ScriptVariant_t( val, true ) { } - ScriptVariant_t( QAngle &&val ) : ScriptVariant_t( val, true ) { } + ScriptVariant_t( Vector &&val ) = delete; + ScriptVariant_t( QAngle &&val ) = delete; #endif bool IsNull() const { return (m_type == FIELD_VOID ); } @@ -442,42 +408,29 @@ struct ScriptVariant_t operator int() const { Assert( m_type == FIELD_INTEGER ); return m_int; } operator float() const { Assert( m_type == FIELD_FLOAT ); return m_float; } operator const char *() const { Assert( m_type == FIELD_CSTRING ); return ( m_pszString ) ? m_pszString : ""; } - operator const Vector &() const { Assert( m_type == FIELD_VECTOR ); static Vector vecNull(0, 0, 0); return (m_pVector) ? *m_pVector : vecNull; } + operator const Vector &() const { Assert( m_type == FIELD_VECTOR ); return (m_pVector) ? *m_pVector : vec3_origin; } operator char() const { Assert( m_type == FIELD_CHARACTER ); return m_char; } operator bool() const { Assert( m_type == FIELD_BOOLEAN ); return m_bool; } operator HSCRIPT() const { Assert( m_type == FIELD_HSCRIPT ); return m_hScript; } #ifdef MAPBASE_VSCRIPT - operator const QAngle &() const { Assert( m_type == FIELD_VECTOR ); static QAngle vecNull(0, 0, 0); return (m_pAngle) ? *m_pAngle : vecNull; } + operator const QAngle &() const { Assert( m_type == FIELD_VECTOR ); return (m_pAngle) ? *m_pAngle : vec3_angle; } #endif - void operator=( int i ) { m_type = FIELD_INTEGER; m_int = i; } - void operator=( float f ) { m_type = FIELD_FLOAT; m_float = f; } - void operator=( double f ) { m_type = FIELD_FLOAT; m_float = (float)f; } - void operator=( const Vector &vec ) { m_type = FIELD_VECTOR; m_pVector = &vec; } - void operator=( const Vector *vec ) { m_type = FIELD_VECTOR; m_pVector = vec; } - void operator=( const char *psz ) { m_type = FIELD_CSTRING; m_pszString = psz; } - void operator=( char c ) { m_type = FIELD_CHARACTER; m_char = c; } - void operator=( bool b ) { m_type = FIELD_BOOLEAN; m_bool = b; } - void operator=( HSCRIPT h ) { m_type = FIELD_HSCRIPT; m_hScript = h; } + void operator=( int i ) { m_type = FIELD_INTEGER; m_flags = 0; m_int = i; } + void operator=( float f ) { m_type = FIELD_FLOAT; m_flags = 0; m_float = f; } + void operator=( double f ) { m_type = FIELD_FLOAT; m_flags = 0; m_float = (float)f; } + void operator=( const Vector &vec ) { m_type = FIELD_VECTOR; m_flags = 0; m_pVector = &vec; } + void operator=( const Vector *vec ) { m_type = FIELD_VECTOR; m_flags = 0; m_pVector = vec; } + void operator=( const char *psz ) { m_type = FIELD_CSTRING; m_flags = 0; m_pszString = psz; } + void operator=( char c ) { m_type = FIELD_CHARACTER; m_flags = 0; m_char = c; } + void operator=( bool b ) { m_type = FIELD_BOOLEAN; m_flags = 0; m_bool = b; } + void operator=( HSCRIPT h ) { m_type = FIELD_HSCRIPT; m_flags = 0; m_hScript = h; } #ifdef MAPBASE_VSCRIPT - void operator=( const QAngle &vec ) { m_type = FIELD_VECTOR; m_pAngle = &vec; } - void operator=( const QAngle *vec ) { m_type = FIELD_VECTOR; m_pAngle = vec; } + void operator=( const QAngle &ang ) { m_type = FIELD_VECTOR; m_flags = 0; m_pAngle = ∠ } + void operator=( const QAngle *ang ) { m_type = FIELD_VECTOR; m_flags = 0; m_pAngle = ang; } - void operator=( Vector &&vec ) - { - m_type = FIELD_VECTOR; - m_pVector = (Vector*)malloc( sizeof( Vector ) ); - new ( (Vector*)m_pVector ) Vector( vec ); - m_flags |= SV_FREE; - } - - void operator=( QAngle &&ang ) - { - m_type = FIELD_VECTOR; - m_pAngle = (QAngle*)malloc( sizeof( QAngle ) ); - new ( (QAngle*)m_pAngle ) QAngle( ang ); - m_flags |= SV_FREE; - } + void operator=( Vector &&vec ) = delete; + void operator=( QAngle &&ang ) = delete; #endif void Free() @@ -651,6 +604,16 @@ inline void ScriptVariant_t::EmplaceAllocedVector( const Vector &vec ) #define SCRIPT_VARIANT_NULL ScriptVariant_t() +union ScriptVariantTemporaryStorage_t +{ + // members must be initialized via placement-new + ScriptVariantTemporaryStorage_t() { } + + // members must have trivial destructor, since no destructor will be invoked + Vector m_vec; + QAngle m_ang; +}; + #ifdef MAPBASE_VSCRIPT //--------------------------------------------------------- struct ScriptConstantBinding_t @@ -743,7 +706,7 @@ static inline int ToConstantVariant(int value) // This is used for registering variants (particularly vectors) not tied to existing variables. // The principal difference is that m_data is initted with bCopy set to true. #define ScriptRegisterConstantFromTemp( pVM, constant, description ) ScriptRegisterConstantFromTempNamed( pVM, constant, #constant, description ) -#define ScriptRegisterConstantFromTempNamed( pVM, constant, scriptName, description ) do { static ScriptConstantBinding_t binding; binding.m_pszScriptName = scriptName; binding.m_pszDescription = description; binding.m_data = ScriptVariant_t( constant, true ); pVM->RegisterConstant( &binding ); } while (0) +#define ScriptRegisterConstantFromTempNamed( pVM, constant, scriptName, description ) do { static const auto constantStorage = constant; static ScriptConstantBinding_t binding; binding.m_pszScriptName = scriptName; binding.m_pszDescription = description; binding.m_data = ScriptVariant_t( constantStorage ); pVM->RegisterConstant( &binding ); } while (0) //----------------------------------------------------------------------------- // diff --git a/sp/src/public/vscript/vscript_templates.h b/sp/src/public/vscript/vscript_templates.h index af020fe6..9f2054cc 100644 --- a/sp/src/public/vscript/vscript_templates.h +++ b/sp/src/public/vscript/vscript_templates.h @@ -326,8 +326,8 @@ inline FUNCPTR_TYPE ScriptConvertFuncPtrFromVoid( void *p ) class CNonMemberScriptBinding##N \ { \ public: \ - static bool Call( void *pFunction, void *pContext, ScriptVariant_t *pArguments, int nArguments, ScriptVariant_t *pReturn ) \ - { \ + static bool Call( void *pFunction, void *pContext, ScriptVariant_t *pArguments, int nArguments, ScriptVariant_t *pReturn, ScriptVariantTemporaryStorage_t &temporaryReturnStorage ) \ + { \ Assert( nArguments == N ); \ Assert( pReturn ); \ Assert( !pContext ); \ @@ -337,15 +337,15 @@ inline FUNCPTR_TYPE ScriptConvertFuncPtrFromVoid( void *p ) return false; \ } \ *pReturn = ((FUNC_TYPE)pFunction)( SCRIPT_BINDING_ARGS_##N ); \ - return true; \ - } \ + return true; \ + } \ }; \ \ template \ class CNonMemberScriptBinding##N \ { \ public: \ - static bool Call( void *pFunction, void *pContext, ScriptVariant_t *pArguments, int nArguments, ScriptVariant_t *pReturn ) \ + static bool Call( void *pFunction, void *pContext, ScriptVariant_t *pArguments, int nArguments, ScriptVariant_t *pReturn, ScriptVariantTemporaryStorage_t &temporaryReturnStorage ) \ { \ Assert( nArguments == N ); \ Assert( !pReturn ); \ @@ -360,12 +360,52 @@ inline FUNCPTR_TYPE ScriptConvertFuncPtrFromVoid( void *p ) } \ }; \ \ + template \ + class CNonMemberScriptBinding##N \ + { \ + public: \ + static bool Call( void *pFunction, void *pContext, ScriptVariant_t *pArguments, int nArguments, ScriptVariant_t *pReturn, ScriptVariantTemporaryStorage_t &temporaryReturnStorage ) \ + { \ + Assert( nArguments == N ); \ + Assert( pReturn ); \ + Assert( !pContext ); \ + \ + if ( nArguments != N || !pReturn || pContext ) \ + { \ + return false; \ + } \ + new ( &temporaryReturnStorage.m_vec ) Vector( ((FUNC_TYPE)pFunction)( SCRIPT_BINDING_ARGS_##N ) ); \ + *pReturn = temporaryReturnStorage.m_vec; \ + return true; \ + } \ + }; \ + \ + template \ + class CNonMemberScriptBinding##N \ + { \ + public: \ + static bool Call( void *pFunction, void *pContext, ScriptVariant_t *pArguments, int nArguments, ScriptVariant_t *pReturn, ScriptVariantTemporaryStorage_t &temporaryReturnStorage ) \ + { \ + Assert( nArguments == N ); \ + Assert( pReturn ); \ + Assert( !pContext ); \ + \ + if ( nArguments != N || !pReturn || pContext ) \ + { \ + return false; \ + } \ + new ( &temporaryReturnStorage.m_ang ) QAngle( ((FUNC_TYPE)pFunction)( SCRIPT_BINDING_ARGS_##N ) ); \ + *pReturn = temporaryReturnStorage.m_ang; \ + return true; \ + } \ + }; \ + \ template \ class CMemberScriptBinding##N \ { \ public: \ - static bool Call( void *pFunction, void *pContext, ScriptVariant_t *pArguments, int nArguments, ScriptVariant_t *pReturn ) \ - { \ + static bool Call( void *pFunction, void *pContext, ScriptVariant_t *pArguments, int nArguments, ScriptVariant_t *pReturn, ScriptVariantTemporaryStorage_t &temporaryReturnStorage ) \ + { \ Assert( nArguments == N ); \ Assert( pReturn ); \ Assert( pContext ); \ @@ -375,15 +415,15 @@ inline FUNCPTR_TYPE ScriptConvertFuncPtrFromVoid( void *p ) return false; \ } \ *pReturn = (((OBJECT_TYPE_PTR)(pContext))->*ScriptConvertFuncPtrFromVoid(pFunction))( SCRIPT_BINDING_ARGS_##N ); \ - return true; \ - } \ + return true; \ + } \ }; \ \ template \ class CMemberScriptBinding##N \ { \ public: \ - static bool Call( void *pFunction, void *pContext, ScriptVariant_t *pArguments, int nArguments, ScriptVariant_t *pReturn ) \ + static bool Call( void *pFunction, void *pContext, ScriptVariant_t *pArguments, int nArguments, ScriptVariant_t *pReturn, ScriptVariantTemporaryStorage_t &temporaryReturnStorage ) \ { \ Assert( nArguments == N ); \ Assert( !pReturn ); \ @@ -398,6 +438,46 @@ inline FUNCPTR_TYPE ScriptConvertFuncPtrFromVoid( void *p ) } \ }; \ \ + template \ + class CMemberScriptBinding##N \ + { \ + public: \ + static bool Call( void *pFunction, void *pContext, ScriptVariant_t *pArguments, int nArguments, ScriptVariant_t *pReturn, ScriptVariantTemporaryStorage_t &temporaryReturnStorage ) \ + { \ + Assert( nArguments == N ); \ + Assert( pReturn ); \ + Assert( pContext ); \ + \ + if ( nArguments != N || !pReturn || !pContext ) \ + { \ + return false; \ + } \ + new ( &temporaryReturnStorage.m_vec ) Vector( (((OBJECT_TYPE_PTR)(pContext))->*ScriptConvertFuncPtrFromVoid(pFunction))( SCRIPT_BINDING_ARGS_##N ) ); \ + *pReturn = temporaryReturnStorage.m_vec; \ + return true; \ + } \ + }; \ + \ + template \ + class CMemberScriptBinding##N \ + { \ + public: \ + static bool Call( void *pFunction, void *pContext, ScriptVariant_t *pArguments, int nArguments, ScriptVariant_t *pReturn, ScriptVariantTemporaryStorage_t &temporaryReturnStorage ) \ + { \ + Assert( nArguments == N ); \ + Assert( pReturn ); \ + Assert( pContext ); \ + \ + if ( nArguments != N || !pReturn || !pContext ) \ + { \ + return false; \ + } \ + new ( &temporaryReturnStorage.m_ang ) QAngle( (((OBJECT_TYPE_PTR)(pContext))->*ScriptConvertFuncPtrFromVoid(pFunction))( SCRIPT_BINDING_ARGS_##N ) ); \ + *pReturn = temporaryReturnStorage.m_ang; \ + return true; \ + } \ + }; \ + \ template \ inline ScriptBindingFunc_t ScriptCreateBinding(FUNCTION_RETTYPE (*pfnProxied)( FUNC_BASE_TEMPLATE_FUNC_PARAMS_##N ) ) \ { \ @@ -419,7 +499,11 @@ inline FUNCPTR_TYPE ScriptConvertFuncPtrFromVoid( void *p ) return &CMemberScriptBinding##N::Call; \ } +//note: no memory is actually allocated in the functions that get defined, +// it merely uses placement-new for which we need to disable this +#include "tier0/memdbgoff.h" FUNC_GENERATE_ALL( DEFINE_SCRIPT_BINDINGS ); +#include "tier0/memdbgon.h" //----------------------------------------------------------------------------- // diff --git a/sp/src/vscript/vscript_squirrel.cpp b/sp/src/vscript/vscript_squirrel.cpp index b9ba659c..c17f3c64 100644 --- a/sp/src/vscript/vscript_squirrel.cpp +++ b/sp/src/vscript/vscript_squirrel.cpp @@ -1399,6 +1399,7 @@ SQInteger function_stub(HSQUIRRELVM vm) } ScriptVariant_t retval; + ScriptVariantTemporaryStorage_t retval_storage; SquirrelVM* pSquirrelVM = (SquirrelVM*)sq_getforeignptr(vm); assert(pSquirrelVM); @@ -1406,7 +1407,7 @@ SQInteger function_stub(HSQUIRRELVM vm) sq_resetobject(&pSquirrelVM->lastError_); (*pFunc->m_pfnBinding)(pFunc->m_pFunction, instance, params.Base(), nargs, - pFunc->m_desc.m_ReturnType == FIELD_VOID ? nullptr : &retval); + pFunc->m_desc.m_ReturnType == FIELD_VOID ? nullptr : &retval, retval_storage); if (!sq_isnull(pSquirrelVM->lastError_)) { @@ -1417,7 +1418,9 @@ SQInteger function_stub(HSQUIRRELVM vm) PushVariant(vm, retval); - retval.Free(); + // strings never get copied here, Vector and QAngle are stored in script_retval_storage + // everything else is stored inline, so there should be no memory to free + Assert(!(retval.m_flags & SV_FREE)); return pFunc->m_desc.m_ReturnType != FIELD_VOID; } From fe82da8f1b4e07e33d3359e9ad7b9f51dd12bf23 Mon Sep 17 00:00:00 2001 From: Alexander 'z33ky' Hirsch <1zeeky@gmail.com> Date: Tue, 12 Nov 2024 23:56:28 +0100 Subject: [PATCH 05/11] Remove unused generic ScriptVariant_t::{Get,AssignTo}() They wouldn't even work. --- sp/src/public/vscript/ivscript.h | 46 -------------------------------- 1 file changed, 46 deletions(-) diff --git a/sp/src/public/vscript/ivscript.h b/sp/src/public/vscript/ivscript.h index c9a24e4b..7e4c1d48 100644 --- a/sp/src/public/vscript/ivscript.h +++ b/sp/src/public/vscript/ivscript.h @@ -445,52 +445,6 @@ struct ScriptVariant_t free( (void*)m_pszString ); } - template - T Get() - { - T value; - AssignTo( &value ); - return value; - } - - template - bool AssignTo( T *pDest ) - { - ScriptDataType_t destType = ScriptDeduceType( T ); - if ( destType == FIELD_TYPEUNKNOWN ) - { - DevWarning( "Unable to convert script variant to unknown type\n" ); - } - if ( destType == m_type ) - { - *pDest = *this; - return true; - } - - if ( m_type != FIELD_VECTOR && m_type != FIELD_CSTRING && destType != FIELD_VECTOR && destType != FIELD_CSTRING ) - { - switch ( m_type ) - { - case FIELD_VOID: *pDest = 0; break; - case FIELD_INTEGER: *pDest = m_int; return true; - case FIELD_FLOAT: *pDest = m_float; return true; - case FIELD_CHARACTER: *pDest = m_char; return true; - case FIELD_BOOLEAN: *pDest = m_bool; return true; - case FIELD_HSCRIPT: *pDest = m_hScript; return true; - } - } - else - { - DevWarning( "No free conversion of %s script variant to %s right now\n", - ScriptFieldTypeName( m_type ), ScriptFieldTypeName() ); - if ( destType != FIELD_VECTOR ) - { - *pDest = 0; - } - } - return false; - } - bool AssignTo( float *pDest ) { switch( m_type ) From a2e43a567f00db5a4088951bf714e4ba9d77e5f4 Mon Sep 17 00:00:00 2001 From: Alexander 'z33ky' Hirsch <1zeeky@gmail.com> Date: Tue, 12 Nov 2024 23:57:06 +0100 Subject: [PATCH 06/11] Fix (potential?) memory leaks from getVariant() (SquirrelVM) Free values before assignment. --- sp/src/public/vscript/ivscript.h | 1 + sp/src/vscript/vscript_squirrel.cpp | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/sp/src/public/vscript/ivscript.h b/sp/src/public/vscript/ivscript.h index 7e4c1d48..0c8f9007 100644 --- a/sp/src/public/vscript/ivscript.h +++ b/sp/src/public/vscript/ivscript.h @@ -497,6 +497,7 @@ struct ScriptVariant_t bool AssignTo( ScriptVariant_t *pDest ) { + pDest->Free(); pDest->m_type = m_type; if ( m_flags & SV_FREE ) { diff --git a/sp/src/vscript/vscript_squirrel.cpp b/sp/src/vscript/vscript_squirrel.cpp index c17f3c64..02d057aa 100644 --- a/sp/src/vscript/vscript_squirrel.cpp +++ b/sp/src/vscript/vscript_squirrel.cpp @@ -1205,6 +1205,7 @@ bool getVariant(HSQUIRRELVM vm, SQInteger idx, ScriptVariant_t& variant) { case OT_NULL: { + variant.Free(); variant.m_flags = 0; // TODO: Should this be (HSCRIPT)nullptr variant.m_type = FIELD_VOID; @@ -1217,6 +1218,7 @@ bool getVariant(HSQUIRRELVM vm, SQInteger idx, ScriptVariant_t& variant) { return false; } + variant.Free(); variant = (int)val; return true; } @@ -1227,6 +1229,7 @@ bool getVariant(HSQUIRRELVM vm, SQInteger idx, ScriptVariant_t& variant) { return false; } + variant.Free(); variant = (float)val; return true; } @@ -1237,6 +1240,7 @@ bool getVariant(HSQUIRRELVM vm, SQInteger idx, ScriptVariant_t& variant) { return false; } + variant.Free(); variant = val ? true : false; return true; } @@ -1248,6 +1252,7 @@ bool getVariant(HSQUIRRELVM vm, SQInteger idx, ScriptVariant_t& variant) { return false; } + variant.Free(); char* buffer = (char*)malloc(size + 1); V_memcpy(buffer, val, size); buffer[size] = 0; @@ -1263,6 +1268,7 @@ bool getVariant(HSQUIRRELVM vm, SQInteger idx, ScriptVariant_t& variant) tag == TYPETAG_VECTOR && SQ_SUCCEEDED(sq_getinstanceup(vm, idx, (SQUserPointer*)&v, TYPETAG_VECTOR))) { + variant.Free(); variant = (Vector*)malloc(sizeof(Vector)); variant.EmplaceAllocedVector(*v); variant.m_flags |= SV_FREE; @@ -1272,6 +1278,7 @@ bool getVariant(HSQUIRRELVM vm, SQInteger idx, ScriptVariant_t& variant) } default: { + variant.Free(); HSQOBJECT* obj = new HSQOBJECT; sq_resetobject(obj); sq_getstackobj(vm, idx, obj); From cbb60e1afdd193695584ddac358c9eb65d67f0d4 Mon Sep 17 00:00:00 2001 From: Alexander 'z33ky' Hirsch <1zeeky@gmail.com> Date: Mon, 18 Nov 2024 14:52:38 +0100 Subject: [PATCH 07/11] Remove VScript Squrrel arithmetic operator helper They were broken, and only add for Quaternions was implemented, for which there is a working QuaternionAdd() script function instead. Fixing it seems like unnecessary work. --- sp/src/public/vscript/ivscript.h | 5 - sp/src/vscript/vscript_bindings_math.cpp | 17 ---- sp/src/vscript/vscript_bindings_math.h | 5 - sp/src/vscript/vscript_squirrel.cpp | 116 ----------------------- 4 files changed, 143 deletions(-) diff --git a/sp/src/public/vscript/ivscript.h b/sp/src/public/vscript/ivscript.h index 0c8f9007..950348bf 100644 --- a/sp/src/public/vscript/ivscript.h +++ b/sp/src/public/vscript/ivscript.h @@ -322,11 +322,6 @@ public: #ifdef MAPBASE_VSCRIPT virtual bool Get( void *p, const char *pszKey, ScriptVariant_t &variant ) { return false; } virtual bool Set( void *p, const char *pszKey, ScriptVariant_t &variant ) { return false; } - - virtual ScriptVariant_t *Add( void *p, ScriptVariant_t &variant ) { return NULL; } - virtual ScriptVariant_t *Subtract( void *p, ScriptVariant_t &variant ) { return NULL; } - virtual ScriptVariant_t *Multiply( void *p, ScriptVariant_t &variant ) { return NULL; } - virtual ScriptVariant_t *Divide( void *p, ScriptVariant_t &variant ) { return NULL; } #endif }; diff --git a/sp/src/vscript/vscript_bindings_math.cpp b/sp/src/vscript/vscript_bindings_math.cpp index cb1567d5..fb9c8334 100644 --- a/sp/src/vscript/vscript_bindings_math.cpp +++ b/sp/src/vscript/vscript_bindings_math.cpp @@ -275,23 +275,6 @@ bool CScriptQuaternionInstanceHelper::Set( void *p, const char *pszKey, ScriptVa return false; } -ScriptVariant_t *CScriptQuaternionInstanceHelper::Add( void *p, ScriptVariant_t &variant ) -{ - Quaternion *pQuat = ((Quaternion *)p); - - float flAdd; - variant.AssignTo( &flAdd ); - - (*pQuat)[0] += flAdd; - (*pQuat)[1] += flAdd; - (*pQuat)[2] += flAdd; - (*pQuat)[3] += flAdd; - - static ScriptVariant_t result; - result = (HSCRIPT)p; - return &result; -} - //----------------------------------------------------------------------------- //----------------------------------------------------------------------------- diff --git a/sp/src/vscript/vscript_bindings_math.h b/sp/src/vscript/vscript_bindings_math.h index c2d960b5..20039b61 100644 --- a/sp/src/vscript/vscript_bindings_math.h +++ b/sp/src/vscript/vscript_bindings_math.h @@ -40,11 +40,6 @@ class CScriptQuaternionInstanceHelper : public IScriptInstanceHelper bool Get( void *p, const char *pszKey, ScriptVariant_t &variant ); bool Set( void *p, const char *pszKey, ScriptVariant_t &variant ); - - ScriptVariant_t *Add( void *p, ScriptVariant_t &variant ); - //ScriptVariant_t *Subtract( void *p, ScriptVariant_t &variant ); - //ScriptVariant_t *Multiply( void *p, ScriptVariant_t &variant ); - //ScriptVariant_t *Divide( void *p, ScriptVariant_t &variant ); }; inline Quaternion *ToQuaternion( HSCRIPT hQuat ) { return HScriptToClass( hQuat ); } diff --git a/sp/src/vscript/vscript_squirrel.cpp b/sp/src/vscript/vscript_squirrel.cpp index 02d057aa..6eb9eab5 100644 --- a/sp/src/vscript/vscript_squirrel.cpp +++ b/sp/src/vscript/vscript_squirrel.cpp @@ -1578,106 +1578,6 @@ SQInteger set_stub(HSQUIRRELVM vm) return 0; } -SQInteger add_stub(HSQUIRRELVM vm) -{ - ClassInstanceData* classInstanceData = nullptr; - sq_getinstanceup(vm, 1, (SQUserPointer*)&classInstanceData, 0); - - ScriptVariant_t var; - getVariant( vm, 1, var ); - - if (classInstanceData && - classInstanceData->instance && - classInstanceData->desc->pHelper) - { - ScriptVariant_t *result = classInstanceData->desc->pHelper->Add( classInstanceData->instance, var ); - if (result != nullptr) - { - PushVariant( vm, *result ); - sq_pop(vm, 1); - return 1; - } - } - - sq_pop(vm, 1); - return sqstd_throwerrorf(vm, "invalid arith op +"); -} - -SQInteger sub_stub(HSQUIRRELVM vm) -{ - ClassInstanceData* classInstanceData = nullptr; - sq_getinstanceup(vm, 1, (SQUserPointer*)&classInstanceData, 0); - - ScriptVariant_t var; - getVariant( vm, 1, var ); - - if (classInstanceData && - classInstanceData->instance && - classInstanceData->desc->pHelper) - { - ScriptVariant_t *result = classInstanceData->desc->pHelper->Subtract( classInstanceData->instance, var ); - if (result != nullptr) - { - PushVariant( vm, *result ); - sq_pop(vm, 1); - return 1; - } - } - - sq_pop(vm, 1); - return sqstd_throwerrorf(vm, "invalid arith op -"); -} - -SQInteger mul_stub(HSQUIRRELVM vm) -{ - ClassInstanceData* classInstanceData = nullptr; - sq_getinstanceup(vm, 1, (SQUserPointer*)&classInstanceData, 0); - - ScriptVariant_t var; - getVariant( vm, 1, var ); - - if (classInstanceData && - classInstanceData->instance && - classInstanceData->desc->pHelper ) - { - ScriptVariant_t *result = classInstanceData->desc->pHelper->Add( classInstanceData->instance, var ); - if (result != nullptr) - { - PushVariant( vm, *result ); - sq_pop(vm, 1); - return 1; - } - } - - sq_pop(vm, 1); - return sqstd_throwerrorf(vm, "invalid arith op *"); -} - -SQInteger div_stub(HSQUIRRELVM vm) -{ - ClassInstanceData* classInstanceData = nullptr; - sq_getinstanceup(vm, 1, (SQUserPointer*)&classInstanceData, 0); - - ScriptVariant_t var; - getVariant( vm, 1, var ); - - if (classInstanceData && - classInstanceData->instance && - classInstanceData->desc->pHelper ) - { - ScriptVariant_t *result = classInstanceData->desc->pHelper->Add( classInstanceData->instance, var ); - if (result != nullptr) - { - PushVariant( vm, *result ); - sq_pop(vm, 1); - return 1; - } - } - - sq_pop(vm, 1); - return sqstd_throwerrorf(vm, "invalid arith op /"); -} - SQInteger IsValid_stub(HSQUIRRELVM vm) { ClassInstanceData* classInstanceData = nullptr; @@ -2511,22 +2411,6 @@ bool SquirrelVM::RegisterClass(ScriptClassDesc_t* pClassDesc) sq_newclosure(vm_, set_stub, 0); sq_newslot(vm_, -3, SQFalse); - sq_pushstring(vm_, "_add", -1); - sq_newclosure(vm_, add_stub, 0); - sq_newslot(vm_, -3, SQFalse); - - sq_pushstring(vm_, "_sub", -1); - sq_newclosure(vm_, sub_stub, 0); - sq_newslot(vm_, -3, SQFalse); - - sq_pushstring(vm_, "_mul", -1); - sq_newclosure(vm_, mul_stub, 0); - sq_newslot(vm_, -3, SQFalse); - - sq_pushstring(vm_, "_div", -1); - sq_newclosure(vm_, div_stub, 0); - sq_newslot(vm_, -3, SQFalse); - sq_pushstring(vm_, "IsValid", -1); sq_newclosure(vm_, IsValid_stub, 0); sq_newslot(vm_, -3, SQFalse); From c0e12a2f58fbc935ba043aaca76364a0b56eab0c Mon Sep 17 00:00:00 2001 From: Alexander 'z33ky' Hirsch <1zeeky@gmail.com> Date: Mon, 18 Nov 2024 15:03:09 +0100 Subject: [PATCH 08/11] Add type check for script helper assignments from ScriptVariant_t --- .../game/shared/mapbase/vscript_funcs_shared.cpp | 14 ++++++++------ sp/src/vscript/vscript_bindings_base.cpp | 6 ++---- sp/src/vscript/vscript_bindings_math.cpp | 12 ++++-------- 3 files changed, 14 insertions(+), 18 deletions(-) diff --git a/sp/src/game/shared/mapbase/vscript_funcs_shared.cpp b/sp/src/game/shared/mapbase/vscript_funcs_shared.cpp index 4c6d4817..8a36c7a0 100644 --- a/sp/src/game/shared/mapbase/vscript_funcs_shared.cpp +++ b/sp/src/game/shared/mapbase/vscript_funcs_shared.cpp @@ -560,16 +560,18 @@ bool CAnimEventTInstanceHelper::Set( void *p, const char *pszKey, ScriptVariant_ animevent_t *ani = ((animevent_t *)p); if (FStrEq( pszKey, "event" )) - ani->event = variant; + return variant.AssignTo( &ani->event ); else if (FStrEq( pszKey, "options" )) - ani->options = variant; + // broken: return variant.AssignTo( &ani->options ); + // variant memory is freed afterwards + return false; else if (FStrEq( pszKey, "cycle" )) - ani->cycle = variant; + return variant.AssignTo( &ani->cycle ); else if (FStrEq( pszKey, "eventtime" )) - ani->eventtime = variant; + return variant.AssignTo( &ani->eventtime ); else if (FStrEq( pszKey, "type" )) - ani->type = variant; - else if (FStrEq( pszKey, "source" )) + return variant.AssignTo( &ani->type ); + else if (FStrEq( pszKey, "source" ) && variant.m_type == FIELD_HSCRIPT) { CBaseEntity *pEnt = ToEnt( variant.m_hScript ); if (pEnt) diff --git a/sp/src/vscript/vscript_bindings_base.cpp b/sp/src/vscript/vscript_bindings_base.cpp index 99b6194a..e6ea396b 100644 --- a/sp/src/vscript/vscript_bindings_base.cpp +++ b/sp/src/vscript/vscript_bindings_base.cpp @@ -440,13 +440,11 @@ bool CScriptColorInstanceHelper::Get( void *p, const char *pszKey, ScriptVariant bool CScriptColorInstanceHelper::Set( void *p, const char *pszKey, ScriptVariant_t &variant ) { Color *pClr = ((Color *)p); - if ( strlen(pszKey) == 1 ) + int iVal; + if ( strlen(pszKey) == 1 && variant.AssignTo( &iVal ) ) { - int iVal; - variant.AssignTo( &iVal ); switch (pszKey[0]) { - // variant.AssignTo( &(*pClr)[0] ); case 'r': (*pClr)[0] = iVal; return true; diff --git a/sp/src/vscript/vscript_bindings_math.cpp b/sp/src/vscript/vscript_bindings_math.cpp index fb9c8334..3c83aeec 100644 --- a/sp/src/vscript/vscript_bindings_math.cpp +++ b/sp/src/vscript/vscript_bindings_math.cpp @@ -259,17 +259,13 @@ bool CScriptQuaternionInstanceHelper::Set( void *p, const char *pszKey, ScriptVa switch (pszKey[0]) { case 'x': - variant.AssignTo( &pQuat->x ); - return true; + return variant.AssignTo( &pQuat->x ); case 'y': - variant.AssignTo( &pQuat->y ); - return true; + return variant.AssignTo( &pQuat->y ); case 'z': - variant.AssignTo( &pQuat->z ); - return true; + return variant.AssignTo( &pQuat->z ); case 'w': - variant.AssignTo( &pQuat->w ); - return true; + return variant.AssignTo( &pQuat->w ); } } return false; From 1ecaa44c49f4ff054418e90e0a5d1a2dce078dd6 Mon Sep 17 00:00:00 2001 From: Alexander 'z33ky' Hirsch <1zeeky@gmail.com> Date: Mon, 18 Nov 2024 15:09:55 +0100 Subject: [PATCH 09/11] Release ScriptVariant_t memory in all paths of squirrel function stubs --- sp/src/vscript/vscript_squirrel.cpp | 59 +++++++++++++++++------------ 1 file changed, 34 insertions(+), 25 deletions(-) diff --git a/sp/src/vscript/vscript_squirrel.cpp b/sp/src/vscript/vscript_squirrel.cpp index 6eb9eab5..f8b2b6eb 100644 --- a/sp/src/vscript/vscript_squirrel.cpp +++ b/sp/src/vscript/vscript_squirrel.cpp @@ -1405,31 +1405,39 @@ SQInteger function_stub(HSQUIRRELVM vm) instance = ((ClassInstanceData*)self)->instance; } - ScriptVariant_t retval; - ScriptVariantTemporaryStorage_t retval_storage; + ScriptVariant_t script_retval; + ScriptVariantTemporaryStorage_t script_retval_storage; SquirrelVM* pSquirrelVM = (SquirrelVM*)sq_getforeignptr(vm); assert(pSquirrelVM); sq_resetobject(&pSquirrelVM->lastError_); - (*pFunc->m_pfnBinding)(pFunc->m_pFunction, instance, params.Base(), nargs, - pFunc->m_desc.m_ReturnType == FIELD_VOID ? nullptr : &retval, retval_storage); + bool call_success = (*pFunc->m_pfnBinding)(pFunc->m_pFunction, instance, params.Base(), nargs, + pFunc->m_desc.m_ReturnType == FIELD_VOID ? nullptr : &script_retval, script_retval_storage); + Assert(call_success); + (void)call_success; + SQInteger sq_retval; if (!sq_isnull(pSquirrelVM->lastError_)) { sq_pushobject(vm, pSquirrelVM->lastError_); sq_resetobject(&pSquirrelVM->lastError_); - return sq_throwobject(vm); + sq_retval = sq_throwobject(vm); } + else + { + Assert(script_retval.m_type == pFunc->m_desc.m_ReturnType); - PushVariant(vm, retval); + PushVariant(vm, script_retval); + sq_retval = 1; + } // strings never get copied here, Vector and QAngle are stored in script_retval_storage // everything else is stored inline, so there should be no memory to free - Assert(!(retval.m_flags & SV_FREE)); + Assert(!(script_retval.m_flags & SV_FREE)); - return pFunc->m_desc.m_ReturnType != FIELD_VOID; + return sq_retval; } @@ -1469,12 +1477,8 @@ SQInteger constructor_stub(HSQUIRRELVM vm) void* instance = pClassDesc->m_pfnConstruct(); - if (!sq_isnull(pSquirrelVM->lastError_)) - { - sq_pushobject(vm, pSquirrelVM->lastError_); - sq_resetobject(&pSquirrelVM->lastError_); - return sq_throwobject(vm); - } + // expect construction to always succeed + Assert(sq_isnull(pSquirrelVM->lastError_)); { SQUserPointer p; @@ -1531,19 +1535,22 @@ SQInteger get_stub(HSQUIRRELVM vm) } ScriptVariant_t var; + SQInteger sq_retval = 0; if (classInstanceData && classInstanceData->instance && classInstanceData->desc->pHelper && classInstanceData->desc->pHelper->Get(classInstanceData->instance, key, var)) { PushVariant(vm, var); + sq_retval = 1; } else { - return sqstd_throwerrorf(vm, "the index '%.50s' does not exist", key); + sq_retval = sqstd_throwerrorf(vm, "the index '%.50s' does not exist", key); } - return 1; + var.Free(); + return sq_retval; } SQInteger set_stub(HSQUIRRELVM vm) @@ -1560,22 +1567,22 @@ SQInteger set_stub(HSQUIRRELVM vm) } ScriptVariant_t var; + SQInteger sq_retval = 0; getVariant( vm, -1, var ); - if (classInstanceData && + if (!( + classInstanceData && classInstanceData->instance && classInstanceData->desc->pHelper && - classInstanceData->desc->pHelper->Set(classInstanceData->instance, key, var)) + classInstanceData->desc->pHelper->Set(classInstanceData->instance, key, var) + )) { - sq_pop(vm, 1); - } - else - { - sq_pop(vm, 1); - return sqstd_throwerrorf(vm, "the index '%.50s' does not exist", key); + sq_retval = sqstd_throwerrorf(vm, "the index '%.50s' does not exist", key); } - return 0; + var.Free(); + sq_pop(vm, 1); + return sq_retval; } SQInteger IsValid_stub(HSQUIRRELVM vm) @@ -2326,6 +2333,7 @@ void SquirrelVM::RegisterFunction(ScriptFunctionBinding_t* pScriptFunction) return; char typemask[64]; + Assert(pScriptFunction->m_desc.m_Parameters.Count() < sizeof(typemask)); if (!CreateParamCheck(*pScriptFunction, typemask)) { return; @@ -2429,6 +2437,7 @@ bool SquirrelVM::RegisterClass(ScriptClassDesc_t* pClassDesc) auto& scriptFunction = pClassDesc->m_FunctionBindings[i]; char typemask[64]; + Assert(scriptFunction.m_desc.m_Parameters.Count() < sizeof(typemask)); if (!CreateParamCheck(scriptFunction, typemask)) { Warning("Unable to create param check for %s.%s\n", From 8e8bdfc37125fcbcf44cd024b1b483f742a99f77 Mon Sep 17 00:00:00 2001 From: Alexander 'z33ky' Hirsch <1zeeky@gmail.com> Date: Mon, 18 Nov 2024 15:11:43 +0100 Subject: [PATCH 10/11] Optimize void return in squirrel function call stub --- sp/src/vscript/vscript_squirrel.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/sp/src/vscript/vscript_squirrel.cpp b/sp/src/vscript/vscript_squirrel.cpp index f8b2b6eb..48b2ff31 100644 --- a/sp/src/vscript/vscript_squirrel.cpp +++ b/sp/src/vscript/vscript_squirrel.cpp @@ -1429,8 +1429,15 @@ SQInteger function_stub(HSQUIRRELVM vm) { Assert(script_retval.m_type == pFunc->m_desc.m_ReturnType); - PushVariant(vm, script_retval); - sq_retval = 1; + if (pFunc->m_desc.m_ReturnType != FIELD_VOID) + { + PushVariant(vm, script_retval); + sq_retval = 1; + } + else + { + sq_retval = 0; + } } // strings never get copied here, Vector and QAngle are stored in script_retval_storage From 7d78384b269aeb640985dbc331161dd5d613e17f Mon Sep 17 00:00:00 2001 From: Alexander 'z33ky' Hirsch <1zeeky@gmail.com> Date: Tue, 19 Nov 2024 13:17:48 +0100 Subject: [PATCH 11/11] Fix scriptanimevent_t::SetOption() The string argument refers to memory managed by the Squirrel VM. It has no guarantee that the string will continue to exist after the hook, from which this can be called, has finished executing. To fix this, allocate memory to copy the string into and manage it as needed. --- sp/src/game/server/BaseAnimatingOverlay.cpp | 7 ++- sp/src/game/server/baseanimating.cpp | 7 +-- sp/src/game/server/baseanimating.h | 5 ++- .../shared/mapbase/vscript_funcs_shared.cpp | 18 +++++--- .../shared/mapbase/vscript_funcs_shared.h | 43 +++++++++++++------ 5 files changed, 57 insertions(+), 23 deletions(-) diff --git a/sp/src/game/server/BaseAnimatingOverlay.cpp b/sp/src/game/server/BaseAnimatingOverlay.cpp index 06bf690e..d399d511 100644 --- a/sp/src/game/server/BaseAnimatingOverlay.cpp +++ b/sp/src/game/server/BaseAnimatingOverlay.cpp @@ -16,6 +16,10 @@ #include "saverestore_utlvector.h" #include "dt_utlvector_send.h" +#ifdef MAPBASE_VSCRIPT +#include "mapbase/vscript_funcs_shared.h" +#endif + // memdbgon must be the last include file in a .cpp file!!! #include "tier0/memdbgon.h" @@ -471,7 +475,8 @@ void CAnimationLayer::DispatchAnimEvents( CBaseAnimating *eventHandler, CBaseAni } #ifdef MAPBASE_VSCRIPT - if (eventHandler->m_ScriptScope.IsInitialized() && eventHandler->ScriptHookHandleAnimEvent( &event ) == false) + scriptanimevent_t wrapper( event ); + if (eventHandler->m_ScriptScope.IsInitialized() && !eventHandler->ScriptHookHandleAnimEvent( wrapper )) continue; #endif diff --git a/sp/src/game/server/baseanimating.cpp b/sp/src/game/server/baseanimating.cpp index 0e226209..86fcb1dc 100644 --- a/sp/src/game/server/baseanimating.cpp +++ b/sp/src/game/server/baseanimating.cpp @@ -1261,7 +1261,8 @@ void CBaseAnimating::DispatchAnimEvents ( CBaseAnimating *eventHandler ) } #ifdef MAPBASE_VSCRIPT - if (eventHandler->ScriptHookHandleAnimEvent( &event ) == false) + scriptanimevent_t wrapper( event ); + if (!eventHandler->ScriptHookHandleAnimEvent( wrapper )) continue; #endif @@ -1299,11 +1300,11 @@ void CBaseAnimating::DispatchAnimEvents ( CBaseAnimating *eventHandler ) //----------------------------------------------------------------------------- // Purpose: //----------------------------------------------------------------------------- -bool CBaseAnimating::ScriptHookHandleAnimEvent( animevent_t *pEvent ) +bool CBaseAnimating::ScriptHookHandleAnimEvent( scriptanimevent_t &event ) { if (m_ScriptScope.IsInitialized() && g_Hook_HandleAnimEvent.CanRunInScope(m_ScriptScope)) { - HSCRIPT hEvent = g_pScriptVM->RegisterInstance( reinterpret_cast(pEvent) ); + HSCRIPT hEvent = g_pScriptVM->RegisterInstance( &event ); // event ScriptVariant_t args[] = { hEvent }; diff --git a/sp/src/game/server/baseanimating.h b/sp/src/game/server/baseanimating.h index 4fb3c47c..3d18c3cb 100644 --- a/sp/src/game/server/baseanimating.h +++ b/sp/src/game/server/baseanimating.h @@ -16,6 +16,9 @@ #include "datacache/idatacache.h" #include "tier0/threadtools.h" +#ifdef MAPBASE_VSCRIPT +struct scriptanimevent_t; +#endif struct animevent_t; struct matrix3x4_t; @@ -146,7 +149,7 @@ public: virtual void DispatchAnimEvents ( CBaseAnimating *eventHandler ); // Handle events that have happend since last time called up until X seconds into the future virtual void HandleAnimEvent( animevent_t *pEvent ); #ifdef MAPBASE_VSCRIPT - bool ScriptHookHandleAnimEvent( animevent_t *pEvent ); + bool ScriptHookHandleAnimEvent( scriptanimevent_t &event ); #endif int LookupPoseParameter( CStudioHdr *pStudioHdr, const char *szName ); diff --git a/sp/src/game/shared/mapbase/vscript_funcs_shared.cpp b/sp/src/game/shared/mapbase/vscript_funcs_shared.cpp index 8a36c7a0..e77cfd5a 100644 --- a/sp/src/game/shared/mapbase/vscript_funcs_shared.cpp +++ b/sp/src/game/shared/mapbase/vscript_funcs_shared.cpp @@ -535,7 +535,7 @@ bool CAnimEventTInstanceHelper::Get( void *p, const char *pszKey, ScriptVariant_ { DevWarning( "VScript animevent_t.%s: animevent_t metamethod members are deprecated! Use 'script_help animevent_t' to see the correct functions.\n", pszKey ); - animevent_t *ani = ((animevent_t *)p); + animevent_t *ani = &((scriptanimevent_t *)p)->event; if (FStrEq( pszKey, "event" )) variant = ani->event; else if (FStrEq( pszKey, "options" )) @@ -558,13 +558,21 @@ bool CAnimEventTInstanceHelper::Set( void *p, const char *pszKey, ScriptVariant_ { DevWarning( "VScript animevent_t.%s: animevent_t metamethod members are deprecated! Use 'script_help animevent_t' to see the correct functions.\n", pszKey ); - animevent_t *ani = ((animevent_t *)p); + scriptanimevent_t *script_ani = ((scriptanimevent_t *)p); + animevent_t *ani = &script_ani->event; if (FStrEq( pszKey, "event" )) + { return variant.AssignTo( &ani->event ); + } else if (FStrEq( pszKey, "options" )) - // broken: return variant.AssignTo( &ani->options ); - // variant memory is freed afterwards - return false; + { + char *szOptions; + if (!variant.AssignTo( &szOptions )) + { + return false; + } + script_ani->SetOptions( szOptions ); + } else if (FStrEq( pszKey, "cycle" )) return variant.AssignTo( &ani->cycle ); else if (FStrEq( pszKey, "eventtime" )) diff --git a/sp/src/game/shared/mapbase/vscript_funcs_shared.h b/sp/src/game/shared/mapbase/vscript_funcs_shared.h index bcf91741..e525d682 100644 --- a/sp/src/game/shared/mapbase/vscript_funcs_shared.h +++ b/sp/src/game/shared/mapbase/vscript_funcs_shared.h @@ -172,30 +172,47 @@ private: //----------------------------------------------------------------------------- // Exposes animevent_t to VScript //----------------------------------------------------------------------------- -struct scriptanimevent_t : public animevent_t +struct scriptanimevent_t { - int GetEvent() { return event; } - void SetEvent( int nEvent ) { event = nEvent; } + friend class CAnimEventTInstanceHelper; - const char *GetOptions() { return options; } - void SetOptions( const char *pOptions ) { options = pOptions; } +public: + scriptanimevent_t( animevent_t &event ) : event( event ), options( NULL ) { } + ~scriptanimevent_t( ) { delete[] options; } - float GetCycle() { return cycle; } - void SetCycle( float flCycle ) { cycle = flCycle; } + int GetEvent() { return event.event; } + void SetEvent( int nEvent ) { event.event = nEvent; } - float GetEventTime() { return eventtime; } - void SetEventTime( float flEventTime ) { eventtime = flEventTime; } + const char *GetOptions() { return event.options; } + void SetOptions( const char *pOptions ) + { + size_t len = strlen( pOptions ); + delete[] options; + event.options = options = new char[len + 1]; + memcpy( options, pOptions, len + 1 ); + } - int GetType() { return type; } - void SetType( int nType ) { eventtime = type; } + float GetCycle() { return event.cycle; } + void SetCycle( float flCycle ) { event.cycle = flCycle; } - HSCRIPT GetSource() { return ToHScript( pSource ); } + float GetEventTime() { return event.eventtime; } + void SetEventTime( float flEventTime ) { event.eventtime = flEventTime; } + + int GetType() { return event.type; } + void SetType( int nType ) { event.type = nType; } + + HSCRIPT GetSource() { return ToHScript( event.pSource ); } void SetSource( HSCRIPT hSource ) { CBaseEntity *pEnt = ToEnt( hSource ); if (pEnt) - pSource = pEnt->GetBaseAnimating(); + event.pSource = pEnt->GetBaseAnimating(); } + +private: + animevent_t &event; + // storage for ScriptVariant_t string, which may be temporary + char *options; }; class CAnimEventTInstanceHelper : public IScriptInstanceHelper