From be9e1176d93a91484c8eeaa5796be74bdcdc0c8b Mon Sep 17 00:00:00 2001 From: H0zen Date: Fri, 11 Mar 2016 20:53:59 +0200 Subject: [PATCH] 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; --- src/game/Object/ObjectAccessor.cpp | 7 +++++-- src/game/Object/ObjectAccessor.h | 1 + src/game/WorldHandlers/GridMap.cpp | 14 +++++++------- src/game/WorldHandlers/GridMap.h | 5 +++-- src/game/WorldHandlers/GridStates.cpp | 5 +++++ src/game/WorldHandlers/GridStates.h | 1 + src/game/WorldHandlers/MapManager.cpp | 18 +++++++++--------- src/game/WorldHandlers/MapManager.h | 8 +++----- 8 files changed, 34 insertions(+), 25 deletions(-) diff --git a/src/game/Object/ObjectAccessor.cpp b/src/game/Object/ObjectAccessor.cpp index 0c4993f6..83217106 100644 --- a/src/game/Object/ObjectAccessor.cpp +++ b/src/game/Object/ObjectAccessor.cpp @@ -43,7 +43,10 @@ INSTANTIATE_SINGLETON_2(ObjectAccessor, CLASS_LOCK); INSTANTIATE_CLASS_MUTEX(ObjectAccessor, ACE_Thread_Mutex); -ObjectAccessor::ObjectAccessor() {} +ObjectAccessor::ObjectAccessor() : i_playerGuard(), i_corpseGuard() +{ +} + ObjectAccessor::~ObjectAccessor() { 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 template typename HashMapHolder::MapType HashMapHolder::m_objectMap; -template ACE_RW_Thread_Mutex HashMapHolder::i_lock; +template typename HashMapHolder::LockType HashMapHolder::i_lock; /// Global definitions for the hashmap storage diff --git a/src/game/Object/ObjectAccessor.h b/src/game/Object/ObjectAccessor.h index 18d3b6f9..8b5bfa6d 100644 --- a/src/game/Object/ObjectAccessor.h +++ b/src/game/Object/ObjectAccessor.h @@ -137,6 +137,7 @@ class ObjectAccessor : public MaNGOS::Singleton 0) { return (iRef -= 1); } @@ -1151,7 +1151,7 @@ GridMap* TerrainInfo::LoadMapAndVMap(const uint32 x, const uint32 y) // double checked lock pattern if (!m_GridMaps[x][y]) { - LOCK_GUARD lock(m_mutex); + ACE_GUARD_RETURN(LOCK_TYPE, lock, m_mutex, NULL) 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_CLASS_MUTEX(TerrainManager, ACE_Thread_Mutex); -TerrainManager::TerrainManager() +TerrainManager::TerrainManager() : m_mutex() { } @@ -1237,7 +1237,7 @@ TerrainManager::~TerrainManager() TerrainInfo* TerrainManager::LoadTerrain(const uint32 mapId) { - Guard _guard(*this); + ACE_GUARD_RETURN(LOCK_TYPE, _guard, m_mutex, NULL) TerrainInfo* ptr = NULL; 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) { return; } - Guard _guard(*this); + ACE_GUARD(LOCK_TYPE, _guard, m_mutex) TerrainDataMap::iterator iter = i_TerrainMap.find(mapId); if (iter != i_TerrainMap.end()) diff --git a/src/game/WorldHandlers/GridMap.h b/src/game/WorldHandlers/GridMap.h index 6d70f43b..a648df49 100644 --- a/src/game/WorldHandlers/GridMap.h +++ b/src/game/WorldHandlers/GridMap.h @@ -281,8 +281,8 @@ class TerrainInfo : public Referencable ShortIntervalTimer i_timer; typedef ACE_Thread_Mutex LOCK_TYPE; - typedef ACE_Guard LOCK_GUARD; LOCK_TYPE m_mutex; + char _cache_guard[1024]; LOCK_TYPE m_refMutex; }; @@ -328,7 +328,8 @@ class TerrainManager : public MaNGOS::Singleton::Lock Guard; + typedef ACE_Thread_Mutex LOCK_TYPE; + LOCK_TYPE m_mutex; TerrainDataMap i_TerrainMap; }; diff --git a/src/game/WorldHandlers/GridStates.cpp b/src/game/WorldHandlers/GridStates.cpp index 9b7420f7..694325ee 100644 --- a/src/game/WorldHandlers/GridStates.cpp +++ b/src/game/WorldHandlers/GridStates.cpp @@ -27,6 +27,11 @@ #include "GameSystem/Grid.h" #include "Log.h" +GridState::~GridState() +{ + DEBUG_LOG("GridState destroyed"); +} + void InvalidState::Update(Map&, NGridType&, GridInfo&, const uint32& /*x*/, const uint32& /*y*/, const uint32&) const { diff --git a/src/game/WorldHandlers/GridStates.h b/src/game/WorldHandlers/GridStates.h index 408c5c6d..0fa92d4d 100644 --- a/src/game/WorldHandlers/GridStates.h +++ b/src/game/WorldHandlers/GridStates.h @@ -32,6 +32,7 @@ class GridState public: virtual void Update(Map&, NGridType&, GridInfo&, const uint32& x, const uint32& y, const uint32& t_diff) const = 0; + virtual ~GridState(); }; class InvalidState : public GridState diff --git a/src/game/WorldHandlers/MapManager.cpp b/src/game/WorldHandlers/MapManager.cpp index d7eb8686..5dea36c7 100644 --- a/src/game/WorldHandlers/MapManager.cpp +++ b/src/game/WorldHandlers/MapManager.cpp @@ -39,7 +39,7 @@ INSTANTIATE_SINGLETON_2(MapManager, CLASS_LOCK); INSTANTIATE_CLASS_MUTEX(MapManager, ACE_Recursive_Thread_Mutex); 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)); } @@ -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. Map* MapManager::CreateMap(uint32 id, const WorldObject* obj) { - Guard _guard(*this); + ACE_GUARD_RETURN(LOCK_TYPE, _guard, m_lock, NULL) Map* m = NULL; @@ -139,13 +139,13 @@ Map* MapManager::CreateBgMap(uint32 mapid, BattleGround* bg) { sTerrainMgr.LoadTerrain(mapid); - Guard _guard(*this); + ACE_GUARD_RETURN(LOCK_TYPE, _guard, m_lock, NULL) return CreateBattleGroundMap(mapid, sMapMgr.GenerateInstanceId(), bg); } 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)); if (iter == i_maps.end()) @@ -163,7 +163,7 @@ Map* MapManager::FindMap(uint32 mapid, uint32 instanceId) const 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)); if (iter != i_maps.end()) @@ -276,9 +276,9 @@ void MapManager::InitMaxInstanceId() uint32 MapManager::GetNumInstances() { - Guard guard(*this); - uint32 ret = 0; + + ACE_GUARD_RETURN(LOCK_TYPE, _guard, m_lock, ret) for (MapMapType::iterator itr = i_maps.begin(); itr != i_maps.end(); ++itr) { Map* map = itr->second; @@ -290,9 +290,9 @@ uint32 MapManager::GetNumInstances() uint32 MapManager::GetNumPlayersInInstances() { - Guard guard(*this); - uint32 ret = 0; + + ACE_GUARD_RETURN(LOCK_TYPE, _guard, m_lock, ret) for (MapMapType::iterator itr = i_maps.begin(); itr != i_maps.end(); ++itr) { Map* map = itr->second; diff --git a/src/game/WorldHandlers/MapManager.h b/src/game/WorldHandlers/MapManager.h index 38a365d1..36b2469a 100644 --- a/src/game/WorldHandlers/MapManager.h +++ b/src/game/WorldHandlers/MapManager.h @@ -59,10 +59,6 @@ class MapManager : public MaNGOS::Singleton; - typedef ACE_Recursive_Thread_Mutex LOCK_TYPE; - typedef ACE_Guard LOCK_TYPE_GUARD; - typedef MaNGOS::ClassLevelLockable::Lock Guard; - public: typedef std::map MapMapType; @@ -185,8 +181,10 @@ class MapManager : public MaNGOS::Singleton