Crash with CFuncTank

[leonid9851]

Hi there! There's a problem with gamedll, hope u could fix this.

So, let's start :

Build version of hlds is 7882

Modules list:

Fun 1.8.2 AMX Mod X Dev Team running Engine 1.8.2 AMX Mod X Dev Team running FakeMeta 1.8.2 AMX Mod X Dev Team running Sockets 1.8.2 HLSW Dev Team running RegEx 1.8.2 AMX Mod X Dev Team running nVault 1.8.2 AMX Mod X Dev Team running Ham Sandwich 1.8.2 AMX Mod X Dev Team running Orpheu 2.6.3 joaquimandrade & Ar running MySQL 1.8.2 AMX Mod X Dev Team running CStrike 1.8.2 AMX Mod X Dev Team running

Crash log:

CRASH: Wed Jul 17 21:37:30 MSK 2019
Start Line: ./hlds_linux -game cstrike +ip x.x.x.x +port xxxx +map jail_xmf +maxplayers 20 -condebug -debug -insecure -timeout 60 +sys_ticrate 10000 -noipx -norestart -pidfile game.pid
[New LWP 10519]
[New LWP 10535]
[New LWP 10534]
[New LWP 10537]
[New LWP 10542]
[New LWP 10541]
[New LWP 10544]
[New LWP 10543]
[New LWP 10573]
[New LWP 12819]
[New LWP 9871]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
Core was generated by `./hlds_linux -game cstrike +ip x.x.x.x +port xxxx +map jail_xmf +maxplaye'.
Program terminated with signal 11, Segmentation fault.
#0  CFuncTank::TrackTarget (this=0x924f880) at ../cstrike/dlls/func_tank.cpp:500
#0  CFuncTank::TrackTarget (this=0x924f880) at ../cstrike/dlls/func_tank.cpp:500
#1  0xf30ed4ee in CFuncTank::Think (this=0x924f880) at ../cstrike/dlls/func_tank.cpp:480
#2  0xf32f9df7 in api_caller_void_args_p(void const*, void const*) () from /game/./cstrike/addons/metamod/metamod.so
#3  0xf32fdbf0 in main_hook_function_void(unsigned int, enum_api_t, unsigned int, void const*) () from /game/./cstrike/addons/metamod/metamod.so
#4  0xf3300fa0 in mm_DispatchThink(edict_s*) () from /game/./cstrike/addons/metamod/metamod.so
#5  0xf6f67298 in SV_Physics () at ../engine/sv_phys.c:2038
#6  0xf6f5ee56 in SV_Frame () at ../engine/sv_main.c:9257
#7  0xf6f27002 in _Host_Frame (time=0.00113931496) at ../engine/host.c:1404
#8  0xf6f273c2 in Host_Frame (time=0.00113931496, iState=1, stateInfo=0xffafe19c) at ../engine/host.c:1522
#9  0xf6f4ba7c in CEngine::Frame (this=0xf6fe26a0 <g_Engine>) at ../engine/sys_engine.cpp:245
#10 0xf6f48de3 in RunFrame (this=<optimized out>) at ../engine/sys_dll2.cpp:1235
#11 CDedicatedServerAPI::RunFrame (this=0xf6fddd00 <__g_CDedicatedServerAPI_singleton>) at ../engine/sys_dll2.cpp:1226
#12 0x08049c65 in RunServer () at ../dedicated/sys_ded.cpp:766
#13 0x08049472 in main (argc=22, argv=0xffafe3d4) at ../dedicated/sys_ded.cpp:1146
lineOfSight = <optimized out>
targetPosition = {x = -1.79516538e+31, y = -2.49708444e+33, z = -1.79514024e+31}
distX = <optimized out>
tr = {fAllSolid = -5251400, fStartSolid = -146363258, fInOpen = 0, fInWater = -211643040, flFraction = 1, vecEndPos = {x = -nan(0x2fdeec), y = -1.79514024e+31, z = -1.79514024e+31}, flPlaneDist = 3.12489558e-43, vecPlaneNormal = {x = 2.51241604e-40, y = -1.79514024e+31, z = -2.50518196e+33}, pHit = 0xf362941c, iHitgroup = -204614408}
updateTime = 0
angles = {x = 7.20267411e-43, y = 0, z = 1}
barrelEnd = {x = 0, y = -nan(0x2fdfc4), z = -nan(0x2fdfd0)}
pTarget = <optimized out>
pPlayer = 0xf35fd7c0
direction = {x = -1.79516538e+31, y = 3.12489558e-43, z = 2.51241604e-40}
distY = <optimized out>
From        To          Syms Read   Shared Object Library
0xf76ad914  0xf76f3c78  Yes         ./libstdc++.so.6
0xf765ba90  0xf765c97c  Yes (*)     /lib/libdl.so.2
0xf7644780  0xf7650857  Yes (*)     /lib/libpthread.so.0
0xf7496320  0xf75dc91b  Yes (*)     /lib/libc.so.6
0xf7441460  0xf746bc06  Yes (*)     /lib/libm.so.6
0xf7429e04  0xf7439490  Yes         ./libgcc_s.so.1
0xf774a8a0  0xf7765261  Yes (*)     /lib/ld-linux.so.2
0xf6f0d600  0xf6f915ac  Yes         /game/engine_i486.so
0xf6eb35c0  0xf6ebdd74  Yes (*)     ./libsteam_api.so
0xf6ea9900  0xf6eacdca  Yes (*)     /lib/librt.so.1
0xf6e8ff40  0xf6ea13f8  Yes (*)     /game/filesystem_stdio.so
0xf5be01c0  0xf6959b04  Yes (*)     /.steam/sdk32/steamclient.so
0xf32f8df0  0xf330d614  Yes         /game/./cstrike/addons/metamod/metamod.so
0xf30bd370  0xf31e13a0  Yes         /game/cstrike/dlls/cs.so
0xf2f7ea00  0xf2fde768  Yes (*)     /game/cstrike/addons/amxmodx/dlls/amxmodx_mm_i386.so
0xf2f5fc10  0xf2f62c98  Yes (*)     cstrike/addons/amxmodx/modules/fun_amxx_i386.so
0xf2f4e0d0  0xf2f59478  Yes (*)     cstrike/addons/amxmodx/modules/engine_amxx_i386.so
0xf2f15c50  0xf2f424a8  Yes (*)     cstrike/addons/amxmodx/modules/fakemeta_amxx_i386.so
0xf7661d90  0xf7662d08  Yes (*)     cstrike/addons/amxmodx/modules/sockets_amxx_i386.so
0xf2eee350  0xf2ef6fc8  Yes (*)     cstrike/addons/amxmodx/modules/regex_amxx_i386.so
0xf2ee1490  0xf2ee8d78  Yes (*)     cstrike/addons/amxmodx/modules/nvault_amxx_i386.so
0xf2ead870  0xf2ed90b8  Yes (*)     cstrike/addons/amxmodx/modules/hamsandwich_amxx_i386.so
0xf2e319d0  0xf2e5a618  Yes (*)     cstrike/addons/amxmodx/modules/orpheu_amxx_i386.so
0xf2c138c0  0xf2dc1571  Yes (*)     /game/cstrike/addons/vtc/VoiceTranscoder.so
                        Yes (*)     /game/cstrike/addons/processcmds/processcmds_mm_i386.so
                        Yes (*)     /lib/libvectcc.so.8
0xf2b30d70  0xf2b6b9b8  Yes (*)     ./libssl.so.1.0.0
0xf29ab040  0xf2aad328  Yes (*)     ./libcrypto.so.1.0.0
0xf294c860  0xf29593a4  Yes (*)     /lib/libz.so.1
0xf269e110  0xf26c9650  Yes (*)     /game/cstrike/addons/dproto/dproto_i386.so
0xf23df490  0xf246f228  Yes (*)     cstrike/addons/amxmodx/modules/mysql_amxx_i386.so
0xf2284220  0xf228a9d8  Yes (*)     cstrike/addons/amxmodx/modules/cstrike_amxx_i386.so
0xee654200  0xef4ed3c4  Yes (*)     ./steamclient.so
0xf22ff670  0xf2375020  Yes (*)     ./crashhandler.so
0xf22e6a50  0xf22edfb9  Yes (*)     /lib/libnss_files.so.2
0xf22dec00  0xf22e206e  Yes (*)     /lib/libnss_dns.so.2
0xf22c66a0  0xf22d5694  Yes (*)     /lib/libresolv.so.2
0xf22bde60  0xf22beb70  Yes (*)     /lib/libstdvc++.so.1
0xf22b3380  0xf22b4428  Yes (*)     /lib/libvectc++.so.7
(*): Shared library is missing debugging information.
Stack level 0, frame at 0xffafdf90:
 eip = 0xf30ecc6a in CFuncTank::TrackTarget (../cstrike/dlls/func_tank.cpp:500); saved eip 0xf30ed4ee
 called by frame at 0xffafdfc0
 source language c++.
 Arglist at unknown address.
 Locals at unknown address, Previous frame's sp is 0xffafdf90
 Saved registers:
  ebx at 0xffafdf7c, ebp at 0xffafdf88, esi at 0xffafdf80, edi at 0xffafdf84, eip at 0xffafdf8c
End of crash report
----------------------------------------------

[SamVanheer]

Plugins like Metamod can interfere with normal operation of games so i'd advise reporting issues occurring on vanilla servers only, or reproducing issues on vanilla servers if you've encountered them on modified servers.

In cases like this reporting them anyway is a good thing since it seems to be happening regardless of plugins though.

I looked into this and it seems that this line is causing it: https://github.com/ValveSoftware/halflife/blob/c7240b965743a53a29491dd49320c88eecf6257b/dlls/func_tank.cpp#L496

The controller is non-null but invalid, i suspect the following scenario occurred:

• A player was controlling a func_tank
• The player disconnected
• Another player connected into the same slot, causing the disconnected player's CBasePlayer instance to be freed, invalidating pointers to it
• The tank's m_pController pointer wasn't zeroed out because there is no code to handle player disconnection while controlling tanks
• The func_tank thinks and tries to access the controller's variables, causing a segfault

Fixing it requires disconnecting clients to stop using func_tank. This should be done somewhere in here: https://github.com/ValveSoftware/halflife/blob/c7240b965743a53a29491dd49320c88eecf6257b/dlls/client.cpp#L97-L136

I would recommend moving this bit: https://github.com/ValveSoftware/halflife/blob/c7240b965743a53a29491dd49320c88eecf6257b/dlls/client.cpp#L120-L133

Into a new method:

void CBasePlayer::OnDisconnect()
{
    CSound *pSound = CSoundEnt::SoundPointerForIndex( CSoundEnt::ClientSoundIndex( edict() ) );
    {
        // since this client isn't around to think anymore, reset their sound. 
        if ( pSound )
        {
            pSound->Reset();
        }
    }

    // since the edict doesn't get deleted, fix it so it doesn't interfere.
    pev->takedamage = DAMAGE_NO;// don't attract autoaim
    pev->solid = SOLID_NOT;// nonsolid
    UTIL_SetOrigin ( pev, pev->origin );

    if ( m_pTank != NULL )
    {
        // Stop controlling the tank
        m_pTank->Use( this, this, USE_OFF, 0 );
        m_pTank = NULL;
    }
}

And then calling it in ClientDisconnect.

This applies to all GoldSource engine games since they use the same code for tanks.

[RauliTop]

@mikela-valve this must be fixed.

It is an easy exploit to crash a server...

[leonid9851]

On Pawn language, it had helped me already:

#include <amxmodx>
#include <fakemeta>
#include <hamsandwich>
#include <engine>

#define PLUGIN  "Fix Tank"
#define VERSION "1.8.2"
#define AUTHOR  "SamVanheer"

#define USE_OFF 0

const m_pTank = 1408;

new iEnt;

public plugin_init(){

    register_plugin(PLUGIN, VERSION, AUTHOR);

    iEnt = find_ent_by_class(-1, "func_tank");
}

public client_disconnect(id){

    if(!iEnt) return PLUGIN_CONTINUE;

    static iCheck; iCheck = get_pdata_ent(id, m_pTank);

    if(iCheck != 0){

        ExecuteHamB(Ham_Use, iEnt, id, id, USE_OFF, 0.0);

        set_pdata_int(id, m_pTank, false, 20);

    }
    return PLUGIN_CONTINUE;
}

[SamVanheer]

That plugin only works if there is only one func_tank in the map.

[leonid9851]

Yeap. Are there any other classes work like func_tank in cs ? That must be enough, mustn't it?

[SamVanheer]

There is more than one type of func_tank, but if there is more than one func_tank entity instance it also won't work. Your code will make the first func_tank stop being controlled, even if the disconnecting player was using another tank.

According to this: https://wiki.alliedmods.net/AMX_Mod_X_1.9_API_Changes

You can use get_pdata_ehandle and set_pdata_ehandle to directly use the tank ehandle. Use that instead of locating the func_tank separately.

See documentation here: https://www.amxmodx.org/api/fakemeta/get_pdata_ehandle https://www.amxmodx.org/api/fakemeta/set_pdata_ehandle

This should work with any number of func_tanks, regardless of type.

[leonid9851]

Hm.. really, that's work just for a one. I have Amxx 1.8.2, 'cause I don't know why, but my Jb engine is falling from time to time with 1.9.0 without any reasons, instead of 1.8.2 .

I think smth like that should be the solution for a several func_tank entities(not a great, but anyway..), correct me if I'm wrong:

#include <amxmodx>
#include <fakemeta>
#include <hamsandwich>
#include <engine>

#define PLUGIN  "Fix Tank"
#define VERSION "1.8.2"
#define AUTHOR  "SamVanheer"

#define USE_OFF 0

const m_pTank = 1408;

new g_fwrdSpawn;

static Array:g_DataEntities, iSize, bool:g_CheckFuncs;

public plugin_precache(){

    g_DataEntities=ArrayCreate(4);

    g_fwrdSpawn = register_forward(FM_Spawn, "HookSpawn");
}

public HookSpawn(iEnt)
{
    if(!pev_valid(iEnt)) return FMRES_IGNORED;

    static szClassName[33]; pev(iEnt, pev_classname, szClassName, charsmax(szClassName));

    if(equal(szClassName, "func_tank")){

        ArrayPushCell(g_DataEntities, iEnt);

        if(!g_CheckFuncs) g_CheckFuncs = true;
    }

    return FMRES_IGNORED;
}

public plugin_init(){

    unregister_forward(FM_Spawn, g_fwrdSpawn);

    iSize = ArraySize(g_DataEntities);

    register_plugin(PLUGIN, VERSION, AUTHOR);
}

public client_disconnect(id){

    if(!g_CheckFuncs) return PLUGIN_CONTINUE;

    static iCheck; iCheck = get_pdata_ent(id, m_pTank);

    if(iCheck != 0){

        for( new iStr; iStr < iSize; iStr++ )
        {
            new iEnt = ArrayGetCell(g_DataEntities,iStr);
            ExecuteHamB(Ham_Use, iEnt, id, id, USE_OFF, 0.0);
        }

        set_pdata_int(id, m_pTank, false, 20);
    }

    return PLUGIN_CONTINUE;
}

Hope it won't crash your server :)

[RauliTop]

func_tank isn't the only name of that entity. It can be func_tankmortar, func_tankrocket or func_tanklaser too (I don't know if there are more).

Is m_pTank offset returning true or false only? Is there another way to check the entity player is using?

The way you are currently using in the plugin can be improved a lot.

[justgo97]

is this fixed on rehlds / regamedll ?

[leonid9851]

@RauliTop, I've done it simply for my server and there was just one thing for a using. U could do next for a several classes:

if(equal(szClassName, "func_tank")) -----------------------------------------> if(equal(szClassName, "func_tank") || equal(szClassName, "func_tankrocket") || equal(szClassName, "func_tanklaser"))

or make these checks of the classnames in a cycle one by one. I think the solution of @SamVanheer is the best with

get_pdata_ehandle/set_pdata_ehandle

@justgo97, I don't see in Re cleaning data with using tank, when a player is leaving the server.

[RauliTop]

• A player was controlling a func_tank
• The player disconnected
• Another player connected into the same slot, causing the disconnected player's CBasePlayer instance to be freed, invalidating pointers to it
• The tank's m_pController pointer wasn't zeroed out because there is no code to handle player disconnection while controlling tanks
• The func_tank thinks and tries to access the controller's variables, causing a segfault

Impossible to reproduce it for me. This is what I tried:

• Control a func_tankmortar.
• Disconnect while I'm shooting.
• Connect again (server connects me to the same slot, I've checked typing status in console).
• Server stills online.

I'm pretty sure there is a crash problem with this entities. I have blocked them at the past on my server because were doing crashes when players were using them constantly.

Probably there's another way to produce the crash or I forgot something.

@leonid9851 and @SamVanheer , did you test something?

[SamVanheer]

I tested this with a vanilla Half-Life SDK build and in that codebase it does stop controlling the tank because of this: https://github.com/ValveSoftware/halflife/blob/c7240b965743a53a29491dd49320c88eecf6257b/dlls/client.cpp#L135 https://github.com/ValveSoftware/halflife/blob/c7240b965743a53a29491dd49320c88eecf6257b/dlls/multiplay_gamerules.cpp#L501 https://github.com/ValveSoftware/halflife/blob/c7240b965743a53a29491dd49320c88eecf6257b/dlls/player.cpp#L818-L822

However i don't see this code in Counter-Strike's version of CBasePlayer::RemoveAllItems.

I did some further testing and i found that disconnecting and reconnecting reallocated the CBasePlayer instance at the same memory location, so the pointer becomes valid again.

I'm guessing the presence of Metamod and AMXMod resulted in additional memory allocations that alter the address and eventually causes the pointer to be invalid, resulting in a segfault.

Re-adding the code to stop using tanks in CBasePlayer::RemoveAllItems should fix this, but i don't expect it will work when using another gamerules (CHalfLifeRules, though i don't see any uses of it). CHalfLifeTraining inherits from CHalfLifeMultiplay so it should work with that.

[RauliTop]

I did some further testing and i found that disconnecting and reconnecting reallocated the CBasePlayer instance at the same memory location, so the pointer becomes valid again.

I'm guessing the presence of Metamod and AMXMod resulted in additional memory allocations that alter the address and eventually causes the pointer to be invalid, resulting in a segfault.

Do you mean disconnecting and reconnecting the same player? Or is it the same for other player?

The test I realized was in a server using Metamod and AMXModX

[SamVanheer]

The same player, but the engine just picks the first free slot. Getting the underlying memory allocator to return a different address requires you to leave a free chunk of memory that occurs earlier in the list of memory blocks so it's hard to get it to happen in an isolated test.

[RauliTop]

The same player, but the engine just picks the first free slot. Getting the underlying memory allocator to return a different address requires you to leave a free chunk of memory that occurs earlier in the list of memory blocks so it's hard to get it to happen in an isolated test.

So, the crash will happen only when a player disconnects and another player connects to his slot. It will be impossible to reproduce by one player doing retry, we need two different players.

Is that what you mean?

[SamVanheer]

You can reproduce the issue by allocating the original memory region to another temporary entity:

static int connectCount = 0;

/*
===========
ClientPutInServer

called each time a player is spawned
============
*/
void ClientPutInServer( edict_t *pEntity )
{
    CBasePlayer* pDummy = nullptr;

    if( connectCount > 0 )
    {
        pDummy = GetClassPtr<CBasePlayer>( nullptr );
    }

    ++connectCount;

    CBasePlayer *pPlayer;

    entvars_t *pev = &pEntity->v;

    pPlayer = GetClassPtr((CBasePlayer *)pev);
    pPlayer->SetCustomDecalFrames(-1); // Assume none;

    if( pDummy )
    {
        REMOVE_ENTITY( pDummy->edict() );
    }

    // Allocate a CBasePlayer for pev, and call spawn
    pPlayer->Spawn() ;

    // Reset interpolation during first frame
    pPlayer->pev->effects |= EF_NOINTERP;

    pPlayer->pev->iuser1 = 0;   // disable any spec modes
    pPlayer->pev->iuser2 = 0; 
}

The first time you connect it allocates the original memory region to your player. Then start controlling a tank, disconnect and reconnect.

This code will allocate the original memory region to a dummy player entity (it's a player to match the size in bytes, which ensures no part of the original region could be assigned to your player entity), allocates another region to your player entity, and frees the dummy.

If you do this then the game will crash:

>   hl.dll!CFuncTank::TrackTarget() Line 496    C++
    hl.dll!CFuncTank::Think() Line 478  C++
    hl.dll!DispatchThink(edict_s * pent) Line 236   C++
    swds.dll!02906fbf() Unknown

The line numbers are off by a few but it's the same code.

Make sure to also check my notes above for disabling the code in CBasePlayer::RemoveAllItems to reproduce this in the SDK.

[RauliTop]

I did one more test. Again crash was impossible to reproduce alone.

I notice an important aspect, after disconnect and connect again using a func_tank:

• Control a func_tank
• Disconnect using it
• Reconnect and control another func_tank (only will be possible on maps with multiple func_tanks)
• Disconnect again
• Reconnect. Now If you look to those entities you have controlled, they follow your angles movement with the camera. Really a bug.

So, it's true that func_tank doesn't reset when a player disconnects using it. Probably the crash will happen when another player tries to control that func_tank after doing all steps above.

[SamVanheer]

Yeah it still copies your angles because the controller pointer points to your CBasePlayer instance.

If you didn't modify the code (either in SDK or using Metamod/AMXMod) then it will pretty much always reuse the same memory region. If you use a pre and post hook for ClientPutInServer to replicate my code you can probably reproduce the crash.

[RauliTop]

I finally reproduce the crash.

We need two different players:

• Connect the two players
• 1st player use the func_tank
• Disconnect while using (1st player)
• Reconnect (1st player). Note: 1st player will be able to move the angles of the entity without using it.
• 2nd player use the func_tank. Now func_tank attached to 2nd player and 1st can't move the angles of the entity.
• 2nd player uses retry command while using func_tank
• Finally crash server

I made a plugin to fix it.

I will later post at alliedmodders and edit here. Plugin posted: https://forums.alliedmods.net/showthread.php?t=317763

[RauliTop]

@mikela-valve seems that it was an attempt to fix this or something related: https://steamcommunity.com/games/70/announcements/detail/2199168953960582492

But the bug stills there. Should be solved on Next release because can cause server crash.

[SamVanheer]

I've fixed this issue like this: https://github.com/Solokiller/halflife-updated/commit/de2751ca4127d0e98bca1d15e9566d1118fbb1c8

It also accounts for the possibility that the player CBaseEntity instance is null (if the player connects and immediately disconnects there could be a small window where this can happen).