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

(cherry picked from commit d05dbaaecbd4bf7e1c0fd32d0fef34341b4067f6)

5 files changed, 35 insertions, 38 deletions

diff --git a/src/common/Collision/Management/VMapManager2.cpp b/src/common/Collision/Management/VMapManager2.cpp
index 21f73e2e7c8..93a1f7e4995 100644
--- a/src/common/Collision/Management/VMapManager2.cpp
+++ b/src/common/Collision/Management/VMapManager2.cpp
@@ -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)
diff --git a/src/common/Collision/Maps/MapTree.cpp b/src/common/Collision/Maps/MapTree.cpp
index b168134486f..bde07b30065 100644
--- a/src/common/Collision/Maps/MapTree.cpp
+++ b/src/common/Collision/Maps/MapTree.cpp
@@ -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();
                 }
             }
         }
diff --git a/src/common/Collision/Models/WorldModel.h b/src/common/Collision/Models/WorldModel.h
index 0958e9c308a..5e6d2a8a2fb 100644
--- a/src/common/Collision/Models/WorldModel.h
+++ b/src/common/Collision/Models/WorldModel.h
@@ -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
 
diff --git a/src/tools/mmaps_generator/PathGenerator.cpp b/src/tools/mmaps_generator/PathGenerator.cpp
index 803c9e653b4..4b9620bff1f 100644
--- a/src/tools/mmaps_generator/PathGenerator.cpp
+++ b/src/tools/mmaps_generator/PathGenerator.cpp
@@ -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;
diff --git a/src/tools/vmap4_assembler/TileAssembler.cpp b/src/tools/vmap4_assembler/TileAssembler.cpp
index 162c1bad29e..2f96d3d4442 100644
--- a/src/tools/vmap4_assembler/TileAssembler.cpp
+++ b/src/tools/vmap4_assembler/TileAssembler.cpp
@@ -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;
             });
         }