Crash due to ConVar overlap and release. #421 #430

Activity

1. CookStar

   added a commit that references this issue on Oct 30, 2021

   Fixed #429.

   aa6f457
2. Ayuto commented on Oct 31, 2021

   

   Ayuto

   on Oct 31, 2021

   Member

   More actions

   Sorry, for joning the discussion so late, but here is my opinion on this topic.

   Deleting ConVars:

   The issue we have with deleting ConVars is that we don't have a reference counter. With a reference counter we could easily keep track whether or not a ConVar can be deleted, but without one we have a little problem. We could implement our own reference counter for ConVars we created on our own, but that would only work in a world where we are the only VSP. This isn't the case, obviously. So, we should think about a different solution as I don't want to have any avoidable incompatibilities with SM. And the only solution I can think of (currently) is not deleting any ConVars. This isn't the best solution, but it's not like we have a constant memory leak that grows with every function call. Once the ConVar is created the memory is reserved and it's not going to grow further. This will never cause a server to reserve all available RAM, which then results in a crash due to unsufficient memory. I wouldn't even delete the ConVar if SP is being unloaded, because we don't know if there is another VSP or SM plugin that relies on the same ConVar (e. g. because of an unfortune coincidence or because the foreign plugin extended an SP plugin).

   ConCommands overriding ConVars

   I'm not really sure about this one. I would probably raise an exception if someone tries to create a ConCommand with the same name like a ConVar or vice-versa. This has two simple reasons:

   1. I'm not sure if the behaviour is the same on every game. If it's different and we allow something like that our cross-game compatibility suffers.
   2. I simply don't see the benefit of doing this. But if you really want to have this behaviour you could still hook ConVar changes.
3. jordanbriere commented on Oct 31, 2021

   

   jordanbriere

   on Oct 31, 2021

   Contributor

   More actions

   The issue we have with deleting ConVars is that we don't have a reference counter.

   Actually, we already do have a reference counter (shared_ptr), we just don't use it and create a new unmanaged one every time. In fact, this is very easy to implement with a simple mapping from ConVar * → weak_ptr. Here is a quick example:

   ../cvars/cvars.h

   /**
   * =============================================================================
   * Source Python
   * Copyright (C) 2012-2015 Source Python Development Team.  All rights reserved.
   * =============================================================================
   *
   * This program is free software; you can redistribute it and/or modify it under
   * the terms of the GNU General Public License, version 3.0, as published by the
   * Free Software Foundation.
   *
   * This program is distributed in the hope that it will be useful, but WITHOUT
   * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
   * FOR A PARTICULAR PURPOSE.  See the GNU General Public License for more
   * details.
   *
   * You should have received a copy of the GNU General Public License along with
   * this program.  If not, see <http://www.gnu.org/licenses/>.
   *
   * As a special exception, the Source Python Team gives you permission
   * to link the code of this program (as well as its derivative works) to
   * "Half-Life 2," the "Source Engine," and any Game MODs that run on software
   * by the Valve Corporation.  You must obey the GNU General Public License in
   * all respects for all other code used.  Additionally, the Source.Python
   * Development Team grants this exception to all derivative works.
   */
   
   #ifndef _CVARS_H
   #define _CVARS_H
   
   //-----------------------------------------------------------------------------
   // Includes.
   //-----------------------------------------------------------------------------
   #include "icvar.h"
   #include "utilities/convar.h"
   #include "utilities/sp_util.h"
   #include "modules/commands/commands_server.h"
   
   
   //-----------------------------------------------------------------------------
   // ICvar shared extension class.
   //-----------------------------------------------------------------------------
   class ICVarSharedExt
   {
   public:
   	static ConVar *FindVar(ICvar *pCVar, const char *szName)
   	{
   		ConCommandBase *pBase = pCVar->FindCommandBase(szName);
   		if (pBase)
   		{
   			if (pBase->IsCommand()) {
   				CServerCommandManager *pManager = dynamic_cast<CServerCommandManager *>(pBase);
   				if (pManager && pManager->m_pOldCommand && !pManager->m_pOldCommand->IsCommand()) {
   					return static_cast<ConVar *>(pManager->m_pOldCommand);
   				}
   			}
   			else {
   				return static_cast<ConVar *>(pBase);
   			}
   		}
   
   		return NULL;
   	};
   };
   
   
   #include "boost/smart_ptr/weak_ptr.hpp"
   #include "boost/unordered_map.hpp"
   
   inline boost::unordered_map<ConVar *, boost::weak_ptr<ConVar> > *owned_references()
   {
   	static boost::unordered_map<ConVar *, boost::weak_ptr<ConVar> > *s = new boost::unordered_map<ConVar *, boost::weak_ptr<ConVar> >();
   	return s;
   }
   
   //-----------------------------------------------------------------------------
   // ConVar extension class.
   //-----------------------------------------------------------------------------
   class ConVarExt
   {
   public:
   	static boost::shared_ptr<ConVar> __init__(const char* name, const char* value,
   		const char* description, int flags, object min_value, object max_value)
   	{
   		if (!name || name[0] == '\0')
   			BOOST_RAISE_EXCEPTION(PyExc_ValueError, "An empty string is not a valid ConVar name.")
   
   		float fMin = 0;
   		float fMax = 0;
   
   		try {
   			fMin = extract<float>(min_value);
   		}
   		catch (...) {
   			PyErr_Clear();
   		}
   
   		try {
   			fMax = extract<float>(max_value);
   		}
   		catch (...) {
   			PyErr_Clear();
   		}
   
   		ConVar *pConVar = ICVarSharedExt::FindVar(g_pCVar, name);
   		if (pConVar) {
   			boost::unordered_map<ConVar *, boost::weak_ptr<ConVar> >::const_iterator it = owned_references()->find(pConVar);
   			if (it != owned_references()->end()) {
   				printf(">> Returning existing reference for %s (%d)\n", pConVar->GetName(), pConVar);
   				return boost::shared_ptr<ConVar>(it->second);
   			}
   		}
   		else
   		{
   			ConVar* pConVar = new ConVar(strdup(name), strdup(value), flags,
   				strdup(description), !min_value.is_none(), fMin, !max_value.is_none(), fMax);
   			printf(">> Creating new owned reference for %s (%d)\n", pConVar->GetName(), pConVar);
   			boost::shared_ptr<ConVar> s = boost::shared_ptr<ConVar>(pConVar, &Deleter);
   			owned_references()->insert(std::make_pair(pConVar, boost::weak_ptr<ConVar>(s)));
   			return s;
   		}
   
   		return boost::shared_ptr<ConVar>(pConVar, &NeverDeleteDeleter<ConVar *>);
   	}
   
   	static void Deleter(ConVar *pConVar)
   	{
   		ConCommandBase *pBase = g_pCVar->FindCommandBase(pConVar->GetName());
   		if (pBase) {
   			CServerCommandManager *pManager = dynamic_cast<CServerCommandManager *>(pBase);
   			if (pManager && pManager->m_pOldCommand == pConVar) {
   				pManager->m_pOldCommand = NULL;
   			}
   			else if (pBase == pConVar) {
   				printf(">> Unregistering %s (%d)\n", pConVar->GetName(), pConVar);
   				g_pCVar->UnregisterConCommand(pConVar);
   			}
   		}
   
   		boost::unordered_map<ConVar *, boost::weak_ptr<ConVar> >::const_iterator it = owned_references()->find(pConVar);
   		if (it != owned_references()->end()) {
   			printf(">> Erasing weak reference for %s (%d)\n", pConVar->GetName(), pConVar);
   			owned_references()->erase(it);
   		}
   
   		printf(">> Deleting owned reference for %s (%d)\n", pConVar->GetName(), pConVar);
   		delete pConVar;
   	}
   
   	static bool HasMin(ConVar* pConVar)
   	{
   		float fMin;
   		return pConVar->GetMin(fMin);
   	}
   
   	static bool HasMax(ConVar* pConVar)
   	{
   		float fMax;
   		return pConVar->GetMax(fMax);
   	}
   
   	static float GetMin(ConVar* pConVar)
   	{
   		float fMin;
   		pConVar->GetMin(fMin);
   		return fMin;
   	}
   
   	static bool GetMax(ConVar* pConVar)
   	{
   		float fMax;
   		pConVar->GetMax(fMax);
   		return fMax;
   	}
   
   	static void SetValue(ConVar* pConVar, bool bValue)
   	{
   		pConVar->SetValue(bValue);
   	}
   
   	static void MakePublic(ConVar* pConVar)
   	{
   		AddConCommandFlags(pConVar, FCVAR_NOTIFY);
   		g_pCVar->CallGlobalChangeCallbacks(pConVar, pConVar->GetString(), pConVar->GetFloat());
   	}
   
   	static void RemovePublic(ConVar* pConVar)
   	{
   		RemoveConCommandFlags(pConVar, FCVAR_NOTIFY);
   		g_pCVar->CallGlobalChangeCallbacks(pConVar, pConVar->GetString(), pConVar->GetFloat());
   	}
   };
   
   
   #endif // _CVARS_H
   

   ../test1/test1.py

   from cvars import ConVar
   test_convar = ConVar("test_convar", "1", "Test Convar(test1).")

   ../test2/test2.py

   from commands.server import ServerCommand
   from cvars import ConVar
   test_convar = ConVar("test_convar", "2", "Test Convar(test2).")
   
   @ServerCommand("set_test")
   def test_command(*args):
       test_convar.set_string("3")

   ../collect/collect.py

   import gc
   gc.collect()

   sp plugin load test1;test_convar;sp plugin load test2;sp plugin unload test1;set_test;test_convar;sp plugin unload test2;sp plugin reload collect;test_convar
   [SP] Loading plugin 'test1'...
   >> Creating new owned reference for test1_version (435835568)
   >> Creating new owned reference for test_convar (435835408)
   [SP] Successfully loaded plugin 'test1'.
   "test_convar" = "1"
    - Test Convar(test1).
   [SP] Loading plugin 'test2'...
   >> Creating new owned reference for test2_version (435835808)
   >> Returning existing reference for test_convar (435835408)
   [SP] Successfully loaded plugin 'test2'.
   [SP] Unloading plugin 'test1'...
   >> Unregistering test1_version (435835568)
   >> Erasing weak reference for test1_version (435835568)
   >> Deleting owned reference for test1_version (435835568)
   [SP] Successfully unloaded plugin 'test1'.
   "test_convar" = "3" ( def. "1" )
    - Test Convar(test1).
   [SP] Unloading plugin 'test2'...
   >> Unregistering test2_version (435835808)
   >> Erasing weak reference for test2_version (435835808)
   >> Deleting owned reference for test2_version (435835808)
   [SP] Successfully unloaded plugin 'test2'.
   [SP] Unloading plugin 'collect'...
   [SP] Unable to unload plugin 'collect' as it is not currently loaded.
   [SP] Loading plugin 'collect'...
   >> Creating new owned reference for collect_version (435834128)
   >> Unregistering test_convar (435835408)
   >> Erasing weak reference for test_convar (435835408)
   >> Deleting owned reference for test_convar (435835408)
   [SP] Successfully loaded plugin 'collect'.
   Unknown command "test_convar"
   

   So, we should think about a different solution as I don't want to have any avoidable incompatibilities with SM.

   Despite the assumptions that have been presented as facts countless times; they are not affected whether we release our ConVar or not because, a stated into the other thread, they are unlinking their references as soon as we remove them from the global list. The incompatibilities are only one way; us wrapping their memory as if we own it which remain the case whether we release or purposely leak our ConVar.

   That said, as also stated into the other thread, our commands act the same exact way. For example:

   ../test1/test1.py

   from commands.server import *
   
   @ServerCommand('test1')
   def test1(command):
       ...

   ../test2/test2.py

   from commands.server import *
   from commands import *
   
   test1 = get_server_command('test1')
   
   @ServerCommand('test2')
   def test2(command):
       test1.dispatch(Command())

   Then running sp plugin load test1;sp plugin load test2;sp plugin unload test1;test2 will crash, etc. Leaking one, but not the other, seem inconsistent to me considering they are pretty much the same internally and should be treated as such. Though, commands cannot really be leaked since we also own their callbacks.

   Anyways, I just wanted to clarify these points. You guys go ahead and take the final decision and make the appropriate changes.
4. jordanbriere

   closed this as completedin 312ecbaon Dec 3, 2021
5. vinci6k

   added a commit that references this issue on Jul 6, 2025

   Re-introduced memory leaks. 😖 (Fixes Source-Python-Dev-Team#430)

   This issue will close once commit 312ecba is merged into the 'master' branch.

   312ecba