Core/Vmaps: Fixed use after free and deadlocks when loading a model file fails

This commit is contained in:
Shauren
2024-07-06 22:52:11 +02:00
parent 518fe1fd1e
commit d05dbaaecb
5 changed files with 35 additions and 38 deletions

View File

@@ -32,17 +32,18 @@ namespace VMAP
class ManagedModel
{
public:
explicit ManagedModel(VMapManager2& mgr) : _mgr(mgr) { }
explicit ManagedModel(VMapManager2& mgr, std::string const& name) : _mgr(mgr), _name(name) { }
~ManagedModel()
{
_mgr.releaseModelInstance(Model.GetName());
_mgr.releaseModelInstance(_name);
}
WorldModel Model;
private:
VMapManager2& _mgr;
std::string const& _name; // valid only while model is held in VMapManager2::iLoadedModelFiles
};
bool readChunk(FILE* rf, char* dest, const char* compare, uint32 len)
@@ -264,15 +265,17 @@ namespace VMAP
std::shared_ptr<WorldModel> VMapManager2::acquireModelInstance(std::string const& basepath, std::string const& filename)
{
std::shared_ptr<ManagedModel> worldmodel; // this is intentionally declared before lock so that it is destroyed after it to prevent deadlocks in releaseModelInstance
//! Critical section, thread safe access to iLoadedModelFiles
std::lock_guard<std::mutex> lock(LoadedModelFilesLock);
auto& [key, model] = *iLoadedModelFiles.try_emplace(filename).first;
std::shared_ptr<ManagedModel> worldmodel = model.lock();
worldmodel = model.lock();
if (worldmodel)
return std::shared_ptr<WorldModel>(worldmodel, &worldmodel->Model);
worldmodel = std::make_shared<ManagedModel>(*this);
worldmodel = std::make_shared<ManagedModel>(*this, key);
if (!worldmodel->Model.readFile(basepath + filename + ".vmo"))
{
TC_LOG_ERROR("misc", "VMapManager2: could not load '{}{}.vmo'", basepath, filename);
@@ -280,8 +283,6 @@ namespace VMAP
}
TC_LOG_DEBUG("maps", "VMapManager2: loading file '{}{}'", basepath, filename);
worldmodel->Model.SetName(&key);
model = worldmodel;
return std::shared_ptr<WorldModel>(worldmodel, &worldmodel->Model);
@@ -292,13 +293,11 @@ namespace VMAP
//! Critical section, thread safe access to iLoadedModelFiles
std::lock_guard<std::mutex> lock(LoadedModelFilesLock);
TC_LOG_DEBUG("maps", "VMapManager2: unloading file '{}'", filename);
std::size_t erased = iLoadedModelFiles.erase(filename);
if (!erased)
{
TC_LOG_ERROR("misc", "VMapManager2: trying to unload non-loaded file '{}'", filename);
return;
}
TC_LOG_DEBUG("maps", "VMapManager2: unloading file '{}'", filename);
}
LoadResult VMapManager2::existsMap(char const* basePath, unsigned int mapId, int x, int y)

View File

@@ -335,12 +335,13 @@ namespace VMAP
{
TC_LOG_ERROR("maps", "StaticMapTree::LoadMapTile() : invalid tree element (spawn {}) referenced in tile {} by map {}", spawn.ID, fileResult.Name, iMapID);
result = LoadResult::ReadFromFileFailed;
break;
continue;
}
if (referencedVal >= iTreeValues.size())
{
TC_LOG_ERROR("maps", "StaticMapTree::LoadMapTile() : invalid tree element ({}/{}) referenced in tile {}", referencedVal, iTreeValues.size(), fileResult.Name);
result = LoadResult::ReadFromFileFailed;
continue;
}
@@ -404,29 +405,29 @@ namespace VMAP
{
// read model spawns
ModelSpawn spawn;
result = ModelSpawn::readFromFile(fileResult.TileFile.get(), spawn);
if (result)
if (!ModelSpawn::readFromFile(fileResult.TileFile.get(), spawn))
break;
// update tree
uint32 referencedNode = 0;
if (fread(&referencedNode, sizeof(uint32), 1, fileResult.SpawnIndicesFile.get()) != 1)
{
// update tree
uint32 referencedNode = 0;
if (fread(&referencedNode, sizeof(uint32), 1, fileResult.SpawnIndicesFile.get()) != 1)
{
TC_LOG_ERROR("maps", "StaticMapTree::LoadMapTile() : invalid tree element (spawn {}) referenced in tile {} by map {}", spawn.ID, fileResult.Name, iMapID);
result = false;
break;
}
if (referencedNode >= iTreeValues.size())
{
TC_LOG_ERROR("maps", "StaticMapTree::LoadMapTile() : invalid tree element ({}/{}) referenced in tile {}", referencedNode, iTreeValues.size(), fileResult.Name);
continue;
}
if (!iTreeValues[referencedNode].getWorldModel())
TC_LOG_ERROR("misc", "StaticMapTree::UnloadMapTile() : trying to unload non-referenced model '{}' (ID:{})", spawn.name, spawn.ID);
else if (!iTreeValues[referencedNode].RemoveTileReference())
iTreeValues[referencedNode].setUnloaded();
TC_LOG_ERROR("maps", "StaticMapTree::LoadMapTile() : invalid tree element (spawn {}) referenced in tile {} by map {}", spawn.ID, fileResult.Name, iMapID);
result = false;
continue;
}
if (referencedNode >= iTreeValues.size())
{
TC_LOG_ERROR("maps", "StaticMapTree::LoadMapTile() : invalid tree element ({}/{}) referenced in tile {}", referencedNode, iTreeValues.size(), fileResult.Name);
result = false;
continue;
}
if (!iTreeValues[referencedNode].getWorldModel())
TC_LOG_ERROR("misc", "StaticMapTree::UnloadMapTile() : trying to unload non-referenced model '{}' (ID:{})", spawn.name, spawn.ID);
else if (!iTreeValues[referencedNode].RemoveTileReference())
iTreeValues[referencedNode].setUnloaded();
}
}
}

View File

@@ -117,7 +117,7 @@ namespace VMAP
class TC_COMMON_API WorldModel
{
public:
WorldModel(): Flags(ModelFlags::None), RootWMOID(0), name(nullptr) { }
WorldModel(): Flags(ModelFlags::None), RootWMOID(0) { }
//! pass group models to WorldModel and create BIH. Passed vector is swapped with old geometry!
void setGroupModels(std::vector<GroupModel> &models);
@@ -129,14 +129,11 @@ namespace VMAP
bool readFile(const std::string &filename);
bool IsM2() const { return Flags.HasFlag(ModelFlags::IsM2); }
std::vector<GroupModel> const& getGroupModels() const { return groupModels; }
std::string const& GetName() const { return *name; }
void SetName(std::string const* newName) { name = newName; }
protected:
EnumFlag<ModelFlags> Flags;
uint32 RootWMOID;
std::vector<GroupModel> groupModels;
BIH groupTree;
std::string const* name; // valid only while model is held in VMapManager2::iLoadedModelFiles
};
} // namespace VMAP

View File

@@ -79,7 +79,7 @@ bool checkDirectories(bool debugOutput, std::vector<std::string>& dbcLocales)
}
dirFiles.clear();
if (getDirContents(dirFiles, "vmaps", "*.vmtree") == LISTFILE_DIRECTORY_NOT_FOUND || dirFiles.empty())
if (getDirContents(dirFiles, "vmaps/0000", "*.vmtree") == LISTFILE_DIRECTORY_NOT_FOUND || dirFiles.empty())
{
printf("'vmaps' directory is empty or does not exist\n");
return false;

View File

@@ -100,7 +100,6 @@ namespace VMAP
threadPool.PostWork([this, file = directoryEntry.path(), &abortThreads, &workerIndexGen, &spawnedModelFilesByThread, &mapsToProcess]
{
thread_local std::size_t workerIndex = workerIndexGen++;
--mapsToProcess;
auto dirf = OpenFile(file, "rb");
if (!dirf)
@@ -127,6 +126,7 @@ namespace VMAP
return abortThreads();
spawnedModelFilesByThread[workerIndex].merge(data.SpawnedModelFiles);
--mapsToProcess;
});
}