VScript member function call safety

[z33ky]

This inserts some checks on the script instance for which to invoke a native member function on.

The first commit prevents this from crashing(/corrupting) the game:

CBaseEntity.BreakIt <- CBaseEntity.GetClassname.bindenv(this)
local ent = Entities.FindByName(null,"*")
ent.BreakIt()

and the second commit, this

CBaseEntity.BreakIt <- CScriptKeyValues.FindOrCreateKey
local ent = Entities.FindByName(null,"*")
ent.BreakIt("")

---

PR Checklist

• My PR follows all guidelines in the CONTRIBUTING.md file
• My PR targets a develop branch OR targets another branch with a specific goal in mind

[samisalreadytaken]

A simpler test code would be:

access uninitialised stack variable self - sq_getinstanceup fails but self is not null

CBaseEntity.GetHealth.call( null )

interpret CHL2_Player* as CTriggerCamera* - no crash because offsets are valid

CTriggerCamera.GetFov.call( player )

crash via invalid offset

CScriptKeyValues.FindOrCreateKey.call( player, "" )

The results of sq_getinstanceup and sq_gettypetag are also unchecked in SQVector::Construct and constructor_stub.

access uninitialised stack variable

Vector.constructor.call( null )

crash via null ptr deref

class C{}
CBaseEntity.constructor.call( C() )

crash via invalid offset

CBaseEntity.constructor.call( Vector() )

get/set/tostring_stub are only accessible through the squirrel debugger, not through code.

[z33ky]

I'm afk for the rest of the week. I'd amend the constructor(_stub) when I'm back, or you can merge the current branch and do it yourself as well if you don't want to wait. I wasn't aware of the .call() syntax. I think I've looked at the construction functions, but thought it would't be possible to misuse.

[z33ky]

I tested that all of the provided scripts throw an error (and none cause a crash).
I'm not 100% confident in the commit descriptions, it's some guess work limited by my marginal understanding of the Squirrel VM.

[z33ky]

The latest changes also fix CBaseEntity.GetHealth.call( Vector() ) and

class C{}
Vector.constructor.call( C() )

[samisalreadytaken]

"Expected class userpointer" error doesn't really make sense, the data is attached to the instance. sq_getinstanceup already sets the error message, I feel like we could just let that pass and return SQ_ERROR. "invalid type tag" error is as vague as "Expected userpointer" to the end user anyway.

The way to access get/set/tostring_stub through the debugger is to view class metamethods, copy the function address, then call or assign it from variable watches (*0x00000000).call(null). Exploiting this requires a malicious client to connect locally or through an open port to the debugger which the user had to have enabled themselves. It doesn't really hurt to check for them too though.

Somewhat related issues I just noticed that you could fix here if you want to:

sq_resetobject(&pSquirrelVM->lastError_) lines in constructor_stub and function_stub can be removed since that object can only change through SquirrelVM::RaiseException which should only be called from function_stub binding call m_pfnBinding() where lastError_ is reset if it was changed.

The error message is also leaked; sq_release should be called on lastError_ after pushing it to the stack on if (!sq_isnull(pSquirrelVM->lastError_)) check in function_stub.

In getVariant() OT_INSTANCE case, sq_gettypetag check can be removed because it is only used for TYPETAG_VECTOR validation which is already done by sq_getinstanceup.

An irrelevant issue, SQVector::{_get, _set} are accessing out of bounds at index 48 with key['0']. I remember seeing this years ago but I forgot about it.

[samisalreadytaken]

This check isn't needed, it is guaranteed to be a userpointer from SQVM::CallNative due to closure registration sq_newclosure(vm_, function_stub, 1);

This line also could be simplified by removing userptr variable: sq_getuserpointer(vm, top, (SQUserPointer*)&pFunc);

[samisalreadytaken]

sq_getsharedforeignptr line should be after m_pfnConstruct() line because it is only used for lastError_ assertion. This extra block indentation could also be removed, it's a bit out of place in this small function.

[samisalreadytaken]

This could also be ClassInstanceData *self; and sq_getinstanceup(vm, 1, (SQUserPointer*)&self, 0)

Having known types makes it easier to inspect in debuggers.

[z33ky]

Thanks for the review @samisalreadytaken. I checked off all your suggestions (the 6 points in your overview comment and the 3 inline comments).

[z33ky]

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?

[samisalreadytaken]

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.

[samisalreadytaken]

Looks like 2 changes were amended to the wrong commits (sq_getuserpointer line at b578bf4, indentation at 9c2f8da), and the description of "RaiseException resetting lastError" is irrelevant, but those are not important