Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Aug 28, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 62 additions & 16 deletions userspace/libsinsp/state/dynamic_struct.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

/**
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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
{
Expand Down Expand Up @@ -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;
};
Expand Down
4 changes: 4 additions & 0 deletions userspace/libsinsp/state/table.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
5 changes: 5 additions & 0 deletions userspace/libsinsp/state/table_adapters.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,11 @@
}
}

virtual void destroy_dynamic_fields() override final

Check warning on line 130 in userspace/libsinsp/state/table_adapters.h

View check run for this annotation

Codecov / codecov/patch

userspace/libsinsp/state/table_adapters.h#L130 

Added line #L130 was not covered by tests
{
// nothing to do
}

private:
T* m_value;
};
Expand Down
75 changes: 73 additions & 2 deletions userspace/libsinsp/test/state.ut.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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);
Expand Down Expand Up @@ -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

Check warning on line 235 in userspace/libsinsp/test/state.ut.cpp

View check run for this annotation

Codecov / codecov/patch

userspace/libsinsp/test/state.ut.cpp#L235 

Added line #L235 was not covered by tests
{
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>
Expand Down
Loading