diff --git a/test/falco_tests.yaml b/test/falco_tests.yaml index f8dedc0c40f..6e209c215f9 100644 --- a/test/falco_tests.yaml +++ b/test/falco_tests.yaml @@ -358,6 +358,16 @@ trace_files: !mux validate_rules_file: - rules/invalid_macro_without_condition.yaml trace_file: trace_files/cat_write.scap + + invalid_macro_loop: + exit_status: 1 + validate_errors: + - item_type: macro + item_name: macro_a + code: LOAD_ERR_VALIDATE + message_contains: "reference loop in macro" + validate_rules_file: + - rules/invalid_macro_loop.yaml invalid_rule_without_output: exit_status: 1 @@ -403,6 +413,16 @@ trace_files: !mux - rules/list_append_failure.yaml trace_file: trace_files/cat_write.scap + invalid_list_loop: + exit_status: 1 + validate_errors: + - item_type: rule + item_name: sample rule + code: LOAD_ERR_COMPILE_CONDITION + message: "unknown event type list_a" + validate_rules_file: + - rules/invalid_list_loop.yaml + invalid_rule_append_dangling: exit_status: 1 validate_errors: diff --git a/test/rules/invalid_list_loop.yaml b/test/rules/invalid_list_loop.yaml new file mode 100644 index 00000000000..bb8fc4937ec --- /dev/null +++ b/test/rules/invalid_list_loop.yaml @@ -0,0 +1,17 @@ +- list: list_a + items: [open] + +- list: list_b + items: [list_a] + +- list: list_a + items: [list_b] + +- macro: macro_a + condition: evt.type in (list_a) + +- rule: sample rule + priority: WARNING + output: test + desc: testdesc + condition: macro_a \ No newline at end of file diff --git a/test/rules/invalid_macro_loop.yaml b/test/rules/invalid_macro_loop.yaml new file mode 100644 index 00000000000..38ed41c7894 --- /dev/null +++ b/test/rules/invalid_macro_loop.yaml @@ -0,0 +1,8 @@ +- macro: macro_a + condition: evt.type=open + +- macro: macro_b + condition: macro_a + +- macro: macro_a + condition: macro_b diff --git a/tests/engine/test_filter_macro_resolver.cpp b/tests/engine/test_filter_macro_resolver.cpp index 20b5f11472d..96b966d7542 100644 --- a/tests/engine/test_filter_macro_resolver.cpp +++ b/tests/engine/test_filter_macro_resolver.cpp @@ -20,6 +20,16 @@ limitations under the License. using namespace std; using namespace libsinsp::filter::ast; +static std::vector::const_iterator find_value( + const std::vector& values, + const std::string& ref) +{ + return std::find_if( + values.begin(), + values.end(), + [&ref](const filter_macro_resolver::value_info& v) { return v.first == ref; }); +} + TEST_CASE("Should resolve macros on a filter AST", "[rule_loader]") { string macro_name = "test_macro"; @@ -117,12 +127,12 @@ TEST_CASE("Should resolve macros on a filter AST", "[rule_loader]") // first run REQUIRE(resolver.run(filter) == true); REQUIRE(resolver.get_resolved_macros().size() == 2); - auto a_resolved_itr = resolver.get_resolved_macros().find(a_macro_name); + auto a_resolved_itr = find_value(resolver.get_resolved_macros(), a_macro_name); REQUIRE(a_resolved_itr != resolver.get_resolved_macros().end()); REQUIRE(a_resolved_itr->first == a_macro_name); REQUIRE(a_resolved_itr->second == a_macro_pos); - auto b_resolved_itr = resolver.get_resolved_macros().find(b_macro_name); + auto b_resolved_itr = find_value(resolver.get_resolved_macros(), b_macro_name); REQUIRE(b_resolved_itr != resolver.get_resolved_macros().end()); REQUIRE(resolver.get_unknown_macros().empty()); REQUIRE(b_resolved_itr->first == b_macro_name); @@ -166,12 +176,12 @@ TEST_CASE("Should resolve macros on a filter AST", "[rule_loader]") // first run REQUIRE(resolver.run(filter) == true); REQUIRE(resolver.get_resolved_macros().size() == 2); - auto a_resolved_itr = resolver.get_resolved_macros().find(a_macro_name); + auto a_resolved_itr = find_value(resolver.get_resolved_macros(), a_macro_name); REQUIRE(a_resolved_itr != resolver.get_resolved_macros().end()); REQUIRE(a_resolved_itr->first == a_macro_name); REQUIRE(a_resolved_itr->second == a_macro_pos); - auto b_resolved_itr = resolver.get_resolved_macros().find(b_macro_name); + auto b_resolved_itr = find_value(resolver.get_resolved_macros(), b_macro_name); REQUIRE(b_resolved_itr != resolver.get_resolved_macros().end()); REQUIRE(resolver.get_unknown_macros().empty()); REQUIRE(b_resolved_itr->first == b_macro_name); diff --git a/userspace/engine/filter_macro_resolver.cpp b/userspace/engine/filter_macro_resolver.cpp index 81cf598d2b7..9a8f4bfd169 100644 --- a/userspace/engine/filter_macro_resolver.cpp +++ b/userspace/engine/filter_macro_resolver.cpp @@ -15,6 +15,7 @@ limitations under the License. */ #include "filter_macro_resolver.h" +#include "falco_common.h" using namespace std; using namespace libsinsp::filter; @@ -23,8 +24,9 @@ bool filter_macro_resolver::run(libsinsp::filter::ast::expr*& filter) { m_unknown_macros.clear(); m_resolved_macros.clear(); + m_errors.clear(); - visitor v(m_unknown_macros, m_resolved_macros, m_macros); + visitor v(m_errors, m_unknown_macros, m_resolved_macros, m_macros); v.m_node_substitute = nullptr; filter->accept(&v); if (v.m_node_substitute) @@ -39,8 +41,9 @@ bool filter_macro_resolver::run(std::shared_ptr& fi { m_unknown_macros.clear(); m_resolved_macros.clear(); + m_errors.clear(); - visitor v(m_unknown_macros, m_resolved_macros, m_macros); + visitor v(m_errors, m_unknown_macros, m_resolved_macros, m_macros); v.m_node_substitute = nullptr; filter->accept(&v); if (v.m_node_substitute) @@ -57,12 +60,17 @@ void filter_macro_resolver::set_macro( m_macros[name] = macro; } -const filter_macro_resolver::macro_info_map& filter_macro_resolver::get_unknown_macros() const +const std::vector& filter_macro_resolver::get_unknown_macros() const { return m_unknown_macros; } -const filter_macro_resolver::macro_info_map& filter_macro_resolver::get_resolved_macros() const +const std::vector& filter_macro_resolver::get_errors() const +{ + return m_errors; +} + +const std::vector& filter_macro_resolver::get_resolved_macros() const { return m_resolved_macros; } @@ -125,9 +133,21 @@ void filter_macro_resolver::visitor::visit(ast::value_expr* e) // we are supposed to get here only in case // of identier-only children from either a 'not', // an 'and' or an 'or'. - auto macro = m_macros.find(e->value); + const auto& macro = m_macros.find(e->value); if (macro != m_macros.end() && macro->second) // skip null-ptr macros { + // note: checks for loop detection + const auto& prevref = std::find(m_macros_path.begin(), m_macros_path.end(), macro->first); + if (prevref != m_macros_path.end()) + { + auto msg = "reference loop in macro '" + macro->first + "'"; + m_errors.push_back({msg, e->get_pos()}); + m_node_substitute = nullptr; + m_unknown_macros.push_back({e->value, e->get_pos()}); + return; + } + + m_macros_path.push_back(macro->first); m_node_substitute = nullptr; auto new_node = ast::clone(macro->second.get()); new_node->accept(this); @@ -137,11 +157,12 @@ void filter_macro_resolver::visitor::visit(ast::value_expr* e) { m_node_substitute = std::move(new_node); } - m_resolved_macros[e->value] = e->get_pos(); + m_resolved_macros.push_back({e->value, e->get_pos()}); + m_macros_path.pop_back(); } else { m_node_substitute = nullptr; - m_unknown_macros[e->value] = e->get_pos(); + m_unknown_macros.push_back({e->value, e->get_pos()}); } } diff --git a/userspace/engine/filter_macro_resolver.h b/userspace/engine/filter_macro_resolver.h index 8c798e81fc5..4c3888e6cae 100644 --- a/userspace/engine/filter_macro_resolver.h +++ b/userspace/engine/filter_macro_resolver.h @@ -59,16 +59,17 @@ class filter_macro_resolver std::shared_ptr macro); /*! - \brief used in get_{resolved,unknown}_macros + \brief used in get_{resolved,unknown}_macros and get_errors + to represent an identifier/string value along with an AST position. */ - typedef std::unordered_map macro_info_map; + typedef std::pair value_info; /*! \brief Returns a set containing the names of all the macros substituted during the last invocation of run(). Should be non-empty if the last invocation of run() returned true. */ - const macro_info_map& get_resolved_macros() const; + const std::vector& get_resolved_macros() const; /*! \brief Returns a set containing the names of all the macros @@ -76,7 +77,13 @@ class filter_macro_resolver A macro remains unresolved if it is found inside the processed filter but it was not defined with set_macro(); */ - const macro_info_map& get_unknown_macros() const; + const std::vector& get_unknown_macros() const; + + /*! + \brief Returns a list of errors occurred during + the latest invocation of run(). + */ + const std::vector& get_errors() const; private: typedef std::unordered_map< @@ -86,17 +93,25 @@ class filter_macro_resolver struct visitor : public libsinsp::filter::ast::expr_visitor { - visitor(macro_info_map& unknown_macros, macro_info_map& resolved_macros, macro_defs& macros) - : m_unknown_macros(unknown_macros), m_resolved_macros(resolved_macros), m_macros(macros) {} + visitor( + std::vector& errors, + std::vector& unknown_macros, + std::vector& resolved_macros, + macro_defs& macros): + m_errors(errors), + m_unknown_macros(unknown_macros), + m_resolved_macros(resolved_macros), + m_macros(macros) {} visitor(visitor&&) = default; visitor& operator = (visitor&&) = default; visitor(const visitor&) = delete; visitor& operator = (const visitor&) = delete; + std::vector m_macros_path; std::unique_ptr m_node_substitute; - macro_info_map& m_unknown_macros; - macro_info_map& m_resolved_macros; - + std::vector& m_errors; + std::vector& m_unknown_macros; + std::vector& m_resolved_macros; macro_defs& m_macros; void visit(libsinsp::filter::ast::and_expr* e) override; @@ -108,7 +123,8 @@ class filter_macro_resolver void visit(libsinsp::filter::ast::binary_check_expr* e) override; }; - macro_info_map m_unknown_macros; - macro_info_map m_resolved_macros; + std::vector m_errors; + std::vector m_unknown_macros; + std::vector m_resolved_macros; macro_defs m_macros; }; diff --git a/userspace/engine/rule_loader_compiler.cpp b/userspace/engine/rule_loader_compiler.cpp index b54d84aed3f..112f9060b1a 100644 --- a/userspace/engine/rule_loader_compiler.cpp +++ b/userspace/engine/rule_loader_compiler.cpp @@ -248,18 +248,19 @@ static void resolve_macros( } macro_resolver.run(ast); - // Note: only complaining about the first unknown macro - const filter_macro_resolver::macro_info_map& unresolved_macros = macro_resolver.get_unknown_macros(); - if(!unresolved_macros.empty()) + // Note: only complaining about the first error or unknown macro + const auto& errors_macros = macro_resolver.get_errors(); + const auto& unresolved_macros = macro_resolver.get_unknown_macros(); + if(!errors_macros.empty() || !unresolved_macros.empty()) { - auto it = unresolved_macros.begin(); - const rule_loader::context cond_ctx(it->second, condition, ctx); - - THROW(true, - std::string("Undefined macro '") - + it->first - + "' used in filter.", - cond_ctx); + auto errpos = !errors_macros.empty() + ? errors_macros.begin()->second + : unresolved_macros.begin()->second; + std::string errmsg = !errors_macros.empty() + ? errors_macros.begin()->first + : ("Undefined macro '" + unresolved_macros.begin()->first + "' used in filter."); + const rule_loader::context cond_ctx(errpos, condition, ctx); + THROW(true, errmsg, cond_ctx); } for (auto &it : macro_resolver.get_resolved_macros())