Fix(engine): include parse positions in compile errors

Now that ASTs have parse positions and the compiler will return the
position of the last error, use that in falco rules to return errors
within condition strings instead of reporting the position as the
beginning of the condition.

This led to a change in the filter_ruleset interface--now, an ast is
compiled to a filter before being passed to the filter_ruleset
object. That avoids polluting the interface with a lot of details
about rule_loader contexts, errors, etc. The ast is still provided in
case the filter_ruleset wants to do indexing/analysis of the filter.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

tests/engine/test_rulesets.cpp

@@ -27,30 +27,51 @@ static uint16_t other_non_default_ruleset = 2;
 static std::set<std::string> tags = {"some_tag", "some_other_tag"};
 static std::set<uint16_t> evttypes = { ppm_event_type::PPME_GENERIC_E };
 
-static std::shared_ptr<libsinsp::filter::ast::expr> create_filter()
+static std::shared_ptr<gen_event_filter_factory> create_factory()
 {
-	libsinsp::filter::parser parser("evt.type=open");
-	std::shared_ptr<libsinsp::filter::ast::expr> ret(parser.parse());
+	std::shared_ptr<gen_event_filter_factory> ret(new sinsp_filter_factory(NULL));
+
 	return ret;
 }
 
-static std::shared_ptr<filter_ruleset> create_ruleset()
+static std::shared_ptr<libsinsp::filter::ast::expr> create_ast(
+	std::shared_ptr<gen_event_filter_factory> f)
+{
+	libsinsp::filter::parser parser("evt.type=open");
+	std::shared_ptr<libsinsp::filter::ast::expr> ret(parser.parse());
+
+	return ret;
+}
+
+static std::shared_ptr<gen_event_filter> create_filter(
+	std::shared_ptr<gen_event_filter_factory> f,
+	std::shared_ptr<libsinsp::filter::ast::expr> ast)
+{
+	sinsp_filter_compiler compiler(f, ast.get());
+	std::shared_ptr<gen_event_filter> filter(compiler.compile());
+
+	return filter;
+}
+
+static std::shared_ptr<filter_ruleset> create_ruleset(
+	std::shared_ptr<gen_event_filter_factory> f)
 {
-	std::shared_ptr<gen_event_filter_factory> f(new sinsp_filter_factory(NULL));
 	std::shared_ptr<filter_ruleset> ret(new evttype_index_ruleset(f));
 	return ret;
 }
 
 TEST_CASE("Should enable/disable on ruleset", "[rulesets]")
 {
-	auto r = create_ruleset();
-	auto filter = create_filter();
+	auto f = create_factory();
+	auto r = create_ruleset(f);
+	auto ast = create_ast(f);
+	auto filter = create_filter(f, ast);
 	falco_rule rule;
 	rule.name = "one_rule";
 	rule.source = falco_common::syscall_source;
 	rule.tags = tags;
 
-	r->add(rule, filter);
+	r->add(rule, filter, ast);
 
 	SECTION("Should enable/disable for exact match w/ default ruleset")
 	{
@@ -184,21 +205,23 @@ TEST_CASE("Should enable/disable on ruleset", "[rulesets]")
 
 TEST_CASE("Should enable/disable on ruleset for incremental adding tags", "[rulesets]")
 {
-	auto r = create_ruleset();
+	auto f = create_factory();
+	auto r = create_ruleset(f);
+	auto ast = create_ast(f);
 
-	auto rule1_filter = create_filter();
+	auto rule1_filter = create_filter(f, ast);
 	falco_rule rule1;
 	rule1.name = "one_rule";
 	rule1.source = falco_common::syscall_source;
 	rule1.tags =  {"rule1_tag"};
-	r->add(rule1, rule1_filter);
+	r->add(rule1, rule1_filter, ast);
 
-	auto rule2_filter = create_filter();
+	auto rule2_filter = create_filter(f, ast);
 	falco_rule rule2;
 	rule2.name = "two_rule";
 	rule2.source = falco_common::syscall_source;
 	rule2.tags =  {"rule2_tag"};
-	r->add(rule2, rule2_filter);
+	r->add(rule2, rule2_filter, ast);
 
 	std::set<std::string> want_tags;
 

userspace/engine/evttype_index_ruleset.cpp

@@ -153,12 +153,11 @@ void evttype_index_ruleset::ruleset_filters::evttypes_for_ruleset(std::set<uint1
 
 void evttype_index_ruleset::add(
 		const falco_rule& rule,
+		std::shared_ptr<gen_event_filter> filter,
 		std::shared_ptr<libsinsp::filter::ast::expr> condition)
 {
 	try
 	{
-		sinsp_filter_compiler compiler(m_filter_factory, condition.get());
-		shared_ptr<gen_event_filter> filter(compiler.compile());
 		std::shared_ptr<filter_wrapper> wrap(new filter_wrapper());
 		wrap->rule = rule;
 		wrap->filter = filter;

userspace/engine/evttype_index_ruleset.h

@@ -41,6 +41,7 @@ public:
 
 	void add(
 		const falco_rule& rule,
+		std::shared_ptr<gen_event_filter> filter,
 		std::shared_ptr<libsinsp::filter::ast::expr> condition) override;
 
 	void clear() override;

userspace/engine/filter_ruleset.h

@@ -32,16 +32,20 @@ public:
 	virtual ~filter_ruleset() = default;
 
 	/*!
-		\brief Adds a rule and its filtering condition inside the manager.
-		An exception is thrown is case of error. This method only adds the rule
-		inside the internal collection, but does not enable it for any ruleset.
-		The rule must be enabled for one or more rulesets with the enable() or
-		enable_tags() methods.
+		\brief Adds a rule and its filtering filter + condition inside the manager.
+	        This method only adds the rule inside the internal collection,
+		but does not enable it for any ruleset.	The rule must be enabled
+		for one or more rulesets with the enable() or enable_tags() methods.
+		The ast representation of the rule's condition is provided to allow
+		the filter_ruleset object to parse the ast to obtain event types
+		or do other analysis/indexing of the condition.
 		\param rule The rule to be added
+		\param the filter representing the rule's filtering condition.
 		\param condition The AST representing the rule's filtering condition
 	*/
 	virtual void add(
 		const falco_rule& rule,
+		std::shared_ptr<gen_event_filter> filter,
 		std::shared_ptr<libsinsp::filter::ast::expr> condition) = 0;
 
 	/*!

userspace/engine/rule_loader_compiler.cpp

@@ -234,6 +234,7 @@ static bool resolve_list(std::string& cnd, const rule_loader::list_info& list)
 static void resolve_macros(
 	indexed_vector<rule_loader::macro_info>& macros,
 	std::shared_ptr<ast::expr>& ast,
+	const std::string& condition,
 	uint32_t visibility,
 	const rule_loader::context &ctx)
 {
@@ -248,15 +249,22 @@ static void resolve_macros(
 	macro_resolver.run(ast);
 
 	// Note: only complaining about the first unknown macro
-	THROW(!macro_resolver.get_unknown_macros().empty(),
-	      std::string("Undefined macro '")
-	      + *macro_resolver.get_unknown_macros().begin()
-	      + "' used in filter.",
-	      ctx);
-
-	for (auto &m : macro_resolver.get_resolved_macros())
+	const filter_macro_resolver::macro_info_map& unresolved_macros = macro_resolver.get_unknown_macros();
+	if(!unresolved_macros.empty())
 	{
-		macros.at(m)->used = true;
+		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);
+	}
+
+	for (auto &it : macro_resolver.get_resolved_macros())
+	{
+		macros.at(it.first)->used = true;
 	}
 }
 
@@ -363,7 +371,7 @@ void rule_loader::compiler::compile_macros_infos(
 
 	for (auto &m : out)
 	{
-		resolve_macros(out, m.cond_ast, m.visibility, m.ctx);
+		resolve_macros(out, m.cond_ast, m.cond, m.visibility, m.ctx);
 	}
 }
 
@@ -404,7 +412,7 @@ void rule_loader::compiler::compile_rule_infos(
 				r.exceptions, rule.exception_fields, condition);
 		}
 		auto ast = parse_condition(condition, lists, r.cond_ctx);
-		resolve_macros(macros, ast, MAX_VISIBILITY, r.ctx);
+		resolve_macros(macros, ast, condition, MAX_VISIBILITY, r.ctx);
 
 		// check for warnings in the filtering condition
 		warn_codes.clear();
@@ -444,10 +452,12 @@ void rule_loader::compiler::compile_rule_infos(
 		// This also compiles the filter, and might throw a
 		// falco_exception with details on the compilation
 		// failure.
+		sinsp_filter_compiler compiler(cfg.sources.at(r.source)->filter_factory, ast.get());
 		try {
-			source->ruleset->add(*out.at(rule_id), ast);
+			shared_ptr<gen_event_filter> filter(compiler.compile());
+			source->ruleset->add(*out.at(rule_id), filter, ast);
 		}
-		catch (const falco_exception& e)
+		catch (const sinsp_exception& e)
 		{
 			// Allow errors containing "nonexistent field" if
 			// skip_if_unknown_filter is true
@@ -463,10 +473,14 @@ void rule_loader::compiler::compile_rule_infos(
 			}
 			else
 			{
+				rule_loader::context ctx(compiler.get_pos(),
+							 condition,
+							 r.cond_ctx);
+
 				throw rule_loader::rule_load_exception(
 					falco::load_result::load_result::LOAD_ERR_COMPILE_CONDITION,
 					e.what(),
-					r.cond_ctx);
+					ctx);
 			}
 		}