diff --git a/src/common/Collision/Maps/MapTree.cpp b/src/common/Collision/Maps/MapTree.cpp index e9920a9ac2..572c848a8d 100644 --- a/src/common/Collision/Maps/MapTree.cpp +++ b/src/common/Collision/Maps/MapTree.cpp @@ -285,19 +285,9 @@ namespace VMAP fclose(rf); return LoadResult::FileNotFound; } - else - { - std::string tilefile = basePath + getTileFileName(mapID, tileX, tileY); - FILE* tf = fopen(tilefile.c_str(), "rb"); - if (!tf) - result = LoadResult::FileNotFound; - else - { - if (!readChunk(tf, chunk, VMAP_MAGIC, 8)) - result = LoadResult::VersionMismatch; - fclose(tf); - } - } + if (!readChunk(tf, chunk, VMAP_MAGIC, 8)) + result = LoadResult::VersionMismatch; + fclose(tf); fclose(rf); return result; } @@ -349,15 +339,13 @@ namespace VMAP void StaticMapTree::UnloadMap(VMapManager2* vm) { - for (loadedSpawnMap::iterator i = iLoadedSpawns.begin(); i != iLoadedSpawns.end(); ++i) + while (!iLoadedTiles.empty()) { - for (uint32 refCount = 0; refCount < i->second; ++refCount) - vm->releaseModelInstance(iTreeValues[i->first].getWorldModel()->GetName()); - - iTreeValues[i->first].setUnloaded(); + uint32 tileX, tileY; + unpackTileID(iLoadedTiles.begin()->first, tileX, tileY); + UnloadMapTile(tileX, tileY, vm); } iLoadedSpawns.clear(); - iLoadedTiles.clear(); } //========================================================= @@ -369,7 +357,13 @@ namespace VMAP TC_LOG_ERROR("misc", "StaticMapTree::LoadMapTile() : tree has not been initialized [%u, %u]", tileX, tileY); return LoadResult::ReadFromFileFailed; } + uint32 tileID = packTileID(tileX, tileY); + auto loaded = iLoadedTiles.find(tileID); + if (loaded != iLoadedTiles.end()) + return loaded->second.Result; + LoadResult result = LoadResult::FileNotFound; + LoadedTile tile; TileFileOpenResult fileResult = OpenMapTileFile(iBasePath, iMapID, tileX, tileY, vm); if (fileResult.File) @@ -388,29 +382,38 @@ namespace VMAP ModelSpawn spawn; if (ModelSpawn::readFromFile(fileResult.File, spawn)) { - // acquire model instance - WorldModel* model = vm->acquireModelInstance(iBasePath, spawn.name, spawn.flags); - if (!model) - TC_LOG_ERROR("misc", "StaticMapTree::LoadMapTile() : could not acquire WorldModel pointer [%u, %u]", tileX, tileY); - // update tree auto spawnIndex = iSpawnIndices.find(spawn.ID); if (spawnIndex != iSpawnIndices.end()) { uint32 referencedVal = spawnIndex->second; + if (referencedVal >= iNTreeValues) + { + TC_LOG_ERROR("maps", "StaticMapTree::LoadMapTile() : invalid tree element (%u/%u) referenced in tile %s", referencedVal, iNTreeValues, fileResult.Name.c_str()); + continue; + } + + WorldModel* model = vm->acquireModelInstance(iBasePath, spawn.name, spawn.flags); + if (!model) + { + TC_LOG_ERROR("misc", "StaticMapTree::LoadMapTile() : could not acquire WorldModel pointer [%u, %u]", tileX, tileY); + continue; + } + if (!iLoadedSpawns.count(referencedVal)) { - if (referencedVal >= iNTreeValues) - { - TC_LOG_ERROR("maps", "StaticMapTree::LoadMapTile() : invalid tree element (%u/%u) referenced in tile %s", referencedVal, iNTreeValues, fileResult.Name.c_str()); - continue; - } - iTreeValues[referencedVal] = ModelInstance(spawn, model); iLoadedSpawns[referencedVal] = 1; } else { + if (iTreeValues[referencedVal].getWorldModel() != model) + { + TC_LOG_ERROR("maps", "StaticMapTree::LoadMapTile() : conflicting model for spawn %u in tile %s", spawn.ID, fileResult.Name.c_str()); + vm->releaseModelInstance(spawn.name); + result = LoadResult::ReadFromFileFailed; + continue; + } ++iLoadedSpawns[referencedVal]; #ifdef VMAP_DEBUG if (iTreeValues[referencedVal].ID != spawn.ID) @@ -419,6 +422,7 @@ namespace VMAP TC_LOG_DEBUG("maps", "StaticMapTree::LoadMapTile() : name collision on GUID=%u", spawn.ID); #endif } + tile.Models.push_back({ std::move(spawn.name), referencedVal }); } else if (int32(iMapID) == fileResult.UsedMapId) { @@ -435,11 +439,10 @@ namespace VMAP result = LoadResult::ReadFromFileFailed; } } - iLoadedTiles[packTileID(tileX, tileY)] = true; fclose(fileResult.File); } - else - iLoadedTiles[packTileID(tileX, tileY)] = false; + tile.Result = result; + iLoadedTiles.emplace(tileID, std::move(tile)); TC_METRIC_EVENT("map_events", "LoadMapTile", "Map: " + std::to_string(iMapID) + " TileX: " + std::to_string(tileX) + " TileY: " + std::to_string(tileY)); return result; @@ -456,49 +459,17 @@ namespace VMAP VMAP_ERROR_LOG("misc", "StaticMapTree::UnloadMapTile() : trying to unload non-loaded tile - Map:%u X:%u Y:%u", iMapID, tileX, tileY); return; } - if (tile->second) // file associated with tile + for (LoadedModelReference const& model : tile->second.Models) { - TileFileOpenResult fileResult = OpenMapTileFile(iBasePath, iMapID, tileX, tileY, vm); - if (fileResult.File) + auto referencedNode = iLoadedSpawns.find(model.TreeIndex); + if (referencedNode == iLoadedSpawns.end()) + VMAP_ERROR_LOG("misc", "StaticMapTree::UnloadMapTile() : trying to unload non-referenced model '%s'", model.Name.c_str()); + else if (--referencedNode->second == 0) { - bool result=true; - char chunk[8]; - if (!readChunk(fileResult.File, chunk, VMAP_MAGIC, 8)) - result = false; - uint32 numSpawns; - if (fread(&numSpawns, sizeof(uint32), 1, fileResult.File) != 1) - result = false; - for (uint32 i=0; ireleaseModelInstance(spawn.name); - - // update tree - auto spawnIndex = iSpawnIndices.find(spawn.ID); - if (spawnIndex == iSpawnIndices.end()) - result = false; - else - { - uint32 referencedNode = spawnIndex->second; - if (!iLoadedSpawns.count(referencedNode)) - VMAP_ERROR_LOG("misc", "StaticMapTree::UnloadMapTile() : trying to unload non-referenced model '%s' (ID:%u)", spawn.name.c_str(), spawn.ID); - else if (--iLoadedSpawns[referencedNode] == 0) - { - iTreeValues[referencedNode].setUnloaded(); - iLoadedSpawns.erase(referencedNode); - } - } - } - else if (int32(iMapID) == fileResult.UsedMapId) // logic documented in StaticMapTree::LoadMapTile - result = false; - } - fclose(fileResult.File); + iTreeValues[model.TreeIndex].setUnloaded(); + iLoadedSpawns.erase(referencedNode); } + vm->releaseModelInstance(model.Name); } iLoadedTiles.erase(tile); TC_METRIC_EVENT("map_events", "UnloadMapTile", diff --git a/src/common/Collision/Maps/MapTree.h b/src/common/Collision/Maps/MapTree.h index 3be573445c..00287681ea 100644 --- a/src/common/Collision/Maps/MapTree.h +++ b/src/common/Collision/Maps/MapTree.h @@ -48,7 +48,17 @@ namespace VMAP class TC_COMMON_API StaticMapTree { - typedef std::unordered_map loadedTileMap; + struct LoadedModelReference + { + std::string Name; + uint32 TreeIndex; + }; + struct LoadedTile + { + LoadResult Result; + std::vector Models; + }; + typedef std::unordered_map loadedTileMap; typedef std::unordered_map loadedSpawnMap; private: uint32 iMapID; @@ -59,7 +69,8 @@ namespace VMAP // Store all the map tile idents that are loaded for that map // some maps are not splitted into tiles and we have to make sure, not removing the map before all tiles are removed - // empty tiles have no tile file, hence map with bool instead of just a set (consistency check) + // Keep precisely the references acquired by each tile. A failed load + // must never release another tile's or a dynamic object's model. loadedTileMap iLoadedTiles; std::vector> iLoadedPrimaryTiles; // stores to invalidate tree values, unload map, and to be able to report errors diff --git a/tests/clientless/test_vmap_tile_lifetime.py b/tests/clientless/test_vmap_tile_lifetime.py new file mode 100644 index 0000000000..ef3d0c4491 --- /dev/null +++ b/tests/clientless/test_vmap_tile_lifetime.py @@ -0,0 +1,149 @@ +#!/usr/bin/env python3 +"""Run production VMAP tile lifecycle methods against temporary files and a model cache. + +--baseline reproduces both crash prerequisites using the specified revision. +""" +from pathlib import Path +import subprocess +import sys +import tempfile + +ROOT = Path(__file__).resolve().parents[2] +BASELINE = sys.argv[sys.argv.index('--baseline') + 1] if '--baseline' in sys.argv else None + +def read(relative): + if BASELINE: + return subprocess.check_output(['git', 'show', BASELINE + ':' + relative], cwd=ROOT, text=True) + return (ROOT / relative).read_text() + +def function(source, marker): + start = source.index(marker) + opening = source.index('{', start) + depth, end = 1, opening + 1 + while depth: + depth += (source[end] == '{') - (source[end] == '}') + end += 1 + return source[start:end] + +source = read('src/common/Collision/Maps/MapTree.cpp') +header = read('src/common/Collision/Maps/MapTree.h') +header = '\n'.join(line for line in header.splitlines() if not line.startswith('#')) +header = header.replace('private:', 'public:').replace('TC_COMMON_API ', '') +methods = '\n'.join(function(source, marker) for marker in [ + 'std::string StaticMapTree::getTileFileName(', + 'StaticMapTree::StaticMapTree(', 'StaticMapTree::~StaticMapTree(', + 'StaticMapTree::TileFileOpenResult StaticMapTree::OpenMapTileFile(', + 'LoadResult StaticMapTree::CanLoadMap(', 'void StaticMapTree::UnloadMap(', + 'LoadResult StaticMapTree::LoadMapTile(', 'void StaticMapTree::UnloadMapTile(' +]) +preamble = r''' +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +using uint8=uint8_t;using uint32=uint32_t;using int32=int32_t; +#define TC_LOG_DEBUG(...) ((void)0) +#define TC_LOG_ERROR(...) ((void)0) +#define VMAP_ERROR_LOG(...) ((void)0) +#define TC_METRIC_EVENT(...) ((void)0) +namespace G3D{struct Vector3{};struct Ray{};float finf(){return 1e30f;}} +namespace VMAP{ +constexpr char VMAP_MAGIC[]="VMAPtest"; +bool readChunk(FILE*f,char*buffer,char const*magic,int length){return int(fread(buffer,1,length,f))==length&&!std::memcmp(buffer,magic,length);} +enum class LoadResult:uint8{Success,FileNotFound,VersionMismatch,ReadFromFileFailed,DisabledInConfig}; +struct BIH{};class GroupModel{}; +class WorldModel{public:std::string name;std::string const&GetName()const{return name;}}; +struct ModelSpawn{ + uint32 ID=0,flags=0;std::string name; + static bool readFromFile(FILE*f,ModelSpawn&s){uint32 n;if(fread(&s.ID,4,1,f)!=1||fread(&n,4,1,f)!=1||n>10000)return false;s.name.resize(n);return fread(s.name.data(),1,n,f)==n;} +}; +class ModelInstance{ + WorldModel*model=nullptr; + public:ModelInstance()=default;ModelInstance(ModelSpawn const&,WorldModel*m):model(m){}; + WorldModel*getWorldModel(){return model;}void setUnloaded(){model=nullptr;} +}; +class VMapManager2{ + public: + struct Cached{WorldModel*model;int refs;};std::mapcache; + int failures=0,acquires=0,releases=0,badReleases=0; + ~VMapManager2(){for(auto&pair:cache)delete pair.second.model;} + int32 getParentMapId(uint32 map){return map==1?0:-1;} + static std::string getMapFileName(uint32 map){std::ostringstream out;out<second.refs;return it->second.model;} + void releaseModelInstance(std::string const&name){++releases;auto it=cache.find(name);if(it==cache.end()){++badReleases;return;}if(!--it->second.refs){delete it->second.model;cache.erase(it);}} + int refs(std::string const&name){auto it=cache.find(name);return it==cache.end()?0:it->second.refs;} +}; +} +''' +tests = r''' +namespace VMAP{ +void init(StaticMapTree&t){t.iNTreeValues=2;t.iTreeValues=new ModelInstance[2];t.iSpawnIndices={{7,0},{8,1}};} +void writeTile(std::string const&base,uint32 map,uint32 x,uint32 y,std::vector> const&models,uint32 declared=0){ + auto path=base+StaticMapTree::getTileFileName(map,x,y);FILE*f=fopen(path.c_str(),"wb");assert(f);fwrite(VMAP_MAGIC,1,8,f);uint32 count=declared?declared:uint32(models.size());fwrite(&count,4,1,f); + for(auto const&model:models){uint32 id=model.first,n=uint32(model.second.size());fwrite(&id,4,1,f);fwrite(&n,4,1,f);fwrite(model.second.data(),1,n,f);}fclose(f); +} +size_t fds(){size_t count=0;for(auto const&entry:std::filesystem::directory_iterator("/proc/self/fd")){(void)entry;++count;}return count;} +void probe(std::string const&base){ + {std::ofstream f(base+"000.vmtree",std::ios::binary);f.write(VMAP_MAGIC,8);}writeTile(base,0,2,3,{{7,"crystal"}}); + VMapManager2 vm;auto before=fds();for(int i=0;i<128;++i)assert(StaticMapTree::CanLoadMap(base,0,2,3,&vm)==LoadResult::Success);assert(fds()==before); + // A real parent-map fallback must validate the already opened parent tile. + {std::ofstream f(base+"001.vmtree",std::ios::binary);f.write(VMAP_MAGIC,8);} + assert(StaticMapTree::CanLoadMap(base,1,2,3,&vm)==LoadResult::Success);assert(fds()==before); + {std::ofstream f(base+StaticMapTree::getTileFileName(0,2,3),std::ios::binary);f<<"badmagic";} + assert(StaticMapTree::CanLoadMap(base,0,2,3,&vm)==LoadResult::VersionMismatch);assert(fds()==before); +} +void lifetime(std::string const&base){ + VMapManager2 vm;StaticMapTree tree(0,base);init(tree);writeTile(base,0,4,5,{{7,"crystal"}}); + // Transient failure while a tile stream consumes the last file descriptor. + vm.failures=1;tree.LoadMapTile(4,5,&vm); + // Later the stream closes, letting a dynamic GameObject load this same model. + auto dynamic=vm.acquireModelInstance(base,"crystal");assert(dynamic&&vm.refs("crystal")==1); + tree.UnloadMapTile(4,5,&vm);assert(vm.refs("crystal")==1&&vm.releases==0);vm.releaseModelInstance("crystal"); +} +void shared(std::string const&base){ + VMapManager2 vm;StaticMapTree tree(0,base);init(tree);auto dynamic=vm.acquireModelInstance(base,"shared");assert(dynamic); + writeTile(base,0,2,3,{{7,"shared"}});writeTile(base,0,4,5,{{7,"shared"}}); + tree.LoadMapTile(2,3,&vm);tree.LoadMapTile(4,5,&vm);assert(vm.refs("shared")==3); + tree.LoadMapTile(2,3,&vm);assert(vm.refs("shared")==3); // repeated checks do not acquire twice + std::filesystem::remove(base+StaticMapTree::getTileFileName(0,2,3)); // unloading cannot depend on reopening data + tree.UnloadMapTile(2,3,&vm);assert(vm.refs("shared")==2&&tree.iTreeValues[0].getWorldModel()); + tree.UnloadMap(&vm);assert(vm.refs("shared")==1&&!tree.iTreeValues[0].getWorldModel()&&!tree.numLoadedTiles()); + tree.UnloadMap(&vm);assert(vm.refs("shared")==1&&vm.badReleases==0);vm.releaseModelInstance("shared"); + // Truncated tile: release exactly the successfully read prefix, even if the file is removed. + writeTile(base,0,6,7,{{7,"prefix"}},2);assert(tree.LoadMapTile(6,7,&vm)==LoadResult::ReadFromFileFailed); + assert(vm.refs("prefix")==1);std::filesystem::remove(base+StaticMapTree::getTileFileName(0,6,7));tree.UnloadMap(&vm);assert(!vm.refs("prefix")&&!vm.badReleases); + // Missing data is tracked as an empty tile without acquiring/releasing any models. + assert(tree.LoadMapTile(30,31,&vm)==LoadResult::FileNotFound);tree.UnloadMap(&vm);assert(vm.badReleases==0); + // Overlapping parent spawns absent from the child's index are skipped without leaking references. + StaticMapTree child(1,base);init(child);writeTile(base,0,8,9,{{99,"parent-only"},{7,"indexed"}}); + assert(child.LoadMapTile(8,9,&vm)==LoadResult::Success);assert(!vm.refs("parent-only")&&vm.refs("indexed")==1);child.UnloadMap(&vm);assert(!vm.refs("indexed")&&!vm.badReleases); +} +} +int main(int argc,char**argv){assert(argc==3);std::string base=std::string(argv[2])+"/";std::string test=argv[1];if(test=="fd")VMAP::probe(base);else if(test=="lifetime")VMAP::lifetime(base);else VMAP::shared(base);} +''' +with tempfile.TemporaryDirectory(prefix='vcore-vmap-lifetime-') as folder: + folder = Path(folder) + cpp = folder / 'test.cpp' + cpp.write_text(preamble + header + '\nnamespace VMAP{\n' + methods + '\n}\n' + tests) + exe = folder / 'test' + subprocess.run(['g++', '-std=c++17', '-O0', '-g', '-Wall', '-Wextra', str(cpp), '-o', str(exe)], check=True) + for case in (['fd', 'lifetime'] if BASELINE else ['fd', 'lifetime', 'shared']): + data = folder / case + data.mkdir() + result = subprocess.run([str(exe), case, str(data)], capture_output=True, text=True) + if BASELINE: + assert result.returncode != 0, (case, 'baseline unexpectedly passed') + print('Baseline reproduced:', case, 'exit', result.returncode, result.stderr.strip()) + else: + assert result.returncode == 0, (case, result.stdout, result.stderr) + print('Passed:', case)