Memory Corruption Bug Options

[JRob]

I've been looking at the entity handle code, and I think it is bugged and causing crashes.

http://sourceforge.net/p/rcbot2/code/HEAD/...t_ehandle.h#l59

First of all, it should null m_pEnt if it is free or the serial number changed. It shoudn't just leave it until another entity reuses it. even though the chances might be low.

Next, there is a bunch of places where these MyEHandle aren't checked for null (the wrapped entity, not the MyEHandle itself). The first thing that crashed for me was here

http://sourceforge.net/p/rcbot2/code/HEAD/...rtress.cpp#l671

and there are probably a lot more. I fixed CBotFortress :: setVisible though.

CODE

bool CBotFortress :: setVisible ( edict_t *pEntity, bool bVisible )
{
    bool bValid = CBot::setVisible(pEntity,bVisible);

    // check for people to heal
    if ( m_iClass == TF_CLASS_MEDIC )
    {
        if ( bValid && bVisible )
        {
            if (CBotGlobals::isPlayer(pEntity) ) // player
            {
                CBotWeapon *pMedigun = m_pWeapons->getWeapon(CWeapons::getWeapon(TF2_WEAPON_MEDIGUN));
                bool bIsSpy = CClassInterface::getTF2Class(pEntity)==TF_CLASS_SPY;
                int iDisguise = 0;

                if ( bIsSpy )
                {
                    CClassInterface::getTF2SpyDisguised(pEntity,&iDisguise,NULL,NULL,NULL);
                }

                if ( pMedigun && pMedigun->hasWeapon() &&
                    (  // Heal my team member or a spy if I think he is on my team
                       (CBotGlobals::getTeam(pEntity) == getTeam()) ||
                       ((bIsSpy&&!thinkSpyIsEnemy(pEntity,(TF_Class)iDisguise)))
                    ) )
                {
                    Vector vPlayer = CBotGlobals::entityOrigin(pEntity);

                    if ( distanceFrom(vPlayer) <= CWaypointLocations::REACHABLE_RANGE )
                    {
                        float fFactor;

                        if ( (fFactor = getHealFactor(pEntity)) > 0 )
                        {
                            if ( m_pHeal.get() != NULL )
                            {
                                if ( m_pHeal != pEntity )
                                {
                                    if ( fFactor > m_fHealFactor )
                                    {            
                                        m_pHeal = pEntity;
                                        m_fHealFactor = fFactor;
                                        updateCondition(CONDITION_SEE_HEAL);
                                    }
                                }
                                else
                                {
                                    // not healing -- what am I doing?
                                    if ( !m_pSchedules->hasSchedule(SCHED_HEAL) )
                                    {
                                        // not healing -- what am I doing?
                                        m_pSchedules->freeMemory();
                                        m_pSchedules->addFront(new CBotTF2HealSched(m_pHeal));
                                    }
                                }
                            }
                            else
                            {                                
                                m_fHealFactor = fFactor;
                                m_pHeal = pEntity;
                                updateCondition(CONDITION_SEE_HEAL);

                                if ( !m_pSchedules->hasSchedule(SCHED_HEAL) )
                                {
                                    // not healing -- what am I doing?
                                    m_pSchedules->freeMemory();
                                    m_pSchedules->addFront(new CBotTF2HealSched(m_pHeal));
                                }
                            }
                        }
                    }
                }
            }
        }
        else if ( m_pHeal == pEntity )
        {
            m_pHeal = NULL;
            removeCondition(CONDITION_SEE_HEAL);
        }
    }
    //else if ( m_iClass == TF_CLASS_SPY ) // Fix
    //{
        // Look for nearest sentry to sap!!!
    if ( bValid && bVisible )
    {
        if ( CTeamFortress2Mod::isSentry(pEntity,CTeamFortress2Mod::getEnemyTeam(getTeam())) )
        {
            if ( (m_iClass!=TF_CLASS_ENGINEER)||!CClassInterface::isObjectCarried(pEntity) )
            {
                if ( !m_pNearestEnemySentry || m_pNearestEnemySentry.notValid() || ((pEntity != m_pNearestEnemySentry) && (distanceFrom(pEntity) < distanceFrom(m_pNearestEnemySentry)) ))
                {
                    m_pNearestEnemySentry = pEntity;
                }
            }
        }
        else if ( CTeamFortress2Mod::isTeleporter(pEntity,CTeamFortress2Mod::getEnemyTeam(getTeam())) )
        {
            if ( !m_pNearestEnemyTeleporter || m_pNearestEnemyTeleporter.notValid() || ((pEntity != m_pNearestEnemyTeleporter)&&(distanceFrom(pEntity)<distanceFrom(m_pNearestEnemyTeleporter))))
            {
                m_pNearestEnemyTeleporter = pEntity;
            }
        }
        else if ( CTeamFortress2Mod::isDispenser(pEntity,CTeamFortress2Mod::getEnemyTeam(getTeam())) )
        {
            if ( !m_pNearestEnemyDisp || m_pNearestEnemyDisp.notValid() || ((pEntity != m_pNearestEnemyDisp)&&(distanceFrom(pEntity)<distanceFrom(m_pNearestEnemyDisp))))
            {
                m_pNearestEnemyDisp = pEntity;
            }
        }
        else if ( CTeamFortress2Mod::isHurtfulPipeGrenade(pEntity,m_pEdict) )
        {
            if ( !m_pNearestPipeGren || m_pNearestPipeGren.notValid() || ((pEntity != m_pNearestPipeGren)&&(distanceFrom(pEntity)<distanceFrom(m_pNearestPipeGren))))
            {
                m_pNearestPipeGren = pEntity;
            }
        }
    }
    else if ( pEntity == m_pNearestEnemySentry )
    {
        m_pNearestEnemySentry = NULL;
    }
    else if ( pEntity == m_pNearestEnemyTeleporter )
    {
        m_pNearestEnemyTeleporter = NULL;
    }
    else if ( pEntity == m_pNearestEnemyDisp )
    {
        m_pNearestEnemyDisp = NULL;
    }
    else if ( pEntity == m_pNearestPipeGren )
    {
        m_pNearestPipeGren = NULL;
    }

    //}

    // Check for nearest Dispenser for health/ammo & flag
    if ( bValid && bVisible && !(CClassInterface::getEffects(pEntity)&EF_NODRAW) ) // EF_NODRAW == invisible
    {
        if ( m_pFlag != pEntity && CTeamFortress2Mod::isFlag(pEntity,getTeam()) )
            m_pFlag = pEntity;
        else if ( (m_pNearestAllySentry != pEntity) && CTeamFortress2Mod::isSentry(pEntity,getTeam()) )
        {
            if ( !m_pNearestAllySentry || m_pNearestAllySentry.notValid() || (distanceFrom(pEntity) < distanceFrom(m_pNearestAllySentry)))
                m_pNearestAllySentry = pEntity;
        }
        else if ( (m_pNearestDisp != pEntity) && CTeamFortress2Mod::isDispenser(pEntity,getTeam()) )
        {
            if ( !m_pNearestDisp || m_pNearestDisp.notValid() || (distanceFrom(pEntity) < distanceFrom(m_pNearestDisp)) )
                m_pNearestDisp = pEntity;
        }
        else if ( (pEntity != m_pNearestTeleEntrance) && CTeamFortress2Mod::isTeleporterEntrance(pEntity,getTeam()) )
        {
            if ( !m_pNearestTeleEntrance || m_pNearestTeleEntrance.notValid() || (distanceFrom(pEntity) < distanceFrom(m_pNearestTeleEntrance)))
                m_pNearestTeleEntrance = pEntity;
        }
        else if ( (pEntity != m_pAmmo) && CTeamFortress2Mod::isAmmo(pEntity) )
        {
            static float fDistance;

            fDistance = distanceFrom(pEntity);

            if ( fDistance <= 200 )
            {

                if ( !m_pAmmo || m_pAmmo.notValid() || (fDistance < distanceFrom(m_pAmmo)))
                    m_pAmmo = pEntity;
            }
            
        }
        else if ( (pEntity != m_pHealthkit) && CTeamFortress2Mod::isHealthKit(pEntity) )
        {
            static float fDistance;

            fDistance = distanceFrom(pEntity);

            if ( fDistance <= 200 )
            {
                if ( !m_pHealthkit || m_pHealthkit.notValid() || (fDistance < distanceFrom(m_pHealthkit)))
                    m_pHealthkit = pEntity;
            }
        }
    }
    else
    {
        if ( pEntity == m_pFlag.get_old() )
            m_pFlag = NULL;
        else if ( pEntity == m_pNearestDisp.get_old() )
            m_pNearestDisp = NULL;
        else if ( pEntity == m_pAmmo.get_old() )
            m_pAmmo = NULL;
        else if ( pEntity == m_pHealthkit.get_old() )
            m_pHealthkit = NULL;
        else if ( pEntity == m_pHeal.get_old() )
            m_pHeal = NULL;
        else if ( pEntity == m_pNearestPipeGren.get_old() )
            m_pNearestPipeGren = NULL;
    }

    return bValid;
}

Here are the MyEHandles in bot_fortress.cpp. It looks like you are correctly checking some of them.

MyEHandle m_pHeal;
MyEHandle m_pLastHeal;
MyEHandle m_pSentryGun;
MyEHandle m_pDispenser;
MyEHandle m_pTeleEntrance;
MyEHandle m_pTeleExit;

MyEHandle m_pAmmo;
MyEHandle m_pHealthkit;

MyEHandle m_pNearestDisp;
MyEHandle m_pNearestEnemySentry;
MyEHandle m_pNearestAllySentry;
MyEHandle m_pNearestEnemyTeleporter;
MyEHandle m_pNearestEnemyDisp;
MyEHandle m_pNearestTeleEntrance;
MyEHandle m_pNearestPipeGren;

MyEHandle m_pFlag;
MyEHandle m_pPrevSpy;

MyEHandle m_pHealer;

MyEHandle m_pLastEnemySentry;
MyEHandle m_NearestEnemyRocket;
MyEHandle m_NearestEnemyGrenade;


MyEHandle m_pDefendPayloadBomb;
MyEHandle m_pPushPayloadBomb;
MyEHandle m_pRedPayloadBomb;
MyEHandle m_pBluePayloadBomb;

[JRob]

Found another crash, possibly related.

This code is accessing out the bounds of the m_SentryGuns array.

http://sourceforge.net/p/rcbot2/code/HEAD/...ot_mods.h#l1082

It is going from int i = 1; i <= gpGlobals->maxClients when every other function does i = 0; i < MAX_PLAYERS

You'll probably also need to change INDEXENT(i-1) to INDEXENT(i+1)

And another one.

http://sourceforge.net/p/rcbot2/code/HEAD/...2_mod.cpp#l1412

And another

http://sourceforge.net/p/rcbot2/code/HEAD/...2_mod.cpp#l1461

Another one

http://sourceforge.net/p/rcbot2/code/HEAD/...a/bot.cpp#l1884

It looks like there are a lot of off by 1 bugs involving the use of gpGlobals->maxClients

A missing check for edict->IsFree()

http://sourceforge.net/p/rcbot2/code/HEAD/...a/bot.cpp#l3239

m_Resource needs to be checked for validity.

http://sourceforge.net/p/rcbot2/code/HEAD/...2_points.h#l307

[Cheeseh]

I've fixed the sentry gun stuff, those were silly mistakes

Though the logic here is fine:

CODE

if ( !m_pAmmo || (fDistance < distanceFrom(m_pAmmo)))

It won't execute (fDistance < distanceFrom(m_pAmmo) unless m_pAmmo is not NULL.

If it is crashing here then I've yet to experience it, but it could be because of another unexpected problem within the operator. I'd rather have the code there than adding extra functions

CODE

    inline operator edict_t * const ()
    { // same as get function (inlined for speed)
        if ( m_iSerialNumber && m_pEnt )
        {
            if ( !m_pEnt->IsFree() && (m_iSerialNumber == m_pEnt->m_NetworkSerialNumber) )
                return m_pEnt;
        }
        else if ( m_pEnt )
            m_pEnt = NULL;

        return NULL;
    }

CODE

    if ( (m_iStatsIndex == 0) || ( m_iStatsIndex > gpGlobals->maxClients ) )
    {
        if ( m_iStatsIndex != 0 )
            m_bStatsCanUse = true;


        m_StatsCanUse.data = m_Stats.data;
        m_Stats.data = 0;
        m_iStatsIndex = 0; // reset to be sure in case of m_iStatsIndex > gpGlobals->maxClients


        if ( !m_uSquadDetail.b1.said_area_clear && (m_StatsCanUse.stats.m_iEnemiesInRange == 0) && (m_StatsCanUse.stats.m_iEnemiesVisible == 0) && (m_StatsCanUse.stats.m_iTeamMatesInRange > 0))
        {
            if ( !inSquad() || isSquadLeader() && (m_fLastSeeEnemy && ((m_fLastSeeEnemy + 10.0f)<engine->Time())) )

                areaClear();

            m_uSquadDetail.b1.said_area_clear = true;

        }
    }

    CClient *pClient = CClients::get(m_iStatsIndex++);

This is fine too. Which means if m_iStatsIndex is outside the bounds of players it will reset to zero. Every CClient is the same as an INDEXENT(m_iStatsIndex+1) However it doesn't need to use CClients I'm going to change it just to use INDEXENT.

CODE

    CTeamRoundTimer()::MyEHandle m_Resource;

this is not referenced in the code. it is only set once at map change.

[JRob]

I've fixed the sentry gun stuff, those were silly mistakes

Though the logic here is fine:

CODE

if ( !m_pAmmo || (fDistance < distanceFrom(m_pAmmo)))

It won't execute (fDistance < distanceFrom(m_pAmmo) unless m_pAmmo is not NULL.

If it is crashing here then I've yet to experience it, but it could be because of another unexpected problem within the operator. I'd rather have the code there than adding extra functions

CODE

    inline operator edict_t * const ()
    { // same as get function (inlined for speed)
        if ( m_iSerialNumber && m_pEnt )
        {
            if ( !m_pEnt->IsFree() && (m_iSerialNumber == m_pEnt->m_NetworkSerialNumber) )
                return m_pEnt;
        }
        else if ( m_pEnt )
            m_pEnt = NULL;

        return NULL;
    }

CODE

    if ( (m_iStatsIndex == 0) || ( m_iStatsIndex > gpGlobals->maxClients ) )
    {
        if ( m_iStatsIndex != 0 )
            m_bStatsCanUse = true;
        m_StatsCanUse.data = m_Stats.data;
        m_Stats.data = 0;
        m_iStatsIndex = 0; // reset to be sure in case of m_iStatsIndex > gpGlobals->maxClients
        if ( !m_uSquadDetail.b1.said_area_clear && (m_StatsCanUse.stats.m_iEnemiesInRange == 0) && (m_StatsCanUse.stats.m_iEnemiesVisible == 0) && (m_StatsCanUse.stats.m_iTeamMatesInRange > 0))
        {
            if ( !inSquad() || isSquadLeader() && (m_fLastSeeEnemy && ((m_fLastSeeEnemy + 10.0f)<engine->Time())) )

                areaClear();

            m_uSquadDetail.b1.said_area_clear = true;

        }
    }

    CClient *pClient = CClients::get(m_iStatsIndex++);

This is fine too. Which means if m_iStatsIndex is outside the bounds of players it will reset to zero. Every CClient is the same as an INDEXENT(m_iStatsIndex+1) However it doesn't need to use CClients I'm going to change it just to use INDEXENT.

CODE

    CTeamRoundTimer()::MyEHandle m_Resource;

this is not referenced in the code. it is only set once at map change.

No you still need to check m_pAmmo. Just because m_pAmmo isn't null doesn't mean it is valid anymore because it could have been deleted by code outside of rcbot. That's the whole point of of the EHandle, to tell when an entity was deleted outside your control. And also m_pAmmo being null only means that it doesn't have a pointer to MyEhandle, it doesn't check the underlying entity to see if it is valid.

I assure you that the m_iStatsIndex code isn't fine otherwise it wouldn't be crashing there, and doesn't crash anymore after I fixed it.

You have defined an array from 0 to 32 (33 entries). With that code you are stopping right after 33 since gpGlobals->maxClients is 33 when you have the replay bot or using the player limit increaser. You probably never see it crash because you only test it on your listen server.

Every CClient is the same as an INDEXENT(m_iStatsIndex+1)

This is not correct. All your arrays are from 0 to MAXPLAYERS-1. Also CClients :: slotOfEdict does ENTINDEX(pPlayer)-1, so CClients is 0 based, not 1 based.

static CClient m_Clients[MAX_PLAYERS];

This defines an array from 0 to 32. If you wanted it to go from 1 to 33, you need to do m_teleporters[MAX_PLAYERS+1]

Also you are doing CClients::get(m_iStatsIndex++);. This increments the index pointer after you access it. So you are not accessing m_iStatsIndex+1.

And with https://sourceforge.net/p/rcbot2/code/HEAD/...2_points.h#l307 that is the exact location of the crash as indicated by crash dumps. I am not guessing where crashes might occur, it has actually happened there. The problem is simple to figure out. The underlying entity became invalid and thus m_nSetupTimeLength turned into an invalid access

Just found another crash. https://sourceforge.net/p/rcbot2/code/HEAD/...rtress.cpp#l880

m_pAvoidEntity immediately detects pDetector is invalid and turns into null.

[Cheeseh]

No you still need to check m_pAmmo. Just because m_pAmmo isn't null doesn't mean it is valid anymore because it could have been deleted by code outside of rcbot. That's the whole point of of the EHandle, to tell when an entity was deleted outside your control. And also m_pAmmo being null only means that it doesn't have a pointer to MyEhandle, it doesn't check the underlying entity to see if it is valid.

I understand the point of Ehandle . If you call !m_pAmmo the following code is run : therefore if the serialnumber isn't matched it will return NULL. So it does check whether or not it is valid without extra code.

CODE

   inline operator edict_t * const ()
    { // same as get function (inlined for speed)
        if ( m_iSerialNumber && m_pEnt )
        {
            if ( !m_pEnt->IsFree() && (m_iSerialNumber == m_pEnt->m_NetworkSerialNumber) )
                return m_pEnt;
        }
        else if ( m_pEnt )
            m_pEnt = NULL;

        return NULL;
    }

This works fine in Windows in Visual Studio. Perhaps Linux the operator doesn't overload

[JRob]

You've correctly converted from 1-based index to a 0-based index there, but I am telling you your loop is off by 1.

You terminate the loop at m_iStatsIndex > gpGlobals->maxClients when it should be terminated at m_iStatsIndex = gpGlobals->maxClients.

gpGlobals->maxClients is 33 so the client array goes from 0 to 32. And m_iStatsIndex tries to access index 33.

CODE

inline operator edict_t * const ()

I don't follow how doing !m_pAmmo would execute this code. That code should only run when the variable is casted or put into a parameter that requires an edict_t*. I believe you would have to overload the ! operator to do that.

Also I believe the m_nSetupTimeLength length was happening on random maps.