From d8c3e7fcee4906b6f9bc254222daf25ec10627bf Mon Sep 17 00:00:00 2001 From: momo5502 Date: Fri, 2 Jun 2017 15:36:20 +0200 Subject: [PATCH] [String] Make VA leak-safe --- src/Components/Loader.cpp | 10 +-- src/Components/Loader.hpp | 3 - src/Components/Modules/CardTitles.cpp | 10 +-- src/Components/Modules/Command.cpp | 2 +- src/Components/Modules/Localization.cpp | 6 +- src/Components/Modules/Menus.cpp | 14 ++--- src/Components/Modules/StringTable.cpp | 2 +- src/Components/Modules/ZoneBuilder.cpp | 2 +- src/Game/Functions.cpp | 2 +- src/STDInclude.hpp | 1 + src/Utils/Memory.cpp | 9 ++- src/Utils/Memory.hpp | 5 ++ src/Utils/String.cpp | 34 +---------- src/Utils/String.hpp | 81 +++++++++++++++++++++++-- 14 files changed, 113 insertions(+), 68 deletions(-) diff --git a/src/Components/Loader.cpp b/src/Components/Loader.cpp index b773edff..c2edf063 100644 --- a/src/Components/Loader.cpp +++ b/src/Components/Loader.cpp @@ -6,7 +6,6 @@ namespace Components bool Loader::Postgame = false; bool Loader::ComInitialized = false; std::vector Loader::Components; - Utils::Memory::Allocator Loader::MemAllocator; bool Loader::IsPregame() { @@ -27,7 +26,7 @@ namespace Components { Loader::Pregame = true; Loader::Postgame = false; - Loader::MemAllocator.clear(); + Utils::Memory::GetAllocator()->clear(); Loader::ComInitialized = false; if (!Loader::PerformingUnitTests() && !Utils::IsWineEnvironment()) Loader::ComInitialized = (CoInitialize(nullptr) == S_OK); @@ -122,7 +121,7 @@ namespace Components } Loader::Components.clear(); - Loader::MemAllocator.clear(); + Utils::Memory::GetAllocator()->clear(); if (!Loader::PerformingUnitTests() && !Utils::IsWineEnvironment() && Loader::ComInitialized) CoUninitialize(); } @@ -202,9 +201,4 @@ namespace Components Loader::Components.push_back(component); } } - - Utils::Memory::Allocator* Loader::GetAlloctor() - { - return &Loader::MemAllocator; - } } diff --git a/src/Components/Loader.hpp b/src/Components/Loader.hpp index 5887db16..f5ddc630 100644 --- a/src/Components/Loader.hpp +++ b/src/Components/Loader.hpp @@ -53,14 +53,11 @@ namespace Components return nullptr; } - static Utils::Memory::Allocator* GetAlloctor(); - private: static bool Pregame; static bool Postgame; static bool ComInitialized; static std::vector Components; - static Utils::Memory::Allocator MemAllocator; }; } diff --git a/src/Components/Modules/CardTitles.cpp b/src/Components/Modules/CardTitles.cpp index d00bcd7a..2c019829 100644 --- a/src/Components/Modules/CardTitles.cpp +++ b/src/Components/Modules/CardTitles.cpp @@ -72,6 +72,8 @@ namespace Components std::uint8_t prefix = (request->tableRow >> (8 * 3)) & 0xFF; std::uint8_t data = (request->tableRow >> (8 * 2)) & 0xFF; + if (data >= ARRAYSIZE(CardTitles::CustomTitles)) return nullptr; + if (request->tablename == "mp/cardTitleTable.csv"s) { if (prefix != 0x00) @@ -81,10 +83,10 @@ namespace Components { if (prefix == 0xFE) { - if (!CustomTitleDvar.get().empty()) + if (!CardTitles::CustomTitleDvar.get().empty()) { // 0xFF in front of the title to skip localization. Or else it will wait for a couple of seconds for the asset of type localize - const char* title = Utils::String::VA("\x15%s", CustomTitleDvar.get()); + const char* title = Utils::String::VA("\x15%s", CardTitles::CustomTitleDvar.get()); // prepare return value operand->internals.stringVal.string = title; @@ -95,9 +97,9 @@ namespace Components } else if (prefix == 0xFF) { - if (!CustomTitles[data].empty()) + if (!CardTitles::CustomTitles[data].empty()) { - const char* title = Utils::String::VA("\x15%s", CustomTitles[data].data()); + const char* title = Utils::String::VA("\x15%s", CardTitles::CustomTitles[data].data()); // prepare return value operand->internals.stringVal.string = title; diff --git a/src/Components/Modules/Command.cpp b/src/Components/Modules/Command.cpp index 48c7719d..90806c69 100644 --- a/src/Components/Modules/Command.cpp +++ b/src/Components/Modules/Command.cpp @@ -129,7 +129,7 @@ namespace Components Game::cmd_function_t* Command::Allocate() { - return Loader::GetAlloctor()->allocate(); + return Utils::Memory::GetAllocator()->allocate(); } void Command::MainCallback() diff --git a/src/Components/Modules/Localization.cpp b/src/Components/Modules/Localization.cpp index 8141c522..cad3ac32 100644 --- a/src/Components/Modules/Localization.cpp +++ b/src/Components/Modules/Localization.cpp @@ -10,7 +10,7 @@ namespace Components void Localization::Set(std::string key, std::string value) { std::lock_guard _(Localization::LocalizeMutex); - Utils::Memory::Allocator* allocator = Loader::GetAlloctor(); + Utils::Memory::Allocator* allocator = Utils::Memory::GetAllocator(); if (Localization::LocalizeMap.find(key) != Localization::LocalizeMap.end()) { @@ -76,7 +76,7 @@ namespace Components void Localization::SetTemp(std::string key, std::string value) { std::lock_guard _(Localization::LocalizeMutex); - Utils::Memory::Allocator* allocator = Loader::GetAlloctor(); + Utils::Memory::Allocator* allocator = Utils::Memory::GetAllocator(); if (Localization::TempLocalizeMap.find(key) != Localization::TempLocalizeMap.end()) { @@ -111,7 +111,7 @@ namespace Components void Localization::ClearTemp() { std::lock_guard _(Localization::LocalizeMutex); - Utils::Memory::Allocator* allocator = Loader::GetAlloctor(); + Utils::Memory::Allocator* allocator = Utils::Memory::GetAllocator(); for (auto i = Localization::TempLocalizeMap.begin(); i != Localization::TempLocalizeMap.end(); ++i) { diff --git a/src/Components/Modules/Menus.cpp b/src/Components/Modules/Menus.cpp index 16221294..68ba3277 100644 --- a/src/Components/Modules/Menus.cpp +++ b/src/Components/Modules/Menus.cpp @@ -55,7 +55,7 @@ namespace Components int Menus::LoadMenuSource(std::string name, std::string buffer) { - Utils::Memory::Allocator* allocator = Loader::GetAlloctor(); + Utils::Memory::Allocator* allocator = Utils::Memory::GetAllocator(); int handle = Menus::ReserveSourceHandle(); if (!Menus::IsValidSourceHandle(handle)) return 0; // No free source slot! @@ -114,7 +114,7 @@ namespace Components Game::menuDef_t* Menus::ParseMenu(int handle) { - Utils::Memory::Allocator* allocator = Loader::GetAlloctor(); + Utils::Memory::Allocator* allocator = Utils::Memory::GetAllocator(); Game::menuDef_t* menu = allocator->allocate(); if (!menu) return nullptr; @@ -239,7 +239,7 @@ namespace Components Game::MenuList* Menus::LoadScriptMenu(const char* menu) { - Utils::Memory::Allocator* allocator = Loader::GetAlloctor(); + Utils::Memory::Allocator* allocator = Utils::Memory::GetAllocator(); std::vector> menus = Menus::LoadMenu(menu); if (menus.empty()) return nullptr; @@ -318,7 +318,7 @@ namespace Components Game::MenuList* Menus::LoadMenuList(Game::MenuList* menuList) { - Utils::Memory::Allocator* allocator = Loader::GetAlloctor(); + Utils::Memory::Allocator* allocator = Utils::Memory::GetAllocator(); std::vector> menus; @@ -376,7 +376,7 @@ namespace Components void Menus::FreeMenuSource(int handle) { - Utils::Memory::Allocator* allocator = Loader::GetAlloctor(); + Utils::Memory::Allocator* allocator = Utils::Memory::GetAllocator(); if (!Menus::IsValidSourceHandle(handle)) return; @@ -419,7 +419,7 @@ namespace Components void Menus::FreeMenu(Game::menuDef_t* menudef) { - Utils::Memory::Allocator* allocator = Loader::GetAlloctor(); + Utils::Memory::Allocator* allocator = Utils::Memory::GetAllocator(); // Do i need to free expressions and strings? // Or does the game take care of it? @@ -444,7 +444,7 @@ namespace Components void Menus::FreeMenuList(Game::MenuList* menuList) { if (!menuList) return; - Utils::Memory::Allocator* allocator = Loader::GetAlloctor(); + Utils::Memory::Allocator* allocator = Utils::Memory::GetAllocator(); // Keep our compiler happy Game::MenuList list = { menuList->name, menuList->menuCount, menuList->menus }; diff --git a/src/Components/Modules/StringTable.cpp b/src/Components/Modules/StringTable.cpp index a1f36451..fc84448c 100644 --- a/src/Components/Modules/StringTable.cpp +++ b/src/Components/Modules/StringTable.cpp @@ -20,7 +20,7 @@ namespace Components Game::StringTable* StringTable::LoadObject(std::string filename) { - Utils::Memory::Allocator* allocator = Loader::GetAlloctor(); + Utils::Memory::Allocator* allocator = Utils::Memory::GetAllocator(); filename = Utils::String::ToLower(filename); diff --git a/src/Components/Modules/ZoneBuilder.cpp b/src/Components/Modules/ZoneBuilder.cpp index 6edc0295..7e5e9aa7 100644 --- a/src/Components/Modules/ZoneBuilder.cpp +++ b/src/Components/Modules/ZoneBuilder.cpp @@ -921,7 +921,7 @@ namespace Components void ZoneBuilder::HandleError(int level, const char* format, ...) { - char buffer[256]; + char buffer[256] = { 0 }; va_list args; va_start(args, format); vsnprintf_s(buffer, 256, format, args); diff --git a/src/Game/Functions.cpp b/src/Game/Functions.cpp index 2e205576..b3a3efbc 100644 --- a/src/Game/Functions.cpp +++ b/src/Game/Functions.cpp @@ -388,7 +388,7 @@ namespace Game XAssetHeader ReallocateAssetPool(XAssetType type, unsigned int newSize) { int elSize = DB_GetXAssetSizeHandlers[type](); - XAssetHeader poolEntry = { Components::Loader::GetAlloctor()->allocate(newSize * elSize) }; + XAssetHeader poolEntry = { Utils::Memory::GetAllocator()->allocate(newSize * elSize) }; DB_XAssetPool[type] = poolEntry; g_poolSize[type] = newSize; return poolEntry; diff --git a/src/STDInclude.hpp b/src/STDInclude.hpp index 851182ea..370532e4 100644 --- a/src/STDInclude.hpp +++ b/src/STDInclude.hpp @@ -9,6 +9,7 @@ #define WIN32_LEAN_AND_MEAN // Requires Visual Leak Detector plugin: http://vld.codeplex.com/ +#define VLD_FORCE_ENABLE //#include #include diff --git a/src/Utils/Memory.cpp b/src/Utils/Memory.cpp index e767739d..59bb2147 100644 --- a/src/Utils/Memory.cpp +++ b/src/Utils/Memory.cpp @@ -2,11 +2,13 @@ namespace Utils { + Utils::Memory::Allocator Memory::MemAllocator; + void* Memory::AllocateAlign(size_t length, size_t alignment) { void* data = _aligned_malloc(length, alignment); assert(data != nullptr); - ZeroMemory(data, length); + if(data) ZeroMemory(data, length); return data; } @@ -95,4 +97,9 @@ namespace Utils } return true; } + + Utils::Memory::Allocator* Memory::GetAllocator() + { + return &Memory::MemAllocator; + } } diff --git a/src/Utils/Memory.hpp b/src/Utils/Memory.hpp index 7d7efe8e..2c854a64 100644 --- a/src/Utils/Memory.hpp +++ b/src/Utils/Memory.hpp @@ -154,5 +154,10 @@ namespace Utils static bool IsBadReadPtr(const void* ptr); static bool IsBadCodePtr(const void* ptr); + + static Utils::Memory::Allocator* GetAllocator(); + + private: + static Utils::Memory::Allocator MemAllocator; }; } diff --git a/src/Utils/String.cpp b/src/Utils/String.cpp index b81bb7d1..dce41d6b 100644 --- a/src/Utils/String.cpp +++ b/src/Utils/String.cpp @@ -7,41 +7,9 @@ namespace Utils { namespace String { - VAProvider::VAProvider(size_t buffers) : currentBuffer(0) - { - this->stringBuffers.resize(buffers); - } - - char* VAProvider::get(const char* format, va_list ap) - { - ++this->currentBuffer %= this->stringBuffers.size(); - auto& buffer = this->stringBuffers[this->currentBuffer]; - - if (!buffer.first || !buffer.second) - { - buffer.first = 256; - if (buffer.second) this->allocator.free(buffer.second); - buffer.second = this->allocator.allocateArray(buffer.first + 1); - } - - while(true) - { - int res = vsnprintf_s(buffer.second, buffer.first, _TRUNCATE, format, ap); - if (res > 0) break; // Success - if (res == 0) return ""; // Error - - buffer.first *= 2; - - this->allocator.free(buffer.second); - buffer.second = this->allocator.allocateArray(buffer.first + 1); - } - - return buffer.second; - } - const char *VA(const char *fmt, ...) { - static thread_local VAProvider provider; + static VAProvider<8, 256> provider; va_list ap; va_start(ap, fmt); diff --git a/src/Utils/String.hpp b/src/Utils/String.hpp index 00b4a07e..d70b5a83 100644 --- a/src/Utils/String.hpp +++ b/src/Utils/String.hpp @@ -4,18 +4,89 @@ namespace Utils { namespace String { + template class VAProvider { public: - VAProvider(size_t buffers = 8); + typename std::enable_if<(Buffers != 0 && MinBufferSize != 0), char*>::type + get(const char* format, va_list ap) + { + std::lock_guard _(this->accessMutex); - char* get(const char* format, va_list ap); + auto threadBuffers = this->stringBuffers.find(std::this_thread::get_id()); + if (threadBuffers == this->stringBuffers.end()) + { + this->stringBuffers[std::this_thread::get_id()] = Pool(); + threadBuffers = this->stringBuffers.find(std::this_thread::get_id()); + } + + if (!threadBuffers->second.stringPool.size()) threadBuffers->second.stringPool.resize(Buffers); + + ++threadBuffers->second.currentBuffer %= threadBuffers->second.stringPool.size(); + auto& entry = threadBuffers->second.stringPool[threadBuffers->second.currentBuffer]; + + if (!entry.size || !entry.buffer) + { + entry = Entry(MinBufferSize); + } + + while (true) + { + int res = vsnprintf_s(entry.buffer, entry.size, _TRUNCATE, format, ap); + if (res > 0) break; // Success + if (res == 0) return ""; // Error + + entry.doubleSize(); + } + + return entry.buffer; + } private: - size_t currentBuffer; - std::vector> stringBuffers; + class Entry + { + public: + Entry(size_t _size = MinBufferSize) : size(_size), buffer(nullptr) + { + if (this->size < MinBufferSize) this->size = MinBufferSize; + this->allocate(); + } - Utils::Memory::Allocator allocator; + ~Entry() + { + if(this->buffer) Utils::Memory::GetAllocator()->free(this->buffer); + } + + void allocate() + { + if (this->buffer) Utils::Memory::GetAllocator()->free(this->buffer); + this->buffer = Utils::Memory::GetAllocator()->allocateArray(this->size + 1); + } + + void doubleSize() + { + this->size *= 2; + this->allocate(); + } + + size_t size; + char* buffer; + }; + + class Pool + { + public: + Pool() : currentBuffer(0) + { + this->stringPool.resize(Buffers); + } + + size_t currentBuffer; + std::vector stringPool; + }; + + std::mutex accessMutex; + std::unordered_map stringBuffers; }; const char *VA(const char *fmt, ...);