From 6ea091841f87df9be34a7fbce31f0a62781861ee Mon Sep 17 00:00:00 2001 From: John Kastner Date: Mon, 1 Apr 2024 18:40:14 -0400 Subject: [PATCH] Fix memory leak loading malformed `SawyerChunk` (#21508) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix memory leak loading malformed `SawyerChunk` A temporary buffer was not free'd after failing to parse in `SawyerChunkReader::ReadChunkTrack`. Fix this following the pattern used in `SawyerChunkReader::ReadChunk` by wrapping the relevant code in a `try` block with `FreeLargeTempBuffer` called when an exception is caught. * Use unique_ptr * Remove `AllocateLargeTempBuffer` --------- Co-authored-by: Michał Janiszewski --- src/openrct2/rct12/SawyerChunk.cpp | 9 +--- src/openrct2/rct12/SawyerChunk.h | 9 ++-- src/openrct2/rct12/SawyerChunkReader.cpp | 58 +++++------------------- src/openrct2/rct12/SawyerChunkReader.h | 8 ---- 4 files changed, 18 insertions(+), 66 deletions(-) diff --git a/src/openrct2/rct12/SawyerChunk.cpp b/src/openrct2/rct12/SawyerChunk.cpp index 65779a1bb3..1f77f6b551 100644 --- a/src/openrct2/rct12/SawyerChunk.cpp +++ b/src/openrct2/rct12/SawyerChunk.cpp @@ -12,14 +12,9 @@ #include "../core/Memory.hpp" #include "SawyerChunkReader.h" -SawyerChunk::SawyerChunk(SAWYER_ENCODING encoding, void* data, size_t length) +SawyerChunk::SawyerChunk(SAWYER_ENCODING encoding, std::unique_ptr data, size_t length) { _encoding = encoding; - _data = data; + _data = std::move(data); _length = length; } - -SawyerChunk::~SawyerChunk() -{ - SawyerChunkReader::FreeChunk(_data); -} diff --git a/src/openrct2/rct12/SawyerChunk.h b/src/openrct2/rct12/SawyerChunk.h index 1a6a4ca5af..05e8bc2d7a 100644 --- a/src/openrct2/rct12/SawyerChunk.h +++ b/src/openrct2/rct12/SawyerChunk.h @@ -11,6 +11,8 @@ #include "../common.h" +#include + /** * The type of encoding / compression for a sawyer encoded chunk. */ @@ -28,14 +30,14 @@ enum class SAWYER_ENCODING : uint8_t class SawyerChunk final { private: - void* _data = nullptr; + std::unique_ptr _data; size_t _length = 0; SAWYER_ENCODING _encoding = SAWYER_ENCODING::NONE; public: const void* GetData() const { - return _data; + return _data.get(); } size_t GetLength() const { @@ -46,6 +48,5 @@ public: return _encoding; } - SawyerChunk(SAWYER_ENCODING encoding, void* data, size_t length); - ~SawyerChunk(); + SawyerChunk(SAWYER_ENCODING encoding, std::unique_ptr data, size_t length); }; diff --git a/src/openrct2/rct12/SawyerChunkReader.cpp b/src/openrct2/rct12/SawyerChunkReader.cpp index eaf4220583..df1e38b95f 100644 --- a/src/openrct2/rct12/SawyerChunkReader.cpp +++ b/src/openrct2/rct12/SawyerChunkReader.cpp @@ -71,22 +71,15 @@ std::shared_ptr SawyerChunkReader::ReadChunk() throw SawyerChunkException(EXCEPTION_MSG_CORRUPT_CHUNK_SIZE); } - auto buffer = static_cast(AllocateLargeTempBuffer()); - try + auto buffer = std::make_unique(MAX_UNCOMPRESSED_CHUNK_SIZE); + size_t uncompressedLength = DecodeChunk( + buffer.get(), MAX_UNCOMPRESSED_CHUNK_SIZE, compressedData.get(), header); + if (uncompressedLength == 0) { - size_t uncompressedLength = DecodeChunk(buffer, MAX_UNCOMPRESSED_CHUNK_SIZE, compressedData.get(), header); - if (uncompressedLength == 0) - { - throw SawyerChunkException(EXCEPTION_MSG_ZERO_SIZED_CHUNK); - } - return std::make_shared( - static_cast(header.encoding), buffer, uncompressedLength); - } - catch (const std::exception&) - { - FreeLargeTempBuffer(buffer); - throw; + throw SawyerChunkException(EXCEPTION_MSG_ZERO_SIZED_CHUNK); } + return std::make_shared( + static_cast(header.encoding), std::move(buffer), uncompressedLength); } default: throw SawyerChunkException(EXCEPTION_MSG_INVALID_CHUNK_ENCODING); @@ -119,14 +112,14 @@ std::shared_ptr SawyerChunkReader::ReadChunkTrack() throw SawyerChunkException(EXCEPTION_MSG_CORRUPT_CHUNK_SIZE); } - auto buffer = static_cast(AllocateLargeTempBuffer()); + auto buffer = std::make_unique(MAX_UNCOMPRESSED_CHUNK_SIZE); SawyerCodingChunkHeader header{ CHUNK_ENCODING_RLE, compressedDataLength }; - size_t uncompressedLength = DecodeChunk(buffer, MAX_UNCOMPRESSED_CHUNK_SIZE, compressedData.get(), header); + size_t uncompressedLength = DecodeChunk(buffer.get(), MAX_UNCOMPRESSED_CHUNK_SIZE, compressedData.get(), header); if (uncompressedLength == 0) { throw SawyerChunkException(EXCEPTION_MSG_ZERO_SIZED_CHUNK); } - return std::make_shared(SAWYER_ENCODING::RLE, buffer, uncompressedLength); + return std::make_shared(SAWYER_ENCODING::RLE, std::move(buffer), uncompressedLength); } catch (const std::exception&) { @@ -157,11 +150,6 @@ void SawyerChunkReader::ReadChunk(void* dst, size_t length) } } -void SawyerChunkReader::FreeChunk(void* data) -{ - FreeLargeTempBuffer(data); -} - size_t SawyerChunkReader::DecodeChunk(void* dst, size_t dstCapacity, const void* src, const SawyerCodingChunkHeader& header) { size_t resultLength; @@ -192,8 +180,7 @@ size_t SawyerChunkReader::DecodeChunk(void* dst, size_t dstCapacity, const void* size_t SawyerChunkReader::DecodeChunkRLERepeat(void* dst, size_t dstCapacity, const void* src, size_t srcLength) { - auto immBuffer = std::unique_ptr( - static_cast(AllocateLargeTempBuffer()), &FreeLargeTempBuffer); + auto immBuffer = std::make_unique(MAX_UNCOMPRESSED_CHUNK_SIZE); auto immLength = DecodeChunkRLE(immBuffer.get(), MAX_UNCOMPRESSED_CHUNK_SIZE, src, srcLength); auto size = DecodeChunkRepeat(dst, dstCapacity, immBuffer.get(), immLength); return size; @@ -301,26 +288,3 @@ size_t SawyerChunkReader::DecodeChunkRotate(void* dst, size_t dstCapacity, const } return srcLength; } - -void* SawyerChunkReader::AllocateLargeTempBuffer() -{ -#ifdef __USE_HEAP_ALLOC__ - auto buffer = HeapAlloc(GetProcessHeap(), 0, MAX_UNCOMPRESSED_CHUNK_SIZE); -#else - auto buffer = std::malloc(MAX_UNCOMPRESSED_CHUNK_SIZE); -#endif - if (buffer == nullptr) - { - throw std::runtime_error("Unable to allocate large temporary buffer."); - } - return buffer; -} - -void SawyerChunkReader::FreeLargeTempBuffer(void* buffer) -{ -#ifdef __USE_HEAP_ALLOC__ - HeapFree(GetProcessHeap(), 0, buffer); -#else - std::free(buffer); -#endif -} diff --git a/src/openrct2/rct12/SawyerChunkReader.h b/src/openrct2/rct12/SawyerChunkReader.h index 60c1e5c572..5bbdc3a79b 100644 --- a/src/openrct2/rct12/SawyerChunkReader.h +++ b/src/openrct2/rct12/SawyerChunkReader.h @@ -85,18 +85,10 @@ public: return result; } - /** - * Frees the chunk data, to be used when destructing SawyerChunks - */ - static void FreeChunk(void* data); - private: static size_t DecodeChunk(void* dst, size_t dstCapacity, const void* src, const SawyerCodingChunkHeader& header); static size_t DecodeChunkRLERepeat(void* dst, size_t dstCapacity, const void* src, size_t srcLength); static size_t DecodeChunkRLE(void* dst, size_t dstCapacity, const void* src, size_t srcLength); static size_t DecodeChunkRepeat(void* dst, size_t dstCapacity, const void* src, size_t srcLength); static size_t DecodeChunkRotate(void* dst, size_t dstCapacity, const void* src, size_t srcLength); - - static void* AllocateLargeTempBuffer(); - static void FreeLargeTempBuffer(void* buffer); };