[PATCH 0/8] nftables: fix and improve error reporting

[PATCH 0/8] nftables: fix and improve error reporting

[Patrick McHardy]

The following patchset fixes a couple of minor issues and improves
error reporting:

- don't show "In file included from internal:0:0-0:" for files parsed with -f
- location fixes
- fix output of implicit relational ops for constant LHS or non-constant RHS
- add missing closing of scope on parsing errors of blocks
- recover from errors in blocks and continue parsing
- evaluate every command immediately after parsing it

The goal of this patchset is to make error reporting show multiple errors
at once so testing and fixing of a ruleset can be done more quickly.


With these patches, we output all errors encountered during parsing and
evaluation in one batch:

tests/error.1:4:8-8: Error: syntax error, unexpected '{', expecting string
filter {
       ^
tests/error.1:5:13-13: Error: syntax error, unexpected newline
filter input
            ^
tests/error.1:6:17-17: Error: syntax error, unexpected newline
filter input tcp
                ^
tests/error.1:7:23-23: Error: syntax error, unexpected newline
filter input tcp dport
                      ^
tests/error.1:8:24-26: Error: datatype mismatch, expected internet network service, expression has type Internet protocol
filter input tcp dport tcp
             ~~~~~~~~~ ^^^
tests/error.1:9:24-32: Error: Right hand side of relational expression (==) must be constant
filter input tcp dport tcp dport
             ~~~~~~~~~~^^^^^^^^^


I'll push this shortly unless I hear objections.


Patrick McHardy (8):
      evaluate: determine implicit relational op before RHS constant checks
      scanner: don't update location's line_offset for newlines
      scanner: update last_line in struct location
      erec: skip includes with INDESC_INTERNAL
      parser: close scope when encountering an error in a table or chain block
      parser: recover from errors in any block
      parser: evaluate commands immediately after parsing
      tests: add two tests for error reporting


 include/nftables.h |  1 +
 include/parser.h   |  2 ++
 include/rule.h     |  2 +-
 src/erec.c         |  4 +++-
 src/evaluate.c     | 35 ++++++++++++-----------------------
 src/main.c         | 11 +++--------
 src/parser.y       | 27 ++++++++++++++++++++++-----
 src/scanner.l      |  2 +-
 tests/error.1      |  9 +++++++++
 tests/error.2      | 18 ++++++++++++++++++
 10 files changed, 72 insertions(+), 39 deletions(-)

[PATCH 1/8] evaluate: determine implicit relational op before RHS constant checks

[Patrick McHardy]

The symbol for the relational op is shown in the error message. Since
OP_IMPLICIT doesn't have a symbol, (null) is shown. Fix by determining
the implicit op before checking for constants.

Error: Right hand side of relational expression ((null)) must be constant
=>
Error: Right hand side of relational expression (==) must be constant

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 src/evaluate.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 215a004..a01d2a5 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -872,17 +872,6 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 		return -1;
 	right = rel->right;
 
-	if (!expr_is_constant(right))
-		return expr_binary_error(ctx, right, rel,
-					 "Right hand side of relational "
-					 "expression (%s) must be constant",
-					 expr_op_symbols[rel->op]);
-	if (expr_is_constant(left))
-		return expr_binary_error(ctx, left, right,
-					 "Relational expression (%s) has "
-					 "constant value",
-					 expr_op_symbols[rel->op]);
-
 	if (rel->op == OP_IMPLICIT) {
 		switch (right->ops->type) {
 		case EXPR_RANGE:
@@ -901,6 +890,17 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 		}
 	}
 
+	if (!expr_is_constant(right))
+		return expr_binary_error(ctx, right, rel,
+					 "Right hand side of relational "
+					 "expression (%s) must be constant",
+					 expr_op_symbols[rel->op]);
+	if (expr_is_constant(left))
+		return expr_binary_error(ctx, left, right,
+					 "Relational expression (%s) has "
+					 "constant value",
+					 expr_op_symbols[rel->op]);
+
 	switch (rel->op) {
 	case OP_LOOKUP:
 		/* A literal set expression implicitly declares the set */
-- 
1.8.5.3

[PATCH 2/8] scanner: don't update location's line_offset for newlines

[Patrick McHardy]

When reset_pos() is invoked, YY_USER_ACTION() has already advanced the
line offset to the next line. This causes errors for unexpected newlines
to incorrectly show the following line when reading from files.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 src/scanner.l | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/scanner.l b/src/scanner.l
index 47ab1e2..11965cd 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -88,7 +88,6 @@ static void reset_pos(struct parser_state *state, struct location *loc)
 	state->indesc->line_offset	= state->indesc->token_offset;
 	state->indesc->lineno		+= 1;
 	state->indesc->column		= 1;
-	loc->line_offset		= state->indesc->line_offset;
 }
 
 #define YY_USER_ACTION {					\
-- 
1.8.5.3

[PATCH 3/8] scanner: update last_line in struct location

[Patrick McHardy]

Currently always has the value 0.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 src/scanner.l | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/scanner.l b/src/scanner.l
index 11965cd..e4cb398 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -70,6 +70,7 @@ static void update_pos(struct parser_state *state, struct location *loc,
 {
 	loc->indesc			= state->indesc;
 	loc->first_line			= state->indesc->lineno;
+	loc->last_line			= state->indesc->lineno;
 	loc->first_column		= state->indesc->column;
 	loc->last_column		= state->indesc->column + len - 1;
 	state->indesc->column		+= len;
-- 
1.8.5.3

[PATCH 4/8] erec: skip includes with INDESC_INTERNAL

[Patrick McHardy]

Don't display "In file included from internal:0:0-0:" for errors occuring
in a parsed file.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 src/erec.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/erec.c b/src/erec.c
index a157d2f..4930085 100644
--- a/src/erec.c
+++ b/src/erec.c
@@ -114,7 +114,9 @@ void erec_print(FILE *f, const struct error_record *erec)
 		if (indesc->location.indesc != NULL) {
 			const char *prefix = "In file included from";
 			iloc = &indesc->location;
-			for (tmp = iloc->indesc; tmp != NULL; tmp = iloc->indesc) {
+			for (tmp = iloc->indesc;
+			     tmp != NULL && tmp->type != INDESC_INTERNAL;
+			     tmp = iloc->indesc) {
 				fprintf(f, "%s %s:%u:%u-%u:\n", prefix,
 					tmp->name,
 					iloc->first_line, iloc->first_column,
-- 
1.8.5.3

[PATCH 5/8] parser: close scope when encountering an error in a table or chain block

[Patrick McHardy]

Close the scopes when destroying a table_block/chain_block. Also add
assertions to open_scope()/close_scope() to catch memory corruption
early.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 src/parser.y | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/parser.y b/src/parser.y
index 24f022a..fa33b57 100644
--- a/src/parser.y
+++ b/src/parser.y
@@ -52,12 +52,14 @@ static struct scope *current_scope(const struct parser_state *state)
 
 static void open_scope(struct parser_state *state, struct scope *scope)
 {
+	assert(state->scope < array_size(state->scopes) - 1);
 	scope_init(scope, current_scope(state));
 	state->scopes[++state->scope] = scope;
 }
 
 static void close_scope(struct parser_state *state)
 {
+	assert(state->scope > 0);
 	state->scope--;
 }
 
@@ -367,9 +369,9 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 %type <val>			handle_spec family_spec position_spec
 
 %type <table>			table_block_alloc table_block
-%destructor { table_free($$); }	table_block_alloc
+%destructor { close_scope(state); table_free($$); }	table_block_alloc
 %type <chain>			chain_block_alloc chain_block
-%destructor { chain_free($$); }	chain_block_alloc
+%destructor { close_scope(state); chain_free($$); }	chain_block_alloc
 %type <rule>			rule
 %destructor { rule_free($$); }	rule
 
-- 
1.8.5.3

[PATCH 6/8] parser: recover from errors in any block

[Patrick McHardy]

Move error recovery to the common_block definition to handle errors
in any block. Queue those errors and abort parsing once a threshold
is reached.

With this in place, we can continue parsing when errors occur and
show all of them to the user at once.

tests/error.1:3:8-8: Error: syntax error, unexpected '{', expecting string
filter {
       ^
tests/error.1:4:13-13: Error: syntax error, unexpected newline
filter input
            ^
tests/error.1:5:17-17: Error: syntax error, unexpected newline
filter input tcp
                ^
tests/error.1:6:23-23: Error: syntax error, unexpected newline
filter input tcp dport

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 include/nftables.h | 1 +
 include/parser.h   | 1 +
 src/main.c         | 3 ++-
 src/parser.y       | 7 ++++++-
 4 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/nftables.h b/include/nftables.h
index 5a00087..7f3968d 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -24,6 +24,7 @@ enum debug_level {
 
 #define INCLUDE_PATHS_MAX	16
 
+extern unsigned int max_errors;
 extern unsigned int numeric_output;
 extern unsigned int handle_output;
 extern unsigned int debug_level;
diff --git a/include/parser.h b/include/parser.h
index f5dd6f4..7a1c2db 100644
--- a/include/parser.h
+++ b/include/parser.h
@@ -19,6 +19,7 @@ struct parser_state {
 	unsigned int			indesc_idx;
 
 	struct list_head		*msgs;
+	unsigned int			nerrs;
 
 	struct scope			top_scope;
 	struct scope			*scopes[SCOPE_NEST_MAX];
diff --git a/src/main.c b/src/main.c
index 28ce1aa..2320a82 100644
--- a/src/main.c
+++ b/src/main.c
@@ -26,6 +26,7 @@
 #include <erec.h>
 #include <mnl.h>
 
+unsigned int max_errors = 10;
 unsigned int numeric_output;
 unsigned int handle_output;
 #ifdef DEBUG
@@ -219,7 +220,7 @@ int nft_run(void *scanner, struct parser_state *state, struct list_head *msgs)
 	int ret = 0;
 
 	ret = nft_parse(scanner, state);
-	if (ret != 0)
+	if (ret != 0 || state->nerrs > 0)
 		return -1;
 
 	memset(&ctx, 0, sizeof(ctx));
diff --git a/src/parser.y b/src/parser.y
index fa33b57..0dad036 100644
--- a/src/parser.y
+++ b/src/parser.y
@@ -518,6 +518,12 @@ common_block		:	INCLUDE		QUOTED_STRING	stmt_seperator
 				symbol_bind(current_scope(state), $2, $4);
 				xfree($2);
 			}
+			|	error		stmt_seperator
+			{
+				if (++state->nerrs == max_errors)
+					YYABORT;
+				yyerrok;
+			}
 			;
 
 line			:	common_block			{ $$ = NULL; }
@@ -542,7 +548,6 @@ line			:	common_block			{ $$ = NULL; }
 
 				YYACCEPT;
 			}
-			|	base_cmd	error		{ $$ = $1; }
 			;
 
 base_cmd		:	/* empty */	add_cmd		{ $$ = $1; }
-- 
1.8.5.3

[PATCH 7/8] parser: evaluate commands immediately after parsing

[Patrick McHardy]

We currently do parsing and evaluation in two seperate stages. This means
that if any error occurs during parsing, we won't evaluate the syntactical
correct commands and detect possible evaluation errors in them.

In order to improve error reporting, change this to evaluate every command
as soon as it is fully parsed.

With this in place, the ruleset can be fully validated and all errors
reported in one step:

tests/error.1:6:23-23: Error: syntax error, unexpected newline
filter input tcp dport
                      ^
tests/error.1:7:24-26: Error: datatype mismatch, expected internet network service, expression has type Internet protocol
filter input tcp dport tcp
             ~~~~~~~~~ ^^^
tests/error.1:8:24-32: Error: Right hand side of relational expression (==) must be constant
filter input tcp dport tcp dport
             ~~~~~~~~~~^^^^^^^^^

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 include/parser.h |  1 +
 include/rule.h   |  2 +-
 src/evaluate.c   | 13 +------------
 src/main.c       |  8 +-------
 src/parser.y     | 14 ++++++++++++--
 5 files changed, 16 insertions(+), 22 deletions(-)

diff --git a/include/parser.h b/include/parser.h
index 7a1c2db..92beab2 100644
--- a/include/parser.h
+++ b/include/parser.h
@@ -26,6 +26,7 @@ struct parser_state {
 	unsigned int			scope;
 
 	struct list_head		cmds;
+	struct eval_ctx			ectx;
 };
 
 extern void parser_init(struct parser_state *state, struct list_head *msgs);
diff --git a/include/rule.h b/include/rule.h
index 47dd6ab..e06444e 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -303,7 +303,7 @@ struct eval_ctx {
 	struct proto_ctx	pctx;
 };
 
-extern int evaluate(struct eval_ctx *ctx, struct list_head *commands);
+extern int cmd_evaluate(struct eval_ctx *ctx, struct cmd *cmd);
 
 extern struct error_record *rule_postprocess(struct rule *rule);
 
diff --git a/src/evaluate.c b/src/evaluate.c
index a01d2a5..8e51a63 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1384,7 +1384,7 @@ static int cmd_evaluate_delete(struct eval_ctx *ctx, struct cmd *cmd)
 	}
 }
 
-static int cmd_evaluate(struct eval_ctx *ctx, struct cmd *cmd)
+int cmd_evaluate(struct eval_ctx *ctx, struct cmd *cmd)
 {
 #ifdef DEBUG
 	if (debug_level & DEBUG_EVALUATION) {
@@ -1411,14 +1411,3 @@ static int cmd_evaluate(struct eval_ctx *ctx, struct cmd *cmd)
 		BUG("invalid command operation %u\n", cmd->op);
 	};
 }
-
-int evaluate(struct eval_ctx *ctx, struct list_head *commands)
-{
-	struct cmd *cmd;
-
-	list_for_each_entry(cmd, commands, list) {
-		if (cmd_evaluate(ctx, cmd) < 0)
-			return -1;
-	}
-	return 0;
-}
diff --git a/src/main.c b/src/main.c
index 2320a82..9d50577 100644
--- a/src/main.c
+++ b/src/main.c
@@ -216,18 +216,12 @@ out:
 
 int nft_run(void *scanner, struct parser_state *state, struct list_head *msgs)
 {
-	struct eval_ctx ctx;
-	int ret = 0;
+	int ret;
 
 	ret = nft_parse(scanner, state);
 	if (ret != 0 || state->nerrs > 0)
 		return -1;
 
-	memset(&ctx, 0, sizeof(ctx));
-	ctx.msgs = msgs;
-	if (evaluate(&ctx, &state->cmds) < 0)
-		return -1;
-
 	return nft_netlink(state, msgs);
 }
 
diff --git a/src/parser.y b/src/parser.y
index 0dad036..cc0aed6 100644
--- a/src/parser.y
+++ b/src/parser.y
@@ -37,6 +37,7 @@ void parser_init(struct parser_state *state, struct list_head *msgs)
 	init_list_head(&state->top_scope.symbols);
 	state->msgs = msgs;
 	state->scopes[0] = scope_init(&state->top_scope, NULL);
+	state->ectx.msgs = msgs;
 }
 
 static void yyerror(struct location *loc, void *scanner,
@@ -492,7 +493,11 @@ input			:	/* empty */
 			{
 				if ($2 != NULL) {
 					$2->location = @2;
-					list_add_tail(&$2->list, &state->cmds);
+					if (cmd_evaluate(&state->ectx, $2) < 0) {
+						if (++state->nerrs == max_errors)
+							YYABORT;
+					} else
+						list_add_tail(&$2->list, &state->cmds);
 				}
 			}
 			;
@@ -542,7 +547,12 @@ line			:	common_block			{ $$ = NULL; }
 				 */
 				if ($1 != NULL) {
 					$1->location = @1;
-					list_add_tail(&$1->list, &state->cmds);
+
+					if (cmd_evaluate(&state->ectx, $1) < 0) {
+						if (++state->nerrs == max_errors)
+							YYABORT;
+					} else
+						list_add_tail(&$1->list, &state->cmds);
 				}
 				$$ = NULL;
 
-- 
1.8.5.3

[PATCH 8/8] tests: add two tests for error reporting

[Patrick McHardy]

Mixed syntactical and non-syntactical errors in individual commands and
blocks.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 tests/error.1 |  9 +++++++++
 tests/error.2 | 18 ++++++++++++++++++
 2 files changed, 27 insertions(+)
 create mode 100644 tests/error.1
 create mode 100644 tests/error.2

diff --git a/tests/error.1 b/tests/error.1
new file mode 100644
index 0000000..bc3bf16
--- /dev/null
+++ b/tests/error.1
@@ -0,0 +1,9 @@
+#! nft -f
+
+# mixed syntactical and non-syntactical errors
+filter {
+filter input
+filter input tcp
+filter input tcp dport
+filter input tcp dport tcp
+filter input tcp dport tcp dport
diff --git a/tests/error.2 b/tests/error.2
new file mode 100644
index 0000000..744a63d
--- /dev/null
+++ b/tests/error.2
@@ -0,0 +1,18 @@
+#! nft -f
+
+# mixed syntactical and non-syntactical errors in blocks
+table filter {
+	# missing identifier
+	chain
+
+	# missing chain block
+	chain output
+
+	chain output {
+		tcp
+		tcp dport
+		tcp dport tcp
+		tcp dport tcp dport
+		tcp dport ssh
+	}
+}
-- 
1.8.5.3