[VSCRIPT] Memory leak on native function calls #123

samisalreadytaken · 2021-05-10T13:56:43Z

Describe the bug

HSCRIPT parameters passed to native functions are not automatically released.

Steps to reproduce

Watch the memory usage after calling functions with HSCRIPT parameters. The best example is an entity input.

// leak
for ( local i = 1000000; i--; )
	player.SetOwner( player )

But functions where the input values are manually freed are fine.

// no leak
for ( local i = 1000000; i--; )
	player.SetContextThink( "", function(...){ print("newfunc\n") }, 0.01 )
	// Convars.RegisterCommand( "slot1", function(...){ print("newf\n") }, "", 0 )

Additional context

It is possible to fix this like so:

@@ -1249,6 +1252,7 @@ SQInteger function_stub(HSQUIRRELVM vm)
                return sq_throwerror(vm, "Invalid number of parameters");
        }

+       bool bFreeParams = false;
        CUtlVector<ScriptVariant_t> params;
        params.SetCount(nargs);

@@ -1320,6 +1324,8 @@ SQInteger function_stub(HSQUIRRELVM vm)
                                *pObject = val;
                                sq_addref(vm, pObject);
                                params[i] = (HSCRIPT)pObject;
+                               params[i].m_flags |= SV_FREE;
+                               bFreeParams = true;
                        }
                        break;
                }
@@ -1354,6 +1360,14 @@ SQInteger function_stub(HSQUIRRELVM vm)
        (*pFunc->m_pfnBinding)(pFunc->m_pFunction, instance, params.Base(), nargs,
                pFunc->m_desc.m_ReturnType == FIELD_VOID ? nullptr : &retval);

+       if ( bFreeParams )
+       {
+               for ( int i = 0; i < params.Count(); ++i )
+               {
+                       params[i].Free();
+               }
+       }
+
        if (!sq_isnull(pSquirrelVM->lastError_))
        {
                sq_pushobject(vm, pSquirrelVM->lastError_);

but fixing this will break functionality that depend on caching HSCRIPT inputs (game event listeners, entity thinking, concommands). So this should not be fixed on its own without also changing how those are cached. Simply incrementing the ref count with sq_addref does not work.

A related invalid script-function-return-value leak exists in getVariant(), and it can be fixed in a similiar fashion (but with no complications like the previous one). While this fixes the underlying problem, to actually fix the leaks all script function calls with return values in C++ need to call ScriptVariant_t::Free() to free the invalid return values (string, vector, hscript).

This one exists in Valve's games as well. For example in CBaseEntity::ScriptThink() you can see the return value is not freed.

// leak
player.SetContextThink( "0", function(self) { return self }, 0 )

// no leak
player.SetContextThink( "1", function(self) { return Vector() }, 0 )

@@ -1223,6 +1226,7 @@ bool getVariant(HSQUIRRELVM vm, SQInteger idx, ScriptVariant_t& variant)
                sq_getstackobj(vm, idx, obj);
                sq_addref(vm, obj);
                variant = (HSCRIPT)obj;
+               variant.m_flags |= SV_FREE;
        }
        };

In both of these situations ref counts are incremented and SV_FREE flag is not set. I don't think adding ref does anything here either, everything seems to work fine without it. Am I missing something here, or is this design intended?

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VSCRIPT] Memory leak on native function calls #123

[VSCRIPT] Memory leak on native function calls #123

samisalreadytaken commented May 10, 2021

[VSCRIPT] Memory leak on native function calls #123

[VSCRIPT] Memory leak on native function calls #123

Comments

samisalreadytaken commented May 10, 2021

Describe the bug

Steps to reproduce

Additional context