From 45aad7ce77268e1e93f91311e2e9130b6bfedfb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=98ystein=20Dale?= Date: Wed, 18 Jul 2018 00:07:33 +0200 Subject: [PATCH 1/5] Add self to contributors.md --- contributors.md | 1 + 1 file changed, 1 insertion(+) diff --git a/contributors.md b/contributors.md index be04a6a6ba..21a6293c65 100644 --- a/contributors.md +++ b/contributors.md @@ -112,6 +112,7 @@ The following people are not part of the development team, but have been contrib * Patrick Martinez (martip23) * Andy Ford (AndyTWF) * Matthew Beaudin (mattbeaudin) +* Øystein Dale (oystedal) ## Toolchain * (Balletie) - macOS From 34cf068650364330a5cfa4703ade445be64b5656 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=98ystein=20Dale?= Date: Tue, 17 Jul 2018 23:27:16 +0200 Subject: [PATCH 2/5] Fix memory leak when deserialising objects Assigning a pointer to std::string appears to only perform a copy and does not transfer ownership of the pointer, thus the allocated memory is will never be freed. Use IStream::ReadStdString instead to return a std::string directly, thus transferring ownership correctly. --- src/openrct2/object/ObjectRepository.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/openrct2/object/ObjectRepository.cpp b/src/openrct2/object/ObjectRepository.cpp index 0412128b96..66e032c095 100644 --- a/src/openrct2/object/ObjectRepository.cpp +++ b/src/openrct2/object/ObjectRepository.cpp @@ -156,8 +156,8 @@ protected: ObjectRepositoryItem item; item.ObjectEntry = stream->ReadValue(); - item.Path = stream->ReadString(); - item.Name = stream->ReadString(); + item.Path = stream->ReadStdString(); + item.Name = stream->ReadStdString(); switch (object_entry_get_type(&item.ObjectEntry)) { case OBJECT_TYPE_RIDE: From 934e53869c989b4214bdb3051ef1ca9ec1b0a88a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=98ystein=20Dale?= Date: Tue, 17 Jul 2018 23:37:56 +0200 Subject: [PATCH 3/5] Fix memory leaks in localisation Assigning a pointer to std::string appears to only perform a copy and does not transfer ownership of the pointer, thus the allocated memory is will never be freed. Implement a method to construct an std::string from a StringBuilder to avoid memory leaks when retreiving the contents of a StringBuilder and storing it in a std::string. --- src/openrct2/core/StringBuilder.hpp | 15 +++++++++++++++ src/openrct2/localisation/LanguagePack.cpp | 4 ++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/openrct2/core/StringBuilder.hpp b/src/openrct2/core/StringBuilder.hpp index 4d072fb424..e54d851123 100644 --- a/src/openrct2/core/StringBuilder.hpp +++ b/src/openrct2/core/StringBuilder.hpp @@ -10,6 +10,7 @@ #pragma once #include +#include #include "../common.h" #include "Math.hpp" @@ -132,6 +133,20 @@ public: return result; } + /** + * Returns the current string buffer as a standard string. + */ + std::string GetStdString() const + { + std::string result; + result.reserve(_length); + for (std::size_t i = 0U; i < _length; i++) + { + result.push_back(_buffer[i]); + } + return result; + } + /** * Gets the current state of the StringBuilder. Warning: this represents the StringBuilder's current working buffer and will * be deallocated when the StringBuilder is destructed. diff --git a/src/openrct2/localisation/LanguagePack.cpp b/src/openrct2/localisation/LanguagePack.cpp index 49183186a6..a8e079277a 100644 --- a/src/openrct2/localisation/LanguagePack.cpp +++ b/src/openrct2/localisation/LanguagePack.cpp @@ -389,7 +389,7 @@ private: } if (sb.GetLength() == 8) { - _currentGroup = sb.GetString(); + _currentGroup = sb.GetStdString(); _currentObjectOverride = GetObjectOverride(_currentGroup); _currentScenarioOverride = nullptr; if (_currentObjectOverride == nullptr) @@ -432,7 +432,7 @@ private: if (closedCorrectly) { - _currentGroup = sb.GetString(); + _currentGroup = sb.GetStdString(); _currentObjectOverride = nullptr; _currentScenarioOverride = GetScenarioOverride(_currentGroup); if (_currentScenarioOverride == nullptr) From 5744509b8780ebb2ac1390dc045b45a492512268 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=98ystein=20Dale?= Date: Wed, 18 Jul 2018 00:57:37 +0200 Subject: [PATCH 4/5] Fix memory leak in config Assigning a pointer to std::string appears to only perform a copy and does not transfer ownership of the pointer, thus the allocated memory is will never be freed. Use StringBuilder::GetStdString() to avoid the memory leak. --- src/openrct2/config/IniReader.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openrct2/config/IniReader.cpp b/src/openrct2/config/IniReader.cpp index 112f37a595..5dc2233c47 100644 --- a/src/openrct2/config/IniReader.cpp +++ b/src/openrct2/config/IniReader.cpp @@ -354,7 +354,7 @@ private: sb.Append(&c, 1); } } - return std::string(sb.GetString()); + return sb.GetStdString(); } std::string GetLine(size_t index) From de464e27836317fbf53d861106f43c6db643567f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=98ystein=20Dale?= Date: Wed, 18 Jul 2018 23:56:29 +0200 Subject: [PATCH 5/5] Use a better constructor in StringBuilder::GetStdString --- src/openrct2/core/StringBuilder.hpp | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/openrct2/core/StringBuilder.hpp b/src/openrct2/core/StringBuilder.hpp index e54d851123..b993f21820 100644 --- a/src/openrct2/core/StringBuilder.hpp +++ b/src/openrct2/core/StringBuilder.hpp @@ -138,13 +138,7 @@ public: */ std::string GetStdString() const { - std::string result; - result.reserve(_length); - for (std::size_t i = 0U; i < _length; i++) - { - result.push_back(_buffer[i]); - } - return result; + return std::string(_buffer, _length); } /**