Skip to content

VScript member function call safety #436

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Blixibon merged 11 commits into from
Jul 24, 2025
Merged

VScript member function call safety #436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c851fc9
Ensure the squirrel instance for native member function call actually…
z33ky Jun 11, 2025
681a75a
Check ScriptClassDesc_t of squirrel instance for native member functi…
z33ky Jun 11, 2025
655679e
Check type of SQVector construction instance
z33ky Jun 23, 2025
ca7bc5d
Guard Squirrel constructor_stub() invocations from invalid class para…
z33ky Jun 23, 2025
9c494b6
fxp streaml
z33ky Jul 1, 2025
9c740a8
Check type of Squirrel constructor_stub() instance
z33ky Jun 23, 2025
503fdd2
Fix SQVector member access check
z33ky Jun 24, 2025
be3ad93
Remove redundant typetag check from vscript_squirrel getVariant()
z33ky Jun 24, 2025
9c38e4e
Remove redundant sq_resetobject() for SquirrelVM::lastError_
z33ky Jun 24, 2025
3301edc
Guard get/set/tostring_stub() against illegal Squirrel instances
z33ky Jun 24, 2025
c659af5
Release SquirrelVM::lastError_ after pushing it to the VM stack
z33ky Jun 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions sp/src/public/vscript/ivscript.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,18 +263,22 @@ inline const char * ScriptFieldTypeName( int16 eType)

//---------------------------------------------------------

struct ScriptClassDesc_t;

struct ScriptFuncDescriptor_t
{
ScriptFuncDescriptor_t()
{
m_pszFunction = NULL;
m_ReturnType = FIELD_TYPEUNKNOWN;
m_pszDescription = NULL;
m_pScriptClassDesc = NULL;
}

const char *m_pszScriptName;
const char *m_pszFunction;
const char *m_pszDescription;
ScriptClassDesc_t *m_pScriptClassDesc;
ScriptDataType_t m_ReturnType;
CUtlVector<ScriptDataType_t> m_Parameters;
};
Expand Down
2 changes: 1 addition & 1 deletion sp/src/public/vscript/vscript_templates.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ FUNC_GENERATE_ALL( DEFINE_MEMBER_FUNC_TYPE_DEDUCER );

FUNC_GENERATE_ALL( DEFINE_CONST_MEMBER_FUNC_TYPE_DEDUCER );

#define ScriptInitMemberFuncDescriptor_( pDesc, class, func, scriptName ) if ( 0 ) {} else { (pDesc)->m_pszScriptName = scriptName; (pDesc)->m_pszFunction = #func; ScriptDeduceFunctionSignature( pDesc, (class *)(0), &class::func ); }
#define ScriptInitMemberFuncDescriptor_( pDesc, class, func, scriptName ) if ( 0 ) {} else { (pDesc)->m_pszScriptName = scriptName; (pDesc)->m_pszFunction = #func; ScriptDeduceFunctionSignature( pDesc, (class *)(0), &class::func ); (pDesc)->m_pScriptClassDesc = GetScriptDesc<class>(nullptr); }

#define ScriptInitFuncDescriptorNamed( pDesc, func, scriptName ) if ( 0 ) {} else { (pDesc)->m_pszScriptName = scriptName; (pDesc)->m_pszFunction = #func; ScriptDeduceFunctionSignature( pDesc, &func ); }
#define ScriptInitFuncDescriptor( pDesc, func ) ScriptInitFuncDescriptorNamed( pDesc, func, #func )
Expand Down
112 changes: 75 additions & 37 deletions sp/src/vscript/vscript_squirrel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,16 @@ namespace SQVector
}

SQUserPointer p;
sq_getinstanceup(vm, 1, &p, 0);
if (SQ_FAILED(sq_getinstanceup(vm, 1, &p, 0)))
{
return SQ_ERROR;
}

if (!p)
{
return sq_throwerror(vm, "Accessed null instance");
}

new (p) Vector(x, y, z);

return 0;
Expand All @@ -343,7 +352,7 @@ namespace SQVector
return sq_throwerror(vm, "Expected Vector._get(string)");
}

if (key[0] < 'x' || key['0'] > 'z' || key[1] != '\0')
if (key[0] < 'x' || key[0] > 'z' || key[1] != '\0')
{
return sqstd_throwerrorf(vm, "the index '%.50s' does not exist", key);
}
Expand All @@ -369,7 +378,7 @@ namespace SQVector
return sq_throwerror(vm, "Expected Vector._set(string)");
}

if (key[0] < 'x' || key['0'] > 'z' || key[1] != '\0')
if (key[0] < 'x' || key[0] > 'z' || key[1] != '\0')
{
return sqstd_throwerrorf(vm, "the index '%.50s' does not exist", key);
}
Expand Down Expand Up @@ -1291,10 +1300,7 @@ bool getVariant(HSQUIRRELVM vm, SQInteger idx, ScriptVariant_t& variant)
case OT_INSTANCE:
{
Vector* v = nullptr;
SQUserPointer tag;
if (SQ_SUCCEEDED(sq_gettypetag(vm, idx, &tag)) &&
tag == TYPETAG_VECTOR &&
SQ_SUCCEEDED(sq_getinstanceup(vm, idx, (SQUserPointer*)&v, TYPETAG_VECTOR)))
if (SQ_SUCCEEDED(sq_getinstanceup(vm, idx, (SQUserPointer*)&v, TYPETAG_VECTOR)))
{
variant.Free();
variant = (Vector*)malloc(sizeof(Vector));
Expand Down Expand Up @@ -1323,12 +1329,10 @@ SQInteger function_stub(HSQUIRRELVM vm)
{
SQInteger top = sq_gettop(vm);

SQUserPointer userptr = nullptr;
sq_getuserpointer(vm, top, &userptr);

Assert(userptr);
ScriptFunctionBinding_t* pFunc = nullptr;
sq_getuserpointer(vm, top, (SQUserPointer*)&pFunc);

ScriptFunctionBinding_t* pFunc = (ScriptFunctionBinding_t*)userptr;
Assert(pFunc);

int nargs = pFunc->m_desc.m_Parameters.Count();
int nLastHScriptIdx = -1;
Expand Down Expand Up @@ -1424,15 +1428,30 @@ SQInteger function_stub(HSQUIRRELVM vm)

if (pFunc->m_flags & SF_MEMBER_FUNC)
{
SQUserPointer self;
sq_getinstanceup(vm, 1, &self, nullptr);
ClassInstanceData* classInstanceData;
if (SQ_FAILED(sq_getinstanceup(vm, 1, (SQUserPointer*)&classInstanceData, 0)))
{
return SQ_ERROR;
}

if (!self)
if (!classInstanceData)
{
return sq_throwerror(vm, "Accessed null instance");
}

instance = ((ClassInstanceData*)self)->instance;
// check that the type of self, or any basetype, matches the function description
ScriptClassDesc_t *selfType = classInstanceData->desc;
while (selfType != pFunc->m_desc.m_pScriptClassDesc)
{
if (!selfType)
{
return sq_throwerror(vm, "Mismatched instance type");
}
selfType = selfType->m_pBaseDesc;
Assert(selfType != classInstanceData->desc); // there should be no infinite loop
}

instance = classInstanceData->instance;
}

ScriptVariant_t script_retval;
Expand All @@ -1441,8 +1460,6 @@ SQInteger function_stub(HSQUIRRELVM vm)
SquirrelVM* pSquirrelVM = (SquirrelVM*)sq_getsharedforeignptr(vm);
Assert(pSquirrelVM);

sq_resetobject(&pSquirrelVM->lastError_);

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);
Expand All @@ -1452,6 +1469,7 @@ SQInteger function_stub(HSQUIRRELVM vm)
if (!sq_isnull(pSquirrelVM->lastError_))
{
sq_pushobject(vm, pSquirrelVM->lastError_);
sq_release(vm, &pSquirrelVM->lastError_);
sq_resetobject(&pSquirrelVM->lastError_);
sq_retval = sq_throwobject(vm);
}
Expand Down Expand Up @@ -1521,28 +1539,42 @@ SQInteger destructor_stub_instance(SQUserPointer p, SQInteger size)
SQInteger constructor_stub(HSQUIRRELVM vm)
{
ScriptClassDesc_t* pClassDesc = nullptr;
sq_gettypetag(vm, 1, (SQUserPointer*)&pClassDesc);
if (SQ_FAILED(sq_gettypetag(vm, 1, (SQUserPointer*)&pClassDesc)))
{
return sq_throwerror(vm, "Expected native class");
}

if (!pClassDesc || (void*)pClassDesc == TYPETAG_VECTOR)
{
return sq_throwerror(vm, "Unable to obtain native class description");
}

if (!pClassDesc->m_pfnConstruct)
{
return sqstd_throwerrorf(vm, "Unable to construct instances of %s", pClassDesc->m_pszScriptName);
}

SquirrelVM* pSquirrelVM = (SquirrelVM*)sq_getsharedforeignptr(vm);
Assert(pSquirrelVM);
SQUserPointer p;
if (SQ_FAILED(sq_getinstanceup(vm, 1, &p, 0)))
{
return SQ_ERROR;
}

sq_resetobject(&pSquirrelVM->lastError_);
if (!p)
{
return sq_throwerror(vm, "Accessed null instance");
}

void* instance = pClassDesc->m_pfnConstruct();

#ifdef DBGFLAG_ASSERT
SquirrelVM* pSquirrelVM = (SquirrelVM*)sq_getsharedforeignptr(vm);
Assert(pSquirrelVM);
// expect construction to always succeed
Assert(sq_isnull(pSquirrelVM->lastError_));
Copy link
Author

@z33ky z33ky Jun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this still need a sq_resetobject(&pSquirrelVM->lastError_); before pClassDesc->m_pfnConstruct()? I don't seem to have issues without it though. I guess sq_release() also nulls the object?

Copy link

@samisalreadytaken samisalreadytaken Jun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in here, we previously established that constructors don't raise script exceptions; it needs to be reset after returning it to the script on function_stub (I guess I wasn't clear on that). sq_release doesn't nullify the input, RaiseException resetting lastError_ is irrelevant

lastError_ is a temporary object that is only meant to carry its state from within m_pfnBinding() call to function_stub() body.

if lastError_ is not null
	sq_pushobject lastError_
	sq_release lastError_
	sq_resetobject lastError_

This is the same logic of returning the result of RegisterInstance and HSCRIPT_RC if you recall that.

#endif

{
SQUserPointer p;
sq_getinstanceup(vm, 1, &p, 0);
new(p) ClassInstanceData(instance, pClassDesc, nullptr, true);
}
new(p) ClassInstanceData(instance, pClassDesc, nullptr, true);

sq_setreleasehook(vm, 1, &destructor_stub);

Expand All @@ -1552,7 +1584,10 @@ SQInteger constructor_stub(HSQUIRRELVM vm)
SQInteger tostring_stub(HSQUIRRELVM vm)
{
ClassInstanceData* classInstanceData = nullptr;
sq_getinstanceup(vm, 1, (SQUserPointer*)&classInstanceData, 0);
if (SQ_FAILED(sq_getinstanceup(vm, 1, (SQUserPointer*)&classInstanceData, 0)))
{
return SQ_ERROR;
}

char buffer[128] = "";

Expand Down Expand Up @@ -1582,7 +1617,10 @@ SQInteger tostring_stub(HSQUIRRELVM vm)
SQInteger get_stub(HSQUIRRELVM vm)
{
ClassInstanceData* classInstanceData = nullptr;
sq_getinstanceup(vm, 1, (SQUserPointer*)&classInstanceData, 0);
if (SQ_FAILED(sq_getinstanceup(vm, 1, (SQUserPointer*)&classInstanceData, 0)))
{
return SQ_ERROR;
}

const char* key = nullptr;
sq_getstring(vm, 2, &key);
Expand Down Expand Up @@ -1614,7 +1652,10 @@ SQInteger get_stub(HSQUIRRELVM vm)
SQInteger set_stub(HSQUIRRELVM vm)
{
ClassInstanceData* classInstanceData = nullptr;
sq_getinstanceup(vm, 1, (SQUserPointer*)&classInstanceData, 0);
if (SQ_FAILED(sq_getinstanceup(vm, 1, (SQUserPointer*)&classInstanceData, 0)))
{
return SQ_ERROR;
}

const char* key = nullptr;
sq_getstring(vm, 2, &key);
Expand Down Expand Up @@ -2710,10 +2751,8 @@ void SquirrelVM::SetInstanceUniqeId(HSCRIPT hInstance, const char* pszId)
HSQOBJECT* obj = (HSQOBJECT*)hInstance;
sq_pushobject(vm_, *obj);

SQUserPointer self;
sq_getinstanceup(vm_, -1, &self, nullptr);

auto classInstanceData = (ClassInstanceData*)self;
ClassInstanceData* classInstanceData;
sq_getinstanceup(vm_, -1, (SQUserPointer*)&classInstanceData, nullptr);

classInstanceData->instanceId = pszId;

Expand Down Expand Up @@ -2771,11 +2810,10 @@ void* SquirrelVM::GetInstanceValue(HSCRIPT hInstance, ScriptClassDesc_t* pExpect
}

sq_pushobject(vm_, *obj);
SQUserPointer self;
sq_getinstanceup(vm_, -1, &self, nullptr);
ClassInstanceData* classInstanceData;
sq_getinstanceup(vm_, -1, (SQUserPointer*)&classInstanceData, nullptr);
sq_pop(vm_, 1);

auto classInstanceData = (ClassInstanceData*)self;

if (!classInstanceData)
{
Expand Down