Skip to content

Commit

Permalink
[11320] Check if Creature::GetRespawnCoord() function returns valid m…
Browse files Browse the repository at this point in the history 
…ap coordinates. Invalid data might cause crashes with movement generators.

Signed-off-by: Ambal <pogrebniak@gala.net>
  • Loading branch information
Ambal committed Apr 6, 2011
1 parent 09aa268 commit 52eb0dd
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 9 deletions.
23 changes: 15 additions & 8 deletions src/game/Creature.cpp
View file
Expand Up @@ -2120,18 +2120,25 @@ void Creature::GetRespawnCoord( float &x, float &y, float &z, float* ori, float*
*ori = data->orientation;
if (dist)
*dist = GetRespawnRadius();

return;
}
else
{
float orient;

float orient;
GetSummonPoint(x, y, z, orient);

GetSummonPoint(x, y, z, orient);
if (ori)
*ori = orient;
if (dist)
*dist = GetRespawnRadius();
}

if (ori)
*ori = orient;
if (dist)
*dist = GetRespawnRadius();
//lets check if our creatures have valid spawn coordinates
if(!MaNGOS::IsValidMapCoord(x, y, z))
{
sLog.outError("Creature with invalid respawn coordinates: mapid = %u, guid = %u, x = %f, y = %f, z = %f", GetMapId(), GetGUIDLow(), x, y, z);
MANGOS_ASSERT(false);
}
}

void Creature::AllLootRemovedFromCorpse()
Expand Down
2 changes: 1 addition & 1 deletion src/shared/revision_nr.h
View file
@@ -1,4 +1,4 @@
#ifndef __REVISION_NR_H__
#define __REVISION_NR_H__
#define REVISION_NR "11319"
#define REVISION_NR "11320"
#endif // __REVISION_NR_H__

7 comments on commit 52eb0dd

@VladimirMangos
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what reason check this? summon point is real point already added creature that already checked at adding. DB data checked at loading.

@Ambal
Copy link
Contributor

@Ambal Ambal commented on 52eb0dd Apr 6, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reason is: https://gist.github.com/904015 . More details in comments for [11306]

@rsa
Copy link
Contributor

@rsa rsa commented on 52eb0dd Apr 6, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cheating, stupid GM's... additional checks - not bad.

@VladimirMangos
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checks in random place useless. If we say about coordinates check then it must checked in sources where wrong value can be added.
NOT in place where it get. Specially unconditionaly not under MANGOS_ASSERT

@Ambal
Copy link
Contributor

@Ambal Ambal commented on 52eb0dd Apr 6, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vlad, if you have thoughts where we can improve such checks to prevent wrong data assignment - you are welcome. Problem is that placing checks in getters() will definitely yield results while you can chase setters() forever since incorrect(!) setter() usage will produce bugs even if all checks will be successful. As an example, checks in WorldObject::GetMap() function allowed us to get rid of all the nasty code in core related to Map data queries.

@VladimirMangos
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not make it in normal way for debug checks: guard by MANGOS_DEBUG #ifdef at least. You add check affect all players because rsa at havy modified sources get crash by assign wrong values in some unknown place. It's maybe assigned long before crash, maybe in prev tick and etc. Maybe rsa just not have set summon pos in one from summon calls and have garbage in related fields...

@Ambal
Copy link
Contributor

@Ambal Ambal commented on 52eb0dd Apr 6, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vlad, we should protect mangos core from bad patches - if clean core will not crash with these checks, then this is not out problem that someone screwed main logic. We shouldn't care about patches which work as they want and not as they should. If you want, you are free to replace MANGOS_ASSERT() by less strict check but keep in mind: crappy code will crash in any way.

Please sign in to comment.