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

@@ -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)
 ------------------------------------------------------------------------

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);

src/openrct2/core/MemoryStream.cpp

@@ -11,6 +11,7 @@
 
 #include "Memory.hpp"
 
+#include <bit>
 #include <cstring>
 
 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<void>(_dataCapacity);
+            // Create a copy when the memory stream owns the memory.
+            _data = Memory::Allocate<std::byte>(_dataCapacity);
             std::memcpy(_data, copy._data, _dataCapacity);
-            _position = reinterpret_cast<void*>(reinterpret_cast<uintptr_t>(_data) + copy.GetPosition());
+        }
+        else
+        {
+            // External pointer.
+            _data = copy._data;
         }
     }
 
     MemoryStream::MemoryStream(size_t capacity)
     {
         _dataCapacity = capacity;
-        _data = Memory::Allocate<void>(capacity);
-        _position = _data;
+        _data = Memory::Allocate<std::byte>(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<std::byte*>(data) }
+        , _position{ 0 }
     {
-        _access = access;
-        _dataCapacity = dataSize;
-        _dataSize = dataSize;
-        _data = data;
-        _position = _data;
     }
 
     MemoryStream::MemoryStream(const void* data, size_t dataSize)
-        : MemoryStream(const_cast<void*>(data), dataSize, MEMORY_ACCESS::READ)
+        : _access{ MEMORY_ACCESS::READ }
+        , _dataCapacity{ dataSize }
+        , _dataSize{ dataSize }
+        , _data{ static_cast<std::byte*>(const_cast<void*>(data)) }
+        , _position{ 0 }
     {
     }
 
-    MemoryStream::MemoryStream(std::vector<uint8_t>&& v)
+    MemoryStream::MemoryStream(const void* data, size_t dataSize, bool)
+        : _access{ MEMORY_ACCESS::READ | MEMORY_ACCESS::OWNER }
+        , _dataCapacity{ dataSize }
+        , _dataSize{ dataSize }
+        , _data{ static_cast<std::byte*>(const_cast<void*>(data)) }
+        , _position{ 0 }
     {
-        _access = MEMORY_ACCESS::OWNER;
-        _dataCapacity = v.size();
-        _dataSize = v.size();
-        _data = Memory::Allocate<void>(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<void>(_dataSize);
-        std::memcpy(result, _data, _dataSize);
-        return result;
-    }
-
-    void* MemoryStream::TakeData()
-    {
-        _access &= ~MEMORY_ACCESS::OWNER;
-        return _data;
+        return static_cast<const void*>(_data);
     }
 
     bool MemoryStream::CanRead() const
@@ -138,7 +140,7 @@ namespace OpenRCT2
 
     uint64_t MemoryStream::GetPosition() const
     {
-        return static_cast<uint64_t>(reinterpret_cast<uintptr_t>(_position) - reinterpret_cast<uintptr_t>(_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<void*>(reinterpret_cast<uintptr_t>(_data) + static_cast<uintptr_t>(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<void*>(reinterpret_cast<uintptr_t>(_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<size_t>(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<void*>(reinterpret_cast<uintptr_t>(_position) + length);
-        _dataSize = std::max<size_t>(_dataSize, static_cast<size_t>(nextPosition));
+        std::memcpy(_data + _position, buffer, length);
+        _position += length;
+        _dataSize = std::max<size_t>(_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<size_t>(8, _dataCapacity);
-            while (newCapacity < capacity)
-            {
-                newCapacity *= 2;
-            }
-
-            uint64_t position = GetPosition();
-            _dataCapacity = newCapacity;
-            _data = Memory::Reallocate(_data, _dataCapacity);
-            _position = reinterpret_cast<void*>(reinterpret_cast<uintptr_t>(_data) + static_cast<uintptr_t>(position));
+            return;
         }
+
+        if ((_access & MEMORY_ACCESS::OWNER) == 0)
+        {
+            throw IOException("Can not grow external memory");
+        }
+
+        size_t newCapacity = std::max<size_t>(32, std::bit_ceil(capacity));
+
+        _dataCapacity = newCapacity;
+        _data = Memory::Reallocate(_data, _dataCapacity);
     }
 
 } // namespace OpenRCT2

src/openrct2/core/MemoryStream.h

@@ -11,8 +11,6 @@
 
 #include "IStream.hpp"
 
-#include <vector>
-
 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<uint8_t>&& 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<size_t N> 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<void*>(reinterpret_cast<uintptr_t>(_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<size_t N> 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<size_t>(nextPosition));
-                }
-                else
-                {
-                    throw IOException("Attempted to write past end of stream.");
-                }
-            }
-
-            std::memcpy(_position, buffer, N);
-            _position = reinterpret_cast<void*>(reinterpret_cast<uintptr_t>(_position) + N);
-            _dataSize = std::max<size_t>(_dataSize, static_cast<size_t>(nextPosition));
-        }
-
         uint64_t TryRead(void* buffer, uint64_t length) override;
 
         void Clear();
 
     private:
         void EnsureCapacity(size_t capacity);
+
+        template<size_t N> 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<size_t N> 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

src/openrct2/core/ZipAndroid.cpp

@@ -111,13 +111,31 @@ public:
         auto dataPtr = reinterpret_cast<uint8_t*>(ptr);
         auto dataSize = this->GetFileSize(index);
 
-        return std::vector<uint8_t>(dataPtr, dataPtr + dataSize);
+        auto res = std::vector<uint8_t>(dataPtr, dataPtr + dataSize);
+
+        Memory::Free(dataPtr);
+        return res;
     }
 
     std::unique_ptr<IStream> GetFileStream(std::string_view path) const override
     {
-        auto data = GetFileData(path);
-        return std::make_unique<MemoryStream>(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<uint8_t*>(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<MemoryStream>(dataPtr, dataSize, true);
     }
 
     void SetFileData(std::string_view path, std::vector<uint8_t>&& data) override