fix(libsinsp/state): ensure deep copy semantics and proper memory own…

…ership in dynamic structs

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>

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 @@ template<typename T> class value_table_entry_adapter : public libsinsp::state::t
 		}
 	}
 
+	virtual void destroy_dynamic_fields() override final
+	{
+		// nothing to do
+	}
+
 private:
 	T* m_value;
 };

userspace/libsinsp/test/state.ut.cpp

@@ -161,7 +161,7 @@ TEST(dynamic_struct, defs_and_access)
     // 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 @@ TEST(dynamic_struct, defs_and_access)
     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 @@ TEST(dynamic_struct, defs_and_access)
     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>