From d44a7dc6035e8d49a09903f87d9902688de271d2 Mon Sep 17 00:00:00 2001 From: Alkis Evlogimenos <alkis@evlogimenos.com> Date: Sat, 8 Jun 2024 02:47:24 +0200 Subject: [PATCH] Fix bugs in in ctl::optional (#1203) Manually manage the lifetime of `value_` by using an anonymous `union`. This fixes a bunch of double-frees and double-constructs. Additionally move the `present_` flag last. When `T` has padding `present_` will be placed there saving `alignof(T)` bytes from `sizeof(optional<T>)`. --- ctl/optional.h | 35 +++++++++++++++++++++++------------ test/ctl/optional_test.cc | 9 +++++++++ 2 files changed, 32 insertions(+), 12 deletions(-) diff --git a/ctl/optional.h b/ctl/optional.h index be7f33929..e3214da31 100644 --- a/ctl/optional.h +++ b/ctl/optional.h @@ -14,38 +14,45 @@ class optional public: using value_type = T; - ~optional() = default; + ~optional() + { + if (present_) + value_.~T(); + } optional() noexcept : present_(false) { } - optional(const T& value) : present_(true), value_(value) + optional(const T& value) : present_(true) { + new (&value_) T(value); } - optional(T&& value) : present_(true), value_(std::move(value)) + optional(T&& value) : present_(true) { + new (&value_) T(std::move(value)); } optional(const optional& other) : present_(other.present_) { - if (present_) + if (other.present_) new (&value_) T(other.value_); } optional(optional&& other) noexcept : present_(other.present_) { - if (present_) - value_ = std::move(other.value_); + if (other.present_) + new (&value_) T(std::move(other.value_)); } optional& operator=(const optional& other) { if (this != &other) { + reset(); + if (other.present_) + new (&value_) T(other.value_); present_ = other.present_; - if (present_) - value_ = other.value_; } return *this; } @@ -53,9 +60,10 @@ class optional optional& operator=(optional&& other) noexcept { if (this != &other) { + reset(); + if (other.present_) + new (&value_) T(std::move(other.value_)); present_ = other.present_; - if (present_) - value_ = std::move(other.value_); } return *this; } @@ -102,8 +110,9 @@ class optional template<typename... Args> void emplace(Args&&... args) { + reset(); present_ = true; - value_ = T(std::forward<Args>(args)...); + new (&value_) T(std::forward<Args>(args)...); } void swap(optional& other) noexcept @@ -120,8 +129,10 @@ class optional } private: + union { + T value_; + }; bool present_; - T value_; }; } // namespace ctl diff --git a/test/ctl/optional_test.cc b/test/ctl/optional_test.cc index de75ebe30..49133e7d9 100644 --- a/test/ctl/optional_test.cc +++ b/test/ctl/optional_test.cc @@ -26,6 +26,8 @@ // #include <string> // #define ctl std +static int g = 0; + int main() { @@ -114,6 +116,13 @@ main() return 24; } + { + struct A { int* p = &g; A() {++*p; } }; + ctl::optional<A> x; + if (g != 0) + return 25; + } + CheckForMemoryLeaks(); return 0; }