Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH nft include v2 0/7] Improve include behaviour
@ 2020-02-10 10:17 Laurent Fasnacht
  2020-02-10 10:17 ` [PATCH nft include v2 1/7] tests: shell: add test for glob includes Laurent Fasnacht
                   ` (6 more replies)
  0 siblings, 7 replies; 18+ messages in thread
From: Laurent Fasnacht @ 2020-02-10 10:17 UTC (permalink / raw)
  To: netfilter-devel

Hello,

Following Pablo's comments (thanks!), here's a new version of the patch series that improves include behavior. It fixes bug #1243, and also errors with bug reporting.

Changes from v1:
- include the test in this patch series
- split in more commits, to improve reviewability
- clean code state after each commit
- fixes an additional bug found while refactoring the patch, where the include chain wasn't displayed correctly while printing errors.

Overall, definitely a lot of improvement, was definitely worth spending some time on it.

Cheers,
Laurent




^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH nft include v2 1/7] tests: shell: add test for glob includes
  2020-02-10 10:17 [PATCH nft include v2 0/7] Improve include behaviour Laurent Fasnacht
@ 2020-02-10 10:17 ` Laurent Fasnacht
  2020-02-12 20:45   ` Pablo Neira Ayuso
  2020-02-10 10:17 ` [PATCH nft include v2 2/7] scanner: move the file descriptor to be in the input_descriptor structure Laurent Fasnacht
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 18+ messages in thread
From: Laurent Fasnacht @ 2020-02-10 10:17 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Laurent Fasnacht

Including more than MAX_INCLUDE_DEPTH file in one statement should succeed.

This reproduces bug #1243.

Signed-off-by: Laurent Fasnacht <fasnacht@protonmail.ch>
---
 .../include/0017glob_more_than_maxdepth_1     | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100755 tests/shell/testcases/include/0017glob_more_than_maxdepth_1

diff --git a/tests/shell/testcases/include/0017glob_more_than_maxdepth_1 b/tests/shell/testcases/include/0017glob_more_than_maxdepth_1
new file mode 100755
index 00000000..6499bcc8
--- /dev/null
+++ b/tests/shell/testcases/include/0017glob_more_than_maxdepth_1
@@ -0,0 +1,39 @@
+#!/bin/bash
+
+set -e
+
+tmpfile=$(mktemp)
+if [ ! -w $tmpfile ] ; then
+        echo "Failed to create tmp file" >&2
+        exit 0
+fi
+
+tmpdir1=$(mktemp -d)
+if [ ! -d $tmpdir1 ] ; then
+        echo "Failed to create tmp directory" >&2
+        exit 0
+fi
+
+tmpfiles=""
+for i in `seq -w 1 32`; do
+        tmpfile2=$(mktemp -p $tmpdir1)
+        if [ ! -w $tmpfile2 ] ; then
+                echo "Failed to create tmp file" >&2
+                exit 0
+        fi
+        tmpfiles="$tmpfiles $tmpfile2"
+done
+
+trap "rm -rf $tmpfile $tmpfiles && rmdir $tmpdir1" EXIT # cleanup if aborted
+
+RULESET=" \
+include \"$tmpdir1/*\"
+"
+
+echo "$RULESET" > $tmpfile
+
+$NFT -f $tmpfile
+if [ $? -ne 0 ] ; then
+	echo "E: unable to load good ruleset" >&2
+	exit 1
+fi
-- 
2.20.1



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH nft include v2 2/7] scanner: move the file descriptor to be in the input_descriptor structure
  2020-02-10 10:17 [PATCH nft include v2 0/7] Improve include behaviour Laurent Fasnacht
  2020-02-10 10:17 ` [PATCH nft include v2 1/7] tests: shell: add test for glob includes Laurent Fasnacht
@ 2020-02-10 10:17 ` Laurent Fasnacht
  2020-02-10 22:46   ` Pablo Neira Ayuso
  2020-02-10 10:17 ` [PATCH nft include v2 3/7] scanner: move indesc list append in scanner_push_indesc Laurent Fasnacht
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 18+ messages in thread
From: Laurent Fasnacht @ 2020-02-10 10:17 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Laurent Fasnacht

This prevents a static allocation of file descriptors array, thus allows
more flexibility.

Signed-off-by: Laurent Fasnacht <fasnacht@protonmail.ch>
---
 include/nftables.h |  3 ++-
 src/scanner.l      | 17 ++++++++---------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/nftables.h b/include/nftables.h
index 90d33196..ca0fbcaf 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -122,7 +122,6 @@ struct nft_ctx {
 	void			*scanner;
 	struct scope		*top_scope;
 	void			*json_root;
-	FILE			*f[MAX_INCLUDE_DEPTH];
 };
 
 enum nftables_exit_codes {
@@ -176,6 +175,7 @@ enum input_descriptor_types {
  * struct input_descriptor
  *
  * @location:		location, used for include statements
+ * @f:          file descriptor
  * @type:		input descriptor type
  * @name:		name describing the input
  * @union:		buffer or file descriptor, depending on type
@@ -186,6 +186,7 @@ enum input_descriptor_types {
  */
 struct input_descriptor {
 	struct list_head		list;
+	FILE*                   f;
 	struct location			location;
 	enum input_descriptor_types	type;
 	const char			*name;
diff --git a/src/scanner.l b/src/scanner.l
index 99ee8355..2016acd5 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -691,13 +691,13 @@ static void scanner_pop_buffer(yyscan_t scanner)
 }
 
 static void scanner_push_file(struct nft_ctx *nft, void *scanner,
-			      const char *filename, const struct location *loc)
+				  FILE *f, const char *filename, const struct location *loc)
 {
 	struct parser_state *state = yyget_extra(scanner);
 	struct input_descriptor *indesc;
 	YY_BUFFER_STATE b;
 
-	b = yy_create_buffer(nft->f[state->indesc_idx], YY_BUF_SIZE, scanner);
+	b = yy_create_buffer(f, YY_BUF_SIZE, scanner);
 	yypush_buffer_state(b, scanner);
 
 	indesc = xzalloc(sizeof(struct input_descriptor));
@@ -706,6 +706,7 @@ static void scanner_push_file(struct nft_ctx *nft, void *scanner,
 		indesc->location = *loc;
 	indesc->type	= INDESC_FILE;
 	indesc->name	= xstrdup(filename);
+	indesc->f = f;
 	init_pos(indesc);
 
 	scanner_push_indesc(state, indesc);
@@ -731,8 +732,7 @@ static int include_file(struct nft_ctx *nft, void *scanner,
 			     filename, strerror(errno));
 		goto err;
 	}
-	nft->f[state->indesc_idx] = f;
-	scanner_push_file(nft, scanner, filename, loc);
+	scanner_push_file(nft, scanner, f, filename, loc);
 	return 0;
 err:
 	erec_queue(erec, state->msgs);
@@ -944,6 +944,10 @@ static void input_descriptor_list_destroy(struct parser_state *state)
 	struct input_descriptor *indesc, *next;
 
 	list_for_each_entry_safe(indesc, next, &state->indesc_list, list) {
+		if (indesc->f) {
+			fclose(indesc->f);
+			indesc->f = NULL;
+		}
 		list_del(&indesc->list);
 		input_descriptor_destroy(indesc);
 	}
@@ -955,11 +959,6 @@ void scanner_destroy(struct nft_ctx *nft)
 
 	do {
 		yypop_buffer_state(nft->scanner);
-
-		if (nft->f[state->indesc_idx]) {
-			fclose(nft->f[state->indesc_idx]);
-			nft->f[state->indesc_idx] = NULL;
-		}
 	} while (state->indesc_idx--);
 
 	input_descriptor_list_destroy(state);
-- 
2.20.1



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH nft include v2 3/7] scanner: move indesc list append in scanner_push_indesc
  2020-02-10 10:17 [PATCH nft include v2 0/7] Improve include behaviour Laurent Fasnacht
  2020-02-10 10:17 ` [PATCH nft include v2 1/7] tests: shell: add test for glob includes Laurent Fasnacht
  2020-02-10 10:17 ` [PATCH nft include v2 2/7] scanner: move the file descriptor to be in the input_descriptor structure Laurent Fasnacht
@ 2020-02-10 10:17 ` Laurent Fasnacht
  2020-02-10 22:46   ` Pablo Neira Ayuso
  2020-02-10 10:17 ` [PATCH nft include v2 4/7] scanner: remove parser_state->indescs static array Laurent Fasnacht
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 18+ messages in thread
From: Laurent Fasnacht @ 2020-02-10 10:17 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Laurent Fasnacht

Having a single point makes refactoring easier.

Signed-off-by: Laurent Fasnacht <fasnacht@protonmail.ch>
---
 src/scanner.l | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/src/scanner.l b/src/scanner.l
index 2016acd5..37b01639 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -670,6 +670,7 @@ static void scanner_push_indesc(struct parser_state *state,
 {
 	state->indescs[state->indesc_idx] = indesc;
 	state->indesc = state->indescs[state->indesc_idx++];
+	list_add_tail(&indesc->list, &state->indesc_list);
 }
 
 static void scanner_pop_indesc(struct parser_state *state)
@@ -710,7 +711,6 @@ static void scanner_push_file(struct nft_ctx *nft, void *scanner,
 	init_pos(indesc);
 
 	scanner_push_indesc(state, indesc);
-	list_add_tail(&indesc->list, &state->indesc_list);
 }
 
 static int include_file(struct nft_ctx *nft, void *scanner,
@@ -907,15 +907,14 @@ void scanner_push_buffer(void *scanner, const struct input_descriptor *indesc,
 {
 	struct parser_state *state = yyget_extra(scanner);
 	YY_BUFFER_STATE b;
+	struct input_descriptor *new_indesc;
 
-	state->indesc = xzalloc(sizeof(struct input_descriptor));
-	state->indescs[state->indesc_idx] = state->indesc;
-	state->indesc_idx++;
+	new_indesc = xzalloc(sizeof(struct input_descriptor));
 
-	memcpy(state->indesc, indesc, sizeof(*state->indesc));
-	state->indesc->data = buffer;
-	state->indesc->name = NULL;
-	list_add_tail(&state->indesc->list, &state->indesc_list);
+	memcpy(new_indesc, indesc, sizeof(*new_indesc));
+	new_indesc->data = buffer;
+	new_indesc->name = NULL;
+	scanner_push_indesc(state, new_indesc);
 
 	b = yy_scan_string(buffer, scanner);
 	assert(b != NULL);
-- 
2.20.1



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH nft include v2 4/7] scanner: remove parser_state->indescs static array
  2020-02-10 10:17 [PATCH nft include v2 0/7] Improve include behaviour Laurent Fasnacht
                   ` (2 preceding siblings ...)
  2020-02-10 10:17 ` [PATCH nft include v2 3/7] scanner: move indesc list append in scanner_push_indesc Laurent Fasnacht
@ 2020-02-10 10:17 ` Laurent Fasnacht
  2020-02-10 23:32   ` Pablo Neira Ayuso
  2020-02-12 20:45   ` Pablo Neira Ayuso
  2020-02-10 10:17 ` [PATCH nft include v2 5/7] scanner: correctly compute include depth Laurent Fasnacht
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 18+ messages in thread
From: Laurent Fasnacht @ 2020-02-10 10:17 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Laurent Fasnacht

This static array is redundant wth the indesc_list structure, but
is less flexible.

Signed-off-by: Laurent Fasnacht <fasnacht@protonmail.ch>
---
 include/parser.h |  1 -
 src/scanner.l    | 12 ++++++------
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/include/parser.h b/include/parser.h
index 949284d9..66db92d8 100644
--- a/include/parser.h
+++ b/include/parser.h
@@ -15,7 +15,6 @@
 
 struct parser_state {
 	struct input_descriptor		*indesc;
-	struct input_descriptor		*indescs[MAX_INCLUDE_DEPTH];
 	unsigned int			indesc_idx;
 	struct list_head		indesc_list;
 
diff --git a/src/scanner.l b/src/scanner.l
index 37b01639..8397846b 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -668,19 +668,19 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 static void scanner_push_indesc(struct parser_state *state,
 				struct input_descriptor *indesc)
 {
-	state->indescs[state->indesc_idx] = indesc;
-	state->indesc = state->indescs[state->indesc_idx++];
 	list_add_tail(&indesc->list, &state->indesc_list);
+	state->indesc = indesc;
+	state->indesc_idx++;
 }
 
 static void scanner_pop_indesc(struct parser_state *state)
 {
 	state->indesc_idx--;
-
-	if (state->indesc_idx > 0)
-		state->indesc = state->indescs[state->indesc_idx - 1];
-	else
+	if (!list_empty(&state->indesc_list)) {
+		state->indesc = list_entry(state->indesc->list.prev, struct input_descriptor, list);
+	} else {
 		state->indesc = NULL;
+	}
 }
 
 static void scanner_pop_buffer(yyscan_t scanner)
-- 
2.20.1



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH nft include v2 5/7] scanner: correctly compute include depth
  2020-02-10 10:17 [PATCH nft include v2 0/7] Improve include behaviour Laurent Fasnacht
                   ` (3 preceding siblings ...)
  2020-02-10 10:17 ` [PATCH nft include v2 4/7] scanner: remove parser_state->indescs static array Laurent Fasnacht
@ 2020-02-10 10:17 ` Laurent Fasnacht
  2020-02-10 10:17 ` [PATCH nft include v2 6/7] scanner: fix indesc_list stack to be in the correct order Laurent Fasnacht
  2020-02-10 10:17 ` [PATCH nft include v2 7/7] scanner: remove parser_state->indesc_idx Laurent Fasnacht
  6 siblings, 0 replies; 18+ messages in thread
From: Laurent Fasnacht @ 2020-02-10 10:17 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Laurent Fasnacht

Inclusion depth was computed incorrectly for glob includes.

Signed-off-by: Laurent Fasnacht <fasnacht@protonmail.ch>
---
 include/nftables.h |  2 ++
 src/scanner.l      | 20 ++++++++++++++------
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/include/nftables.h b/include/nftables.h
index ca0fbcaf..1d423738 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -176,6 +176,7 @@ enum input_descriptor_types {
  *
  * @location:		location, used for include statements
  * @f:          file descriptor
+ * @depth:      include depth of the descriptor
  * @type:		input descriptor type
  * @name:		name describing the input
  * @union:		buffer or file descriptor, depending on type
@@ -187,6 +188,7 @@ enum input_descriptor_types {
 struct input_descriptor {
 	struct list_head		list;
 	FILE*                   f;
+	unsigned int            depth;
 	struct location			location;
 	enum input_descriptor_types	type;
 	const char			*name;
diff --git a/src/scanner.l b/src/scanner.l
index 8397846b..7f40c5c1 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -692,7 +692,8 @@ static void scanner_pop_buffer(yyscan_t scanner)
 }
 
 static void scanner_push_file(struct nft_ctx *nft, void *scanner,
-				  FILE *f, const char *filename, const struct location *loc)
+				  FILE *f, const char *filename, const struct location *loc,
+				  const struct input_descriptor *parent_indesc)
 {
 	struct parser_state *state = yyget_extra(scanner);
 	struct input_descriptor *indesc;
@@ -707,6 +708,11 @@ static void scanner_push_file(struct nft_ctx *nft, void *scanner,
 		indesc->location = *loc;
 	indesc->type	= INDESC_FILE;
 	indesc->name	= xstrdup(filename);
+	if (!parent_indesc) {
+		indesc->depth = 1;
+	} else {
+		indesc->depth = parent_indesc->depth + 1;
+	}
 	indesc->f = f;
 	init_pos(indesc);
 
@@ -714,13 +720,14 @@ static void scanner_push_file(struct nft_ctx *nft, void *scanner,
 }
 
 static int include_file(struct nft_ctx *nft, void *scanner,
-			const char *filename, const struct location *loc)
+			const char *filename, const struct location *loc,
+			const struct input_descriptor *parent_indesc)
 {
 	struct parser_state *state = yyget_extra(scanner);
 	struct error_record *erec;
 	FILE *f;
 
-	if (state->indesc_idx == MAX_INCLUDE_DEPTH) {
+	if (parent_indesc && parent_indesc->depth == MAX_INCLUDE_DEPTH) {
 		erec = error(loc, "Include nested too deeply, max %u levels",
 			     MAX_INCLUDE_DEPTH);
 		goto err;
@@ -732,7 +739,7 @@ static int include_file(struct nft_ctx *nft, void *scanner,
 			     filename, strerror(errno));
 		goto err;
 	}
-	scanner_push_file(nft, scanner, f, filename, loc);
+	scanner_push_file(nft, scanner, f, filename, loc, parent_indesc);
 	return 0;
 err:
 	erec_queue(erec, state->msgs);
@@ -743,6 +750,7 @@ static int include_glob(struct nft_ctx *nft, void *scanner, const char *pattern,
 			const struct location *loc)
 {
 	struct parser_state *state = yyget_extra(scanner);
+	struct input_descriptor *indesc = state->indesc;
 	struct error_record *erec = NULL;
 	bool wildcard = false;
 	glob_t glob_data;
@@ -803,7 +811,7 @@ static int include_glob(struct nft_ctx *nft, void *scanner, const char *pattern,
 			if (len == 0 || path[len - 1] == '/')
 				continue;
 
-			ret = include_file(nft, scanner, path, loc);
+			ret = include_file(nft, scanner, path, loc, indesc);
 			if (ret != 0)
 				goto err;
 		}
@@ -840,7 +848,7 @@ err:
 int scanner_read_file(struct nft_ctx *nft, const char *filename,
 		      const struct location *loc)
 {
-	return include_file(nft, nft->scanner, filename, loc);
+	return include_file(nft, nft->scanner, filename, loc, NULL);
 }
 
 static bool search_in_include_path(const char *filename)
-- 
2.20.1



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH nft include v2 6/7] scanner: fix indesc_list stack to be in the correct order
  2020-02-10 10:17 [PATCH nft include v2 0/7] Improve include behaviour Laurent Fasnacht
                   ` (4 preceding siblings ...)
  2020-02-10 10:17 ` [PATCH nft include v2 5/7] scanner: correctly compute include depth Laurent Fasnacht
@ 2020-02-10 10:17 ` Laurent Fasnacht
  2020-02-10 22:33   ` Pablo Neira Ayuso
  2020-02-10 10:17 ` [PATCH nft include v2 7/7] scanner: remove parser_state->indesc_idx Laurent Fasnacht
  6 siblings, 1 reply; 18+ messages in thread
From: Laurent Fasnacht @ 2020-02-10 10:17 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Laurent Fasnacht

This fixes the location displayed in error messages.

Signed-off-by: Laurent Fasnacht <fasnacht@protonmail.ch>
---
 src/scanner.l | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/scanner.l b/src/scanner.l
index 7f40c5c1..8407a2a1 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -668,7 +668,11 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 static void scanner_push_indesc(struct parser_state *state,
 				struct input_descriptor *indesc)
 {
-	list_add_tail(&indesc->list, &state->indesc_list);
+	if (!state->indesc) {
+		list_add_tail(&indesc->list, &state->indesc_list);
+	} else {
+		list_add(&indesc->list, &state->indesc->list);
+	}
 	state->indesc = indesc;
 	state->indesc_idx++;
 }
-- 
2.20.1



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH nft include v2 7/7] scanner: remove parser_state->indesc_idx
  2020-02-10 10:17 [PATCH nft include v2 0/7] Improve include behaviour Laurent Fasnacht
                   ` (5 preceding siblings ...)
  2020-02-10 10:17 ` [PATCH nft include v2 6/7] scanner: fix indesc_list stack to be in the correct order Laurent Fasnacht
@ 2020-02-10 10:17 ` Laurent Fasnacht
  2020-02-10 22:31   ` Pablo Neira Ayuso
  6 siblings, 1 reply; 18+ messages in thread
From: Laurent Fasnacht @ 2020-02-10 10:17 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Laurent Fasnacht

Now that we have a proper stack implementation, we don't need an
additional counter for the number of buffer state pushed.

Signed-off-by: Laurent Fasnacht <fasnacht@protonmail.ch>
---
 include/parser.h | 1 -
 src/scanner.l    | 6 ------
 2 files changed, 7 deletions(-)

diff --git a/include/parser.h b/include/parser.h
index 66db92d8..636d1c88 100644
--- a/include/parser.h
+++ b/include/parser.h
@@ -15,7 +15,6 @@
 
 struct parser_state {
 	struct input_descriptor		*indesc;
-	unsigned int			indesc_idx;
 	struct list_head		indesc_list;
 
 	struct list_head		*msgs;
diff --git a/src/scanner.l b/src/scanner.l
index 8407a2a1..39e7ae0f 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -674,12 +674,10 @@ static void scanner_push_indesc(struct parser_state *state,
 		list_add(&indesc->list, &state->indesc->list);
 	}
 	state->indesc = indesc;
-	state->indesc_idx++;
 }
 
 static void scanner_pop_indesc(struct parser_state *state)
 {
-	state->indesc_idx--;
 	if (!list_empty(&state->indesc_list)) {
 		state->indesc = list_entry(state->indesc->list.prev, struct input_descriptor, list);
 	} else {
@@ -968,10 +966,6 @@ void scanner_destroy(struct nft_ctx *nft)
 {
 	struct parser_state *state = yyget_extra(nft->scanner);
 
-	do {
-		yypop_buffer_state(nft->scanner);
-	} while (state->indesc_idx--);
-
 	input_descriptor_list_destroy(state);
 	yylex_destroy(nft->scanner);
 }
-- 
2.20.1



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH nft include v2 7/7] scanner: remove parser_state->indesc_idx
  2020-02-10 10:17 ` [PATCH nft include v2 7/7] scanner: remove parser_state->indesc_idx Laurent Fasnacht
@ 2020-02-10 22:31   ` Pablo Neira Ayuso
  2020-02-11  5:04     ` Laurent Fasnacht
  0 siblings, 1 reply; 18+ messages in thread
From: Pablo Neira Ayuso @ 2020-02-10 22:31 UTC (permalink / raw)
  To: Laurent Fasnacht; +Cc: netfilter-devel

On Mon, Feb 10, 2020 at 10:17:28AM +0000, Laurent Fasnacht wrote:
> Now that we have a proper stack implementation, we don't need an
> additional counter for the number of buffer state pushed.
> 
> Signed-off-by: Laurent Fasnacht <fasnacht@protonmail.ch>
> ---
>  include/parser.h | 1 -
>  src/scanner.l    | 6 ------
>  2 files changed, 7 deletions(-)
> 
> diff --git a/include/parser.h b/include/parser.h
> index 66db92d8..636d1c88 100644
> --- a/include/parser.h
> +++ b/include/parser.h
> @@ -15,7 +15,6 @@
>  
>  struct parser_state {
>  	struct input_descriptor		*indesc;
> -	unsigned int			indesc_idx;
>  	struct list_head		indesc_list;
>  
>  	struct list_head		*msgs;
> diff --git a/src/scanner.l b/src/scanner.l
> index 8407a2a1..39e7ae0f 100644
> --- a/src/scanner.l
> +++ b/src/scanner.l
> @@ -674,12 +674,10 @@ static void scanner_push_indesc(struct parser_state *state,
>  		list_add(&indesc->list, &state->indesc->list);
>  	}
>  	state->indesc = indesc;
> -	state->indesc_idx++;
>  }
>  
>  static void scanner_pop_indesc(struct parser_state *state)
>  {
> -	state->indesc_idx--;
>  	if (!list_empty(&state->indesc_list)) {
>  		state->indesc = list_entry(state->indesc->list.prev, struct input_descriptor, list);
>  	} else {
> @@ -968,10 +966,6 @@ void scanner_destroy(struct nft_ctx *nft)
>  {
>  	struct parser_state *state = yyget_extra(nft->scanner);
>  
> -	do {
> -		yypop_buffer_state(nft->scanner);

nft_pop_buffer_state calls free().

Are you sure this can be removed without incurring in memleaks?

Thanks.

> -	} while (state->indesc_idx--);
> -
>  	input_descriptor_list_destroy(state);
>  	yylex_destroy(nft->scanner);
>  }
> -- 
> 2.20.1
> 
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH nft include v2 6/7] scanner: fix indesc_list stack to be in the correct order
  2020-02-10 10:17 ` [PATCH nft include v2 6/7] scanner: fix indesc_list stack to be in the correct order Laurent Fasnacht
@ 2020-02-10 22:33   ` Pablo Neira Ayuso
  2020-02-11  4:42     ` Laurent Fasnacht
  0 siblings, 1 reply; 18+ messages in thread
From: Pablo Neira Ayuso @ 2020-02-10 22:33 UTC (permalink / raw)
  To: Laurent Fasnacht; +Cc: netfilter-devel

On Mon, Feb 10, 2020 at 10:17:27AM +0000, Laurent Fasnacht wrote:
> This fixes the location displayed in error messages.
> 
> Signed-off-by: Laurent Fasnacht <fasnacht@protonmail.ch>
> ---
>  src/scanner.l | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/scanner.l b/src/scanner.l
> index 7f40c5c1..8407a2a1 100644
> --- a/src/scanner.l
> +++ b/src/scanner.l
> @@ -668,7 +668,11 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
>  static void scanner_push_indesc(struct parser_state *state,
>  				struct input_descriptor *indesc)
>  {
> -	list_add_tail(&indesc->list, &state->indesc_list);
> +	if (!state->indesc) {
> +		list_add_tail(&indesc->list, &state->indesc_list);
> +	} else {
> +		list_add(&indesc->list, &state->indesc->list);
> +	}

Probably belongs to patch 4/7 ?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH nft include v2 2/7] scanner: move the file descriptor to be in the input_descriptor structure
  2020-02-10 10:17 ` [PATCH nft include v2 2/7] scanner: move the file descriptor to be in the input_descriptor structure Laurent Fasnacht
@ 2020-02-10 22:46   ` Pablo Neira Ayuso
  0 siblings, 0 replies; 18+ messages in thread
From: Pablo Neira Ayuso @ 2020-02-10 22:46 UTC (permalink / raw)
  To: Laurent Fasnacht; +Cc: netfilter-devel

On Mon, Feb 10, 2020 at 10:17:21AM +0000, Laurent Fasnacht wrote:
> This prevents a static allocation of file descriptors array, thus allows
> more flexibility.

This one is applied, thanks.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH nft include v2 3/7] scanner: move indesc list append in scanner_push_indesc
  2020-02-10 10:17 ` [PATCH nft include v2 3/7] scanner: move indesc list append in scanner_push_indesc Laurent Fasnacht
@ 2020-02-10 22:46   ` Pablo Neira Ayuso
  0 siblings, 0 replies; 18+ messages in thread
From: Pablo Neira Ayuso @ 2020-02-10 22:46 UTC (permalink / raw)
  To: Laurent Fasnacht; +Cc: netfilter-devel

On Mon, Feb 10, 2020 at 10:17:22AM +0000, Laurent Fasnacht wrote:
> Having a single point makes refactoring easier.

Also applied this one. Thanks.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH nft include v2 4/7] scanner: remove parser_state->indescs static array
  2020-02-10 10:17 ` [PATCH nft include v2 4/7] scanner: remove parser_state->indescs static array Laurent Fasnacht
@ 2020-02-10 23:32   ` Pablo Neira Ayuso
  2020-02-11  4:36     ` Laurent Fasnacht
  2020-02-12 20:45   ` Pablo Neira Ayuso
  1 sibling, 1 reply; 18+ messages in thread
From: Pablo Neira Ayuso @ 2020-02-10 23:32 UTC (permalink / raw)
  To: Laurent Fasnacht; +Cc: netfilter-devel

On Mon, Feb 10, 2020 at 10:17:24AM +0000, Laurent Fasnacht wrote:
> This static array is redundant wth the indesc_list structure, but
> is less flexible.

Skipping this patch and taking 5/7, I get a crash.

Sorry, I'm trying to understand if there is an easy fix for this,
without simplifying things. I mean, first fix things, then move on and
improve everything.

The removal of the array stack looks like mandatory dependency to fix
this?

Please, help me understand. Thanks.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH nft include v2 4/7] scanner: remove parser_state->indescs static array
  2020-02-10 23:32   ` Pablo Neira Ayuso
@ 2020-02-11  4:36     ` Laurent Fasnacht
  0 siblings, 0 replies; 18+ messages in thread
From: Laurent Fasnacht @ 2020-02-11  4:36 UTC (permalink / raw)
  To: netfilter-devel; +Cc: pablo

On Tuesday, February 11, 2020 12:32 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:

> On Mon, Feb 10, 2020 at 10:17:24AM +0000, Laurent Fasnacht wrote:
>
> > This static array is redundant wth the indesc_list structure, but
> > is less flexible.
>
> Skipping this patch and taking 5/7, I get a crash.

Probably was out of bounds in state->indescs[MAX_INCLUDE_DEPTH].

> Sorry, I'm trying to understand if there is an easy fix for this,
> without simplifying things. I mean, first fix things, then move on and
> improve everything.

In my opinion, there isn't, unfortunately. That's why it took me quite some time to fix. Let me explain:

The data structure the original code is trying to achieve is a stack of input descriptors, where one entry in that stack is one level of include. This stack is implemented using the indesc_idx variable and the indescs and fd array.

Unfortunately, there is an issue with that design for glob includes: glob includes add all the discovered files to that stack at once (in reverse alphabetic order so the parsing happens in alphabetic order). The bound check for the static array incorrectly leads to the error message that the maximum inclusion depth is reached. However, removing that check alone will result in out of bound access of both fd and indescs arrays)

So basically, in order to apply patch 5/7, you need 2/7 and 4/7 (and 3/7, which is minor refactoring) to get rid the static arrays.

As an example, the test supplied in patch 1/7 only has one level of inclusion, but adds all the files to the stack, so the static arrays will overflow.

As a side note, the stack implementation is incorrect, both in the original code and after patch 5/7 is applied (since it's a minimal patch, as you previously ask). The stack implementation is fixed in patch 6/7. That specific patch is however unrelated to the include depth problem. I believe however that it's the correct fix of bug #1383, and should be applied in order to have a sane data structure.

Does that help?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH nft include v2 6/7] scanner: fix indesc_list stack to be in the correct order
  2020-02-10 22:33   ` Pablo Neira Ayuso
@ 2020-02-11  4:42     ` Laurent Fasnacht
  0 siblings, 0 replies; 18+ messages in thread
From: Laurent Fasnacht @ 2020-02-11  4:42 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, February 10, 2020 11:33 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:

> On Mon, Feb 10, 2020 at 10:17:27AM +0000, Laurent Fasnacht wrote:
>
> > This fixes the location displayed in error messages.
> >
> > Signed-off-by: Laurent Fasnacht fasnacht@protonmail.ch
> >
> > -------------------------------------------------------
> >
> > src/scanner.l | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> > diff --git a/src/scanner.l b/src/scanner.l
> > index 7f40c5c1..8407a2a1 100644
> > --- a/src/scanner.l
> > +++ b/src/scanner.l
> > @@ -668,7 +668,11 @@ addrstring ({macaddr}|{ip4addr}|{ip6addr})
> > static void scanner_push_indesc(struct parser_state *state,
> > struct input_descriptor *indesc)
> > {
> >
> > -   list_add_tail(&indesc->list, &state->indesc_list);
> >
> > -   if (!state->indesc) {
> > -       list_add_tail(&indesc->list, &state->indesc_list);
> >
> >
> > -   } else {
> > -       list_add(&indesc->list, &state->indesc->list);
> >
> >
> > -   }
>
> Probably belongs to patch 4/7 ?

It doesn't. Patch 4/7 is about getting rid of the static arrays in order to apply the fix in patch 5/7, but doesn't modify the data structure itself.

This specific patch modifies the state->indesc_list data structure to fix the stack implementation, in order to provide the correct error messages (in file included from...), and is not required to fix the inclusion bug. It is however to ensure that we implement a correct stack.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH nft include v2 7/7] scanner: remove parser_state->indesc_idx
  2020-02-10 22:31   ` Pablo Neira Ayuso
@ 2020-02-11  5:04     ` Laurent Fasnacht
  0 siblings, 0 replies; 18+ messages in thread
From: Laurent Fasnacht @ 2020-02-11  5:04 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, February 10, 2020 11:31 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:

> On Mon, Feb 10, 2020 at 10:17:28AM +0000, Laurent Fasnacht wrote:
> > static void scanner_pop_indesc(struct parser_state *state)
> > {
> >
> > -   state->indesc_idx--;
> >     if (!list_empty(&state->indesc_list)) {
> >     state->indesc = list_entry(state->indesc->list.prev, struct input_descriptor, list);
> >     } else {
> >     @@ -968,10 +966,6 @@ void scanner_destroy(struct nft_ctx *nft)
> >     {
> >     struct parser_state *state = yyget_extra(nft->scanner);
> >
> > -   do {
> >
> > -       yypop_buffer_state(nft->scanner);
> >
> >
>
> nft_pop_buffer_state calls free().
>
> Are you sure this can be removed without incurring in memleaks?

I'm pretty sure it can, since scanner_destroy is only called from outside of the scanner itself (and I'm expecting calling that function during the parsing might break a lot things ;-).

In my understanding, any file that is being parsed should reach the end of file at some point, and therefore have scanner_pop_buffer called. This is true also in case of parsing errors.

I've tested that understanding by counting the number of yypush and yypop calls in various cases, and by adding an assert in scanner_destroy (which consist in asserting that the "active" part of the stack is empty):

assert(state->indesc->list.next == state->indesc_list.next);

This has succeeded both for the test suite and for the very complex rule set I'm working on currently.

Just as a comment about the stack: in implementation with patch 6/7 applied, it consists in two parts, the active part and the "dangling tail".  The start of the stack is in state->indesc_list, and the active element (in state->indesc) is the end of the active part. All the elements in the active part of the stack have buffer pushed. When parsing a file ends, the buffer is popped and state->indesc is moved (but the input descriptor itself is not removed from the list, so it stays in the tail part until scanner_destroy is called).

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH nft include v2 1/7] tests: shell: add test for glob includes
  2020-02-10 10:17 ` [PATCH nft include v2 1/7] tests: shell: add test for glob includes Laurent Fasnacht
@ 2020-02-12 20:45   ` Pablo Neira Ayuso
  0 siblings, 0 replies; 18+ messages in thread
From: Pablo Neira Ayuso @ 2020-02-12 20:45 UTC (permalink / raw)
  To: Laurent Fasnacht; +Cc: netfilter-devel

On Mon, Feb 10, 2020 at 10:17:20AM +0000, Laurent Fasnacht wrote:
> Including more than MAX_INCLUDE_DEPTH file in one statement should succeed.

Applied, thanks.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH nft include v2 4/7] scanner: remove parser_state->indescs static array
  2020-02-10 10:17 ` [PATCH nft include v2 4/7] scanner: remove parser_state->indescs static array Laurent Fasnacht
  2020-02-10 23:32   ` Pablo Neira Ayuso
@ 2020-02-12 20:45   ` Pablo Neira Ayuso
  1 sibling, 0 replies; 18+ messages in thread
From: Pablo Neira Ayuso @ 2020-02-12 20:45 UTC (permalink / raw)
  To: Laurent Fasnacht; +Cc: netfilter-devel

On Mon, Feb 10, 2020 at 10:17:24AM +0000, Laurent Fasnacht wrote:
> This static array is redundant wth the indesc_list structure, but
> is less flexible.

Patches applied, from 3 to 7.

Thanks.

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, back to index

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-10 10:17 [PATCH nft include v2 0/7] Improve include behaviour Laurent Fasnacht
2020-02-10 10:17 ` [PATCH nft include v2 1/7] tests: shell: add test for glob includes Laurent Fasnacht
2020-02-12 20:45   ` Pablo Neira Ayuso
2020-02-10 10:17 ` [PATCH nft include v2 2/7] scanner: move the file descriptor to be in the input_descriptor structure Laurent Fasnacht
2020-02-10 22:46   ` Pablo Neira Ayuso
2020-02-10 10:17 ` [PATCH nft include v2 3/7] scanner: move indesc list append in scanner_push_indesc Laurent Fasnacht
2020-02-10 22:46   ` Pablo Neira Ayuso
2020-02-10 10:17 ` [PATCH nft include v2 4/7] scanner: remove parser_state->indescs static array Laurent Fasnacht
2020-02-10 23:32   ` Pablo Neira Ayuso
2020-02-11  4:36     ` Laurent Fasnacht
2020-02-12 20:45   ` Pablo Neira Ayuso
2020-02-10 10:17 ` [PATCH nft include v2 5/7] scanner: correctly compute include depth Laurent Fasnacht
2020-02-10 10:17 ` [PATCH nft include v2 6/7] scanner: fix indesc_list stack to be in the correct order Laurent Fasnacht
2020-02-10 22:33   ` Pablo Neira Ayuso
2020-02-11  4:42     ` Laurent Fasnacht
2020-02-10 10:17 ` [PATCH nft include v2 7/7] scanner: remove parser_state->indesc_idx Laurent Fasnacht
2020-02-10 22:31   ` Pablo Neira Ayuso
2020-02-11  5:04     ` Laurent Fasnacht

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git