- Notifications
You must be signed in to change notification settings - Fork36
Fixed the deletion of ConVar/ConCommand not managed by Source.Python.#426
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
jordanbriere commentedOct 10, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Source.Python/src/core/modules/commands/commands_server.cpp Lines 70 to 73 in620d6c3
In fact, it works just as it should on my side: |
CookStar commentedOct 10, 2021
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. |
jordanbriere commentedOct 10, 2021
But it's not a coincidence. We assign our Source.Python/src/core/modules/commands/commands_wrap.cpp Lines 52 to 61 in02e3490
say andsay_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: fromcvarsimport*assertcvar.find_base('sm').dll_identifierisnotSP_CVAR_DLL_IDENTIFIERassertcvar.find_base('meta').dll_identifierisnotSP_CVAR_DLL_IDENTIFIERassertcvar.find_base('sp').dll_identifierisSP_CVAR_DLL_IDENTIFIER Then 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? |
CookStar commentedNov 5, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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: # CommandsfromcommandsimportConCommandBase# CvarsfromcvarsimportcvarfromcvarsimportSP_CVAR_DLL_IDENTIFIER# Memoryfrommemoryimportget_virtual_functionfrommemoryimportmake_objectfrommemory.hooksimportPreHook@PreHook(get_virtual_function(cvar,"RegisterConCommand"))defpre_register_con_command(args):base=make_object(ConCommandBase,args[1])ifnotbase.name:returnprint("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. classCPluginConVarAccessor :publicIConCommandBaseAccessor{public:virtualboolRegisterConCommandBase(ConCommandBase* pCommand){printf("[Source.Python] RegisterConCommandBase\n");if (!g_pCVar->FindCommandBase(pCommand->GetName())) {g_pCVar->RegisterConCommand(pCommand);printf("[Source.Python] Registered\n");returntrue;}printf("[Source.Python] Failed\n");returnfalse;}}; 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. |
| DevMsg(1, MSG_PREFIX"Unregistering ConVar...\n"); | ||
| ConVar_Unregister( ); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please replace with:
// See issue #430.// DevMsg(1, MSG_PREFIX "Unregistering ConVar...\n");// ConVar_Unregister( );
CookStar commentedDec 3, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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. fromcvarsimportSP_CVAR_DLL_IDENTIFIERfromcvarsimportcvardefget_bases():bases=list()base=cvar.commandswhilebaseisnotNone:bases.append((base.dll_identifier,base.name,base.is_command()))base=base.nextreturnbasesbases=get_bases()bases.sort(key=lambdax:x[0])forbaseinbases:identifier,name,is_command=baseifidentifier<0oridentifier>=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. |
CookStar commentedDec 3, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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. |
jordanbriere commentedDec 3, 2021
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. |
CookStar commentedDec 3, 2021
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. |
jordanbriere commentedDec 3, 2021
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, |
CookStar commentedDec 3, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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