From e567e1b820bbea873a6f6d7a5d19d7fd6478489d Mon Sep 17 00:00:00 2001 From: Ivan Savenko Date: Sun, 27 Apr 2025 17:06:08 +0300 Subject: [PATCH 1/9] Fix possible memory leaks in sound handler, simplify API --- client/HeroMovementController.cpp | 2 +- client/battle/BattleAnimationClasses.cpp | 2 +- client/media/CSoundHandler.cpp | 109 +++++++++++++---------- client/media/CSoundHandler.h | 32 +++++-- client/media/ISoundPlayer.h | 7 +- 5 files changed, 91 insertions(+), 61 deletions(-) diff --git a/client/HeroMovementController.cpp b/client/HeroMovementController.cpp index cc665e1e9..ceab5380e 100644 --- a/client/HeroMovementController.cpp +++ b/client/HeroMovementController.cpp @@ -312,7 +312,7 @@ void HeroMovementController::updateMovementSound(const CGHeroInstance * h, int3 ENGINE->sound().stopSound(currentMovementSoundChannel); if(!currentMovementSoundName.empty()) - currentMovementSoundChannel = ENGINE->sound().playSound(currentMovementSoundName, -1, true); + currentMovementSoundChannel = ENGINE->sound().playSoundLooped(currentMovementSoundName); else currentMovementSoundChannel = -1; } diff --git a/client/battle/BattleAnimationClasses.cpp b/client/battle/BattleAnimationClasses.cpp index 0fd2b2179..8ce48fdc1 100644 --- a/client/battle/BattleAnimationClasses.cpp +++ b/client/battle/BattleAnimationClasses.cpp @@ -357,7 +357,7 @@ bool MovementAnimation::init() if (moveSoundHandler == -1) { - moveSoundHandler = ENGINE->sound().playSound(stack->unitType()->sounds.move, -1); + moveSoundHandler = ENGINE->sound().playSoundLooped(stack->unitType()->sounds.move); } Point begPosition = owner.stacksController->getStackPositionAtHex(prevHex, stack); diff --git a/client/media/CSoundHandler.cpp b/client/media/CSoundHandler.cpp index 31014c57d..93ad0bfb7 100644 --- a/client/media/CSoundHandler.cpp +++ b/client/media/CSoundHandler.cpp @@ -52,6 +52,11 @@ CSoundHandler::CSoundHandler(): } } +void CSoundHandler::MixChunkDeleter::operator()(Mix_Chunk * ptr) +{ + Mix_FreeChunk(ptr); +} + CSoundHandler::~CSoundHandler() { if(isInitialized()) @@ -59,36 +64,28 @@ CSoundHandler::~CSoundHandler() Mix_ChannelFinished(nullptr); Mix_HaltChannel(-1); - for(auto & chunk : soundChunks) - { - if(chunk.second.first) - Mix_FreeChunk(chunk.second.first); - } - - for(auto & chunk : soundChunksRaw) - { - if(chunk.second.first) - Mix_FreeChunk(chunk.second.first); - } + soundChunks.clear(); + uncachedPlayingChunks.clear(); } } +Mix_Chunk * CSoundHandler::getSoundChunkCached(const AudioPath & sound) +{ + if (soundChunks.find(sound) == soundChunks.end()) + soundChunks[sound].first = getSoundChunk(sound); + + return soundChunks[sound].first.get(); +} + // Allocate an SDL chunk and cache it. -Mix_Chunk * CSoundHandler::GetSoundChunk(const AudioPath & sound, bool cache) +CSoundHandler::MixChunkPtr CSoundHandler::getSoundChunk(const AudioPath & sound) { try { - if(cache && soundChunks.find(sound) != soundChunks.end()) - return soundChunks[sound].first; - auto data = CResourceHandler::get()->load(sound.addPrefix("SOUNDS/"))->readAll(); SDL_RWops * ops = SDL_RWFromMem(data.first.get(), data.second); Mix_Chunk * chunk = Mix_LoadWAV_RW(ops, 1); // will free ops - - if(cache) - soundChunks.insert({sound, std::make_pair(chunk, std::move(data.first))}); - - return chunk; + return MixChunkPtr(chunk); } catch(std::exception & e) { @@ -97,22 +94,15 @@ Mix_Chunk * CSoundHandler::GetSoundChunk(const AudioPath & sound, bool cache) } } -Mix_Chunk * CSoundHandler::GetSoundChunk(std::pair, si64> & data, bool cache) +CSoundHandler::MixChunkPtr CSoundHandler::getSoundChunk(std::pair, si64> & data) { try { std::vector startBytes = std::vector(data.first.get(), data.first.get() + std::min(static_cast(100), data.second)); - if(cache && soundChunksRaw.find(startBytes) != soundChunksRaw.end()) - return soundChunksRaw[startBytes].first; - SDL_RWops * ops = SDL_RWFromMem(data.first.get(), data.second); Mix_Chunk * chunk = Mix_LoadWAV_RW(ops, 1); // will free ops - - if(cache) - soundChunksRaw.insert({startBytes, std::make_pair(chunk, std::move(data.first))}); - - return chunk; + return MixChunkPtr(chunk); } catch(std::exception & e) { @@ -174,36 +164,53 @@ uint32_t CSoundHandler::getSoundDurationMilliseconds(const AudioPath & sound) } // Plays a sound, and return its channel so we can fade it out later -int CSoundHandler::playSound(soundBase::soundID soundID, int repeats) +int CSoundHandler::playSound(soundBase::soundID soundID) { assert(soundID < soundBase::sound_after_last); auto sound = AudioPath::builtin(soundsList[soundID]); logGlobal->trace("Attempt to play sound %d with file name %s with cache", soundID, sound.getOriginalName()); - return playSound(sound, repeats, true); + return playSoundImpl(sound, 0, true); } -int CSoundHandler::playSound(const AudioPath & sound, int repeats, bool cache) +int CSoundHandler::playSoundLooped(const AudioPath & sound) +{ + return playSoundImpl(sound, -1, true); +} + +int CSoundHandler::playSound(const AudioPath & sound) +{ + return playSoundImpl(sound, 0, false); +} + +int CSoundHandler::playSoundImpl(const AudioPath & sound, int repeats, bool useCache) { if(!isInitialized() || sound.empty()) return -1; int channel; - Mix_Chunk * chunk = GetSoundChunk(sound, cache); + MixChunkPtr chunkPtr = getSoundChunk(sound); + Mix_Chunk * chunk = nullptr; + if (!useCache) + { + chunkPtr = getSoundChunk(sound); + chunk = chunkPtr.get(); + } + else + chunk = getSoundChunkCached(sound); if(chunk) { - channel = Mix_PlayChannel(-1, chunk, repeats); + channel = Mix_PlayChannel(-1, chunk, 0); if(channel == -1) { logGlobal->error("Unable to play sound file %s , error %s", sound.getOriginalName(), Mix_GetError()); - if(!cache) - Mix_FreeChunk(chunk); } - else if(cache) - initCallback(channel); else - initCallback(channel, [chunk](){ Mix_FreeChunk(chunk);}); + { + storeChunk(channel, std::move(chunkPtr)); + initCallback(channel); + } } else channel = -1; @@ -211,22 +218,28 @@ int CSoundHandler::playSound(const AudioPath & sound, int repeats, bool cache) return channel; } -int CSoundHandler::playSound(std::pair, si64> & data, int repeats, bool cache) +void CSoundHandler::storeChunk(int channel, MixChunkPtr chunk) +{ + std::scoped_lock lockGuard(mutexCallbacks); + uncachedPlayingChunks[channel] = std::move(chunk); +} + +int CSoundHandler::playSound(std::pair, si64> & data) { int channel = -1; - if(Mix_Chunk * chunk = GetSoundChunk(data, cache)) + auto chunk = getSoundChunk(data); + if(chunk) { - channel = Mix_PlayChannel(-1, chunk, repeats); + channel = Mix_PlayChannel(-1, chunk.get(), 0); if(channel == -1) { logGlobal->error("Unable to play sound, error %s", Mix_GetError()); - if(!cache) - Mix_FreeChunk(chunk); } - else if(cache) - initCallback(channel); else - initCallback(channel, [chunk](){ Mix_FreeChunk(chunk);}); + { + storeChunk(channel, std::move(chunk)); + initCallback(channel); + } } return channel; } @@ -386,7 +399,7 @@ void CSoundHandler::ambientUpdateChannels(std::map soundsArg) if(!vstd::contains(ambientChannels, soundId)) { - int channel = playSound(soundId, -1); + int channel = playSoundLooped(soundId); int channelVolume = ambientDistToVolume(distance); channelVolumes[channel] = channelVolume; diff --git a/client/media/CSoundHandler.h b/client/media/CSoundHandler.h index e17fd59f2..7ed110772 100644 --- a/client/media/CSoundHandler.h +++ b/client/media/CSoundHandler.h @@ -19,16 +19,29 @@ struct Mix_Chunk; class CSoundHandler final : public CAudioBase, public ISoundPlayer { private: + struct MixChunkDeleter + { + void operator ()(Mix_Chunk *); + }; + using MixChunkPtr = std::unique_ptr; + //update volume on configuration change SettingsListener listener; void onVolumeChange(const JsonNode & volumeNode); - using CachedChunk = std::pair>; - std::map soundChunks; - std::map, CachedChunk> soundChunksRaw; + using CachedChunk = std::pair>; - Mix_Chunk * GetSoundChunk(const AudioPath & sound, bool cache); - Mix_Chunk * GetSoundChunk(std::pair, si64> & data, bool cache); + /// List of all permanently cached sound chunks + std::map soundChunks; + + /// List of all currently playing chunks that are currently playing + /// and should be deallocated once channel playback is over + /// indexed by channel ID + std::map uncachedPlayingChunks; + + MixChunkPtr getSoundChunk(const AudioPath & sound); + Mix_Chunk * getSoundChunkCached(const AudioPath & sound); + MixChunkPtr getSoundChunk(std::pair, si64> & data); /// have entry for every currently active channel /// vector will be empty if callback was not set @@ -51,6 +64,8 @@ private: void initCallback(int channel, const std::function & function); void initCallback(int channel); + void storeChunk(int channel, MixChunkPtr chunk); + int playSoundImpl(const AudioPath & sound, int repeats, bool useCache); public: CSoundHandler(); @@ -62,9 +77,10 @@ public: // Sounds uint32_t getSoundDurationMilliseconds(const AudioPath & sound) final; - int playSound(soundBase::soundID soundID, int repeats = 0) final; - int playSound(const AudioPath & sound, int repeats = 0, bool cache = false) final; - int playSound(std::pair, si64> & data, int repeats = 0, bool cache = false) final; + int playSound(soundBase::soundID soundID) final; + int playSound(const AudioPath & sound) final; + int playSoundLooped(const AudioPath & sound) final; + int playSound(std::pair, si64> & data) final; int playSoundFromSet(std::vector & sound_vec) final; void stopSound(int handler) final; void pauseSound(int handler) final; diff --git a/client/media/ISoundPlayer.h b/client/media/ISoundPlayer.h index ffcb90dce..6d512737c 100644 --- a/client/media/ISoundPlayer.h +++ b/client/media/ISoundPlayer.h @@ -17,9 +17,10 @@ class ISoundPlayer public: virtual ~ISoundPlayer() = default; - virtual int playSound(soundBase::soundID soundID, int repeats = 0) = 0; - virtual int playSound(const AudioPath & sound, int repeats = 0, bool cache = false) = 0; - virtual int playSound(std::pair, si64> & data, int repeats = 0, bool cache = false) = 0; + virtual int playSound(soundBase::soundID soundID) = 0; + virtual int playSound(const AudioPath & sound) = 0; + virtual int playSoundLooped(const AudioPath & sound) = 0; + virtual int playSound(std::pair, si64> & data) = 0; virtual int playSoundFromSet(std::vector & sound_vec) = 0; virtual void stopSound(int handler) = 0; virtual void pauseSound(int handler) = 0; From cd2837a84ecfff7394dcdf61c7b5a2b291ca287d Mon Sep 17 00:00:00 2001 From: Ivan Savenko Date: Sun, 27 Apr 2025 17:06:33 +0300 Subject: [PATCH 2/9] Fix possible memory leak (circular shared_ptr) in networking --- lib/network/NetworkConnection.cpp | 10 +++++----- lib/network/NetworkConnection.h | 6 +++--- lib/network/NetworkHandler.cpp | 8 ++++---- lib/network/NetworkHandler.h | 2 +- lib/network/NetworkServer.cpp | 6 +++--- lib/network/NetworkServer.h | 4 ++-- 6 files changed, 18 insertions(+), 18 deletions(-) diff --git a/lib/network/NetworkConnection.cpp b/lib/network/NetworkConnection.cpp index 937c1dd76..23a440bf5 100644 --- a/lib/network/NetworkConnection.cpp +++ b/lib/network/NetworkConnection.cpp @@ -12,9 +12,9 @@ VCMI_LIB_NAMESPACE_BEGIN -NetworkConnection::NetworkConnection(INetworkConnectionListener & listener, const std::shared_ptr & socket, const std::shared_ptr & context) +NetworkConnection::NetworkConnection(INetworkConnectionListener & listener, const std::shared_ptr & socket, NetworkContext & context) : socket(socket) - , timer(std::make_shared(*context)) + , timer(std::make_shared(context)) , listener(listener) { socket->set_option(boost::asio::ip::tcp::no_delay(true)); @@ -208,7 +208,7 @@ void NetworkConnection::close() //NOTE: ignoring error code, intended } -InternalConnection::InternalConnection(INetworkConnectionListener & listener, const std::shared_ptr & context) +InternalConnection::InternalConnection(INetworkConnectionListener & listener, NetworkContext & context) : io(context) , listener(listener) { @@ -216,7 +216,7 @@ InternalConnection::InternalConnection(INetworkConnectionListener & listener, co void InternalConnection::receivePacket(const std::vector & message) { - boost::asio::post(*io, [self = std::static_pointer_cast(shared_from_this()), message](){ + boost::asio::post(io, [self = std::static_pointer_cast(shared_from_this()), message](){ if (self->connectionActive) self->listener.onPacketReceived(self, message); }); @@ -224,7 +224,7 @@ void InternalConnection::receivePacket(const std::vector & message) void InternalConnection::disconnect() { - boost::asio::post(*io, [self = std::static_pointer_cast(shared_from_this())](){ + boost::asio::post(io, [self = std::static_pointer_cast(shared_from_this())](){ self->listener.onDisconnected(self, "Internal connection has been terminated"); self->otherSideWeak.reset(); self->connectionActive = false; diff --git a/lib/network/NetworkConnection.h b/lib/network/NetworkConnection.h index fe6d34c2b..4a6732294 100644 --- a/lib/network/NetworkConnection.h +++ b/lib/network/NetworkConnection.h @@ -38,7 +38,7 @@ class NetworkConnection final : public INetworkConnection, public std::enable_sh void onDataSent(const boost::system::error_code & ec); public: - NetworkConnection(INetworkConnectionListener & listener, const std::shared_ptr & socket, const std::shared_ptr & context); + NetworkConnection(INetworkConnectionListener & listener, const std::shared_ptr & socket, NetworkContext & context); void start(); void close() override; @@ -49,11 +49,11 @@ public: class InternalConnection final : public IInternalConnection, public std::enable_shared_from_this { std::weak_ptr otherSideWeak; - std::shared_ptr io; + NetworkContext & io; INetworkConnectionListener & listener; bool connectionActive = false; public: - InternalConnection(INetworkConnectionListener & listener, const std::shared_ptr & context); + InternalConnection(INetworkConnectionListener & listener, NetworkContext & context); void receivePacket(const std::vector & message) override; void disconnect() override; diff --git a/lib/network/NetworkHandler.cpp b/lib/network/NetworkHandler.cpp index 4634db414..236bd79e7 100644 --- a/lib/network/NetworkHandler.cpp +++ b/lib/network/NetworkHandler.cpp @@ -21,12 +21,12 @@ std::unique_ptr INetworkHandler::createHandler() } NetworkHandler::NetworkHandler() - : io(std::make_shared()) + : io(std::make_unique()) {} std::unique_ptr NetworkHandler::createServerTCP(INetworkServerListener & listener) { - return std::make_unique(listener, io); + return std::make_unique(listener, *io); } void NetworkHandler::connectToRemote(INetworkClientListener & listener, const std::string & host, uint16_t port) @@ -50,7 +50,7 @@ void NetworkHandler::connectToRemote(INetworkClientListener & listener, const st listener.onConnectionFailed(error.message()); return; } - auto connection = std::make_shared(listener, socket, io); + auto connection = std::make_shared(listener, socket, *io); connection->start(); listener.onConnectionEstablished(connection); @@ -75,7 +75,7 @@ void NetworkHandler::createTimer(INetworkTimerListener & listener, std::chrono:: void NetworkHandler::createInternalConnection(INetworkClientListener & listener, INetworkServer & server) { - auto localConnection = std::make_shared(listener, io); + auto localConnection = std::make_shared(listener, *io); server.receiveInternalConnection(localConnection); diff --git a/lib/network/NetworkHandler.h b/lib/network/NetworkHandler.h index 016ae4311..31cf4cf4d 100644 --- a/lib/network/NetworkHandler.h +++ b/lib/network/NetworkHandler.h @@ -15,7 +15,7 @@ VCMI_LIB_NAMESPACE_BEGIN class NetworkHandler : public INetworkHandler { - std::shared_ptr io; + std::unique_ptr io; public: NetworkHandler(); diff --git a/lib/network/NetworkServer.cpp b/lib/network/NetworkServer.cpp index fbeacd10d..1f8eddd9b 100644 --- a/lib/network/NetworkServer.cpp +++ b/lib/network/NetworkServer.cpp @@ -13,7 +13,7 @@ VCMI_LIB_NAMESPACE_BEGIN -NetworkServer::NetworkServer(INetworkServerListener & listener, const std::shared_ptr & context) +NetworkServer::NetworkServer(INetworkServerListener & listener, NetworkContext & context) : io(context) , listener(listener) { @@ -21,13 +21,13 @@ NetworkServer::NetworkServer(INetworkServerListener & listener, const std::share uint16_t NetworkServer::start(uint16_t port) { - acceptor = std::make_shared(*io, boost::asio::ip::tcp::endpoint(boost::asio::ip::tcp::v4(), port)); + acceptor = std::make_shared(io, boost::asio::ip::tcp::endpoint(boost::asio::ip::tcp::v4(), port)); return startAsyncAccept(); } uint16_t NetworkServer::startAsyncAccept() { - auto upcomingConnection = std::make_shared(*io); + auto upcomingConnection = std::make_shared(io); acceptor->async_accept(*upcomingConnection, [this, upcomingConnection](const auto & ec) { connectionAccepted(upcomingConnection, ec); }); return acceptor->local_endpoint().port(); } diff --git a/lib/network/NetworkServer.h b/lib/network/NetworkServer.h index 2fc4c9d57..766e50a62 100644 --- a/lib/network/NetworkServer.h +++ b/lib/network/NetworkServer.h @@ -15,7 +15,7 @@ VCMI_LIB_NAMESPACE_BEGIN class NetworkServer : public INetworkConnectionListener, public INetworkServer { - std::shared_ptr io; + NetworkContext & io; std::shared_ptr acceptor; std::set> connections; @@ -27,7 +27,7 @@ class NetworkServer : public INetworkConnectionListener, public INetworkServer void onDisconnected(const std::shared_ptr & connection, const std::string & errorMessage) override; void onPacketReceived(const std::shared_ptr & connection, const std::vector & message) override; public: - NetworkServer(INetworkServerListener & listener, const std::shared_ptr & context); + NetworkServer(INetworkServerListener & listener, NetworkContext & context); void receiveInternalConnection(std::shared_ptr remoteConnection) override; From 3547635c058a30490f3b792995db3d643fa17bdc Mon Sep 17 00:00:00 2001 From: Ivan Savenko Date: Sun, 27 Apr 2025 18:05:19 +0300 Subject: [PATCH 3/9] Fix (mostly false-positive) memory leaks in task dispatching --- client/eventsSDL/InputHandler.cpp | 18 +++++++++++++----- client/eventsSDL/InputHandler.h | 3 +++ 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/client/eventsSDL/InputHandler.cpp b/client/eventsSDL/InputHandler.cpp index 9f02fd966..5b20905e9 100644 --- a/client/eventsSDL/InputHandler.cpp +++ b/client/eventsSDL/InputHandler.cpp @@ -405,23 +405,31 @@ int InputHandler::getNumTouchFingers() const void InputHandler::dispatchMainThread(const std::function & functor) { - auto heapFunctor = new std::function(functor); + auto heapFunctor = std::make_unique>(functor); SDL_Event event; event.user.type = SDL_USEREVENT; event.user.code = 0; - event.user.data1 = static_cast (heapFunctor); + event.user.data1 = static_cast (heapFunctor.get()); event.user.data2 = nullptr; SDL_PushEvent(&event); + + // NOTE: approach with dispatchedTasks container is a bit excessive + // used mostly to prevent false-positives leaks in analyzers + dispatchedTasks.push(std::move(heapFunctor)); } void InputHandler::handleUserEvent(const SDL_UserEvent & current) { - auto heapFunctor = static_cast*>(current.data1); + std::unique_ptr> task; - (*heapFunctor)(); + if (!dispatchedTasks.try_pop(task)) + throw std::runtime_error("InputHandler::handleUserEvent received without active task!"); - delete heapFunctor; + if (current.data1 != task.get()) + throw std::runtime_error("InputHandler::handleUserEvent received unknown pointer!"); + + (*task)(); } const Point & InputHandler::getCursorPosition() const diff --git a/client/eventsSDL/InputHandler.h b/client/eventsSDL/InputHandler.h index 093ebdaa0..da0f92dfb 100644 --- a/client/eventsSDL/InputHandler.h +++ b/client/eventsSDL/InputHandler.h @@ -12,6 +12,8 @@ #include "../lib/Rect.h" +#include + enum class EUserEvent; enum class MouseButton; union SDL_Event; @@ -33,6 +35,7 @@ enum class InputMode class InputHandler { std::vector eventsQueue; + tbb::concurrent_queue>> dispatchedTasks; std::mutex eventsMutex; Point cursorPosition; From 4bafab9ad4f9f9999285019409034f860625605d Mon Sep 17 00:00:00 2001 From: Ivan Savenko Date: Sun, 27 Apr 2025 19:55:16 +0300 Subject: [PATCH 4/9] Fix possible leak due to usage of raw pointers in filesystem --- lib/filesystem/AdapterLoaders.cpp | 7 ++-- lib/filesystem/AdapterLoaders.h | 4 +- lib/filesystem/CCompressedStream.cpp | 15 ++++---- lib/filesystem/CCompressedStream.h | 2 +- lib/filesystem/Filesystem.cpp | 55 ++++++++++++++-------------- lib/filesystem/Filesystem.h | 10 ++--- lib/modding/CModHandler.cpp | 14 +++---- lib/modding/CModHandler.h | 2 +- mapeditor/helper.cpp | 8 ++-- test/CVcmiTestConfig.cpp | 12 +++--- 10 files changed, 65 insertions(+), 64 deletions(-) diff --git a/lib/filesystem/AdapterLoaders.cpp b/lib/filesystem/AdapterLoaders.cpp index 90d76ea2c..86d41a009 100644 --- a/lib/filesystem/AdapterLoaders.cpp +++ b/lib/filesystem/AdapterLoaders.cpp @@ -157,11 +157,12 @@ std::vector CFilesystemList::getResourcesWithName return ret; } -void CFilesystemList::addLoader(ISimpleResourceLoader * loader, bool writeable) +void CFilesystemList::addLoader(std::unique_ptr loader, bool writeable) { - loaders.push_back(std::unique_ptr(loader)); if (writeable) - writeableLoaders.insert(loader); + writeableLoaders.insert(loader.get()); + + loaders.push_back(std::move(loader)); } bool CFilesystemList::removeLoader(ISimpleResourceLoader * loader) diff --git a/lib/filesystem/AdapterLoaders.h b/lib/filesystem/AdapterLoaders.h index 6fea768d7..f9b7380f0 100644 --- a/lib/filesystem/AdapterLoaders.h +++ b/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 loader, bool writeable); + /** * Removes loader from the loader list * Take care about memory deallocation diff --git a/lib/filesystem/CCompressedStream.cpp b/lib/filesystem/CCompressedStream.cpp index 8c3970e1f..dad10b4aa 100644 --- a/lib/filesystem/CCompressedStream.cpp +++ b/lib/filesystem/CCompressedStream.cpp @@ -97,7 +97,7 @@ CCompressedStream::CCompressedStream(std::unique_ptr stream, bool assert(gzipStream); // Allocate inflate state - inflateState = new z_stream(); + inflateState = std::make_unique(); inflateState->zalloc = Z_NULL; inflateState->zfree = Z_NULL; inflateState->opaque = Z_NULL; @@ -108,15 +108,14 @@ CCompressedStream::CCompressedStream(std::unique_ptr 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(); diff --git a/lib/filesystem/CCompressedStream.h b/lib/filesystem/CCompressedStream.h index a16d7b67b..51b804c6a 100644 --- a/lib/filesystem/CCompressedStream.h +++ b/lib/filesystem/CCompressedStream.h @@ -132,7 +132,7 @@ private: std::vector compressedBuffer; /** struct with current zlib inflate state */ - z_stream_s * inflateState; + std::unique_ptr inflateState; enum EState { diff --git a/lib/filesystem/Filesystem.cpp b/lib/filesystem/Filesystem.cpp index 5ec40b361..54c44d574 100644 --- a/lib/filesystem/Filesystem.cpp +++ b/lib/filesystem/Filesystem.cpp @@ -28,7 +28,7 @@ std::map CResourceHandler::knownLoaders = s CResourceHandler CResourceHandler::globalResourceHandler; CFilesystemGenerator::CFilesystemGenerator(std::string prefix, bool extractArchives): - filesystem(new CFilesystemList()), + filesystem(std::make_unique()), prefix(std::move(prefix)), extractArchives(extractArchives) { @@ -72,9 +72,9 @@ void CFilesystemGenerator::loadConfig(const JsonNode & config) } } -CFilesystemList * CFilesystemGenerator::getFilesystem() +std::unique_ptr 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(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(mountPoint, *filename), false); } template @@ -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(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(configData.first.get()), configData.second, URI); - filesystem->addLoader(new CMappedFileLoader(mountPoint, configInitial), false); + filesystem->addLoader(std::make_unique(mountPoint, configInitial), false); } } -ISimpleResourceLoader * CResourceHandler::createInitial() +std::unique_ptr 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(); //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(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("", path, 1, true), false); } - initialLoader->addLoader(new CFilesystemLoader("", VCMIDirs::get().userDataPath(), 0, true), false); + initialLoader->addLoader(std::make_unique("", 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("SAVES/", VCMIDirs::get().userSavePath()); + auto configLoader = std::make_unique("CONFIG/", VCMIDirs::get().userConfigPath()); + globalResourceHandler.rootLoader = std::make_unique(); 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(); + 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()); + 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 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(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 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 diff --git a/lib/filesystem/Filesystem.h b/lib/filesystem/Filesystem.h index 532a3b2e7..dbda14a5a 100644 --- a/lib/filesystem/Filesystem.h +++ b/lib/filesystem/Filesystem.h @@ -24,7 +24,7 @@ class DLL_LINKAGE CFilesystemGenerator using TLoadFunctor = std::function; using TLoadFunctorMap = std::map; - CFilesystemList * filesystem; + std::unique_ptr filesystem; std::string prefix; template @@ -44,7 +44,7 @@ public: void loadConfig(const JsonNode & config); /// returns generated filesystem - CFilesystemList * getFilesystem(); + std::unique_ptr 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 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 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 createFileSystem(const std::string &prefix, const JsonNode & fsConfig, bool extractArchives = false); ~CResourceHandler() = default; private: diff --git a/lib/modding/CModHandler.cpp b/lib/modding/CModHandler.cpp index e7fb31dae..b8b775a86 100644 --- a/lib/modding/CModHandler.cpp +++ b/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 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 modFilesystems; + std::map> 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 & modFilesystems) +void CModHandler::checkModFilesystemsConflicts(const std::map> & modFilesystems) { for(const auto & [leftName, leftFilesystem] : modFilesystems) { diff --git a/lib/modding/CModHandler.h b/lib/modding/CModHandler.h index ae7bfc531..f5ada35ac 100644 --- a/lib/modding/CModHandler.h +++ b/lib/modding/CModHandler.h @@ -27,7 +27,7 @@ class DLL_LINKAGE CModHandler final : boost::noncopyable std::set validationPassed; void loadTranslation(const TModID & modName); - void checkModFilesystemsConflicts(const std::map & modFilesystems); + void checkModFilesystemsConflicts(const std::map> & modFilesystems); bool isModValidationNeeded(const ModDescription & mod) const; diff --git a/mapeditor/helper.cpp b/mapeditor/helper.cpp index 4d183299d..62982c4ba 100644 --- a/mapeditor/helper.cpp +++ b/mapeditor/helper.cpp @@ -32,9 +32,9 @@ std::unique_ptr 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("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 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("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"); diff --git a/test/CVcmiTestConfig.cpp b/test/CVcmiTestConfig.cpp index 6e7547e75..3f4bfbd51 100644 --- a/test/CVcmiTestConfig.cpp +++ b/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(CResourceHandler::get("core"))->addLoader(loader, false); + auto loader = std::make_unique("test/", TEST_DATA_DIR); + dynamic_cast(CResourceHandler::get("core"))->addLoader(std::move(loader), false); - loader = new CFilesystemLoader("scripts/test/erm/", TEST_DATA_DIR+"erm/"); - dynamic_cast(CResourceHandler::get("core"))->addLoader(loader, false); + loader = std::make_unique("scripts/test/erm/", TEST_DATA_DIR+"erm/"); + dynamic_cast(CResourceHandler::get("core"))->addLoader(std::move(loader), false); - loader = new CFilesystemLoader("scripts/test/lua/", TEST_DATA_DIR+"lua/"); - dynamic_cast(CResourceHandler::get("core"))->addLoader(loader, false); + loader = std::make_unique("scripts/test/lua/", TEST_DATA_DIR+"lua/"); + dynamic_cast(CResourceHandler::get("core"))->addLoader(std::move(loader), false); } } From c0fb1d1b3b1f71c0a0b352b112ecdf951d0724c6 Mon Sep 17 00:00:00 2001 From: Ivan Savenko Date: Sun, 27 Apr 2025 19:58:21 +0300 Subject: [PATCH 5/9] Replace some raw pointers with unique's or optional --- Global.h | 8 ------- client/lobby/CCampaignInfoScreen.cpp | 14 +++++------- client/lobby/CCampaignInfoScreen.h | 6 +++--- client/lobby/CScenarioInfoScreen.cpp | 14 +++++------- client/lobby/CScenarioInfoScreen.h | 5 ++--- client/lobby/OptionsTab.cpp | 2 +- clientapp/EntryPoint.cpp | 6 ++++-- lib/entities/faction/CFaction.cpp | 9 +------- lib/entities/faction/CFaction.h | 2 +- lib/entities/faction/CTownHandler.cpp | 18 ++++++---------- lib/entities/faction/CTownHandler.h | 3 +-- lib/gameState/InfoAboutArmy.cpp | 24 ++++++--------------- lib/gameState/InfoAboutArmy.h | 8 +++---- lib/mapObjects/CGTownInstance.cpp | 4 ++-- mapeditor/mapsettings/victoryconditions.cpp | 2 +- serverapp/EntryPoint.cpp | 2 +- test/entity/CFactionTest.cpp | 6 +++--- 17 files changed, 47 insertions(+), 86 deletions(-) diff --git a/Global.h b/Global.h index 10e57cb5a..8312a3184 100644 --- a/Global.h +++ b/Global.h @@ -465,14 +465,6 @@ namespace vstd } }; - //deleted pointer and sets it to nullptr - template - void clear_pointer(T* &ptr) - { - delete ptr; - ptr = nullptr; - } - template typename Container::const_reference circularAt(const Container &r, size_t index) { diff --git a/client/lobby/CCampaignInfoScreen.cpp b/client/lobby/CCampaignInfoScreen.cpp index dfc279329..b6edce298 100644 --- a/client/lobby/CCampaignInfoScreen.cpp +++ b/client/lobby/CCampaignInfoScreen.cpp @@ -23,8 +23,8 @@ CCampaignInfoScreen::CCampaignInfoScreen() { OBJECT_CONSTRUCTION; - localSi = new StartInfo(*GAME->interface()->cb->getStartInfo()); - localMi = new CMapInfo(); + localSi = std::make_unique(*GAME->interface()->cb->getStartInfo()); + localMi = std::make_unique(); localMi->mapHeader = std::unique_ptr(new CMapHeader(*GAME->interface()->cb->getMapHeader())); screenType = ESelectionScreen::scenarioInfo; @@ -32,17 +32,13 @@ CCampaignInfoScreen::CCampaignInfoScreen() updateAfterStateChange(); } -CCampaignInfoScreen::~CCampaignInfoScreen() -{ - vstd::clear_pointer(localSi); - vstd::clear_pointer(localMi); -} +CCampaignInfoScreen::~CCampaignInfoScreen() = default; const CMapInfo * CCampaignInfoScreen::getMapInfo() { - return localMi; + return localMi.get(); } const StartInfo * CCampaignInfoScreen::getStartInfo() { - return localSi; + return localSi.get(); } diff --git a/client/lobby/CCampaignInfoScreen.h b/client/lobby/CCampaignInfoScreen.h index bceab47a6..ea933bc3a 100644 --- a/client/lobby/CCampaignInfoScreen.h +++ b/client/lobby/CCampaignInfoScreen.h @@ -14,8 +14,8 @@ class CCampaignInfoScreen : public CBonusSelection, ISelectionScreenInfo { - const StartInfo * localSi; - CMapInfo * localMi; + std::unique_ptr localSi; + std::unique_ptr localMi; public: CCampaignInfoScreen(); @@ -23,4 +23,4 @@ public: const CMapInfo * getMapInfo() override; const StartInfo * getStartInfo() override; -}; \ No newline at end of file +}; diff --git a/client/lobby/CScenarioInfoScreen.cpp b/client/lobby/CScenarioInfoScreen.cpp index f5f893f51..5bd25b3da 100644 --- a/client/lobby/CScenarioInfoScreen.cpp +++ b/client/lobby/CScenarioInfoScreen.cpp @@ -33,8 +33,8 @@ CScenarioInfoScreen::CScenarioInfoScreen() pos.h = 600; pos = center(); - localSi = new StartInfo(*GAME->interface()->cb->getStartInfo()); - localMi = new CMapInfo(); + localSi = std::make_unique(*GAME->interface()->cb->getStartInfo()); + localMi = std::make_unique(); localMi->mapHeader = std::unique_ptr(new CMapHeader(*GAME->interface()->cb->getMapHeader())); screenType = ESelectionScreen::scenarioInfo; @@ -49,18 +49,14 @@ CScenarioInfoScreen::CScenarioInfoScreen() buttonBack = std::make_shared(Point(584, 535), AnimationPath::builtin("SCNRBACK.DEF"), LIBRARY->generaltexth->zelp[105], [this](){ close();}, EShortcut::GLOBAL_CANCEL); } -CScenarioInfoScreen::~CScenarioInfoScreen() -{ - vstd::clear_pointer(localSi); - vstd::clear_pointer(localMi); -} +CScenarioInfoScreen::~CScenarioInfoScreen() = default; const CMapInfo * CScenarioInfoScreen::getMapInfo() { - return localMi; + return localMi.get(); } const StartInfo * CScenarioInfoScreen::getStartInfo() { - return localSi; + return localSi.get(); } diff --git a/client/lobby/CScenarioInfoScreen.h b/client/lobby/CScenarioInfoScreen.h index e2a895569..c1d19e668 100644 --- a/client/lobby/CScenarioInfoScreen.h +++ b/client/lobby/CScenarioInfoScreen.h @@ -14,14 +14,13 @@ /// Scenario information screen shown during the game class CScenarioInfoScreen : public WindowBase, public ISelectionScreenInfo { + std::unique_ptr localSi; + std::unique_ptr localMi; public: std::shared_ptr buttonBack; std::shared_ptr card; std::shared_ptr opt; - const StartInfo * localSi; - CMapInfo * localMi; - CScenarioInfoScreen(); ~CScenarioInfoScreen(); diff --git a/client/lobby/OptionsTab.cpp b/client/lobby/OptionsTab.cpp index 544d3334c..a5645b55b 100644 --- a/client/lobby/OptionsTab.cpp +++ b/client/lobby/OptionsTab.cpp @@ -379,7 +379,7 @@ void OptionsTab::CPlayerOptionTooltipBox::genTownWindow() genHeader(); labelAssociatedCreatures = std::make_shared(pos.w / 2 + 8, 122, FONT_MEDIUM, ETextAlignment::CENTER, Colors::YELLOW, LIBRARY->generaltexth->allTexts[79]); std::vector> components; - const CTown * town = (*LIBRARY->townh)[factionIndex]->town; + const CTown * town = (*LIBRARY->townh)[factionIndex]->town.get(); for(auto & elem : town->creatures) { diff --git a/clientapp/EntryPoint.cpp b/clientapp/EntryPoint.cpp index 1485ed4dc..faa54b499 100644 --- a/clientapp/EntryPoint.cpp +++ b/clientapp/EntryPoint.cpp @@ -416,7 +416,8 @@ int main(int argc, char * argv[]) if(!settings["session"]["headless"].Bool()) { CMessage::dispose(); - vstd::clear_pointer(graphics); + delete graphics; + graphics = nullptr; } // must be executed before reset - since unique_ptr resets pointer to null before calling destructor @@ -424,7 +425,8 @@ int main(int argc, char * argv[]) ENGINE.reset(); - vstd::clear_pointer(LIBRARY); + delete LIBRARY; + LIBRARY = nullptr; logConfigurator.deconfigure(); std::cout << "Ending...\n"; diff --git a/lib/entities/faction/CFaction.cpp b/lib/entities/faction/CFaction.cpp index cab225089..07cb6673d 100644 --- a/lib/entities/faction/CFaction.cpp +++ b/lib/entities/faction/CFaction.cpp @@ -17,14 +17,7 @@ VCMI_LIB_NAMESPACE_BEGIN -CFaction::~CFaction() -{ - if (town) - { - delete town; - town = nullptr; - } -} +CFaction::~CFaction() = default; int32_t CFaction::getIndex() const { diff --git a/lib/entities/faction/CFaction.h b/lib/entities/faction/CFaction.h index 27d6a2239..73c41ae1d 100644 --- a/lib/entities/faction/CFaction.h +++ b/lib/entities/faction/CFaction.h @@ -51,7 +51,7 @@ public: /// and for placing heroes directly on boat (in map editor, water prisons & taverns) BoatId boatType = BoatId::CASTLE; - CTown * town = nullptr; //NOTE: can be null + std::unique_ptr town; //NOTE: can be null ImagePath creatureBg120; ImagePath creatureBg130; diff --git a/lib/entities/faction/CTownHandler.cpp b/lib/entities/faction/CTownHandler.cpp index a2dcbdaad..26c1c19a0 100644 --- a/lib/entities/faction/CTownHandler.cpp +++ b/lib/entities/faction/CTownHandler.cpp @@ -38,19 +38,15 @@ const int NAMES_PER_TOWN=16; // number of town names per faction in H3 files. Js CTownHandler::CTownHandler() : buildingsLibrary(JsonPath::builtin("config/buildingsLibrary")) - , randomTown(new CTown()) - , randomFaction(new CFaction()) + , randomFaction(std::make_unique()) { - randomFaction->town = randomTown; - randomTown->faction = randomFaction; + randomFaction->town = std::make_unique(); + randomFaction->town->faction = randomFaction.get(); randomFaction->identifier = "random"; randomFaction->modScope = "core"; } -CTownHandler::~CTownHandler() -{ - delete randomFaction; // will also delete randomTown -} +CTownHandler::~CTownHandler() = default; JsonNode readBuilding(CLegacyConfigParser & parser) { @@ -767,9 +763,9 @@ std::shared_ptr CTownHandler::loadFromJson(const std::string & scope, if (!source["town"].isNull()) { - faction->town = new CTown(); + faction->town = std::make_unique(); faction->town->faction = faction.get(); - loadTown(faction->town, source["town"]); + loadTown(faction->town.get(), source["town"]); } else faction->town = nullptr; @@ -854,7 +850,7 @@ void CTownHandler::loadRandomFaction() { JsonNode randomFactionJson(JsonPath::builtin("config/factions/random.json")); randomFactionJson.setModScope(ModScope::scopeBuiltin(), true); - loadBuildings(randomTown, randomFactionJson["random"]["town"]["buildings"]); + loadBuildings(randomFaction->town.get(), randomFactionJson["random"]["town"]["buildings"]); } void CTownHandler::loadCustom() diff --git a/lib/entities/faction/CTownHandler.h b/lib/entities/faction/CTownHandler.h index f4066934f..41a861672 100644 --- a/lib/entities/faction/CTownHandler.h +++ b/lib/entities/faction/CTownHandler.h @@ -63,8 +63,7 @@ class DLL_LINKAGE CTownHandler : public CHandlerBase randomFaction; CTownHandler(); ~CTownHandler(); diff --git a/lib/gameState/InfoAboutArmy.cpp b/lib/gameState/InfoAboutArmy.cpp index 6002a5040..cc56290ff 100644 --- a/lib/gameState/InfoAboutArmy.cpp +++ b/lib/gameState/InfoAboutArmy.cpp @@ -71,10 +71,10 @@ void InfoAboutArmy::initFromArmy(const CArmedInstance *Army, bool detailed) void InfoAboutHero::assign(const InfoAboutHero & iah) { - vstd::clear_pointer(details); + details.reset();; InfoAboutArmy::operator = (iah); - details = (iah.details ? new Details(*iah.details) : nullptr); + details = iah.details; hclass = iah.hclass; portraitSource = iah.portraitSource; } @@ -92,11 +92,6 @@ InfoAboutHero::InfoAboutHero(const CGHeroInstance * h, InfoAboutHero::EInfoLevel initFromHero(h, infoLevel); } -InfoAboutHero::~InfoAboutHero() -{ - vstd::clear_pointer(details); -} - InfoAboutHero & InfoAboutHero::operator=(const InfoAboutHero & iah) { assign(iah); @@ -110,7 +105,7 @@ int32_t InfoAboutHero::getIconIndex() const void InfoAboutHero::initFromHero(const CGHeroInstance *h, InfoAboutHero::EInfoLevel infoLevel) { - vstd::clear_pointer(details); + details.reset(); if(!h) return; @@ -125,7 +120,7 @@ void InfoAboutHero::initFromHero(const CGHeroInstance *h, InfoAboutHero::EInfoLe if(detailed) { //include details about hero - details = new Details(); + details = Details(); details->luck = h->luckVal(); details->morale = h->moraleVal(); details->mana = h->mana; @@ -143,7 +138,6 @@ void InfoAboutHero::initFromHero(const CGHeroInstance *h, InfoAboutHero::EInfoLe } InfoAboutTown::InfoAboutTown(): - details(nullptr), tType(nullptr), built(0), fortLevel(0) @@ -152,7 +146,6 @@ InfoAboutTown::InfoAboutTown(): } InfoAboutTown::InfoAboutTown(const CGTownInstance *t, bool detailed): - details(nullptr), tType(nullptr), built(0), fortLevel(0) @@ -160,11 +153,6 @@ InfoAboutTown::InfoAboutTown(const CGTownInstance *t, bool detailed): initFromTown(t, detailed); } -InfoAboutTown::~InfoAboutTown() -{ - vstd::clear_pointer(details); -} - void InfoAboutTown::initFromTown(const CGTownInstance *t, bool detailed) { initFromArmy(t, detailed); @@ -174,12 +162,12 @@ void InfoAboutTown::initFromTown(const CGTownInstance *t, bool detailed) name = t->getNameTranslated(); tType = t->getTown(); - vstd::clear_pointer(details); + details.reset(); if(detailed) { //include details about hero - details = new Details(); + details = Details(); TResources income = t->dailyIncome(); details->goldIncome = income[EGameResID::GOLD]; details->customRes = t->hasBuilt(BuildingID::RESOURCE_SILO); diff --git a/lib/gameState/InfoAboutArmy.h b/lib/gameState/InfoAboutArmy.h index 569084c83..bdbd47e9e 100644 --- a/lib/gameState/InfoAboutArmy.h +++ b/lib/gameState/InfoAboutArmy.h @@ -52,7 +52,7 @@ public: si32 mana, manaLimit, luck, morale; }; - Details * details = nullptr; + std::optional
details; const CHeroClass *hclass; HeroTypeID portraitSource; @@ -67,7 +67,6 @@ public: InfoAboutHero(); InfoAboutHero(const InfoAboutHero & iah); InfoAboutHero(const CGHeroInstance *h, EInfoLevel infoLevel); - ~InfoAboutHero(); InfoAboutHero & operator=(const InfoAboutHero & iah); @@ -84,7 +83,9 @@ struct DLL_LINKAGE InfoAboutTown : public InfoAboutArmy bool customRes; bool garrisonedHero; - } *details; + }; + + std::optional
details; const CTown *tType; @@ -93,7 +94,6 @@ struct DLL_LINKAGE InfoAboutTown : public InfoAboutArmy InfoAboutTown(); InfoAboutTown(const CGTownInstance *t, bool detailed); - ~InfoAboutTown(); void initFromTown(const CGTownInstance *t, bool detailed); }; diff --git a/lib/mapObjects/CGTownInstance.cpp b/lib/mapObjects/CGTownInstance.cpp index fbd488c25..615688279 100644 --- a/lib/mapObjects/CGTownInstance.cpp +++ b/lib/mapObjects/CGTownInstance.cpp @@ -1145,9 +1145,9 @@ const CFaction * CGTownInstance::getFaction() const const CTown * CGTownInstance::getTown() const { if(ID == Obj::RANDOM_TOWN) - return LIBRARY->townh->randomTown; + return LIBRARY->townh->randomFaction->town.get(); - return getFaction()->town; + return getFaction()->town.get(); } FactionID CGTownInstance::getFactionID() const diff --git a/mapeditor/mapsettings/victoryconditions.cpp b/mapeditor/mapsettings/victoryconditions.cpp index e2dae21bf..ac355859a 100644 --- a/mapeditor/mapsettings/victoryconditions.cpp +++ b/mapeditor/mapsettings/victoryconditions.cpp @@ -423,7 +423,7 @@ void VictoryConditions::on_victoryComboBox_currentIndexChanged(int index) case 3: { //EventCondition::HAVE_BUILDING victoryTypeWidget = new QComboBox; ui->victoryParamsLayout->addWidget(victoryTypeWidget); - auto * ctown = LIBRARY->townh->randomTown; + auto * ctown = LIBRARY->townh->randomFaction->town.get(); for(int bId : ctown->getAllBuildings()) victoryTypeWidget->addItem(QString::fromStdString(defaultBuildingIdConversion(BuildingID(bId))), QVariant::fromValue(bId)); diff --git a/serverapp/EntryPoint.cpp b/serverapp/EntryPoint.cpp index 9bcce94b3..d32643137 100644 --- a/serverapp/EntryPoint.cpp +++ b/serverapp/EntryPoint.cpp @@ -100,7 +100,7 @@ int main(int argc, const char * argv[]) } logConfigurator.deconfigure(); - vstd::clear_pointer(LIBRARY); + delete LIBRARY; return 0; } diff --git a/test/entity/CFactionTest.cpp b/test/entity/CFactionTest.cpp index 3e1b9e31f..01bb815a7 100644 --- a/test/entity/CFactionTest.cpp +++ b/test/entity/CFactionTest.cpp @@ -34,9 +34,9 @@ protected: TEST_F(CFactionTest, HasTown) { EXPECT_FALSE(subject->hasTown()); - subject->town = new CTown(); + subject->town = std::make_unique(); EXPECT_TRUE(subject->hasTown()); - vstd::clear_pointer(subject->town); + subject->town.reset(); EXPECT_FALSE(subject->hasTown()); } @@ -56,7 +56,7 @@ TEST_F(CFactionTest, RegistersIcons) registarCb(std::forward(PH1), std::forward(PH2), std::forward(PH3), std::forward(PH4)); }; - subject->town = new CTown(); + subject->town = std::make_unique(); CTown::ClientInfo & info = subject->town->clientInfo; From 554a4143028f610894fd0266168f76ce978508f8 Mon Sep 17 00:00:00 2001 From: Ivan Savenko Date: Mon, 28 Apr 2025 13:55:57 +0300 Subject: [PATCH 6/9] Fix remaining memory leaks caused by API misuse --- client/CServerHandler.cpp | 5 +++++ client/eventsSDL/InputHandler.cpp | 5 +---- client/media/CVideoHandler.cpp | 5 +++++ client/renderSDL/SDLImage.cpp | 3 ++- client/renderSDL/SDLImageScaler.cpp | 4 ++-- client/renderSDL/ScalableImage.cpp | 3 ++- 6 files changed, 17 insertions(+), 8 deletions(-) diff --git a/client/CServerHandler.cpp b/client/CServerHandler.cpp index 5573c0b8e..afe4d462e 100644 --- a/client/CServerHandler.cpp +++ b/client/CServerHandler.cpp @@ -57,6 +57,7 @@ #include "LobbyClientNetPackVisitors.h" #include +#include #include @@ -114,9 +115,13 @@ void CServerHandler::threadRunNetwork() } catch (const TerminationRequestedException &) { + // VCMI can run SDL methods on network thread, leading to usage of thread-local storage by SDL + // Such storage needs to be cleaned up manually for threads that were not created by SDL + SDL_TLSCleanup(); logGlobal->info("Terminating network thread"); return; } + SDL_TLSCleanup(); logGlobal->info("Ending network thread"); } diff --git a/client/eventsSDL/InputHandler.cpp b/client/eventsSDL/InputHandler.cpp index 5b20905e9..4806df355 100644 --- a/client/eventsSDL/InputHandler.cpp +++ b/client/eventsSDL/InputHandler.cpp @@ -410,7 +410,7 @@ void InputHandler::dispatchMainThread(const std::function & functor) SDL_Event event; event.user.type = SDL_USEREVENT; event.user.code = 0; - event.user.data1 = static_cast (heapFunctor.get()); + event.user.data1 = nullptr; event.user.data2 = nullptr; SDL_PushEvent(&event); @@ -426,9 +426,6 @@ void InputHandler::handleUserEvent(const SDL_UserEvent & current) if (!dispatchedTasks.try_pop(task)) throw std::runtime_error("InputHandler::handleUserEvent received without active task!"); - if (current.data1 != task.get()) - throw std::runtime_error("InputHandler::handleUserEvent received unknown pointer!"); - (*task)(); } diff --git a/client/media/CVideoHandler.cpp b/client/media/CVideoHandler.cpp index f8ca00985..e6beec141 100644 --- a/client/media/CVideoHandler.cpp +++ b/client/media/CVideoHandler.cpp @@ -373,6 +373,11 @@ FFMpegStream::~FFMpegStream() avcodec_free_context(&codecContext); avformat_close_input(&formatContext); + + // for some reason, buffer is managed (e.g. reallocated) by FFmpeg + // however, it must still be freed manually by user + if (context->buffer) + av_free(context->buffer); av_free(context); } diff --git a/client/renderSDL/SDLImage.cpp b/client/renderSDL/SDLImage.cpp index bc1554e48..9be02d437 100644 --- a/client/renderSDL/SDLImage.cpp +++ b/client/renderSDL/SDLImage.cpp @@ -448,5 +448,6 @@ void SDLImageShared::savePalette() SDLImageShared::~SDLImageShared() { SDL_FreeSurface(surf); - SDL_FreePalette(originalPalette); + if (originalPalette) + SDL_FreePalette(originalPalette); } diff --git a/client/renderSDL/SDLImageScaler.cpp b/client/renderSDL/SDLImageScaler.cpp index da9484f5e..c52c92004 100644 --- a/client/renderSDL/SDLImageScaler.cpp +++ b/client/renderSDL/SDLImageScaler.cpp @@ -236,8 +236,8 @@ SDLImageScaler::~SDLImageScaler() { ENGINE->dispatchMainThread([surface = intermediate]() { - // potentially SDL bug, execute SDL_FreeSurface in main thread to avoid thread races to its internal state - // may be fixed somewhere between 2.26.5 - 2.30 + // SDL_FreeSurface must be executed in main thread to avoid thread races to its internal state + // will be no longer necessary in SDL 3 SDL_FreeSurface(surface); }); } diff --git a/client/renderSDL/ScalableImage.cpp b/client/renderSDL/ScalableImage.cpp index c1fda7d94..a7a8746f8 100644 --- a/client/renderSDL/ScalableImage.cpp +++ b/client/renderSDL/ScalableImage.cpp @@ -90,7 +90,8 @@ ScalableImageParameters::ScalableImageParameters(const SDL_Palette * originalPal ScalableImageParameters::~ScalableImageParameters() { - SDL_FreePalette(palette); + if (palette) + SDL_FreePalette(palette); } void ScalableImageParameters::preparePalette(const SDL_Palette * originalPalette, EImageBlitMode blitMode) From be2ebc8e56fbc1b3bccae48c9b0115cb9d45323a Mon Sep 17 00:00:00 2001 From: Ivan Savenko Date: Tue, 29 Apr 2025 13:03:08 +0300 Subject: [PATCH 7/9] Fix build --- lib/filesystem/Filesystem.cpp | 2 ++ lib/filesystem/Filesystem.h | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/filesystem/Filesystem.cpp b/lib/filesystem/Filesystem.cpp index 54c44d574..e89f882d6 100644 --- a/lib/filesystem/Filesystem.cpp +++ b/lib/filesystem/Filesystem.cpp @@ -34,6 +34,8 @@ CFilesystemGenerator::CFilesystemGenerator(std::string prefix, bool extractArchi { } +CFilesystemGenerator::~CFilesystemGenerator() = default; + CFilesystemGenerator::TLoadFunctorMap CFilesystemGenerator::genFunctorMap() { TLoadFunctorMap map; diff --git a/lib/filesystem/Filesystem.h b/lib/filesystem/Filesystem.h index dbda14a5a..cc03f6039 100644 --- a/lib/filesystem/Filesystem.h +++ b/lib/filesystem/Filesystem.h @@ -19,7 +19,7 @@ class CFilesystemList; class JsonNode; /// Helper class that allows generation of a ISimpleResourceLoader entry out of Json config(s) -class DLL_LINKAGE CFilesystemGenerator +class DLL_LINKAGE CFilesystemGenerator : boost::noncopyable { using TLoadFunctor = std::function; using TLoadFunctorMap = std::map; @@ -38,6 +38,7 @@ public: /// prefix = prefix that will be given to file entries in all nodes of this filesystem /// extractArchives = Specifies if Original H3 archives should be extracted to a separate folder CFilesystemGenerator(std::string prefix, bool extractArchives = false); + ~CFilesystemGenerator(); /// loads configuration from json /// config - configuration to load, using format of "filesystem" entry in config/filesystem.json From 04ba01719873945912b3b252acb390db48e886fb Mon Sep 17 00:00:00 2001 From: Ivan Savenko Date: Tue, 29 Apr 2025 13:17:21 +0300 Subject: [PATCH 8/9] Deallocate sound chunk immediately on end of playback --- client/media/CSoundHandler.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/client/media/CSoundHandler.cpp b/client/media/CSoundHandler.cpp index 93ad0bfb7..b5a427cf2 100644 --- a/client/media/CSoundHandler.cpp +++ b/client/media/CSoundHandler.cpp @@ -325,6 +325,8 @@ void CSoundHandler::soundFinishedCallback(int channel) { std::scoped_lock lockGuard(mutexCallbacks); + uncachedPlayingChunks.erase(channel); + if(callbacks.count(channel) == 0) return; From 6f587a243b96d957c234837cc5c4642b01764d9d Mon Sep 17 00:00:00 2001 From: Ivan Savenko Date: Tue, 29 Apr 2025 13:28:30 +0300 Subject: [PATCH 9/9] Fix build --- lib/entities/faction/CFaction.cpp | 1 + lib/entities/faction/CFaction.h | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/entities/faction/CFaction.cpp b/lib/entities/faction/CFaction.cpp index 07cb6673d..1d7deaab1 100644 --- a/lib/entities/faction/CFaction.cpp +++ b/lib/entities/faction/CFaction.cpp @@ -17,6 +17,7 @@ VCMI_LIB_NAMESPACE_BEGIN +CFaction::CFaction() = default; CFaction::~CFaction() = default; int32_t CFaction::getIndex() const diff --git a/lib/entities/faction/CFaction.h b/lib/entities/faction/CFaction.h index 73c41ae1d..dc6ca8282 100644 --- a/lib/entities/faction/CFaction.h +++ b/lib/entities/faction/CFaction.h @@ -58,7 +58,7 @@ public: std::vector puzzleMap; - CFaction() = default; + CFaction(); ~CFaction(); int32_t getIndex() const override;