From 590ab65b2a840a0aa4620e1f773fd883547c7e93 Mon Sep 17 00:00:00 2001 From: Matt <5415177+ZehMatt@users.noreply.github.com> Date: Thu, 22 Aug 2024 20:24:00 +0300 Subject: [PATCH] Cleanup MemoryStream code (#22593) * Deduplicate code, use std::bit_ceil to compute new capacity * Remove unused constructor overload * Remove more unused functions * Fix memory leak using assignment operator with move * Make access explicit via constness, do not allow arbitrary access * Move the template specialized Write/Read to private section * Simplify a lot of code by using the right types * Fix copy constructor * Directly copy the member in copy constructor * Fix little mistake * Pluck a memory leak on Android, fix the build * Update changelog.txt --- distribution/changelog.txt | 1 + src/openrct2/Context.cpp | 8 +- src/openrct2/core/MemoryStream.cpp | 127 ++++++++++++++--------------- src/openrct2/core/MemoryStream.h | 68 ++++++--------- src/openrct2/core/ZipAndroid.cpp | 24 +++++- 5 files changed, 112 insertions(+), 116 deletions(-) diff --git a/distribution/changelog.txt b/distribution/changelog.txt index 8f8fdc84e5..eb891f9458 100644 --- a/distribution/changelog.txt +++ b/distribution/changelog.txt @@ -21,6 +21,7 @@ - Fix: [#22457] Potential crash opening the scenario select window. - Fix: [#22520] Virtual floor no longer appears when holding modifier keys during track construction. - Fix: [#22582] Lighting effects are not enabled/disabled correctly, making the game appear frozen. +- Fix: [#22593] Memory leak on Android builds when reading zip files. 0.4.13 (2024-08-04) ------------------------------------------------------------------------ diff --git a/src/openrct2/Context.cpp b/src/openrct2/Context.cpp index 6078d210fd..a3851c8bd4 100644 --- a/src/openrct2/Context.cpp +++ b/src/openrct2/Context.cpp @@ -719,8 +719,8 @@ namespace OpenRCT2 { if (String::IEquals(Path::GetExtension(path), ".sea")) { - auto data = DecryptSea(fs::u8path(path)); - auto ms = MemoryStream(data.data(), data.size(), MEMORY_ACCESS::READ); + const auto data = DecryptSea(fs::u8path(path)); + auto ms = MemoryStream(data.data(), data.size()); if (!LoadParkFromStream(&ms, path, loadTitleScreenOnFail, asScenario)) { throw std::runtime_error(".sea file may have been renamed."); @@ -1060,14 +1060,14 @@ namespace OpenRCT2 { #ifndef DISABLE_HTTP // Download park and open it using its temporary filename - auto data = DownloadPark(gOpenRCT2StartupActionPath); + const auto data = DownloadPark(gOpenRCT2StartupActionPath); if (data.empty()) { nextScene = GetTitleScene(); break; } - auto ms = MemoryStream(data.data(), data.size(), MEMORY_ACCESS::READ); + auto ms = MemoryStream(data.data(), data.size()); if (!LoadParkFromStream(&ms, gOpenRCT2StartupActionPath, true)) { Console::Error::WriteLine("Failed to load '%s'", gOpenRCT2StartupActionPath); diff --git a/src/openrct2/core/MemoryStream.cpp b/src/openrct2/core/MemoryStream.cpp index a0da18d123..058c1ea9e9 100644 --- a/src/openrct2/core/MemoryStream.cpp +++ b/src/openrct2/core/MemoryStream.cpp @@ -11,6 +11,7 @@ #include "Memory.hpp" +#include #include namespace OpenRCT2 @@ -20,44 +21,53 @@ namespace OpenRCT2 _access = copy._access; _dataCapacity = copy._dataCapacity; _dataSize = copy._dataSize; + _position = copy._position; if (_access & MEMORY_ACCESS::OWNER) { - _data = Memory::Allocate(_dataCapacity); + // Create a copy when the memory stream owns the memory. + _data = Memory::Allocate(_dataCapacity); std::memcpy(_data, copy._data, _dataCapacity); - _position = reinterpret_cast(reinterpret_cast(_data) + copy.GetPosition()); + } + else + { + // External pointer. + _data = copy._data; } } MemoryStream::MemoryStream(size_t capacity) { _dataCapacity = capacity; - _data = Memory::Allocate(capacity); - _position = _data; + _data = Memory::Allocate(capacity); + _position = 0; } - MemoryStream::MemoryStream(void* data, size_t dataSize, uint8_t access) + MemoryStream::MemoryStream(void* data, size_t dataSize) + : _access{ MEMORY_ACCESS::READ | MEMORY_ACCESS::WRITE } + , _dataCapacity{ dataSize } + , _dataSize{ dataSize } + , _data{ static_cast(data) } + , _position{ 0 } { - _access = access; - _dataCapacity = dataSize; - _dataSize = dataSize; - _data = data; - _position = _data; } MemoryStream::MemoryStream(const void* data, size_t dataSize) - : MemoryStream(const_cast(data), dataSize, MEMORY_ACCESS::READ) + : _access{ MEMORY_ACCESS::READ } + , _dataCapacity{ dataSize } + , _dataSize{ dataSize } + , _data{ static_cast(const_cast(data)) } + , _position{ 0 } { } - MemoryStream::MemoryStream(std::vector&& v) + MemoryStream::MemoryStream(const void* data, size_t dataSize, bool) + : _access{ MEMORY_ACCESS::READ | MEMORY_ACCESS::OWNER } + , _dataCapacity{ dataSize } + , _dataSize{ dataSize } + , _data{ static_cast(const_cast(data)) } + , _position{ 0 } { - _access = MEMORY_ACCESS::OWNER; - _dataCapacity = v.size(); - _dataSize = v.size(); - _data = Memory::Allocate(v.size()); - _position = _data; - std::memcpy(_data, v.data(), v.size()); } MemoryStream::MemoryStream(MemoryStream&& mv) noexcept @@ -68,7 +78,7 @@ namespace OpenRCT2 , _position(mv._position) { mv._data = nullptr; - mv._position = nullptr; + mv._position = 0; mv._dataCapacity = 0; mv._dataSize = 0; } @@ -88,6 +98,11 @@ namespace OpenRCT2 { if (this != &mv) { + if (_access & MEMORY_ACCESS::OWNER) + { + Memory::Free(_data); + } + _access = mv._access; _dataCapacity = mv._dataCapacity; _data = mv._data; @@ -95,7 +110,7 @@ namespace OpenRCT2 _position = mv._position; mv._data = nullptr; - mv._position = nullptr; + mv._position = 0; mv._dataCapacity = 0; mv._dataSize = 0; } @@ -105,20 +120,7 @@ namespace OpenRCT2 const void* MemoryStream::GetData() const { - return _data; - } - - void* MemoryStream::GetDataCopy() const - { - auto result = Memory::Allocate(_dataSize); - std::memcpy(result, _data, _dataSize); - return result; - } - - void* MemoryStream::TakeData() - { - _access &= ~MEMORY_ACCESS::OWNER; - return _data; + return static_cast(_data); } bool MemoryStream::CanRead() const @@ -138,7 +140,7 @@ namespace OpenRCT2 uint64_t MemoryStream::GetPosition() const { - return static_cast(reinterpret_cast(_position) - reinterpret_cast(_data)); + return _position; } void MemoryStream::SetPosition(uint64_t position) @@ -167,19 +169,18 @@ namespace OpenRCT2 { throw IOException("New position out of bounds."); } - _position = reinterpret_cast(reinterpret_cast(_data) + static_cast(newPosition)); + _position = newPosition; } void MemoryStream::Read(void* buffer, uint64_t length) { - uint64_t position = GetPosition(); - if (position + length > _dataSize) + if (_position + length > _dataSize) { throw IOException("Attempted to read past end of stream."); } - std::memcpy(buffer, _position, length); - _position = reinterpret_cast(reinterpret_cast(_position) + length); + std::memcpy(buffer, _data + _position, length); + _position += length; } void MemoryStream::Read1(void* buffer) @@ -217,23 +218,12 @@ namespace OpenRCT2 void MemoryStream::Write(const void* buffer, uint64_t length) { - uint64_t position = GetPosition(); - uint64_t nextPosition = position + length; - if (nextPosition > _dataCapacity) - { - if (_access & MEMORY_ACCESS::OWNER) - { - EnsureCapacity(static_cast(nextPosition)); - } - else - { - throw IOException("Attempted to write past end of stream."); - } - } + const auto nextPosition = _position + length; + EnsureCapacity(nextPosition); - std::memcpy(_position, buffer, length); - _position = reinterpret_cast(reinterpret_cast(_position) + length); - _dataSize = std::max(_dataSize, static_cast(nextPosition)); + std::memcpy(_data + _position, buffer, length); + _position += length; + _dataSize = std::max(_dataSize, nextPosition); } void MemoryStream::Write1(const void* buffer) @@ -264,24 +254,25 @@ namespace OpenRCT2 void MemoryStream::Clear() { _dataSize = 0; - SetPosition(0); + _position = 0; } void MemoryStream::EnsureCapacity(size_t capacity) { - if (_dataCapacity < capacity) + if (capacity <= _dataCapacity) { - size_t newCapacity = std::max(8, _dataCapacity); - while (newCapacity < capacity) - { - newCapacity *= 2; - } - - uint64_t position = GetPosition(); - _dataCapacity = newCapacity; - _data = Memory::Reallocate(_data, _dataCapacity); - _position = reinterpret_cast(reinterpret_cast(_data) + static_cast(position)); + return; } + + if ((_access & MEMORY_ACCESS::OWNER) == 0) + { + throw IOException("Can not grow external memory"); + } + + size_t newCapacity = std::max(32, std::bit_ceil(capacity)); + + _dataCapacity = newCapacity; + _data = Memory::Reallocate(_data, _dataCapacity); } } // namespace OpenRCT2 diff --git a/src/openrct2/core/MemoryStream.h b/src/openrct2/core/MemoryStream.h index 33363a52fe..e67233932a 100644 --- a/src/openrct2/core/MemoryStream.h +++ b/src/openrct2/core/MemoryStream.h @@ -11,8 +11,6 @@ #include "IStream.hpp" -#include - namespace OpenRCT2 { namespace MEMORY_ACCESS @@ -31,24 +29,24 @@ namespace OpenRCT2 uint8_t _access = MEMORY_ACCESS::READ | MEMORY_ACCESS::WRITE | MEMORY_ACCESS::OWNER; size_t _dataCapacity = 0; size_t _dataSize = 0; - void* _data = nullptr; - void* _position = nullptr; + std::byte* _data = nullptr; + size_t _position = 0; public: MemoryStream() = default; MemoryStream(const MemoryStream& copy); MemoryStream(MemoryStream&& mv) noexcept; explicit MemoryStream(size_t capacity); - MemoryStream(void* data, size_t dataSize, uint8_t access = MEMORY_ACCESS::READ); + MemoryStream(void* data, size_t dataSize); MemoryStream(const void* data, size_t dataSize); - MemoryStream(std::vector&& v); + // Constructor that takes ownership, the memory has to be allocated by Memory::Allocate, this function only exists for + // Android interop reasons. + MemoryStream(const void* data, size_t dataSize, bool); virtual ~MemoryStream(); MemoryStream& operator=(MemoryStream&& mv) noexcept; const void* GetData() const override; - void* GetDataCopy() const; - void* TakeData(); /////////////////////////////////////////////////////////////////////////// // ISteam methods @@ -68,18 +66,6 @@ namespace OpenRCT2 void Read8(void* buffer) override; void Read16(void* buffer) override; - template void Read(void* buffer) - { - uint64_t position = GetPosition(); - if (position + N > _dataSize) - { - throw IOException("Attempted to read past end of stream."); - } - - std::memcpy(buffer, _position, N); - _position = reinterpret_cast(reinterpret_cast(_position) + N); - } - void Write(const void* buffer, uint64_t length) override; void Write1(const void* buffer) override; void Write2(const void* buffer) override; @@ -87,33 +73,33 @@ namespace OpenRCT2 void Write8(const void* buffer) override; void Write16(const void* buffer) override; - template void Write(const void* buffer) - { - uint64_t position = GetPosition(); - uint64_t nextPosition = position + N; - if (nextPosition > _dataCapacity) - { - if (_access & MEMORY_ACCESS::OWNER) - { - EnsureCapacity(static_cast(nextPosition)); - } - else - { - throw IOException("Attempted to write past end of stream."); - } - } - - std::memcpy(_position, buffer, N); - _position = reinterpret_cast(reinterpret_cast(_position) + N); - _dataSize = std::max(_dataSize, static_cast(nextPosition)); - } - uint64_t TryRead(void* buffer, uint64_t length) override; void Clear(); private: void EnsureCapacity(size_t capacity); + + template void Read(void* buffer) + { + if (_position + N > _dataSize) + { + throw IOException("Attempted to read past end of stream."); + } + + std::memcpy(buffer, _data + _position, N); + _position += N; + } + + template void Write(const void* buffer) + { + const auto nextPosition = _position + N; + EnsureCapacity(nextPosition); + + std::memcpy(_data + _position, buffer, N); + _position += N; + _dataSize = std::max(_dataSize, nextPosition); + } }; } // namespace OpenRCT2 diff --git a/src/openrct2/core/ZipAndroid.cpp b/src/openrct2/core/ZipAndroid.cpp index cfd17ca736..2fb20a1818 100644 --- a/src/openrct2/core/ZipAndroid.cpp +++ b/src/openrct2/core/ZipAndroid.cpp @@ -111,13 +111,31 @@ public: auto dataPtr = reinterpret_cast(ptr); auto dataSize = this->GetFileSize(index); - return std::vector(dataPtr, dataPtr + dataSize); + auto res = std::vector(dataPtr, dataPtr + dataSize); + + Memory::Free(dataPtr); + return res; } std::unique_ptr GetFileStream(std::string_view path) const override { - auto data = GetFileData(path); - return std::make_unique(std::move(data)); + // retrieve the JNI environment. + JNIEnv* env = (JNIEnv*)SDL_AndroidGetJNIEnv(); + + jclass zipClass = env->GetObjectClass(_zip); + jstring javaPath = env->NewStringUTF(path.data()); + jmethodID indexMethod = env->GetMethodID(zipClass, "getFileIndex", "(Ljava/lang/String;)I"); + jint index = env->CallIntMethod(_zip, indexMethod, javaPath); + + jmethodID fileMethod = env->GetMethodID(zipClass, "getFile", "(I)J"); + jlong ptr = env->CallLongMethod(_zip, fileMethod, index); + + auto dataPtr = reinterpret_cast(ptr); + auto dataSize = this->GetFileSize(index); + + // The Java code for getFile uses Java_io_openrct2_ZipArchive_allocBytes which + // allocates memory using Memory::Allocate so its safe for MemoryStream to take full ownershp. + return std::make_unique(dataPtr, dataSize, true); } void SetFileData(std::string_view path, std::vector&& data) override