IPB

Welcome Guest ( Log In | Register )

> Memory Corruption Bug
JRob
post Sep 20 2015, 11:33 PM
Post #1


Advanced Member
***

Group: Members
Posts: 52
Joined: 30-April 11
Member No.: 1,970



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;
User is offlineProfile CardPM
Go to the top of the page
+Quote Post
 
Reply to this topicStart new topic
Replies
Cheeseh
post Nov 20 2015, 08:12 AM
Post #2


Admin
*****

Group: Admin
Posts: 3,018
Joined: 11-September 03
From: uk
Member No.: 1



QUOTE(JRob @ Nov 17 2015, 08:39 AM) *

Found another issue here. A null edict_t is being passed to CBotTF2HealSched.

Shouldn't these task functions check if the edict is valid? Also these look like callbacks. Shouldn't they check if the edict is still valid? Shouldn't they all be using EHandles?

#0 0xe6788a88 in CBaseEdict::GetIServerEntity (this=0x0) at ../sdk/hl2sdk-ob-valve/public/edict.h:238
#1 0xe678931c in CClassInterfaceValue::getBool (this=0xffbc4d14, edict=0x0, defaultvalue=244, bIsEdict=false) at bot_getprop.h:196
#2 0xe68224a8 in CFindPathTask::debugString (this=0xb8648d30, string=0x0) at bot_task.cpp:2046
#3 0xe68144b1 in CBotTF2HealSched::CBotTF2HealSched (this=0xb3de6f40, pHeal=0x0) at bot_schedule.cpp:121
#4 0xe67c6eda in CBotFortress::setVisible (this=0xeb37f000, pEntity=0xec0000d8, bVisible=true) at bot_fortress.cpp:596
#5 0xe67d25bb in CBotTF2::setVisible (this=0xeb37f000, pEntity=0xec0000d8, bVisible=true) at bot_fortress.cpp:3945
#6 0xe683a141 in CBotVisibles::updateVisibles (this=0xb2c8c880) at bot_visibles.cpp:316
#7 0xe679771d in CBot::think (this=0xeb37f000) at bot.cpp:957
#8 0xe679fcf7 in CBots::botThink () at bot.cpp:3548
#9 0xe680a4b2 in RCBotPluginMeta::Hook_GameFrame (this=0xe68d8ce0 <g_RCBotPluginMeta>, simulating=true) at bot_plugin_meta.cpp:1413


I don't want to check every function. Some functions should just expect a non NULL pointer. if there is a crash then that's a sign of something going wrong in the code above. I don't know why it's going into DebugString function, it seems to be going wrong in there

QUOTE(JRob @ Nov 18 2015, 08:36 AM) *

I thought I mentioned this

https://sourceforge.net/p/rcbot2/code/480/t...2_mod.cpp#l1542

this needs to be ENTINDEX(pOwner) - 1


cheers
User is offlineProfile CardPM
Go to the top of the page
+Quote Post
Cheeseh
post Nov 21 2015, 09:03 AM
Post #3


Admin
*****

Group: Admin
Posts: 3,018
Joined: 11-September 03
From: uk
Member No.: 1



QUOTE(Cheeseh @ Nov 20 2015, 08:12 AM) *

I don't want to check every function. Some functions should just expect a non NULL pointer. if there is a crash then that's a sign of something going wrong in the code above. I don't know why it's going into DebugString function, it seems to be going wrong in there
cheers


weel the medic crash was caused by the change we made to the ehandle by making the edit NULL . the best idea is not to change it. I changed entityIsValid and the setvisible bValid Boolean and it's not crashing now
User is offlineProfile CardPM
Go to the top of the page
+Quote Post
JRob
post Nov 25 2015, 01:24 AM
Post #4


Advanced Member
***

Group: Members
Posts: 52
Joined: 30-April 11
Member No.: 1,970



QUOTE(Cheeseh @ Nov 21 2015, 09:03 AM) *

weel the medic crash was caused by the change we made to the ehandle by making the edit NULL . the best idea is not to change it. I changed entityIsValid and the setvisible bValid Boolean and it's not crashing now


I don't think you should remove the ehandle changing it to null, because in that case you are returning null anyway if the serial number doesn't match.

In the rare chance that it becomes valid in the future, you will be accessing the wrong entity.

Also do you know for sure that the serial number can't be 0?
User is offlineProfile CardPM
Go to the top of the page
+Quote Post
Cheeseh
post Nov 26 2015, 10:20 AM
Post #5


Admin
*****

Group: Admin
Posts: 3,018
Joined: 11-September 03
From: uk
Member No.: 1



QUOTE(JRob @ Nov 25 2015, 01:24 AM) *

I don't think you should remove the ehandle changing it to null, because in that case you are returning null anyway if the serial number doesn't match.

In the rare chance that it becomes valid in the future, you will be accessing the wrong entity.

Also do you know for sure that the serial number can't be 0?


this ehandle code comes from HL1 not HL2 so there may be some differences, Apparently hl2 invalid serial number is -1 (INVALID_EHANDLE_INDEX) so I might change it to that.

the problem with changing the edict to NULL is, in that in the code above I can assign an ehandle to an edict expecting the ehandle to have a valid edict, although once it goes into the assign code within the ehandle it will nullify the ehandle, therefore making the expected non null ehandle null, causing all sorts of problems. It doesn't matter whether or not its null as it will always return null anyway if its invalid, but the previous invalid edict will still be stored albeit inaccessible.
User is offlineProfile CardPM
Go to the top of the page
+Quote Post
JRob
post Nov 26 2015, 09:02 PM
Post #6


Advanced Member
***

Group: Members
Posts: 52
Joined: 30-April 11
Member No.: 1,970



QUOTE(Cheeseh @ Nov 26 2015, 10:20 AM) *

this ehandle code comes from HL1 not HL2 so there may be some differences, Apparently hl2 invalid serial number is -1 (INVALID_EHANDLE_INDEX) so I might change it to that.

the problem with changing the edict to NULL is, in that in the code above I can assign an ehandle to an edict expecting the ehandle to have a valid edict, although once it goes into the assign code within the ehandle it will nullify the ehandle, therefore making the expected non null ehandle null, causing all sorts of problems. It doesn't matter whether or not its null as it will always return null anyway if its invalid, but the previous invalid edict will still be stored albeit inaccessible.


I'm not sure I understand.

How do you assign an ehandle to an edict before the engine gives you a valid edict? As I understand it, edicts are given to you by the engine and you can't create them without a valid serial number.

Also I don't see the assignment operator checking the serial number. The edict needs to have a valid serial number when it is put into the ehandle because that's the only way and the only time it is saved to m_iSerialNumber.

MyEHandle ( edict_t *pent )
{
m_pEnt = pent;

if ( pent )
m_iSerialNumber = pent->m_NetworkSerialNumber;
else
m_iSerialNumber = 0;
}


inline edict_t * operator = ( edict_t *pent )
{
m_pEnt = pent;

if ( pent )
m_iSerialNumber = pent->m_NetworkSerialNumber;
else
m_iSerialNumber = 0;

return m_pEnt;
}
User is offlineProfile CardPM
Go to the top of the page
+Quote Post

Posts in this topic


Reply to this topicStart new topic
1 User(s) are reading this topic (1 Guests and 0 Anonymous Users)
0 Members:

 



- Lo-Fi Version Time is now: 12th November 2019 - 07:09 AM