Crash due to ConVar overlap and release. #421

[CookStar]

This is an issue derived from #421 and introduced by 02e3490. There is a long discussion going on in #421, and I've created an issue for the record.

The main issue is due to Source.Python releasing the ConVar object created by ConVar(str) when the plugin is unloaded. This will cause other Source.Python/SourceMod plugins to crash if they hold the same ConVar object.

Code:

Plugin1 (test1)

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

Plugin2 (test2) Or a SourceMod plugin with equivalent functionality.

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")

Output;

$ sp plugin load test1
[SP] Loading plugin 'test1'...
[SP] Successfully loaded plugin 'test1'.

$ test_convar
"test_convar" = "1" - Test Convar(test1).

$ sp plugin load test2
[SP] Loading plugin 'test2'...
[SP] Successfully loaded plugin 'test2'.

$ sp plugin unload test1
[SP] Unloading plugin 'test1'...
[SP] Successfully unloaded plugin 'test1'.

$ set_test
Thread 1 "srcds_linux" received signal SIGSEGV, Segmentation fault.
0xf076d5e5 in boost::python::objects::polymorphic_id_generator<ConVar>::execute(void*) ()

The same problem occurs with the wrapping plugin for ConVars.

The root of the problem lies in the fact that ConVar(str) returns an existing registered ConVar.

Source.Python/src/core/modules/cvars/cvars.h

Lines 95 to 104 in 02e3490

	ConVar *pConVar = ICVarSharedExt::FindVar(g_pCVar, name);
	if (!pConVar)
	{
	ConVar* pConVar = new ConVar(strdup(name), strdup(value), flags,
	strdup(description), !min_value.is_none(), fMin, !max_value.is_none(), fMax);
	return boost::shared_ptr<ConVar>(pConVar, &Deleter);
	}
	return boost::shared_ptr<ConVar>(pConVar, &NeverDeleteDeleter<ConVar *>);

In the Valve Server Plugin, the ConVar used by the plugin is created each time it is loaded, and released when it is unloaded. Also, when accessing a ConVar that already exists, the plugin retrieves the ConVar from ICvar.FindVar(g_pCVar->FindVar) and checks if it is valid before accessing it. However, SourcePython's ConVar(str) and SourceMod's CreateConVar return an existing ConVar, so if there are overlapping ConVars, intentionally or unintentionally, the plugin will own a single ConVar, resulting in a crash.

Unfortunately, ConVar(str) is used by many plugins to access an existing ConVar instead of cvar.find_var which should be used, and it is not possible to not return an existing ConVar without causing a major compatibility break.

But either way, when ConVar overlaps, the parent is set and the children access the parent, so when the parent is released, this also causes a crash. (This has been disabled in 02e3490, but should not be disabled for warnings.)

Source.Python/src/core/modules/commands/commands_server.cpp

Lines 54 to 62 in 02e3490

	virtual bool RegisterConCommandBase(ConCommandBase* pCommand)
	{
	if (!g_pCVar->FindCommandBase(pCommand->GetName())) {
	g_pCVar->RegisterConCommand(pCommand);
	return true;
	}
	return false;
	}

We should stop releasing ConVar when unloading plugins, and if we do release it, at least it should be done when unloading Source.Python VSP, so that plugins can work safely. However, if we want to be compatible with SourceMod, I think we should prohibit release of ConVar itself.

[Ayuto]

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.

[jordanbriere]

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.