fix(libsinsp/state): ensure deep copy semantics and proper memory ownership in dynamic structs

userspace/libsinsp/state/dynamic_struct.h

@@ -267,21 +267,26 @@ class dynamic_struct
  };
 
  inline explicit dynamic_struct(const std::shared_ptr<field_infos>& dynamic_fields)
- : m_fields_len(0), m_fields(), m_dynamic_fields(dynamic_fields) { }
+ : m_fields(), m_dynamic_fields(dynamic_fields) { }
+
  inline dynamic_struct(dynamic_struct&&) = default;
- inline dynamic_struct& operator = (dynamic_struct&&) = default;
- inline dynamic_struct(const dynamic_struct& s) = default;
- inline dynamic_struct& operator = (const dynamic_struct& s) = default;
+
+ inline dynamic_struct& operator=(dynamic_struct&&) = default;
+
+ inline dynamic_struct(const dynamic_struct& s)
+ {
+ deep_fields_copy(s);
+ }
+
+ inline dynamic_struct& operator=(const dynamic_struct& s)
+ {
+ deep_fields_copy(s);
+ return *this;
+ }
+
  virtual ~dynamic_struct()
  {
- if (m_dynamic_fields)
- {
- for (size_t i = 0; i < m_fields.size(); i++)
- {
- m_dynamic_fields->m_definitions_ordered[i]->info().destroy(m_fields[i]);
- free(m_fields[i]);
- }
- }
+ destroy_dynamic_fields();
  }
 
  /**
@@ -323,6 +328,10 @@ class dynamic_struct
  */
  virtual void set_dynamic_fields(const std::shared_ptr<field_infos>& defs)
  {
+ if (m_dynamic_fields.get() == defs.get())
+ {
+ return;
+ }
  if (m_dynamic_fields && m_dynamic_fields.use_count() > 1)
  {
  throw sinsp_exception("dynamic struct defintions set twice");
@@ -373,6 +382,23 @@ class dynamic_struct
  }
  }
 
+ /**
+ * @brief Destroys all the dynamic field values currently allocated
+ */
+ virtual void destroy_dynamic_fields()
+ {
+ if (!m_dynamic_fields)
+ {
+ return;
+ }
+ for (size_t i = 0; i < m_fields.size(); i++)
+ {
+ m_dynamic_fields->m_definitions_ordered[i]->info().destroy(m_fields[i]);
+ free(m_fields[i]);
+ }
+ m_fields.clear();
+ }
+
 private:
  inline void _check_defsptr(const field_info& i, bool write) const
  {
@@ -400,18 +426,38 @@ class dynamic_struct
  {
  throw sinsp_exception("dynamic struct access overflow: " + std::to_string(index));
  }
- while (m_fields_len <= index)
+ while (m_fields.size() <= index)
  {
- auto def = m_dynamic_fields->m_definitions_ordered[m_fields_len];
+ auto def = m_dynamic_fields->m_definitions_ordered[m_fields.size()];
  void* fieldbuf = malloc(def->info().size());
  def->info().construct(fieldbuf);
  m_fields.push_back(fieldbuf);
- m_fields_len++;
  }
  return m_fields[index];
  }
 
- size_t m_fields_len;
+ inline void deep_fields_copy(const dynamic_struct& other_const)
+ {
+ // note: const cast should be safe here as we're not going to resize
+ // nor edit the dynamic fields allocated in "other"
+ auto& other = const_cast<dynamic_struct&>(other_const);
+
+ // copy the definitions
+ set_dynamic_fields(other.dynamic_fields());
+
+ // deep copy of all the fields
+ destroy_dynamic_fields();
+ for (size_t i = 0; i < other.m_fields.size(); i++)
+ {
+ const auto info = m_dynamic_fields->m_definitions_ordered[i];
+ // note: we use uintptr_t as it fits all the data types supported for
+ // reading and writing dynamic fields (e.g. uint32_t, uint64_t, const char*, base_table*, ...)
+ uintptr_t val = 0;
+ other.get_dynamic_field(*info, reinterpret_cast<void*>(&val));
+ set_dynamic_field(*info, &val);
+ }
+ }
+
  std::vector<void*> m_fields;
  std::shared_ptr<field_infos> m_dynamic_fields;
 };

userspace/libsinsp/state/table.h

@@ -112,6 +112,10 @@ class base_table
 
  virtual void set_dynamic_fields(const std::shared_ptr<dynamic_struct::field_infos>& dynf)
  {
+ if (m_dynamic_fields.get() == dynf.get())
+ {
+ return;
+ }
  if (!dynf)
  {
  throw sinsp_exception("null definitions passed to set_dynamic_fields");

userspace/libsinsp/state/table_adapters.h

@@ -127,6 +127,11 @@
  }
  }
 
+ virtual void destroy_dynamic_fields() override final
+ {
+ // nothing to do
+ }
+
 private:
  T* m_value;
 };

userspace/libsinsp/test/state.ut.cpp

@@ -161,7 +161,7 @@
  // struct construction and setting fields definition
  sample_struct s(fields);
  ASSERT_ANY_THROW(s.set_dynamic_fields(nullptr));
- ASSERT_ANY_THROW(s.set_dynamic_fields(fields));
+ ASSERT_ANY_THROW(s.set_dynamic_fields(std::make_shared<libsinsp::state::dynamic_struct::field_infos>()));
  // The double paranthesis fixes
  // Error C2063 'std::shared_ptr<libsinsp::state::dynamic_struct::field_infos>' : not a function C
  // on the Windows compiler.
@@ -170,7 +170,7 @@
  ASSERT_NO_THROW(sample_struct(nullptr));
  auto s2 = sample_struct(nullptr);
  s2.set_dynamic_fields(fields);
- ASSERT_ANY_THROW(s2.set_dynamic_fields(fields));
+ ASSERT_NO_THROW(s2.set_dynamic_fields(fields));
 
  // check field definitions
  ASSERT_EQ(fields->fields().size(), 0);
@@ -230,6 +230,77 @@
  ASSERT_ANY_THROW(s.get_dynamic_field(acc_num2, tmp));
 }
 
+TEST(dynamic_struct, mem_ownership)
+{
+ struct sample_struct: public libsinsp::state::dynamic_struct
+ {
+ sample_struct(const std::shared_ptr<field_infos>& i): dynamic_struct(i) { }
+ };
+
+ std::string tmpstr1, tmpstr2;
+ auto defs1 = std::make_shared<libsinsp::state::dynamic_struct::field_infos>();
+
+ // construct two entries, test safety checks
+ sample_struct s1(nullptr);
+ ASSERT_NO_THROW(s1.set_dynamic_fields(nullptr));
+ ASSERT_NO_THROW(s1.set_dynamic_fields(defs1));
+ sample_struct s2(defs1);
+ ASSERT_ANY_THROW(s1.set_dynamic_fields(nullptr));
+ ASSERT_NO_THROW(s1.set_dynamic_fields(defs1));
+ ASSERT_ANY_THROW(s1.set_dynamic_fields(std::make_shared<libsinsp::state::dynamic_struct::field_infos>()));
+
+ // define a string dynamic field
+ auto field_str = defs1->add_field<std::string>("str");
+ auto field_str_acc = field_str.new_accessor<std::string>();
+
+ // write same value in both structs, ensure they have two distinct copies
+ s1.set_dynamic_field(field_str_acc, std::string("hello"));
+ s1.get_dynamic_field(field_str_acc, tmpstr1);
+ ASSERT_EQ(tmpstr1, std::string("hello"));
+ s2.get_dynamic_field(field_str_acc, tmpstr2);
+ ASSERT_EQ(tmpstr2, std::string("")); // s2 should not be influenced
+ s2.set_dynamic_field(field_str_acc, std::string("hello2"));
+ s2.get_dynamic_field(field_str_acc, tmpstr2);
+ ASSERT_EQ(tmpstr2, tmpstr1 + "2");
+ s1.get_dynamic_field(field_str_acc, tmpstr1); // s1 should not be influenced
+ ASSERT_EQ(tmpstr2, tmpstr1 + "2");
+
+ // deep copy and memory ownership (constructor)
+ sample_struct s3(s1);
+ ASSERT_EQ(s1.dynamic_fields().get(), s3.dynamic_fields().get());
+ s1.get_dynamic_field(field_str_acc, tmpstr1);
+ s3.get_dynamic_field(field_str_acc, tmpstr2);
+ ASSERT_EQ(tmpstr1, tmpstr2);
+ s3.set_dynamic_field(field_str_acc, std::string("hello3"));
+ s1.get_dynamic_field(field_str_acc, tmpstr1); // should still be "hello" as before
+ s3.get_dynamic_field(field_str_acc, tmpstr2);
+ ASSERT_NE(tmpstr1, tmpstr2);
+
+ // deep copy and memory ownership (assignment)
+ sample_struct s4(std::make_shared<libsinsp::state::dynamic_struct::field_infos>());
+ s4 = s1;
+ ASSERT_EQ(s1.dynamic_fields().get(), s4.dynamic_fields().get());
+ s1.get_dynamic_field(field_str_acc, tmpstr1);
+ s4.get_dynamic_field(field_str_acc, tmpstr2);
+ ASSERT_EQ(tmpstr1, tmpstr2);
+ s4.set_dynamic_field(field_str_acc, std::string("hello4"));
+ s1.get_dynamic_field(field_str_acc, tmpstr1); // should still be "hello" as before
+ s4.get_dynamic_field(field_str_acc, tmpstr2);
+ ASSERT_NE(tmpstr1, tmpstr2);
+
+ // deep copy and memory ownership (assignment, null initial definitions)
+ sample_struct s5(nullptr);
+ s5 = s1;
+ ASSERT_EQ(s1.dynamic_fields().get(), s5.dynamic_fields().get());
+ s1.get_dynamic_field(field_str_acc, tmpstr1);
+ s5.get_dynamic_field(field_str_acc, tmpstr2);
+ ASSERT_EQ(tmpstr1, tmpstr2);
+ s5.set_dynamic_field(field_str_acc, std::string("hello4"));
+ s1.get_dynamic_field(field_str_acc, tmpstr1); // should still be "hello" as before
+ s5.get_dynamic_field(field_str_acc, tmpstr2);
+ ASSERT_NE(tmpstr1, tmpstr2);
+}
+
 TEST(table_registry, defs_and_access)
 {
  class sample_table: public libsinsp::state::table<uint64_t>