Fix possible leak due to usage of raw pointers in filesystem

lib/filesystem/AdapterLoaders.cpp

@@ -157,11 +157,12 @@ std::vector<const ISimpleResourceLoader *> CFilesystemList::getResourcesWithName
 	return ret;
 }
 
-void CFilesystemList::addLoader(ISimpleResourceLoader * loader, bool writeable)
+void CFilesystemList::addLoader(std::unique_ptr<ISimpleResourceLoader> loader, bool writeable)
 {
-	loaders.push_back(std::unique_ptr<ISimpleResourceLoader>(loader));
 	if (writeable)
-		writeableLoaders.insert(loader);
+		writeableLoaders.insert(loader.get());
+
+	loaders.push_back(std::move(loader));
 }
 
 bool CFilesystemList::removeLoader(ISimpleResourceLoader * loader)

lib/filesystem/AdapterLoaders.h

@@ -85,8 +85,8 @@ public:
 	 * @param loader The simple resource loader object to add
 	 * @param writeable - resource shall be treated as writeable
 	 */
-	void addLoader(ISimpleResourceLoader * loader, bool writeable);
-	
+	void addLoader(std::unique_ptr<ISimpleResourceLoader> loader, bool writeable);
+
 	/**
 	 * Removes loader from the loader list
 	 * Take care about memory deallocation

lib/filesystem/CCompressedStream.cpp

@@ -97,7 +97,7 @@ CCompressedStream::CCompressedStream(std::unique_ptr<CInputStream> stream, bool
 	assert(gzipStream);
 
 	// Allocate inflate state
-	inflateState = new z_stream();
+	inflateState = std::make_unique<z_stream>();
 	inflateState->zalloc = Z_NULL;
 	inflateState->zfree = Z_NULL;
 	inflateState->opaque = Z_NULL;
@@ -108,15 +108,14 @@ CCompressedStream::CCompressedStream(std::unique_ptr<CInputStream> stream, bool
 	if (gzip)
 		wbits += 16;
 
-	int ret = inflateInit2(inflateState, wbits);
+	int ret = inflateInit2(inflateState.get(), wbits);
 	if (ret != Z_OK)
 		throw std::runtime_error("Failed to initialize inflate!\n");
 }
 
 CCompressedStream::~CCompressedStream()
 {
-	inflateEnd(inflateState);
-	vstd::clear_pointer(inflateState);
+	inflateEnd(inflateState.get());
 }
 
 si64 CCompressedStream::readMore(ui8 *data, si64 size)
@@ -149,7 +148,7 @@ si64 CCompressedStream::readMore(ui8 *data, si64 size)
 			inflateState->next_in  = compressedBuffer.data();
 		}
 
-		int ret = inflate(inflateState, Z_NO_FLUSH);
+		int ret = inflate(inflateState.get(), Z_NO_FLUSH);
 
 		if (inflateState->avail_in == 0 && gzipStream == nullptr)
 			fileEnded = true;
@@ -179,8 +178,8 @@ si64 CCompressedStream::readMore(ui8 *data, si64 size)
 	// Clean up and return
 	if (fileEnded)
 	{
-		inflateEnd(inflateState);
-		vstd::clear_pointer(inflateState);
+		inflateEnd(inflateState.get());
+		inflateState.reset();
 	}
 	return decompressed;
 }
@@ -190,7 +189,7 @@ bool CCompressedStream::getNextBlock()
 	if (!inflateState)
 		return false;
 
-	if (inflateReset(inflateState) < 0)
+	if (inflateReset(inflateState.get()) < 0)
 		return false;
 
 	reset();

lib/filesystem/CCompressedStream.h

@@ -132,7 +132,7 @@ private:
 	std::vector<ui8> compressedBuffer;
 
 	/** struct with current zlib inflate state */
-	z_stream_s * inflateState;
+	std::unique_ptr<z_stream_s> inflateState;
 
 	enum EState
 	{

lib/filesystem/Filesystem.cpp

@@ -28,7 +28,7 @@ std::map<std::string, ISimpleResourceLoader*> CResourceHandler::knownLoaders = s
 CResourceHandler CResourceHandler::globalResourceHandler;
 
 CFilesystemGenerator::CFilesystemGenerator(std::string prefix, bool extractArchives):
-	filesystem(new CFilesystemList()),
+	filesystem(std::make_unique<CFilesystemList>()),
 	prefix(std::move(prefix)),
 	extractArchives(extractArchives)
 {
@@ -72,9 +72,9 @@ void CFilesystemGenerator::loadConfig(const JsonNode & config)
 	}
 }
 
-CFilesystemList * CFilesystemGenerator::getFilesystem()
+std::unique_ptr<CFilesystemList> CFilesystemGenerator::acquireFilesystem()
 {
-	return filesystem;
+	return std::move(filesystem);
 }
 
 void CFilesystemGenerator::loadDirectory(const std::string &mountPoint, const JsonNode & config)
@@ -89,7 +89,7 @@ void CFilesystemGenerator::loadDirectory(const std::string &mountPoint, const Js
 	for(auto & loader : CResourceHandler::get("initial")->getResourcesWithName(resID))
 	{
 		auto filename = loader->getResourceName(resID);
-		filesystem->addLoader(new CFilesystemLoader(mountPoint, *filename, depth), false);
+		filesystem->addLoader(std::make_unique<CFilesystemLoader>(mountPoint, *filename, depth), false);
 	}
 }
 
@@ -98,7 +98,7 @@ void CFilesystemGenerator::loadZipArchive(const std::string &mountPoint, const J
 	std::string URI = prefix + config["path"].String();
 	auto filename = CResourceHandler::get("initial")->getResourceName(ResourcePath(URI, EResType::ARCHIVE_ZIP));
 	if (filename)
-		filesystem->addLoader(new CZipLoader(mountPoint, *filename), false);
+		filesystem->addLoader(std::make_unique<CZipLoader>(mountPoint, *filename), false);
 }
 
 template<EResType archiveType>
@@ -107,7 +107,7 @@ void CFilesystemGenerator::loadArchive(const std::string &mountPoint, const Json
 	std::string URI = prefix + config["path"].String();
 	auto filename = CResourceHandler::get("initial")->getResourceName(ResourcePath(URI, archiveType));
 	if (filename)
-		filesystem->addLoader(new CArchiveLoader(mountPoint, *filename, extractArchives), false);
+		filesystem->addLoader(std::make_unique<CArchiveLoader>(mountPoint, *filename, extractArchives), false);
 }
 
 void CFilesystemGenerator::loadJsonMap(const std::string &mountPoint, const JsonNode & config)
@@ -118,15 +118,15 @@ void CFilesystemGenerator::loadJsonMap(const std::string &mountPoint, const Json
 	{
 		auto configData = CResourceHandler::get("initial")->load(JsonPath::builtin(URI))->readAll();
 		const JsonNode configInitial(reinterpret_cast<std::byte *>(configData.first.get()), configData.second, URI);
-		filesystem->addLoader(new CMappedFileLoader(mountPoint, configInitial), false);
+		filesystem->addLoader(std::make_unique<CMappedFileLoader>(mountPoint, configInitial), false);
 	}
 }
 
-ISimpleResourceLoader * CResourceHandler::createInitial()
+std::unique_ptr<ISimpleResourceLoader> CResourceHandler::createInitial()
 {
 	//temporary filesystem that will be used to initialize main one.
 	//used to solve several case-sensivity issues like Mp3 vs MP3
-	auto * initialLoader = new CFilesystemList();
+	auto initialLoader = std::make_unique<CFilesystemList>();
 
 	//recurse only into specific directories
 	auto recurseInDir = [&](const std::string & URI, int depth)
@@ -138,8 +138,8 @@ ISimpleResourceLoader * CResourceHandler::createInitial()
 			auto filename = loader->getResourceName(ID);
 			if (filename)
 			{
-				auto * dir = new CFilesystemLoader(URI + '/', *filename, depth, true);
-				initialLoader->addLoader(dir, false);
+				auto dir = std::make_unique<CFilesystemLoader>(URI + '/', *filename, depth, true);
+				initialLoader->addLoader(std::move(dir), false);
 			}
 		}
 	};
@@ -147,9 +147,9 @@ ISimpleResourceLoader * CResourceHandler::createInitial()
 	for (auto & path : VCMIDirs::get().dataPaths())
 	{
 		if (boost::filesystem::is_directory(path)) // some of system-provided paths may not exist
-			initialLoader->addLoader(new CFilesystemLoader("", path, 1, true), false);
+			initialLoader->addLoader(std::make_unique<CFilesystemLoader>("", path, 1, true), false);
 	}
-	initialLoader->addLoader(new CFilesystemLoader("", VCMIDirs::get().userDataPath(), 0, true), false);
+	initialLoader->addLoader(std::make_unique<CFilesystemLoader>("", VCMIDirs::get().userDataPath(), 0, true), false);
 
 	recurseInDir("CONFIG", 0);// look for configs
 	recurseInDir("DATA", 0); // look for archives
@@ -178,18 +178,21 @@ void CResourceHandler::initialize()
 	if (globalResourceHandler.rootLoader)
 		return;
 
+	auto savesLoader = std::make_unique<CFilesystemLoader>("SAVES/", VCMIDirs::get().userSavePath());
+	auto configLoader = std::make_unique<CFilesystemLoader>("CONFIG/", VCMIDirs::get().userConfigPath());
+
 	globalResourceHandler.rootLoader = std::make_unique<CFilesystemList>();
 	knownLoaders["root"] = globalResourceHandler.rootLoader.get();
-	knownLoaders["saves"] = new CFilesystemLoader("SAVES/", VCMIDirs::get().userSavePath());
-	knownLoaders["config"] = new CFilesystemLoader("CONFIG/", VCMIDirs::get().userConfigPath());
+	knownLoaders["saves"] = savesLoader.get();
+	knownLoaders["config"] = configLoader.get();
 
-	auto * localFS = new CFilesystemList();
-	localFS->addLoader(knownLoaders["saves"], true);
-	localFS->addLoader(knownLoaders["config"], true);
+	auto localFS = std::make_unique<CFilesystemList>();
+	localFS->addLoader(std::move(savesLoader), true);
+	localFS->addLoader(std::move(configLoader), true);
 
 	addFilesystem("root", "initial", createInitial());
-	addFilesystem("root", "data", new CFilesystemList());
-	addFilesystem("root", "local", localFS);
+	addFilesystem("root", "data", std::make_unique<CFilesystemList>());
+	addFilesystem("root", "local", std::move(localFS));
 }
 
 void CResourceHandler::destroy()
@@ -218,26 +221,24 @@ void CResourceHandler::load(const std::string &fsConfigURI, bool extractArchives
 	addFilesystem("data", ModScope::scopeBuiltin(), createFileSystem("", fsConfig["filesystem"], extractArchives));
 }
 
-void CResourceHandler::addFilesystem(const std::string & parent, const std::string & identifier, ISimpleResourceLoader * loader)
+void CResourceHandler::addFilesystem(const std::string & parent, const std::string & identifier, std::unique_ptr<ISimpleResourceLoader> loader)
 {
 	if(knownLoaders.count(identifier) != 0)
 	{
 		logMod->error("[CRITICAL] Virtual filesystem %s already loaded!", identifier);
-		delete loader;
 		return;
 	}
 
 	if(knownLoaders.count(parent) == 0)
 	{
 		logMod->error("[CRITICAL] Parent virtual filesystem %s for %s not found!", parent, identifier);
-		delete loader;
 		return;
 	}
 
 	auto * list = dynamic_cast<CFilesystemList *>(knownLoaders.at(parent));
 	assert(list);
-	list->addLoader(loader, false);
-	knownLoaders[identifier] = loader;
+	knownLoaders[identifier] = loader.get();
+	list->addLoader(std::move(loader), false);
 }
 
 bool CResourceHandler::removeFilesystem(const std::string & parent, const std::string & identifier)
@@ -255,11 +256,11 @@ bool CResourceHandler::removeFilesystem(const std::string & parent, const std::s
 	return true;
 }
 
-ISimpleResourceLoader * CResourceHandler::createFileSystem(const std::string & prefix, const JsonNode &fsConfig, bool extractArchives)
+std::unique_ptr<ISimpleResourceLoader> CResourceHandler::createFileSystem(const std::string & prefix, const JsonNode &fsConfig, bool extractArchives)
 {
 	CFilesystemGenerator generator(prefix, extractArchives);
 	generator.loadConfig(fsConfig);
-	return generator.getFilesystem();
+	return generator.acquireFilesystem();
 }
 
 VCMI_LIB_NAMESPACE_END

lib/filesystem/Filesystem.h

@@ -24,7 +24,7 @@ class DLL_LINKAGE CFilesystemGenerator
 	using TLoadFunctor = std::function<void(const std::string &, const JsonNode &)>;
 	using TLoadFunctorMap = std::map<std::string, TLoadFunctor>;
 
-	CFilesystemList * filesystem;
+	std::unique_ptr<CFilesystemList> filesystem;
 	std::string prefix;
 
 	template<EResType archiveType>
@@ -44,7 +44,7 @@ public:
 	void loadConfig(const JsonNode & config);
 
 	/// returns generated filesystem
-	CFilesystemList * getFilesystem();
+	std::unique_ptr<CFilesystemList> acquireFilesystem();
 
 	/** Specifies if Original H3 archives should be extracted to a separate folder **/
 	bool extractArchives;
@@ -61,7 +61,7 @@ class DLL_LINKAGE CResourceHandler
 	 * @brief createInitial - creates instance of initial loader
 	 * that contains data necessary to load main FS
 	 */
-	static ISimpleResourceLoader * createInitial();
+	static std::unique_ptr<ISimpleResourceLoader> createInitial();
 
 public:
 	/**
@@ -98,7 +98,7 @@ public:
 	 * @param identifier name of this loader by which it can be retrieved later
 	 * @param loader resource loader to add
 	 */
-	static void addFilesystem(const std::string & parent, const std::string & identifier, ISimpleResourceLoader * loader);
+	static void addFilesystem(const std::string & parent, const std::string & identifier, std::unique_ptr<ISimpleResourceLoader> loader);
 	
 	/**
 	 * @brief removeFilesystem removes previously added filesystem from global resource holder
@@ -114,7 +114,7 @@ public:
 	 * @param fsConfig - configuration to load
 	 * @return generated filesystem that contains all config entries
 	 */
-	static ISimpleResourceLoader * createFileSystem(const std::string &prefix, const JsonNode & fsConfig, bool extractArchives = false);
+	static std::unique_ptr<ISimpleResourceLoader> createFileSystem(const std::string &prefix, const JsonNode & fsConfig, bool extractArchives = false);
 
 	~CResourceHandler() = default;
 private:

lib/modding/CModHandler.cpp

@@ -71,7 +71,7 @@ static std::string getModDirectory(const TModID & modName)
 	return "MODS/" + result;
 }
 
-static ISimpleResourceLoader * genModFilesystem(const std::string & modName, const JsonNode & conf)
+static std::unique_ptr<ISimpleResourceLoader> genModFilesystem(const std::string & modName, const JsonNode & conf)
 {
 	static const JsonNode defaultFS = genDefaultFS();
 
@@ -87,20 +87,20 @@ void CModHandler::loadModFilesystems()
 
 	const auto & activeMods = modManager->getActiveMods();
 
-	std::map<TModID, ISimpleResourceLoader *> modFilesystems;
+	std::map<TModID, std::unique_ptr<ISimpleResourceLoader>> modFilesystems;
 
 	for(const TModID & modName : activeMods)
 		modFilesystems[modName] = genModFilesystem(modName, getModInfo(modName).getFilesystemConfig());
 
-	for(const TModID & modName : activeMods)
-		if (modName != "core") // virtual mod 'core' has no filesystem on its own - shared with base install
-			CResourceHandler::addFilesystem("data", modName, modFilesystems[modName]);
-
 	if (settings["mods"]["validation"].String() == "full")
 		checkModFilesystemsConflicts(modFilesystems);
+
+	for(const TModID & modName : activeMods)
+		if (modName != "core") // virtual mod 'core' has no filesystem on its own - shared with base install
+			CResourceHandler::addFilesystem("data", modName, std::move(modFilesystems[modName]));
 }
 
-void CModHandler::checkModFilesystemsConflicts(const std::map<TModID, ISimpleResourceLoader *> & modFilesystems)
+void CModHandler::checkModFilesystemsConflicts(const std::map<TModID, std::unique_ptr<ISimpleResourceLoader>> & modFilesystems)
 {
 	for(const auto & [leftName, leftFilesystem] : modFilesystems)
 	{

lib/modding/CModHandler.h

@@ -27,7 +27,7 @@ class DLL_LINKAGE CModHandler final : boost::noncopyable
 	std::set<std::string> validationPassed;
 
 	void loadTranslation(const TModID & modName);
-	void checkModFilesystemsConflicts(const std::map<TModID, ISimpleResourceLoader *> & modFilesystems);
+	void checkModFilesystemsConflicts(const std::map<TModID, std::unique_ptr<ISimpleResourceLoader>> & modFilesystems);
 
 	bool isModValidationNeeded(const ModDescription & mod) const;
 

mapeditor/helper.cpp

@@ -32,9 +32,9 @@ std::unique_ptr<CMap> Helper::openMapInternal(const QString & filenameSelect)
 	ResourcePath resId("MAPEDITOR/" + fname, EResType::MAP);
 	
 	//addFilesystem takes care about memory deallocation if case of failure, no memory leak here
-	auto * mapEditorFilesystem = new CFilesystemLoader("MAPEDITOR/", fdir, 0);
+	auto mapEditorFilesystem = std::make_unique<CFilesystemLoader>("MAPEDITOR/", fdir, 0);
 	CResourceHandler::removeFilesystem("local", "mapEditor");
-	CResourceHandler::addFilesystem("local", "mapEditor", mapEditorFilesystem);
+	CResourceHandler::addFilesystem("local", "mapEditor", std::move(mapEditorFilesystem));
 	
 	if(!CResourceHandler::get("mapEditor")->existsResource(resId))
 		throw std::runtime_error("Cannot open map from this folder");
@@ -65,9 +65,9 @@ std::shared_ptr<CampaignState> Helper::openCampaignInternal(const QString & file
 	ResourcePath resId("MAPEDITOR/" + fname, EResType::CAMPAIGN);
 	
 	//addFilesystem takes care about memory deallocation if case of failure, no memory leak here
-	auto * mapEditorFilesystem = new CFilesystemLoader("MAPEDITOR/", fdir, 0);
+	auto mapEditorFilesystem = std::make_unique<CFilesystemLoader>("MAPEDITOR/", fdir, 0);
 	CResourceHandler::removeFilesystem("local", "mapEditor");
-	CResourceHandler::addFilesystem("local", "mapEditor", mapEditorFilesystem);
+	CResourceHandler::addFilesystem("local", "mapEditor", std::move(mapEditorFilesystem));
 	
 	if(!CResourceHandler::get("mapEditor")->existsResource(resId))
 		throw std::runtime_error("Cannot open campaign from this folder");

test/CVcmiTestConfig.cpp

@@ -32,14 +32,14 @@ void CVcmiTestConfig::SetUp()
 	auto path = boost::filesystem::current_path();
 	path+= "/" + TEST_DATA_DIR;
 	if(boost::filesystem::exists(path)){
-		auto loader = new CFilesystemLoader("test/", TEST_DATA_DIR);
-		dynamic_cast<CFilesystemList*>(CResourceHandler::get("core"))->addLoader(loader, false);
+		auto loader = std::make_unique<CFilesystemLoader>("test/", TEST_DATA_DIR);
+		dynamic_cast<CFilesystemList*>(CResourceHandler::get("core"))->addLoader(std::move(loader), false);
 
-		loader = new CFilesystemLoader("scripts/test/erm/", TEST_DATA_DIR+"erm/");
-		dynamic_cast<CFilesystemList*>(CResourceHandler::get("core"))->addLoader(loader, false);
+		loader = std::make_unique<CFilesystemLoader>("scripts/test/erm/", TEST_DATA_DIR+"erm/");
+		dynamic_cast<CFilesystemList*>(CResourceHandler::get("core"))->addLoader(std::move(loader), false);
 
-		loader = new CFilesystemLoader("scripts/test/lua/", TEST_DATA_DIR+"lua/");
-		dynamic_cast<CFilesystemList*>(CResourceHandler::get("core"))->addLoader(loader, false);
+		loader = std::make_unique<CFilesystemLoader>("scripts/test/lua/", TEST_DATA_DIR+"lua/");
+		dynamic_cast<CFilesystemList*>(CResourceHandler::get("core"))->addLoader(std::move(loader), false);
 
 	}
 }