From 108314af4e12d5ad0d5170939095433a74ac48d0 Mon Sep 17 00:00:00 2001 From: Anton Alkin Date: Thu, 1 Oct 2026 10:32:58 +0200 Subject: [PATCH 1/2] Fix leaks and self-assignment in Array2D The copy and move assignment operators overwrote the data pointer without releasing the previous buffer, leaking it on every assignment to a non-empty array Copy self-assignment also allocated the new buffer before reading from the source, so the array was filled from its own uninitialised memory. Now guards against self-assignment and releases the old buffer. --- Framework/Core/include/Framework/Array2D.h | 22 +++++--- Framework/Core/test/test_Variants.cxx | 58 ++++++++++++++++++++++ 2 files changed, 74 insertions(+), 6 deletions(-) diff --git a/Framework/Core/include/Framework/Array2D.h b/Framework/Core/include/Framework/Array2D.h index 857e4b3c89f29..de877797105c9 100644 --- a/Framework/Core/include/Framework/Array2D.h +++ b/Framework/Core/include/Framework/Array2D.h @@ -79,19 +79,29 @@ struct Array2D { Array2D& operator=(Array2D const& other) { - this->rows = other.rows; - this->cols = other.cols; - data = new T[rows * cols]; - for (auto i = 0U; i < rows; ++i) { - for (auto j = 0U; j < cols; ++j) { - data[i * cols + j] = *(other.data + (i * cols + j)); + if (this == &other) { + return *this; + } + // Copy into a new buffer first, so that a throwing copy leaves this array untouched + auto* newData = new T[other.rows * other.cols]; + for (auto i = 0U; i < other.rows; ++i) { + for (auto j = 0U; j < other.cols; ++j) { + newData[i * other.cols + j] = other.data[i * other.cols + j]; } } + delete[] data; + data = newData; + this->rows = other.rows; + this->cols = other.cols; return *this; } Array2D& operator=(Array2D&& other) { + if (this == &other) { + return *this; + } + delete[] data; this->rows = other.rows; this->cols = other.cols; data = other.data; diff --git a/Framework/Core/test/test_Variants.cxx b/Framework/Core/test/test_Variants.cxx index da1f39f241e96..c47a54eb68e62 100644 --- a/Framework/Core/test/test_Variants.cxx +++ b/Framework/Core/test/test_Variants.cxx @@ -229,6 +229,64 @@ TEST_CASE("Array2DTest") } } +TEST_CASE("Array2DAssignmentTest") +{ + std::vector v = {1, 2, 3, 4, 5, 6}; + Array2D source(v, 2, 3); + + // copy assignment to an array of different shape makes an independent deep copy + Array2D copied(std::vector{7}, 1, 1); + copied = source; + REQUIRE(copied.rows == 2); + REQUIRE(copied.cols == 3); + REQUIRE(copied.data != source.data); + source[0][0] = 42; + REQUIRE(copied(0, 0) == 1); + for (auto i = 1U; i < 6; ++i) { + REQUIRE(copied(i / 3, i % 3) == v[i]); + } + + // self copy assignment keeps the contents + auto& self = copied; + copied = self; + REQUIRE(copied.rows == 2); + REQUIRE(copied.cols == 3); + REQUIRE(copied(1, 2) == 6); + + // move assignment takes over the buffer and empties the source + auto* buffer = copied.data; + Array2D moved(std::vector{7, 8}, 1, 2); + moved = std::move(copied); + REQUIRE(moved.data == buffer); + REQUIRE(moved.rows == 2); + REQUIRE(moved.cols == 3); + REQUIRE(moved(1, 2) == 6); + REQUIRE(copied.data == nullptr); + REQUIRE(copied.rows == 0); + REQUIRE(copied.cols == 0); + + // self move assignment keeps the contents + auto& selfMoved = moved; + moved = std::move(selfMoved); + REQUIRE(moved.data == buffer); + REQUIRE(moved(1, 2) == 6); + + // assigning an empty array releases the old buffer and leaves an empty array + moved = Array2D{}; + REQUIRE(moved.data == nullptr); + REQUIRE(moved.rows == 0); + REQUIRE(moved.cols == 0); + + // strings are copied element by element + std::vector s = {"one", "two"}; + Array2D ms(s, 2, 1); + Array2D msc; + msc = ms; + ms[0][0] = "changed"; + REQUIRE(msc(0, 0) == "one"); + REQUIRE(msc(1, 0) == "two"); +} + TEST_CASE("LabeledArrayTest") { float m[3][4] = {{0.1, 0.2, 0.3, 0.4}, {0.5, 0.6, 0.7, 0.8}, {0.9, 1.0, 1.1, 1.2}}; From e9b38f11b3930940993d0e491f1447c3d68d5efa Mon Sep 17 00:00:00 2001 From: Anton Alkin Date: Thu, 1 Oct 2026 10:44:39 +0200 Subject: [PATCH 2/2] Fix ownership of the content of Variant Copy and move of Variants holding Array2D, LabeledArray or string array values copied the storage byte by byte, so copies shared the same heap buffers, and the destructor never destroyed these objects, leaking them. Assignments never released the previous content, move assignment of strings and arrays duplicated the buffer and dropped the original one, and move assignment of string arrays corrupted the source vector. --- Framework/Core/include/Framework/Variant.h | 42 ++-- Framework/Core/src/Variant.cxx | 232 ++++++++++++--------- Framework/Core/test/test_Variants.cxx | 168 ++++++++++++++- 3 files changed, 319 insertions(+), 123 deletions(-) diff --git a/Framework/Core/include/Framework/Variant.h b/Framework/Core/include/Framework/Variant.h index 7121c5fad0669..3611fad438e0a 100644 --- a/Framework/Core/include/Framework/Variant.h +++ b/Framework/Core/include/Framework/Variant.h @@ -13,6 +13,7 @@ #include "Framework/RuntimeError.h" #include "Framework/Array2D.h" +#include "Framework/Traits.h" #include #include #include @@ -261,9 +262,9 @@ struct variant_helper { new (reinterpret_cast(store)) T{}; *(reinterpret_cast(store)) = value; } - static void set(void* store, T values, size_t size) + static void set(void* store, std::remove_pointer_t const* values, size_t size) { - *reinterpret_cast(store) = reinterpret_cast(std::memcpy(std::malloc(size * sizeof(std::remove_pointer_t)), reinterpret_cast(values), size * sizeof(std::remove_pointer_t))); + *reinterpret_cast(store) = reinterpret_cast(std::memcpy(std::malloc(size * sizeof(std::remove_pointer_t)), reinterpret_cast(values), size * sizeof(std::remove_pointer_t))); } static T get(const void* store) { return *(reinterpret_cast(store)); } @@ -317,9 +318,10 @@ struct variant_helper { class Variant { public: - Variant(VariantType type = VariantType::Unknown) : mType{type} {} + Variant(VariantType type = VariantType::Unknown); template + requires(!is_specialization_v) Variant(T value) : mType{variant_trait_v} { variant_helper::set(&mStore, value); @@ -331,17 +333,23 @@ class Variant variant_helper::set(&mStore, values, mSize); } + // A Variant owns what it stores: the content of vectors is copied template - Variant(std::vector& values) : mType{variant_trait_v}, mSize{values.size()} + Variant(std::vector const& values) : mType{variant_trait_v}, mSize{values.size()} { variant_helper::set(&mStore, values.data(), mSize); } - Variant(std::vector& values) : mType{VariantType::ArrayString}, mSize{values.size()} + Variant(std::vector const& values) : mType{VariantType::ArrayString}, mSize{values.size()} { variant_helper>::set(&mStore, values); } + // A temporary vector cannot hand over its buffer to a Variant, + // so creating one only to copy it is an error + template + Variant(std::vector&&) = delete; + template Variant(std::initializer_list) { @@ -355,6 +363,8 @@ class Variant ~Variant(); Variant& operator=(const Variant& other); Variant& operator=(Variant&& other) noexcept; + // Assignment from a temporary vector has to be allowed because it is used, but copies + // to make sure Variant owns its content template Variant& operator=(std::vector&& other) noexcept { @@ -381,31 +391,25 @@ class Variant return variant_helper::get(&mStore); } + // The setters replace the current content, releasing it first. template void set(T value) { - return variant_helper::set(&mStore, value); + *this = Variant(value); } template void set(T value, size_t size) { - mSize = size; - return variant_helper::set(&mStore, value, mSize); - } - - template - void set(std::vector& values) - requires(std::is_pod_v) - { - return variant_helper::set(&mStore, values.data(), values.size()); + *this = Variant(value, size); } + /// FIXME: set for vector of strings is not used anywhere, why? template void set(std::vector& values) - requires(std::is_same_v) + requires(std::is_pod_v || std::is_same_v) { - return variant_helper::set(&mStore, values); + *this = Variant(values); } [[nodiscard]] VariantType type() const { return mType; } @@ -414,6 +418,10 @@ class Variant private: friend std::ostream& operator<<(std::ostream& oss, Variant const& val); + // Helpers to manage the store depending on the actual content + void copyStore(Variant const& other); + void moveStore(Variant& other) noexcept; + void destroyStore() noexcept; using storage_t = std::aligned_union<8, int, int8_t, int16_t, int64_t, uint8_t, uint16_t, uint32_t, uint64_t, const char*, float, double, bool, diff --git a/Framework/Core/src/Variant.cxx b/Framework/Core/src/Variant.cxx index e54a973bd4413..f780a1a952f1b 100644 --- a/Framework/Core/src/Variant.cxx +++ b/Framework/Core/src/Variant.cxx @@ -11,7 +11,10 @@ #include "Framework/Variant.h" #include "Framework/VariantPropertyTreeHelpers.h" #include "Framework/VariantJSONHelpers.h" +#include +#include #include +#include #include namespace o2::framework @@ -84,145 +87,166 @@ std::string Variant::asString() const return ss.str(); } -Variant::Variant(const Variant& other) : mType(other.mType) +namespace { - // In case this is an array we need to duplicate it to avoid - // double deletion. - switch (mType) { - case VariantType::String: - mSize = other.mSize; - variant_helper::set(&mStore, other.get()); - return; - case VariantType::ArrayInt: - mSize = other.mSize; - variant_helper::set(&mStore, other.get(), mSize); - return; - case VariantType::ArrayFloat: - mSize = other.mSize; - variant_helper::set(&mStore, other.get(), mSize); - return; - case VariantType::ArrayDouble: - mSize = other.mSize; - variant_helper::set(&mStore, other.get(), mSize); - return; - case VariantType::ArrayBool: - mSize = other.mSize; - variant_helper::set(&mStore, other.get(), mSize); - return; +/// Helper visitor for Variant +template +bool visitStoredObject(VariantType type, F&& f) +{ + switch (type) { case VariantType::ArrayString: - mSize = other.mSize; - variant_helper>::set(&mStore, other.get>()); - return; + f.template operator()>(); + return true; + case VariantType::Array2DInt: + f.template operator()>(); + return true; + case VariantType::Array2DFloat: + f.template operator()>(); + return true; + case VariantType::Array2DDouble: + f.template operator()>(); + return true; + case VariantType::LabeledArrayInt: + f.template operator()>(); + return true; + case VariantType::LabeledArrayFloat: + f.template operator()>(); + return true; + case VariantType::LabeledArrayDouble: + f.template operator()>(); + return true; + case VariantType::LabeledArrayString: + f.template operator()>(); + return true; default: - mStore = other.mStore; - mSize = other.mSize; + return false; } } -Variant::Variant(Variant&& other) noexcept : mType(other.mType) +/// Types for which the storage keeps a pointer to a manually allocated buffer +bool holdsMallocedPointer(VariantType type) { - mStore = other.mStore; - mSize = other.mSize; - switch (mType) { + switch (type) { case VariantType::String: case VariantType::ArrayInt: case VariantType::ArrayFloat: case VariantType::ArrayDouble: case VariantType::ArrayBool: - case VariantType::ArrayString: - *reinterpret_cast(&(other.mStore)) = nullptr; - return; + return true; default: - return; + return false; } } -Variant::~Variant() +template +T* copyBuffer(T const* values, size_t size) +{ + if (values == nullptr) { + return nullptr; + } + return reinterpret_cast(std::memcpy(std::malloc(size * sizeof(T)), values, size * sizeof(T))); +} +} // namespace + +Variant::Variant(VariantType type) : mType{type} +{ + // Make sure that destroying a Variant created without a value is always safe + // by creating a default stored object upfront + if (!visitStoredObject(mType, [this]() { new (&mStore) T{}; })) { + std::memset(&mStore, 0, sizeof(mStore)); + } +} + +void Variant::copyStore(Variant const& other) { - // In case we allocated an array, we - // should delete it. + // Proper objects are simply copied + if (visitStoredObject(mType, [this, &other]() { new (&mStore) T(*reinterpret_cast(&other.mStore)); })) { + return; + } + // Manually allocated buffers have to be managed switch (mType) { - case VariantType::String: + case VariantType::String: { + auto const* value = *reinterpret_cast(&other.mStore); + *reinterpret_cast(&mStore) = value != nullptr ? strdup(value) : nullptr; + return; + } case VariantType::ArrayInt: + *reinterpret_cast(&mStore) = copyBuffer(*reinterpret_cast(&other.mStore), mSize); + return; case VariantType::ArrayFloat: + *reinterpret_cast(&mStore) = copyBuffer(*reinterpret_cast(&other.mStore), mSize); + return; case VariantType::ArrayDouble: - case VariantType::ArrayBool: { - if (reinterpret_cast(&mStore) != nullptr) { - free(*reinterpret_cast(&mStore)); - } + *reinterpret_cast(&mStore) = copyBuffer(*reinterpret_cast(&other.mStore), mSize); return; - } - case VariantType::ArrayString: { - // Allocated with placement new. Nothing to delete. + case VariantType::ArrayBool: + *reinterpret_cast(&mStore) = copyBuffer(*reinterpret_cast(&other.mStore), mSize); return; - } default: - return; + // Trivially copyable content + mStore = other.mStore; + } +} + +void Variant::moveStore(Variant& other) noexcept +{ + // Correct move for objects, leaving proper "moved from" state + if (visitStoredObject(mType, [this, &other]() { new (&mStore) T(std::move(*reinterpret_cast(&other.mStore))); })) { + return; + } + mStore = other.mStore; + // Buffers have to change their owner + if (holdsMallocedPointer(mType)) { + *reinterpret_cast(&other.mStore) = nullptr; + } +} + +void Variant::destroyStore() noexcept +{ + // destroy objects + if (visitStoredObject(mType, [this]() { std::destroy_at(reinterpret_cast(&mStore)); })) { + return; + } + // deallocate buffers + if (holdsMallocedPointer(mType)) { + free(*reinterpret_cast(&mStore)); } } +Variant::Variant(const Variant& other) : mType(other.mType), mSize(other.mSize) +{ + copyStore(other); +} + +Variant::Variant(Variant&& other) noexcept : mType(other.mType), mSize(other.mSize) +{ + moveStore(other); +} + +Variant::~Variant() +{ + destroyStore(); +} + Variant& Variant::operator=(const Variant& other) { - mSize = other.mSize; - mType = other.mType; - switch (mType) { - case VariantType::String: - variant_helper::set(&mStore, other.get()); - return *this; - case VariantType::ArrayInt: - variant_helper::set(&mStore, other.get(), mSize); - return *this; - case VariantType::ArrayFloat: - variant_helper::set(&mStore, other.get(), mSize); - return *this; - case VariantType::ArrayDouble: - variant_helper::set(&mStore, other.get(), mSize); - return *this; - case VariantType::ArrayBool: - variant_helper::set(&mStore, other.get(), mSize); - return *this; - case VariantType::ArrayString: - variant_helper>::set(&mStore, other.get>()); - return *this; - default: - mStore = other.mStore; - return *this; + if (this != &other) { + // Copy first, so that a throwing copy leaves this Variant untouched + Variant copy(other); + *this = std::move(copy); } + return *this; } Variant& Variant::operator=(Variant&& other) noexcept { - mSize = other.mSize; - mType = other.mType; - switch (mType) { - case VariantType::String: - variant_helper::set(&mStore, other.get()); - *reinterpret_cast(&(other.mStore)) = nullptr; - return *this; - case VariantType::ArrayInt: - variant_helper::set(&mStore, other.get(), mSize); - *reinterpret_cast(&(other.mStore)) = nullptr; - return *this; - case VariantType::ArrayFloat: - variant_helper::set(&mStore, other.get(), mSize); - *reinterpret_cast(&(other.mStore)) = nullptr; - return *this; - case VariantType::ArrayDouble: - variant_helper::set(&mStore, other.get(), mSize); - *reinterpret_cast(&(other.mStore)) = nullptr; - return *this; - case VariantType::ArrayBool: - variant_helper::set(&mStore, other.get(), mSize); - *reinterpret_cast(&(other.mStore)) = nullptr; - return *this; - case VariantType::ArrayString: - variant_helper>::set(&mStore, other.get>()); - *reinterpret_cast**>(&(other.mStore)) = nullptr; - return *this; - default: - mStore = other.mStore; - return *this; + if (this != &other) { + destroyStore(); + mType = other.mType; + mSize = other.mSize; + moveStore(other); } + return *this; } std::pair, std::vector> extractLabels(boost::property_tree::ptree const& tree) diff --git a/Framework/Core/test/test_Variants.cxx b/Framework/Core/test/test_Variants.cxx index c47a54eb68e62..244dd3c9359c5 100644 --- a/Framework/Core/test/test_Variants.cxx +++ b/Framework/Core/test/test_Variants.cxx @@ -287,6 +287,88 @@ TEST_CASE("Array2DAssignmentTest") REQUIRE(msc(1, 0) == "two"); } +TEST_CASE("VariantLifecycleTest") +{ + float m[2][3] = {{1, 2, 3}, {4, 5, 6}}; + LabeledArray laf{&m[0][0], 2, 3, {"r1", "r2"}, {"c1", "c2", "c3"}}; + std::vector vs{"s1", "s2", "s3"}; + auto checkLabeled = [&](Variant const& v) { + REQUIRE(v.type() == VariantType::LabeledArrayFloat); + auto la = v.get>(); + REQUIRE(la.rows() == 2); + REQUIRE(la.cols() == 3); + REQUIRE(la.get("r2", "c3") == 6); + REQUIRE(la.getLabelsRows() == std::vector{"r1", "r2"}); + }; + + Variant vl(laf); + // copies are independent and survive the destruction of the original + auto* copy = new Variant(vl); + Variant moved(std::move(*copy)); + delete copy; + checkLabeled(vl); + checkLabeled(moved); + + // assignment across types releases the previous content + Variant vstr("a string"); + vstr = vl; + checkLabeled(vstr); + vstr = Variant("back to a string"); + REQUIRE(vstr.type() == VariantType::String); + REQUIRE(std::string(vstr.get()) == "back to a string"); + vstr = std::move(moved); + checkLabeled(vstr); + Variant vvs(vs); + vstr = vvs; + REQUIRE(vstr.get>() == vs); + + // self assignment keeps the content + auto& self = vl; + vl = self; + checkLabeled(vl); + vl = std::move(self); + checkLabeled(vl); + + // moving a string array leaves a valid moved-from Variant behind + Variant vvsMoved(std::move(vvs)); + REQUIRE(vvsMoved.get>() == vs); + vvs = vvsMoved; + REQUIRE(vvs.get>() == vs); + + // reallocation of a container moves the Variants around + std::vector collection; + for (auto i = 0; i < 20; ++i) { + collection.emplace_back(laf); + collection.emplace_back(Array2D{&m[0][0], 2, 3}); + collection.emplace_back(vs); + collection.emplace_back("a string"); + } + std::vector collectionCopy = collection; + collection.clear(); + for (auto i = 0U; i < collectionCopy.size(); i += 4) { + checkLabeled(collectionCopy[i]); + REQUIRE(collectionCopy[i + 1].get>()(1, 2) == 6); + REQUIRE(collectionCopy[i + 2].get>() == vs); + REQUIRE(std::string(collectionCopy[i + 3].get()) == "a string"); + } + + // a Variant created with only a type can be copied and destroyed + Variant typed(VariantType::LabeledArrayFloat); + Variant typedCopy(typed); + REQUIRE(typedCopy.get>().rows() == 0); + Variant typedString(VariantType::String); + Variant typedStringCopy(typedString); + REQUIRE(typedStringCopy.get() == nullptr); + + // set replaces the content and the type + Variant vset(1); + vset.set(laf); + checkLabeled(vset); + vset.set(vs); + REQUIRE(vset.type() == VariantType::ArrayString); + REQUIRE(vset.get>() == vs); +} + TEST_CASE("LabeledArrayTest") { float m[3][4] = {{0.1, 0.2, 0.3, 0.4}, {0.5, 0.6, 0.7, 0.8}, {0.9, 1.0, 1.1, 1.2}}; @@ -311,11 +393,14 @@ TEST_CASE("LabeledArrayTest") TEST_CASE("VariantTreeConversionsTest") { std::vector vstrings{"0 1", "0 2", "0 3"}; - Variant vvstr(std::move(vstrings)); + Variant vvstr(vstrings); auto tree = vectorToBranch(vvstr.get(), vvstr.size()); - auto v = Variant(vectorFromBranch(tree)); + auto fromTree = vectorFromBranch(tree); + auto v = Variant(fromTree); + REQUIRE(vvstr.size() == vstrings.size()); + REQUIRE(v.size() == vstrings.size()); for (auto i = 0U; i < vvstr.size(); ++i) { REQUIRE(vvstr.get()[i] == v.get()[i]); } @@ -410,3 +495,82 @@ TEST_CASE("VariantThrowing") REQUIRE(error.what == std::string("Variant::get: Mismatch between types 4 0.")); } } + +// A Variant can be created from a vector only by copying its content +static_assert(!std::is_constructible_v>); +static_assert(!std::is_constructible_v>); +static_assert(!std::is_constructible_v>); +static_assert(!std::is_constructible_v>); +static_assert(!std::is_constructible_v>); +// assignment from a temporary vector copies it +static_assert(std::is_assignable_v>); +static_assert(std::is_assignable_v>); +static_assert(std::is_constructible_v&>); +static_assert(std::is_constructible_v const&>); +static_assert(std::is_constructible_v const&>); +static_assert(std::is_assignable_v const&>); + +namespace +{ +template +void checkVectorCopy(VariantType type, std::vector source) +{ + auto check = [&](Variant const& v, std::vector const& from) { + REQUIRE(v.type() == type); + REQUIRE(v.size() == from.size()); + auto const* stored = v.get(); + REQUIRE(stored != from.data()); + for (auto i = 0U; i < from.size(); ++i) { + REQUIRE(stored[i] == from[i]); + } + }; + std::vector const constSource = source; + Variant fromConst(constSource); + check(fromConst, constSource); + + Variant fromMutable(source); + check(fromMutable, source); + // the Variant owns a copy, unaffected by later changes of the source + auto const original = source; + source[0] = source.back(); + source.push_back(source[0]); + check(fromMutable, original); +} +} // namespace + +TEST_CASE("VariantFromVectorTest") +{ + checkVectorCopy(VariantType::ArrayInt, {1, 2, 3, 4}); + checkVectorCopy(VariantType::ArrayFloat, {0.5f, 1.5f, 2.5f}); + checkVectorCopy(VariantType::ArrayDouble, {1e-3, 1e3}); + checkVectorCopy(VariantType::ArrayString, {"a", "bb", "ccc"}); + + // assignment from a vector copies as well + std::vector vi{7, 8, 9}; + Variant v(1); + v = vi; + REQUIRE(v.type() == VariantType::ArrayInt); + REQUIRE(v.size() == 3); + REQUIRE(v.get() != vi.data()); + REQUIRE(v.get()[2] == 9); + // also from a temporary + auto makeStrings = []() { return std::vector{"x", "y"}; }; + v = makeStrings(); + REQUIRE(v.type() == VariantType::ArrayString); + REQUIRE(v.size() == 2); + REQUIRE(v.get()[1] == "y"); + v = std::vector{1.5, 2.5, 3.5}; + REQUIRE(v.type() == VariantType::ArrayDouble); + REQUIRE(v.size() == 3); + REQUIRE(v.get()[2] == 3.5); + + // a Variant created from a const vector round-trips through JSON + std::vector const vd{0.25, 0.5, 0.75}; + Variant vdv(vd); + std::stringstream is(vdv.asString()); + auto read = VariantJSONHelpers::read(is); + REQUIRE(read.size() == vd.size()); + for (auto i = 0U; i < vd.size(); ++i) { + REQUIRE(read.get()[i] == vd[i]); + } +}