Fixed the deletion of ConVar/ConCommand not managed by Source.Python. #426
Conversation
|
Source.Python/src/core/modules/commands/commands_server.cpp Lines 70 to 73 in 620d6c3 In fact, it works just as it should on my side: |
|
It only works because your s_nDLLIdentifier happens to be the same as Source.Python's DLL identifier. There is no guarantee that it will work every time. ConVar_Unregister is a very dumb function. Even if it works correctly, it will only work once. |
But it's not a coincidence. We assign our Source.Python/src/core/modules/commands/commands_wrap.cpp Lines 52 to 61 in 02e3490 say and say_team.
Other than that, It's possible there is a linking issue on Linux and that we ends up sharing the same symbol. If the following assert for you: from cvars import *
assert cvar.find_base('sm').dll_identifier is not SP_CVAR_DLL_IDENTIFIER
assert cvar.find_base('meta').dll_identifier is not SP_CVAR_DLL_IDENTIFIER
assert cvar.find_base('sp').dll_identifier is SP_CVAR_DLL_IDENTIFIERThen that's likely it. Try to add
We don't call it multiple times, only on unload, so that isn't really an issue, is it? |
|
ConVar and Source SDK is so broken that I feel discouraged.
I understand that, but it's not working in practice.
I tried that too, but it doesn't work. Test Code: # Commands
from commands import ConCommandBase
# Cvars
from cvars import cvar
from cvars import SP_CVAR_DLL_IDENTIFIER
# Memory
from memory import get_virtual_function
from memory import make_object
from memory.hooks import PreHook
@PreHook(get_virtual_function(cvar, "RegisterConCommand"))
def pre_register_con_command(args):
base = make_object(ConCommandBase, args[1])
if not base.name:
return
print("RegisterConCommand")
print("Name: ", base.name)
print("Help Text: ", base.help_text)
print("Is Registered: ", base.is_registered())
print("Is Command: ", base.is_command())
print("Address: ", base._ptr().address)
print("dll_identifier: ", base.dll_identifier)
print("Is dll_identifier == sp_cvar_dll_identifier: ", base.dll_identifier == SP_CVAR_DLL_IDENTIFIER)For clarity. class CPluginConVarAccessor : public IConCommandBaseAccessor
{
public:
virtual bool RegisterConCommandBase(ConCommandBase* pCommand)
{
printf("[Source.Python] RegisterConCommandBase\n");
if (!g_pCVar->FindCommandBase(pCommand->GetName())) {
g_pCVar->RegisterConCommand(pCommand);
printf("[Source.Python] Registered\n");
return true;
}
printf("[Source.Python] Failed\n");
return false;
}
};Output (CS:GO Linux tier1.a excluded with clarity code)With distributed binaries (CS:GO Linux SP version: 710)Switch the plugin load order (CS:GO Linux tier1.a excluded)Since this problem affects all ConVars, there is no choice other than to fix it, but #421 and #430 are covered by this problem, which makes things even more complicated. |
| @@ -409,9 +409,6 @@ void CSourcePython::Unload( void ) | |||
| DevMsg(1, MSG_PREFIX "Clearing all commands...\n"); | |||
| ClearAllCommands(); | |||
|
|
|||
| DevMsg(1, MSG_PREFIX "Unregistering ConVar...\n"); | |||
| ConVar_Unregister( ); | |||
|
|
|||
There was a problem hiding this comment.
Please replace with:
// See issue #430.
// DevMsg(1, MSG_PREFIX "Unregistering ConVar...\n");
// ConVar_Unregister( );
|
@jordanbriere Can you confirm this issue on your side?
Since the root cause of the problem could not be determined, it was neglected, but it's actually not that simple. from cvars import SP_CVAR_DLL_IDENTIFIER
from cvars import cvar
def get_bases():
bases = list()
base = cvar.commands
while base is not None:
bases.append((base.dll_identifier, base.name, base.is_command()))
base = base.next
return bases
bases = get_bases()
bases.sort(key=lambda x:x[0])
for base in bases:
identifier, name, is_command = base
if identifier < 0 or identifier >= SP_CVAR_DLL_IDENTIFIER:
print(identifier, name, is_command)In this code, you can see if SP_CVAR_DLL_IDENTIFIER overlaps with the dll_identifier of other metamod plugins. If you unload a metamod plugin with overlapping dll_identifier, the ConCommand will be released without being unregistered, causing a segmentation fault in the code as shown earlier(OR If this is not a Source.Python problem, but a MetaMod problem, I'll just have to apply the earlier fix and see what happens, but causing a clear crash is a problem. |
|
In my environment, the only way to get around this reliably is to load Source.Python after metamod has finished loading. This is just my guess, but it looks like there is a delay in the loading of MetaMod, and Source.Python is catching up and hijacking the ConCommand while MetaMod is loading. |
|
To be honest, I don't think we should care. There is no point into trying to make a non-issue an issue. I mean, so long as loading SM or SP plugins at run-time works, we should not bother if there are issues unloading MM/SM/SP themselves because they can all be fixed with a server reboot and the fact nobody ever reported issues related to this means nobody really does unload and load them on the fly. I'd say make the changes so we know why it was done, and leave it at that. If this becomes a recurrent issue for users then let's address it then. |
It's not just a matter of loading/unloading MetaMod/SourceMod/Source.Python itself. The reason this issue is not more obvious is that problems caused by reloading the MetaMod plugin or SourceMod Extension do not lead to an immediate crash, and even if they do, it is not clear how they are related to Source.Python. |
Estimated population that does so: 0.0000005% I know you like to break stuff... 😄 But this is a non-issue in my opinion. At least, not until it becomes one. THere is really no point into focusing on stuff that is unlikely to happens or that never has been reported. If many users come forward and report issues related to this, then it can be addressed at that time but for now, trying to profile and debug this is a waste of time to me, |
Edited:
I'm working on a fix for the problem in ConVar/ConCommand, I don't know the cause of this problem, but please wait to close this issue as I'm figuring out the workaround. |
The function ConVar_Unregister unregisters all ConVar/ConCommand and should not be used.
Output:
to