Fix VMAP probe handle leaks and tile model ownership
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:
1 parent
6f8f59ec6d
commit
fe03ea8ca5
3 files changed
+206
-75
No files matched your search
@@ -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)
|
||||
Reference in new issue
Block a user