Fix wrong use of uninitialized locks.

Whenever ACE_XXX_Thread_Mutexes are used, there are 3 fundamental rules to obey:
1. Always make sure the lock is initialized before use;
2. Never put 2 locks each other in memory (false sharing effect);
3. Always verify that the lock is really acquired - use ACE_XXX_GUARD macros;
This commit is contained in:
H0zen 2016-03-11 20:53:59 +02:00
parent 96dc9c854f
commit be9e1176d9
8 changed files with 34 additions and 25 deletions

View File

@ -43,7 +43,10 @@
INSTANTIATE_SINGLETON_2(ObjectAccessor, CLASS_LOCK); INSTANTIATE_SINGLETON_2(ObjectAccessor, CLASS_LOCK);
INSTANTIATE_CLASS_MUTEX(ObjectAccessor, ACE_Thread_Mutex); INSTANTIATE_CLASS_MUTEX(ObjectAccessor, ACE_Thread_Mutex);
ObjectAccessor::ObjectAccessor() {} ObjectAccessor::ObjectAccessor() : i_playerGuard(), i_corpseGuard()
{
}
ObjectAccessor::~ObjectAccessor() ObjectAccessor::~ObjectAccessor()
{ {
for (Player2CorpsesMapType::const_iterator itr = i_player2corpse.begin(); itr != i_player2corpse.end(); ++itr) for (Player2CorpsesMapType::const_iterator itr = i_player2corpse.begin(); itr != i_player2corpse.end(); ++itr)
@ -284,7 +287,7 @@ void ObjectAccessor::RemoveOldCorpses()
/// Define the static member of HashMapHolder /// Define the static member of HashMapHolder
template <class T> typename HashMapHolder<T>::MapType HashMapHolder<T>::m_objectMap; template <class T> typename HashMapHolder<T>::MapType HashMapHolder<T>::m_objectMap;
template <class T> ACE_RW_Thread_Mutex HashMapHolder<T>::i_lock; template <class T> typename HashMapHolder<T>::LockType HashMapHolder<T>::i_lock;
/// Global definitions for the hashmap storage /// Global definitions for the hashmap storage

View File

@ -137,6 +137,7 @@ class ObjectAccessor : public MaNGOS::Singleton<ObjectAccessor, MaNGOS::ClassLev
typedef ACE_Thread_Mutex LockType; typedef ACE_Thread_Mutex LockType;
LockType i_playerGuard; LockType i_playerGuard;
char _cache_guard[1024];
LockType i_corpseGuard; LockType i_corpseGuard;
}; };

View File

@ -730,7 +730,7 @@ bool GridMap::ExistVMap(uint32 mapid, int gx, int gy)
} }
////////////////////////////////////////////////////////////////////////// //////////////////////////////////////////////////////////////////////////
TerrainInfo::TerrainInfo(uint32 mapid) : m_mapId(mapid) TerrainInfo::TerrainInfo(uint32 mapid) : m_mapId(mapid), m_refMutex(), m_mutex()
{ {
for (int k = 0; k < MAX_NUMBER_OF_GRIDS; ++k) for (int k = 0; k < MAX_NUMBER_OF_GRIDS; ++k)
{ {
@ -831,7 +831,7 @@ int TerrainInfo::RefGrid(const uint32& x, const uint32& y)
MANGOS_ASSERT(x < MAX_NUMBER_OF_GRIDS); MANGOS_ASSERT(x < MAX_NUMBER_OF_GRIDS);
MANGOS_ASSERT(y < MAX_NUMBER_OF_GRIDS); MANGOS_ASSERT(y < MAX_NUMBER_OF_GRIDS);
LOCK_GUARD _lock(m_refMutex); ACE_GUARD_RETURN(LOCK_TYPE, _lock, m_refMutex, -1)
return (m_GridRef[x][y] += 1); return (m_GridRef[x][y] += 1);
} }
@ -842,7 +842,7 @@ int TerrainInfo::UnrefGrid(const uint32& x, const uint32& y)
int16& iRef = m_GridRef[x][y]; int16& iRef = m_GridRef[x][y];
LOCK_GUARD _lock(m_refMutex); ACE_GUARD_RETURN(LOCK_TYPE, _lock, m_refMutex, -1)
if (iRef > 0) if (iRef > 0)
{ return (iRef -= 1); } { return (iRef -= 1); }
@ -1151,7 +1151,7 @@ GridMap* TerrainInfo::LoadMapAndVMap(const uint32 x, const uint32 y)
// double checked lock pattern // double checked lock pattern
if (!m_GridMaps[x][y]) if (!m_GridMaps[x][y])
{ {
LOCK_GUARD lock(m_mutex); ACE_GUARD_RETURN(LOCK_TYPE, lock, m_mutex, NULL)
if (!m_GridMaps[x][y]) if (!m_GridMaps[x][y])
{ {
@ -1225,7 +1225,7 @@ float TerrainInfo::GetWaterLevel(float x, float y, float z, float* pGround /*= N
INSTANTIATE_SINGLETON_2(TerrainManager, CLASS_LOCK); INSTANTIATE_SINGLETON_2(TerrainManager, CLASS_LOCK);
INSTANTIATE_CLASS_MUTEX(TerrainManager, ACE_Thread_Mutex); INSTANTIATE_CLASS_MUTEX(TerrainManager, ACE_Thread_Mutex);
TerrainManager::TerrainManager() TerrainManager::TerrainManager() : m_mutex()
{ {
} }
@ -1237,7 +1237,7 @@ TerrainManager::~TerrainManager()
TerrainInfo* TerrainManager::LoadTerrain(const uint32 mapId) TerrainInfo* TerrainManager::LoadTerrain(const uint32 mapId)
{ {
Guard _guard(*this); ACE_GUARD_RETURN(LOCK_TYPE, _guard, m_mutex, NULL)
TerrainInfo* ptr = NULL; TerrainInfo* ptr = NULL;
TerrainDataMap::const_iterator iter = i_TerrainMap.find(mapId); TerrainDataMap::const_iterator iter = i_TerrainMap.find(mapId);
@ -1257,7 +1257,7 @@ void TerrainManager::UnloadTerrain(const uint32 mapId)
if (sWorld.getConfig(CONFIG_BOOL_GRID_UNLOAD) == 0) if (sWorld.getConfig(CONFIG_BOOL_GRID_UNLOAD) == 0)
{ return; } { return; }
Guard _guard(*this); ACE_GUARD(LOCK_TYPE, _guard, m_mutex)
TerrainDataMap::iterator iter = i_TerrainMap.find(mapId); TerrainDataMap::iterator iter = i_TerrainMap.find(mapId);
if (iter != i_TerrainMap.end()) if (iter != i_TerrainMap.end())

View File

@ -281,8 +281,8 @@ class TerrainInfo : public Referencable<AtomicLong>
ShortIntervalTimer i_timer; ShortIntervalTimer i_timer;
typedef ACE_Thread_Mutex LOCK_TYPE; typedef ACE_Thread_Mutex LOCK_TYPE;
typedef ACE_Guard<LOCK_TYPE> LOCK_GUARD;
LOCK_TYPE m_mutex; LOCK_TYPE m_mutex;
char _cache_guard[1024];
LOCK_TYPE m_refMutex; LOCK_TYPE m_refMutex;
}; };
@ -328,7 +328,8 @@ class TerrainManager : public MaNGOS::Singleton<TerrainManager, MaNGOS::ClassLev
TerrainManager(const TerrainManager&); TerrainManager(const TerrainManager&);
TerrainManager& operator=(const TerrainManager&); TerrainManager& operator=(const TerrainManager&);
typedef MaNGOS::ClassLevelLockable<TerrainManager, ACE_Thread_Mutex>::Lock Guard; typedef ACE_Thread_Mutex LOCK_TYPE;
LOCK_TYPE m_mutex;
TerrainDataMap i_TerrainMap; TerrainDataMap i_TerrainMap;
}; };

View File

@ -27,6 +27,11 @@
#include "GameSystem/Grid.h" #include "GameSystem/Grid.h"
#include "Log.h" #include "Log.h"
GridState::~GridState()
{
DEBUG_LOG("GridState destroyed");
}
void void
InvalidState::Update(Map&, NGridType&, GridInfo&, const uint32& /*x*/, const uint32& /*y*/, const uint32&) const InvalidState::Update(Map&, NGridType&, GridInfo&, const uint32& /*x*/, const uint32& /*y*/, const uint32&) const
{ {

View File

@ -32,6 +32,7 @@ class GridState
public: public:
virtual void Update(Map&, NGridType&, GridInfo&, const uint32& x, const uint32& y, const uint32& t_diff) const = 0; virtual void Update(Map&, NGridType&, GridInfo&, const uint32& x, const uint32& y, const uint32& t_diff) const = 0;
virtual ~GridState();
}; };
class InvalidState : public GridState class InvalidState : public GridState

View File

@ -39,7 +39,7 @@ INSTANTIATE_SINGLETON_2(MapManager, CLASS_LOCK);
INSTANTIATE_CLASS_MUTEX(MapManager, ACE_Recursive_Thread_Mutex); INSTANTIATE_CLASS_MUTEX(MapManager, ACE_Recursive_Thread_Mutex);
MapManager::MapManager() MapManager::MapManager()
: i_gridCleanUpDelay(sWorld.getConfig(CONFIG_UINT32_INTERVAL_GRIDCLEAN)) : i_gridCleanUpDelay(sWorld.getConfig(CONFIG_UINT32_INTERVAL_GRIDCLEAN)), m_lock()
{ {
i_timer.SetInterval(sWorld.getConfig(CONFIG_UINT32_INTERVAL_MAPUPDATE)); i_timer.SetInterval(sWorld.getConfig(CONFIG_UINT32_INTERVAL_MAPUPDATE));
} }
@ -101,7 +101,7 @@ void MapManager::InitializeVisibilityDistanceInfo()
/// @param id - MapId of the to be created map. @param obj WorldObject for which the map is to be created. Must be player for Instancable maps. /// @param id - MapId of the to be created map. @param obj WorldObject for which the map is to be created. Must be player for Instancable maps.
Map* MapManager::CreateMap(uint32 id, const WorldObject* obj) Map* MapManager::CreateMap(uint32 id, const WorldObject* obj)
{ {
Guard _guard(*this); ACE_GUARD_RETURN(LOCK_TYPE, _guard, m_lock, NULL)
Map* m = NULL; Map* m = NULL;
@ -139,13 +139,13 @@ Map* MapManager::CreateBgMap(uint32 mapid, BattleGround* bg)
{ {
sTerrainMgr.LoadTerrain(mapid); sTerrainMgr.LoadTerrain(mapid);
Guard _guard(*this); ACE_GUARD_RETURN(LOCK_TYPE, _guard, m_lock, NULL)
return CreateBattleGroundMap(mapid, sMapMgr.GenerateInstanceId(), bg); return CreateBattleGroundMap(mapid, sMapMgr.GenerateInstanceId(), bg);
} }
Map* MapManager::FindMap(uint32 mapid, uint32 instanceId) const Map* MapManager::FindMap(uint32 mapid, uint32 instanceId) const
{ {
Guard guard(*this); ACE_GUARD_RETURN(LOCK_TYPE, _guard, m_lock, NULL)
MapMapType::const_iterator iter = i_maps.find(MapID(mapid, instanceId)); MapMapType::const_iterator iter = i_maps.find(MapID(mapid, instanceId));
if (iter == i_maps.end()) if (iter == i_maps.end())
@ -163,7 +163,7 @@ Map* MapManager::FindMap(uint32 mapid, uint32 instanceId) const
void MapManager::DeleteInstance(uint32 mapid, uint32 instanceId) void MapManager::DeleteInstance(uint32 mapid, uint32 instanceId)
{ {
Guard _guard(*this); ACE_GUARD(LOCK_TYPE, _guard, m_lock)
MapMapType::iterator iter = i_maps.find(MapID(mapid, instanceId)); MapMapType::iterator iter = i_maps.find(MapID(mapid, instanceId));
if (iter != i_maps.end()) if (iter != i_maps.end())
@ -276,9 +276,9 @@ void MapManager::InitMaxInstanceId()
uint32 MapManager::GetNumInstances() uint32 MapManager::GetNumInstances()
{ {
Guard guard(*this);
uint32 ret = 0; uint32 ret = 0;
ACE_GUARD_RETURN(LOCK_TYPE, _guard, m_lock, ret)
for (MapMapType::iterator itr = i_maps.begin(); itr != i_maps.end(); ++itr) for (MapMapType::iterator itr = i_maps.begin(); itr != i_maps.end(); ++itr)
{ {
Map* map = itr->second; Map* map = itr->second;
@ -290,9 +290,9 @@ uint32 MapManager::GetNumInstances()
uint32 MapManager::GetNumPlayersInInstances() uint32 MapManager::GetNumPlayersInInstances()
{ {
Guard guard(*this);
uint32 ret = 0; uint32 ret = 0;
ACE_GUARD_RETURN(LOCK_TYPE, _guard, m_lock, ret)
for (MapMapType::iterator itr = i_maps.begin(); itr != i_maps.end(); ++itr) for (MapMapType::iterator itr = i_maps.begin(); itr != i_maps.end(); ++itr)
{ {
Map* map = itr->second; Map* map = itr->second;

View File

@ -59,10 +59,6 @@ class MapManager : public MaNGOS::Singleton<MapManager, MaNGOS::ClassLevelLockab
{ {
friend class MaNGOS::OperatorNew<MapManager>; friend class MaNGOS::OperatorNew<MapManager>;
typedef ACE_Recursive_Thread_Mutex LOCK_TYPE;
typedef ACE_Guard<LOCK_TYPE> LOCK_TYPE_GUARD;
typedef MaNGOS::ClassLevelLockable<MapManager, ACE_Recursive_Thread_Mutex>::Lock Guard;
public: public:
typedef std::map<MapID, Map* > MapMapType; typedef std::map<MapID, Map* > MapMapType;
@ -185,8 +181,10 @@ class MapManager : public MaNGOS::Singleton<MapManager, MaNGOS::ClassLevelLockab
MapMapType i_maps; MapMapType i_maps;
IntervalTimer i_timer; IntervalTimer i_timer;
MapUpdater m_updater; MapUpdater m_updater;
uint32 i_MaxInstanceId; uint32 i_MaxInstanceId;
typedef ACE_Recursive_Thread_Mutex LOCK_TYPE;
mutable LOCK_TYPE m_lock;
}; };
template<typename Do> template<typename Do>