fix(libsinsp): solve field-field comparison pointer instability issues

Signed-off-by: Jason Dellaluce <[email protected]>

userspace/libsinsp/filter.cpp

@@ -214,7 +214,6 @@ std::unique_ptr<sinsp_filter> sinsp_filter_compiler::compile() {
  m_filter = std::make_unique<sinsp_filter>();
  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<sinsp_filter_check_plugin*>(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));
  }

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<sinsp_filter_check> m_last_node_field;
- bool m_last_node_field_is_plugin;
  std::string m_flt_str;
  std::unique_ptr<sinsp_filter> m_filter;
  std::vector<std::string> m_field_values;

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; }
 };
 
 /**

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();

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",

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<sinsp_filter_factory>(&m_inspector, flist);
+ auto cf = std::make_shared<test_sinsp_filter_cache_factory>();
+ 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();

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);