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.

Author: aalkin

Framework/Core/include/Framework/Variant.h

@@ -13,6 +13,7 @@
 
 #include "Framework/RuntimeError.h"
 #include "Framework/Array2D.h"
+#include "Framework/Traits.h"
 #include <type_traits>
 #include <cstring>
 #include <cstdint>
@@ -261,9 +262,9 @@ struct variant_helper {
     new (reinterpret_cast<T*>(store)) T{};
     *(reinterpret_cast<T*>(store)) = value;
   }
-  static void set(void* store, T values, size_t size)
+  static void set(void* store, std::remove_pointer_t<T> const* values, size_t size)
   {
-    *reinterpret_cast<T*>(store) = reinterpret_cast<T>(std::memcpy(std::malloc(size * sizeof(std::remove_pointer_t<T>)), reinterpret_cast<void*>(values), size * sizeof(std::remove_pointer_t<T>)));
+    *reinterpret_cast<T*>(store) = reinterpret_cast<T>(std::memcpy(std::malloc(size * sizeof(std::remove_pointer_t<T>)), reinterpret_cast<void const*>(values), size * sizeof(std::remove_pointer_t<T>)));
   }
 
   static T get(const void* store) { return *(reinterpret_cast<const T*>(store)); }
@@ -317,9 +318,10 @@ struct variant_helper<std::string> {
 class Variant
 {
  public:
-  Variant(VariantType type = VariantType::Unknown) : mType{type} {}
+  Variant(VariantType type = VariantType::Unknown);
 
   template <typename T>
+    requires(!is_specialization_v<T, std::vector>)
   Variant(T value) : mType{variant_trait_v<T>}
   {
     variant_helper<decltype(value)>::set(&mStore, value);
@@ -331,17 +333,23 @@ class Variant
     variant_helper<T>::set(&mStore, values, mSize);
   }
 
+  // A Variant owns what it stores: the content of vectors is copied
   template <typename T>
-  Variant(std::vector<T>& values) : mType{variant_trait_v<T*>}, mSize{values.size()}
+  Variant(std::vector<T> const& values) : mType{variant_trait_v<T*>}, mSize{values.size()}
   {
     variant_helper<T*>::set(&mStore, values.data(), mSize);
   }
 
-  Variant(std::vector<std::string>& values) : mType{VariantType::ArrayString}, mSize{values.size()}
+  Variant(std::vector<std::string> const& values) : mType{VariantType::ArrayString}, mSize{values.size()}
   {
     variant_helper<std::vector<std::string>>::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 <typename T>
+  Variant(std::vector<T>&&) = delete;
+
   template <typename T>
   Variant(std::initializer_list<T>)
   {
@@ -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 <typename T>
   Variant& operator=(std::vector<T>&& other) noexcept
   {
@@ -381,31 +391,25 @@ class Variant
     return variant_helper<T>::get(&mStore);
   }
 
+  // The setters replace the current content, releasing it first.
   template <typename T>
   void set(T value)
   {
-    return variant_helper<T>::set(&mStore, value);
+    *this = Variant(value);
   }
 
   template <typename T>
   void set(T value, size_t size)
   {
-    mSize = size;
-    return variant_helper<T>::set(&mStore, value, mSize);
-  }
-
-  template <typename T>
-  void set(std::vector<T>& values)
-    requires(std::is_pod_v<T>)
-  {
-    return variant_helper<T*>::set(&mStore, values.data(), values.size());
+    *this = Variant(value, size);
   }
 
+  /// FIXME: set for vector of strings is not used anywhere, why?
   template <typename T>
   void set(std::vector<T>& values)
-    requires(std::is_same_v<T, std::string>)
+    requires(std::is_pod_v<T> || std::is_same_v<T, std::string>)
   {
-    return variant_helper<T*>::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,

Framework/Core/src/Variant.cxx

@@ -11,7 +11,10 @@
 #include "Framework/Variant.h"
 #include "Framework/VariantPropertyTreeHelpers.h"
 #include "Framework/VariantJSONHelpers.h"
+#include <cstdlib>
+#include <cstring>
 #include <iostream>
+#include <memory>
 #include <sstream>
 
 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<const char*>::set(&mStore, other.get<const char*>());
-      return;
-    case VariantType::ArrayInt:
-      mSize = other.mSize;
-      variant_helper<int*>::set(&mStore, other.get<int*>(), mSize);
-      return;
-    case VariantType::ArrayFloat:
-      mSize = other.mSize;
-      variant_helper<float*>::set(&mStore, other.get<float*>(), mSize);
-      return;
-    case VariantType::ArrayDouble:
-      mSize = other.mSize;
-      variant_helper<double*>::set(&mStore, other.get<double*>(), mSize);
-      return;
-    case VariantType::ArrayBool:
-      mSize = other.mSize;
-      variant_helper<bool*>::set(&mStore, other.get<bool*>(), mSize);
-      return;
+/// Helper visitor for Variant
+template <typename F>
+bool visitStoredObject(VariantType type, F&& f)
+{
+  switch (type) {
     case VariantType::ArrayString:
-      mSize = other.mSize;
-      variant_helper<std::vector<std::string>>::set(&mStore, other.get<std::vector<std::string>>());
-      return;
+      f.template operator()<std::vector<std::string>>();
+      return true;
+    case VariantType::Array2DInt:
+      f.template operator()<Array2D<int>>();
+      return true;
+    case VariantType::Array2DFloat:
+      f.template operator()<Array2D<float>>();
+      return true;
+    case VariantType::Array2DDouble:
+      f.template operator()<Array2D<double>>();
+      return true;
+    case VariantType::LabeledArrayInt:
+      f.template operator()<LabeledArray<int>>();
+      return true;
+    case VariantType::LabeledArrayFloat:
+      f.template operator()<LabeledArray<float>>();
+      return true;
+    case VariantType::LabeledArrayDouble:
+      f.template operator()<LabeledArray<double>>();
+      return true;
+    case VariantType::LabeledArrayString:
+      f.template operator()<LabeledArray<std::string>>();
+      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<void**>(&(other.mStore)) = nullptr;
-      return;
+      return true;
     default:
-      return;
+      return false;
   }
 }
 
-Variant::~Variant()
+template <typename T>
+T* copyBuffer(T const* values, size_t size)
+{
+  if (values == nullptr) {
+    return nullptr;
+  }
+  return reinterpret_cast<T*>(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]<typename T>() { 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]<typename T>() { new (&mStore) T(*reinterpret_cast<T const*>(&other.mStore)); })) {
+    return;
+  }
+  // Manually allocated buffers have to be managed
   switch (mType) {
-    case VariantType::String:
+    case VariantType::String: {
+      auto const* value = *reinterpret_cast<char const* const*>(&other.mStore);
+      *reinterpret_cast<char**>(&mStore) = value != nullptr ? strdup(value) : nullptr;
+      return;
+    }
     case VariantType::ArrayInt:
+      *reinterpret_cast<int**>(&mStore) = copyBuffer(*reinterpret_cast<int* const*>(&other.mStore), mSize);
+      return;
     case VariantType::ArrayFloat:
+      *reinterpret_cast<float**>(&mStore) = copyBuffer(*reinterpret_cast<float* const*>(&other.mStore), mSize);
+      return;
     case VariantType::ArrayDouble:
-    case VariantType::ArrayBool: {
-      if (reinterpret_cast<void**>(&mStore) != nullptr) {
-        free(*reinterpret_cast<void**>(&mStore));
-      }
+      *reinterpret_cast<double**>(&mStore) = copyBuffer(*reinterpret_cast<double* const*>(&other.mStore), mSize);
       return;
-    }
-    case VariantType::ArrayString: {
-      // Allocated with placement new. Nothing to delete.
+    case VariantType::ArrayBool:
+      *reinterpret_cast<bool**>(&mStore) = copyBuffer(*reinterpret_cast<bool* const*>(&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]<typename T>() { new (&mStore) T(std::move(*reinterpret_cast<T*>(&other.mStore))); })) {
+    return;
+  }
+  mStore = other.mStore;
+  // Buffers have to change their owner
+  if (holdsMallocedPointer(mType)) {
+    *reinterpret_cast<void**>(&other.mStore) = nullptr;
+  }
+}
+
+void Variant::destroyStore() noexcept
+{
+  // destroy objects
+  if (visitStoredObject(mType, [this]<typename T>() { std::destroy_at(reinterpret_cast<T*>(&mStore)); })) {
+    return;
+  }
+  // deallocate buffers
+  if (holdsMallocedPointer(mType)) {
+    free(*reinterpret_cast<void**>(&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<const char*>::set(&mStore, other.get<const char*>());
-      return *this;
-    case VariantType::ArrayInt:
-      variant_helper<int*>::set(&mStore, other.get<int*>(), mSize);
-      return *this;
-    case VariantType::ArrayFloat:
-      variant_helper<float*>::set(&mStore, other.get<float*>(), mSize);
-      return *this;
-    case VariantType::ArrayDouble:
-      variant_helper<double*>::set(&mStore, other.get<double*>(), mSize);
-      return *this;
-    case VariantType::ArrayBool:
-      variant_helper<bool*>::set(&mStore, other.get<bool*>(), mSize);
-      return *this;
-    case VariantType::ArrayString:
-      variant_helper<std::vector<std::string>>::set(&mStore, other.get<std::vector<std::string>>());
-      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<const char*>::set(&mStore, other.get<const char*>());
-      *reinterpret_cast<char**>(&(other.mStore)) = nullptr;
-      return *this;
-    case VariantType::ArrayInt:
-      variant_helper<int*>::set(&mStore, other.get<int*>(), mSize);
-      *reinterpret_cast<int**>(&(other.mStore)) = nullptr;
-      return *this;
-    case VariantType::ArrayFloat:
-      variant_helper<float*>::set(&mStore, other.get<float*>(), mSize);
-      *reinterpret_cast<float**>(&(other.mStore)) = nullptr;
-      return *this;
-    case VariantType::ArrayDouble:
-      variant_helper<double*>::set(&mStore, other.get<double*>(), mSize);
-      *reinterpret_cast<double**>(&(other.mStore)) = nullptr;
-      return *this;
-    case VariantType::ArrayBool:
-      variant_helper<bool*>::set(&mStore, other.get<bool*>(), mSize);
-      *reinterpret_cast<bool**>(&(other.mStore)) = nullptr;
-      return *this;
-    case VariantType::ArrayString:
-      variant_helper<std::vector<std::string>>::set(&mStore, other.get<std::vector<std::string>>());
-      *reinterpret_cast<std::vector<std::string>**>(&(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<std::string>, std::vector<std::string>> extractLabels(boost::property_tree::ptree const& tree)