[11898][Patch] Bug exploit, instance reset.

[Auntie Mangos]

What bug does the patch fix? What features does the patch add?

Leader can reset instance while member is still on it but logged off. After members relog he keep their positions so that permit to bypass all trash.

With this patch, no reset is possible if any members is on instance leader try to reset.

For which repository revision was the patch created?

latest

Is there a thread in the bug report section or at lighthouse? If yes, please add a link to the thread.

found this old http://getmangos.eu/community/topic/11614/exploit-to-reset-instance/

Who has been writing this patch? Please include either forum user names or email addresses.

me

Patch:

Last one http://paste2.org/p/1889282

Old version only for ref http://paste2.org/p/1885673

diff --git a/src/game/Group.cpp b/src/game/Group.cpp
index e0cd4a6..c6f1b36 100644
--- a/src/game/Group.cpp
+++ b/src/game/Group.cpp
@@ -1168,6 +1168,7 @@ bool Group::_addMember(ObjectGuid guid, const char* name, bool isAssistant, uint
    member.name      = name;
    member.group     = group;
    member.assistant = isAssistant;
+    member.lastMap   = player->GetMapId();
    m_memberSlots.push_back(member);

    SubGroupCounterIncrease(group);
@@ -1667,6 +1668,18 @@ bool Group::InCombatToInstance(uint32 instanceId)
    return false;
}

+bool Group::SetPlayerMap(const ObjectGuid guid, uint32 mapid)
+{
+    member_witerator slot = _getMemberWSlot(guid);
+    if (slot != m_memberSlots.end())
+    {
+        slot->lastMap = mapid;
+        DEBUG_LOG("Group::SetPlayerMap> map is updated");
+        return true;
+    }
+    return false;
+}
+
void Group::ResetInstances(InstanceResetMethod method, bool isRaid, Player* SendMsgTo)
{
    if(isBGGroup())
@@ -1677,6 +1690,20 @@ void Group::ResetInstances(InstanceResetMethod method, bool isRaid, Player* Send
    // we assume that when the difficulty changes, all instances that can be reset will be
    Difficulty diff = GetDifficulty(isRaid);

+    OfflineMapList offlineMapList; // to store map of offline players
+
+    if ((method == INSTANCE_RESET_ALL) || (method == INSTANCE_RESET_CHANGE_DIFFICULTY))
+    {
+        // build offline members Map list. For instance reset check.
+        for(member_citerator itr = m_memberSlots.begin(); itr != m_memberSlots.end(); ++itr)
+        {
+            Player* plr = ObjectAccessor::FindPlayer(itr->guid);
+            // add map from offline player in the list.
+            if (!plr)
+                    offlineMapList.push_back(itr->lastMap); // add map from that player
+        }
+    }
+
    for(BoundInstancesMap::iterator itr = m_boundInstances[diff].begin(); itr != m_boundInstances[diff].end()
    {
        DungeonPersistentState *state = itr->second.state;
@@ -1701,7 +1728,20 @@ void Group::ResetInstances(InstanceResetMethod method, bool isRaid, Player* Send
        // if the map is loaded, reset it
        if (Map *map = sMapMgr.FindMap(state->GetMapId(), state->GetInstanceId()))
            if (map->IsDungeon() && !(method == INSTANCE_RESET_GROUP_DISBAND && !state->CanReset()))
-                isEmpty = ((DungeonMap*)map)->Reset(method);
+            {
+                // check if there is offline members
+                if (!offlineMapList.empty())
+                    for (OfflineMapList::iterator mapItr=offlineMapList.begin(); mapItr!=offlineMapList.end();mapItr++)
+                        if ((*mapItr)==map->GetId())    // check if mapId of one offline member is the same as one we want to reset
+                        {
+                            // if at least one of offline player is in map consider map not empty
+                            isEmpty=false;
+                            break;
+                        }
+                // now check if online players are in map
+                if (isEmpty)
+                    isEmpty = ((DungeonMap*)map)->Reset(method);
+            }

        if (SendMsgTo)
        {
diff --git a/src/game/Group.h b/src/game/Group.h
index b674389..1e0f219 100644
--- a/src/game/Group.h
+++ b/src/game/Group.h
@@ -195,6 +195,7 @@ class MANGOS_DLL_SPEC Group
            std::string name;
            uint8       group;
            bool        assistant;
+            uint32      lastMap;
        };
        typedef std::list<MemberSlot> MemberSlotList;
        typedef MemberSlotList::const_iterator member_citerator;
@@ -203,7 +204,7 @@ class MANGOS_DLL_SPEC Group
    protected:
        typedef MemberSlotList::iterator member_witerator;
        typedef std::set<Player*> InvitesList;
-
+        typedef std::list<uint32> OfflineMapList;
        typedef std::vector<Roll*> Rolls;

    public:
@@ -339,6 +340,8 @@ class MANGOS_DLL_SPEC Group

        void RewardGroupAtKill(Unit* pVictim, Player* player_tap);

+        bool SetPlayerMap(const ObjectGuid guid, uint32 mapid);
+
        /*********************************************************/
        /***                   LOOT SYSTEM                     ***/
        /*********************************************************/
diff --git a/src/game/MiscHandler.cpp b/src/game/MiscHandler.cpp
diff --git a/src/game/Player.cpp b/src/game/Player.cpp
index bffcaad..f3ce03f 100644
--- a/src/game/Player.cpp
+++ b/src/game/Player.cpp
@@ -1659,6 +1659,8 @@ bool Player::TeleportTo(uint32 mapid, float x, float y, float z, float orientati
        DEBUG_LOG("Player %s is being teleported to map %u", GetName(), mapid);
    }

+    if (Group* grp = GetGroup())
+        grp->SetPlayerMap(GetObjectGuid(), mapid);
    // if we were on a transport, leave
    if (!(options & TELE_TO_NOT_LEAVE_TRANSPORT) && m_transport)
    {

[VladimirMangos]

I think use DB query is very bad idea for this. Offline group member must still store in group members list data for own map at last online point (it can do it already, or can be added).

[cyberium]

Yeah thats was weird idea. Iam going to investigate it more. Thank for your good comment (has always).

[shauren]

for the record: .clear() method of stringstream does NOT clear the content, you need to call query.str("");

[cyberium]

Patch updated more efficient way. Comment are welcome.

[Schmoozerd]

in [11898]. Thank you

Remark for all interested:

The changes must be continued, we need a way to prevent players to login into a different difficulty-mode

[cyberium]

Crash is possible because player pointer used without test.

Possible solution http://paste2.org/p/1891496

[VladimirMangos]

> HashMapHolder<Player>::Find(guid);

Why use it instead GetPlayer

[cyberium]

More sure to get correct pointer than GetPlayer. It seem GetPlayer did not retrieve pointer in some case.

At least when player is being teleported.

Cheers

[VladimirMangos]

static Player* GetPlayer(ObjectGuid guid) { return ObjectAccessor::FindPlayer(guid); }

Note about static: ObjectMgr::GetPlayer(guid) better use.

Player* ObjectAccessor::FindPlayer(ObjectGuid guid) {     if (!guid)         return NULL;      Player * plr = HashMapHolder<Player>::Find(guid);;     if (!plr || !plr->IsInWorld())         return NULL;      return plr; }

So you right for just get map case.

[cyberium]

And i think for that part too or mysterious problem can occur

//on Group.cpp _addmember
if (player)
   {
       player->SetGroupInvite(NULL);
       //if player is in group and he is being added to BG raid group, then call SetBattleGroundRaid()
       if (player->GetGroup() && isBGGroup())
           player->SetBattleGroundRaid(this, group);
       //if player is in bg raid and we are adding him to normal group, then call SetOriginalGroup()
       else if ( player->GetGroup() )
           player->SetOriginalGroup(this, group);
       //if player is not in group, then call set group
       else
           player->SetGroup(this, group);

       // if the same group invites the player back, cancel the homebind timer
       if (InstanceGroupBind *bind = GetBoundInstance(player->GetMapId(), player))
           if (bind->state->GetInstanceId() == player->GetInstanceId())
               player->m_InstanceValid = true;
   }

This code assumed player is online and will be added to the group. Why (in original method) if for some case player is null we not execute this part??? I think something wrong here.

I think de best is to return false if player is null. It's the easyest way and secure.

[VladimirMangos]

player _must_ be added to group if offline. For example, at login raid member other raid members must be show in raid list independent if its offline or not.

[cyberium]

Thank for your comment Vladimir.

Anyway we talk about _addMember(...) This method is only for add member dynamicaly to group (after accept invitation)

This method are not called when any already member of an group goes offline/online. When member is added he is already in data group and showed correcttly online/offline in group-->sendUpdate().

At loggin group pointer is simply assigned to player because player is already on it.

This is how i understaind actual code, am i wrong?

If not thats why i think if player is null during _addMember so simply return false for this case instead to add player with some data not initialised. But we are ok it's simple way not the good way.

With respect.

[VladimirMangos]

I limited in mangos code view to repo html pages so hard for me follow all function uses. So I not sure that I can clarify case when you confirm that offline show state work without call.

[cyberium]

I understaind, hope you one day you change your mind and reinstall mangos source to take your hand on it

Cheers

[Schmoozerd]

First crash-fix added in 11920. Thank you.

But I agree that there is much more to be checked and improved as well.