Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SDKTools GameRules native crash #1902

Open
5 of 6 tasks
Kenzzer opened this issue Jan 8, 2023 · 3 comments · May be fixed by #1903
Open
5 of 6 tasks

SDKTools GameRules native crash #1902

Kenzzer opened this issue Jan 8, 2023 · 3 comments · May be fixed by #1903

Comments

@Kenzzer
Copy link
Member

Kenzzer commented Jan 8, 2023

Help us help you

  • I have checked that my issue doesn't exist yet.
  • I have tried my absolute best to reduce the problem-space and have provided the absolute smallest test-case possible.
  • I can always reproduce the issue with the provided description below.

Environment

  • Operating System version: Debian Bullseye (Docker Debian Buster)
  • Game/AppID (with version if applicable): 440
  • Current SourceMod version: 1.12.0.6945
  • Current Metamod: Source snapshot: 1.11.0-dev+1145

Description

Through unknown conditions, the native GameRules_SetProp and more specifically, the extension sdktools's GetGameRulesProxyEnt will cause a segmentation fault.

Problematic Code (or Steps to Reproduce)

Call GameRules_SetProp repeatedly during any map. This might or might not trigger the crash.

Logs

https://crash.limetech.org/d42cffz6sta4
But this seems much more widespread than the crash signature I've, if we filter stats
https://crash.limetech.org/stats/sdktools.ext.2.tf2.so/GetGameRulesProxyEnt
Those servers seem to crash in the same location as I, and they're most certainly not running the same plugin setup as I.

@FortyTwoFortyTwo
Copy link
Contributor

I also have this common crash, along with this more rarer crash log related to __cxa_pure_virtual
https://crash.limetech.org/q5e2z2aeydgz

I believe the crash is more specifically from a one entity in FindEntityByNetClass, but couldn't figure out the exact cause of it.

@Kenzzer
Copy link
Member Author

Kenzzer commented Jan 9, 2023

After further investigation I've found the following :

FindEntityByNetClass is inlined in GetGameRulesProxyEnt. Confirming the suspicions of the above comment.
I've also decided to reverse the assembly against sourcemod source code

.text:0003D9FA                 mov     eax, ds:gamehelpers
.text:0003D9FF                 mov     ecx, [eax]
.text:0003DA01                 mov     [esp+4], edi
.text:0003DA05                 mov     [esp], eax
.text:0003DA08                 call    dword ptr [ecx+2Ch]
.text:0003DA0B                 test    eax, eax
.text:0003DA0D                 jz      short loc_3D9F0
.text:0003DA0F                 mov     ebx, eax
.text:0003DA11                 test    byte ptr [eax], 2
.text:0003DA14                 jnz     short loc_3D9F0
.text:0003DA16                 mov     esi, [ebx+8]
.text:0003DA19                 test    esi, esi
.text:0003DA1B                 jz      short loc_3D9F0
.text:0003DA1D                 mov     eax, [esi]
.text:0003DA1F                 mov     [esp], esi
.text:0003DA22                 call    dword ptr [eax] <==== crash
.text:0003DA24                 test    eax, eax
.text:0003DA26                 jz      short loc_3D9F0
.text:0003DA28                 mov     eax, [esi]
.text:0003DA2A                 mov     [esp], esi
.text:0003DA2D                 call    dword ptr [eax+4]
.text:0003DA30                 mov     eax, [eax]
.text:0003DA32                 mov     ecx, [ebp+var_10]
.text:0003DA35                 mov     [esp+4], ecx
.text:0003DA39                 mov     [esp], eax
.text:0003DA3C                 call    strcmp

What I reversed, assuming I didn't make any mistakes :

edict_t* current = gamehelpers->EdictOfIndex(i); // 0x0003DA0FA -- 0x0003DA08
if (current == nullptr) // 0x0003DA0B
	// loop // 0x0003DA0D
if (current->IsFree()) // 0x0003DA11
	// loop // 0x0003DA14
IServerNetworkable* network = current->GetNetworkable(); // 0x0003DA16
if (network == nullptr) // 0x0003DA19
	// loop // 0x0003DA1B
// The crash (vtable offset is 0)
network->(pure virtual function); // 0x0003DA22 - Supposedly GetEntityHandle

Which seems to coroborate the alternative crash dump @FortyTwoFortyTwo linked.

Interestingly enough, sourcemod code base hardly makes any use of that vcall.
Only two results : https://github.com/alliedmodders/sourcemod/search?q=GetEntityHandle

Furthermore FindEntityByNetClass is available elsewhere in the codebase, more specifically tf2 extension. And the Entity Handle check is missing there, home come ?

int FindEntityByNetClass(int start, const char *classname)
{
edict_t *current;
for (int i = ((start != -1) ? start : 0); i < gpGlobals->maxEntities; i++)
{
current = engine->PEntityOfEntIndex(i);
if (current == NULL || current->IsFree())
{
continue;
}
IServerNetworkable *network = current->GetNetworkable();
if (network == NULL)
{
continue;
}
ServerClass *sClass = network->GetServerClass();
const char *name = sClass->GetName();
if (strcmp(name, classname) == 0)
{
return i;
}
}
return -1;
}

I decided to git blame the addition of GetEntityHandle and PR #1089 seems to be blamed for this.

Analysing their bug report, I disagree with the fix that was provided. According to the author the function

ServerClass* CServerNetworkProperty::GetServerClass()
{
	if ( !m_pServerClass )
		m_pServerClass = m_pOuter->GetServerClass();
	return m_pServerClass;
}

is to be blamed because m_pOuter "can be null", but they didn't provide any crash dump to support their claim. Furthermore, m_pOuter can never be nullptr according to what I'll provide next.

CServerNetworkProperty::m_pOuter is assigned when CServerNetworkProperty::Init is called.
https://github.com/ValveSoftware/source-sdk-2013/blob/0d8dceea4310fde5706b3ce1c70609d72a38efdf/sp/src/game/server/ServerNetworkProperty.cpp#L60

void CServerNetworkProperty::Init( CBaseEntity *pEntity )
{
	m_pPev = NULL;
	m_pOuter = pEntity;

And the Init function is called under CBaseEntity ctor
https://github.com/ValveSoftware/source-sdk-2013/blob/0d8dceea4310fde5706b3ce1c70609d72a38efdf/sp/src/game/server/baseentity.cpp#L364

CBaseEntity::CBaseEntity( bool bServerOnly )
{
	[...]
	NetworkProp()->Init( this );

Finally, Sourcemod call to IServerNetworkable* network = current->GetNetworkable(); can only ever return a non nullptr, when the edict_t::m_pNetworkable has been assigned, an assignment which only happens MUCH AFTER CBaseEntity constructor call.
Under CBaseEntity::PostConstructor
https://github.com/ValveSoftware/source-sdk-2013/blob/0d8dceea4310fde5706b3ce1c70609d72a38efdf/sp/src/game/server/baseentity.cpp#L503

Therefore it is impossible, we ever reach the call IServerNetworkable::GetServerClass that has an invalid m_pOuter. And valve themselves don't null check property, because it is most likely intended to never be null. In fact, if we check their network code, they never ever consider the possibility of m_pOuter being null. I therefore suggest a reversal of PR #1089

Extra note

This doesn't address however the main issue we uncovered here. IServerNetworkable::GetEntityHandle vtable entry is not initialised. Which is probably what the author of the other PR encountered but instead with IServerNetworkable::GetServerClass, and most likely wrongly deduced that the src code of that function was to blamed for the nullptr error but the real issue was most likely that the vtable wasn't initialised.

This all points to me, this isn't an error on Sourcemod's part. And there's a wrong IServerNetworkable assignment to an edict by the game or a plugin. And it most likely is an entity that overrides edict index attribution, like a player or something alike, which in turns prevent CBaseEntity::PostConstructor from assigning its valid servernetworkable to the entity. Or maybe it's an entity in the process of being deleted, and its memory has already been freed ?

@KyleSanderson
Copy link
Member

I've certainly crashed from a null m_pOuter. With that being said, the situation always seemed to be collision related.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants