From 289bb6e0cac0f66edf0428408ff7b082c000ff93 Mon Sep 17 00:00:00 2001 From: Jason Dellaluce Date: Thu, 12 Sep 2024 15:58:16 +0000 Subject: [PATCH] fix(libsinsp): solve field-field comparison pointer instability issues Signed-off-by: Jason Dellaluce --- userspace/libsinsp/filter.cpp | 21 +++++++------------ userspace/libsinsp/filter.h | 1 - userspace/libsinsp/filter_field.h | 12 +++++++++++ userspace/libsinsp/plugin.cpp | 4 +++- .../libsinsp/sinsp_filtercheck_event.cpp | 12 +++-------- .../libsinsp/test/filter_compiler.ut.cpp | 13 ++++++++++++ .../libsinsp/test/sinsp_with_test_input.cpp | 3 ++- 7 files changed, 40 insertions(+), 26 deletions(-) diff --git a/userspace/libsinsp/filter.cpp b/userspace/libsinsp/filter.cpp index 04d9b69d81..07abbed7a9 100644 --- a/userspace/libsinsp/filter.cpp +++ b/userspace/libsinsp/filter.cpp @@ -214,7 +214,6 @@ std::unique_ptr sinsp_filter_compiler::compile() { m_filter = std::make_unique(); m_last_boolop = BO_NONE; m_last_node_field = nullptr; - m_last_node_field_is_plugin = false; try { m_flt_ast->accept(this); } catch(const sinsp_exception& e) { @@ -278,7 +277,6 @@ static inline void check_op_type_compatibility(sinsp_filter_check& c) { void sinsp_filter_compiler::visit(const libsinsp::filter::ast::unary_check_expr* e) { m_pos = e->get_pos(); m_last_node_field = nullptr; - m_last_node_field_is_plugin = false; e->left->accept(this); if(!m_last_node_field) { throw sinsp_exception("filter error: missing field in left-hand of unary check"); @@ -320,13 +318,12 @@ static void add_filtercheck_value(sinsp_filter_check* chk, size_t idx, std::stri void sinsp_filter_compiler::visit(const libsinsp::filter::ast::binary_check_expr* e) { m_pos = e->get_pos(); m_last_node_field = nullptr; - m_last_node_field_is_plugin = false; e->left->accept(this); if(!m_last_node_field) { throw sinsp_exception("filter error: missing field in left-hand of binary check"); } - auto left_from_plugin = m_last_node_field_is_plugin; + auto left_ptr_unstable = m_last_node_field->get_field_info()->is_ptr_unstable(); auto check = std::move(m_last_node_field); // install cache on left-hand side extraction field @@ -335,13 +332,13 @@ void sinsp_filter_compiler::visit(const libsinsp::filter::ast::binary_check_expr check->m_cache_metrics = m_cache_factory->new_metrics(e->left.get(), node_info); check->m_extract_cache = m_cache_factory->new_extract_cache(e->left.get(), node_info); - // if the extraction comes from a plugin-implemented ield, then + // if the extraction comes from a plugin-implemented field, then // we need to add a storage transformer as the cache may end up storing a // shallow copy of the value pointers that are not valid anymore. Note that // this should not change the right field's eligibility for caching, as // the storage transformer does not alter the field's info. auto left_has_storage = false; - if(left_from_plugin && check->m_extract_cache) { + if(left_ptr_unstable && check->m_extract_cache) { left_has_storage = true; check->add_transformer(filter_transformer_type::FTR_STORAGE); } @@ -351,7 +348,6 @@ void sinsp_filter_compiler::visit(const libsinsp::filter::ast::binary_check_expr check_op_type_compatibility(*check); // Read the right-hand values of the filtercheck. - m_last_node_field_is_plugin = false; m_field_values.clear(); e->right->accept(this); @@ -379,8 +375,8 @@ void sinsp_filter_compiler::visit(const libsinsp::filter::ast::binary_check_expr // // However, we may have already added a storage modifier to the left field due to issues // with caching, in which case we are good already. - auto right_from_plugin = m_last_node_field_is_plugin; - if(!left_has_storage && left_from_plugin && right_from_plugin) { + auto right_ptr_unstable = m_last_node_field->get_field_info()->is_ptr_unstable(); + if(!left_has_storage && left_ptr_unstable && right_ptr_unstable) { check->add_transformer(filter_transformer_type::FTR_STORAGE); } @@ -404,7 +400,7 @@ void sinsp_filter_compiler::visit(const libsinsp::filter::ast::binary_check_expr // similarly as above, if the right-hand side extraction comes from a // plugin-implemented field, then we need to add an additional storage // layer on it as well - if(right_from_plugin && m_last_node_field->m_extract_cache) { + if(right_ptr_unstable && m_last_node_field->m_extract_cache) { m_last_node_field->add_transformer(filter_transformer_type::FTR_STORAGE); } @@ -554,8 +550,6 @@ void sinsp_filter_compiler::visit(const libsinsp::filter::ast::field_expr* e) { m_pos = e->get_pos(); auto field_name = create_filtercheck_name(e->field, e->arg); m_last_node_field = create_filtercheck(field_name); - m_last_node_field_is_plugin = - dynamic_cast(m_last_node_field.get()) != nullptr; if(m_last_node_field->parse_field_name(field_name, true, true) == -1) { throw sinsp_exception("filter error: can't parse field expression '" + field_name + "'"); } @@ -564,14 +558,13 @@ void sinsp_filter_compiler::visit(const libsinsp::filter::ast::field_expr* e) { void sinsp_filter_compiler::visit(const libsinsp::filter::ast::field_transformer_expr* e) { m_pos = e->get_pos(); m_last_node_field = nullptr; - m_last_node_field_is_plugin = false; e->value->accept(this); if(!m_last_node_field) { throw sinsp_exception("filter error: found null child node on '" + e->transformer + "' transformer"); } - // apply transformer, ignoring the "identity one" + // apply transformer, ignoring the "identity one" (it's just a syntactic construct) if(e->transformer != "val") { m_last_node_field->add_transformer(filter_transformer_from_str(e->transformer)); } diff --git a/userspace/libsinsp/filter.h b/userspace/libsinsp/filter.h index c5bc72cbc7..9a3661ea6e 100644 --- a/userspace/libsinsp/filter.h +++ b/userspace/libsinsp/filter.h @@ -270,7 +270,6 @@ class SINSP_PUBLIC sinsp_filter_compiler : private libsinsp::filter::ast::const_ libsinsp::filter::ast::pos_info m_pos; boolop m_last_boolop; std::unique_ptr m_last_node_field; - bool m_last_node_field_is_plugin; std::string m_flt_str; std::unique_ptr m_filter; std::vector m_field_values; diff --git a/userspace/libsinsp/filter_field.h b/userspace/libsinsp/filter_field.h index b46561d483..95abb635d1 100644 --- a/userspace/libsinsp/filter_field.h +++ b/userspace/libsinsp/filter_field.h @@ -46,6 +46,10 @@ enum filtercheck_field_flags { EPF_NO_TRANSFORMER = 1 << 11, ///< this field cannot have a field transformer. EPF_NO_RHS = 1 << 12, ///< this field cannot have a right-hand side filter check, and cannot be ///< used as a right-hand side filter check. + EPF_NO_PTR_STABILITY = + 1 << 13, ///< data pointers extracted by this field may change across subsequent + ///< extractions (even of other fields), which makes them unsafe to be used + ///< with filter caching or field-to-field comparisons }; /** @@ -93,6 +97,14 @@ struct filtercheck_field_info { // Returns true if this filter check can support an extraction transformer on it. // inline bool is_transformer_supported() const { return !(m_flags & EPF_NO_TRANSFORMER); } + + // + // Return true if this field extracts unstable data pointers that may change + // at subsequent extractions (even of other fields), thus not being safe to + // be used with caches or field-to-field filter comparisons, unless protected + // through a memory buffer copy (e.g. with a FTR_STORAGE transformer) + // + inline bool is_ptr_unstable() const { return m_flags & EPF_NO_PTR_STABILITY; } }; /** diff --git a/userspace/libsinsp/plugin.cpp b/userspace/libsinsp/plugin.cpp index d4e621cd86..406e03e7d7 100644 --- a/userspace/libsinsp/plugin.cpp +++ b/userspace/libsinsp/plugin.cpp @@ -463,7 +463,9 @@ bool sinsp_plugin::resolve_dylib_symbols(std::string& errstr) { m_fields.clear(); for(Json::Value::ArrayIndex j = 0; j < root.size(); j++) { filtercheck_field_info tf; - tf.m_flags = EPF_NONE; + + // plugin fields can't ever be trusted for pointer stability + tf.m_flags = EPF_NO_PTR_STABILITY; const Json::Value& jvtype = root[j]["type"]; string ftype = jvtype.asString(); diff --git a/userspace/libsinsp/sinsp_filtercheck_event.cpp b/userspace/libsinsp/sinsp_filtercheck_event.cpp index 5c4f0cd0a5..cb21635bbb 100644 --- a/userspace/libsinsp/sinsp_filtercheck_event.cpp +++ b/userspace/libsinsp/sinsp_filtercheck_event.cpp @@ -37,12 +37,6 @@ extern sinsp_evttables g_infotables; return (uint8_t*)&(x); \ } while(0) -#define RETURN_EXTRACT_PTR(x) \ - do { \ - *len = sizeof(*(x)); \ - return (uint8_t*)(x); \ - } while(0) - #define RETURN_EXTRACT_STRING(x) \ do { \ *len = (x).size(); \ @@ -162,7 +156,7 @@ const filtercheck_field_info sinsp_filter_check_event_fields[] = { "Arguments", "all the event arguments, aggregated into a single string."}, {PT_CHARBUF, - EPF_ARG_REQUIRED, + EPF_ARG_REQUIRED | EPF_NO_PTR_STABILITY, PF_NA, "evt.arg", "Argument", @@ -184,7 +178,7 @@ const filtercheck_field_info sinsp_filter_check_event_fields[] = { "(like writes to /dev/log) it provides higher level information coming from decoding the " "arguments."}, {PT_BYTEBUF, - EPF_NONE, + EPF_NO_PTR_STABILITY, PF_NA, "evt.buffer", "Buffer", @@ -198,7 +192,7 @@ const filtercheck_field_info sinsp_filter_check_event_fields[] = { "the length of the binary data buffer for events that have one, like read(), recvfrom(), " "etc."}, {PT_CHARBUF, - EPF_NONE, + EPF_NO_PTR_STABILITY, PF_DEC, "evt.res", "Return Value", diff --git a/userspace/libsinsp/test/filter_compiler.ut.cpp b/userspace/libsinsp/test/filter_compiler.ut.cpp index 701fccbd1d..d5c55eaca0 100644 --- a/userspace/libsinsp/test/filter_compiler.ut.cpp +++ b/userspace/libsinsp/test/filter_compiler.ut.cpp @@ -827,6 +827,19 @@ TEST_F(sinsp_with_test_input, filter_cache_corner_cases) { cf->metrics->reset(); } +TEST_F(sinsp_with_test_input, filter_cache_pointer_instability) { + sinsp_filter_check_list flist; + + add_default_init_thread(); + open_inspector(); + + auto ff = std::make_shared(&m_inspector, flist); + auto cf = std::make_shared(); + auto evt = generate_proc_exit_event(2, INIT_TID); + + EXPECT_FALSE(eval_filter(evt, "(evt.arg.ret = val(evt.arg.reaper_tid))")); +} + TEST_F(sinsp_with_test_input, filter_regex_operator_evaluation) { // Basic case just to assert that the basic setup works add_default_init_thread(); diff --git a/userspace/libsinsp/test/sinsp_with_test_input.cpp b/userspace/libsinsp/test/sinsp_with_test_input.cpp index c5431994c3..25653e02cb 100644 --- a/userspace/libsinsp/test/sinsp_with_test_input.cpp +++ b/userspace/libsinsp/test/sinsp_with_test_input.cpp @@ -321,13 +321,14 @@ sinsp_evt* sinsp_with_test_input::generate_proc_exit_event(int64_t tid_to_remove // Scaffolding needed to call the PPME_PROCEXIT_1_E int64_t not_relevant_64 = 0; uint8_t not_relevant_8 = 0; + int64_t ret_err_perm = -1; // mostly non-relevant, used in few tests return add_event_advance_ts(increasing_ts(), tid_to_remove, PPME_PROCEXIT_1_E, 5, not_relevant_64, - not_relevant_64, + ret_err_perm, not_relevant_8, not_relevant_8, reaper_tid);