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.

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

fix: graceful error handling for macros/lists reference loops #2311

Merged
merged 5 commits into from
Dec 13, 2022
Merged
Changes from all commits
Commits
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
20 changes: 20 additions & 0 deletions test/falco_tests.yaml
Original file line number Diff line number Diff line change
@@ -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:
17 changes: 17 additions & 0 deletions test/rules/invalid_list_loop.yaml
Original file line number Diff line number Diff line change
@@ -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
8 changes: 8 additions & 0 deletions test/rules/invalid_macro_loop.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
- macro: macro_a
condition: evt.type=open

- macro: macro_b
condition: macro_a

- macro: macro_a
condition: macro_b
18 changes: 14 additions & 4 deletions tests/engine/test_filter_macro_resolver.cpp
Original file line number Diff line number Diff line change
@@ -20,6 +20,16 @@ limitations under the License.
using namespace std;
using namespace libsinsp::filter::ast;

static std::vector<filter_macro_resolver::value_info>::const_iterator find_value(
const std::vector<filter_macro_resolver::value_info>& 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);
35 changes: 28 additions & 7 deletions userspace/engine/filter_macro_resolver.cpp
Original file line number Diff line number Diff line change
@@ -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<libsinsp::filter::ast::expr>& 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::value_info>& 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::value_info>& filter_macro_resolver::get_errors() const
{
return m_errors;
}

const std::vector<filter_macro_resolver::value_info>& 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()});
}
}
38 changes: 27 additions & 11 deletions userspace/engine/filter_macro_resolver.h
Original file line number Diff line number Diff line change
@@ -59,24 +59,31 @@ class filter_macro_resolver
std::shared_ptr<libsinsp::filter::ast::expr> 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<std::string,libsinsp::filter::ast::pos_info> macro_info_map;
typedef std::pair<std::string,libsinsp::filter::ast::pos_info> 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<value_info>& get_resolved_macros() const;

/*!
\brief Returns a set containing the names of all the macros
that remained unresolved during the last invocation of run().
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<value_info>& get_unknown_macros() const;

/*!
\brief Returns a list of errors occurred during
the latest invocation of run().
*/
const std::vector<value_info>& 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<value_info>& errors,
std::vector<value_info>& unknown_macros,
std::vector<value_info>& 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<std::string> m_macros_path;
std::unique_ptr<libsinsp::filter::ast::expr> m_node_substitute;
macro_info_map& m_unknown_macros;
macro_info_map& m_resolved_macros;

std::vector<value_info>& m_errors;
std::vector<value_info>& m_unknown_macros;
std::vector<value_info>& 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<value_info> m_errors;
std::vector<value_info> m_unknown_macros;
std::vector<value_info> m_resolved_macros;
macro_defs m_macros;
};
23 changes: 12 additions & 11 deletions userspace/engine/rule_loader_compiler.cpp
Original file line number Diff line number Diff line change
@@ -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())