Nico/Viano-Core
1
0 Fork 0
Code Issues Pull Requests Actions 10 Packages Projects Releases Wiki Activity

Fix VMAP probe handle leaks and tile model ownership

Browse Source 
Close the tile stream opened during existence checks and validate parent-map fallback directly. Track each tile's successful model acquisitions so failed or partial loads cannot release references owned by dynamic objects or other tiles.

Add production-method regression tests covering file descriptor stability, transient acquisition failures, shared references, duplicate loads, partial files and parent-map overlaps. Reproduced both original defects on 6f8f59ec6d; build and test-server verification passed.
This commit is contained in:
Nico committed 2026-11-03 13:31:04 +01:00
1 parent 6f8f59ec6d
commit fe03ea8ca5
3 files changed
+206 -75
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

src/common/Collision/Maps/MapTree.cpp
+44 -73
View File
@@ -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; i<numSpawns && result; ++i)
{
// read model spawns
ModelSpawn spawn;
result = ModelSpawn::readFromFile(fileResult.File, spawn);
if (result)
{
// release model instance
vm->releaseModelInstance(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",
src/common/Collision/Maps/MapTree.h
+13 -2
View File
@@ -48,7 +48,17 @@ namespace VMAP
class TC_COMMON_API StaticMapTree
{
typedef std::unordered_map<uint32, bool> loadedTileMap;
struct LoadedModelReference
{
std::string Name;
uint32 TreeIndex;
};
struct LoadedTile
{
LoadResult Result;
std::vector<LoadedModelReference> Models;
};
typedef std::unordered_map<uint32, LoadedTile> loadedTileMap;
typedef std::unordered_map<uint32, uint32> 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<std::pair<int32, int32>> iLoadedPrimaryTiles;
// stores <tree_index, reference_count> to invalidate tree values, unload map, and to be able to report errors
tests/clientless/test_vmap_tile_lifetime.py
+149
View File
@@ -0,0 +1,149 @@
#!/usr/bin/env python3
"""Run production VMAP tile lifecycle methods against temporary files and a model cache.
--baseline <git-ref> 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 <algorithm>
#include <cassert>
#include <cstdio>
#include <cstdint>
#include <cstring>
#include <filesystem>
#include <fstream>
#include <iomanip>
#include <iostream>
#include <map>
#include <sstream>
#include <string>
#include <unordered_map>
#include <vector>
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::map<std::string,Cached>cache;
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<<std::setw(3)<<std::setfill('0')<<map<<".vmtree";return out.str();}
WorldModel*acquireModelInstance(std::string const&,std::string const&name,uint32=0){++acquires;if(failures){--failures;return nullptr;}auto it=cache.find(name);if(it==cache.end())it=cache.emplace(name,Cached{new WorldModel{name},0}).first;++it->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<std::pair<uint32,std::string>> 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)