[PATCH nft include v2 0/7] Improve include behaviour

[PATCH nft include v2 0/7] Improve include behaviour

[Laurent Fasnacht]

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

[PATCH nft include v2 1/7] tests: shell: add test for glob includes

[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

Re: [PATCH nft include v2 1/7] tests: shell: add test for glob includes

[Pablo Neira Ayuso]

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.

[PATCH nft include v2 2/7] scanner: move the file descriptor to be in the input_descriptor structure

[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

Re: [PATCH nft include v2 2/7] scanner: move the file descriptor to be in the input_descriptor structure

[Pablo Neira Ayuso]

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.

[PATCH nft include v2 3/7] scanner: move indesc list append in scanner_push_indesc

[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

Re: [PATCH nft include v2 3/7] scanner: move indesc list append in scanner_push_indesc

[Pablo Neira Ayuso]

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.

[PATCH nft include v2 4/7] scanner: remove parser_state->indescs static array

[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

Re: [PATCH nft include v2 4/7] scanner: remove parser_state->indescs static array

[Pablo Neira Ayuso]

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.

Re: [PATCH nft include v2 4/7] scanner: remove parser_state->indescs static array

[Laurent Fasnacht]

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?

Re: [PATCH nft include v2 4/7] scanner: remove parser_state->indescs static array

[Pablo Neira Ayuso]

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.

[PATCH nft include v2 5/7] scanner: correctly compute include depth

[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

[PATCH nft include v2 6/7] scanner: fix indesc_list stack to be in the correct order

[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

Re: [PATCH nft include v2 6/7] scanner: fix indesc_list stack to be in the correct order

[Pablo Neira Ayuso]

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 ?

Re: [PATCH nft include v2 6/7] scanner: fix indesc_list stack to be in the correct order

[Laurent Fasnacht]

‐‐‐‐‐‐‐ 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.

[PATCH nft include v2 7/7] scanner: remove parser_state->indesc_idx

[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

Re: [PATCH nft include v2 7/7] scanner: remove parser_state->indesc_idx

[Pablo Neira Ayuso]

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
> 
> 

Re: [PATCH nft include v2 7/7] scanner: remove parser_state->indesc_idx

[Laurent Fasnacht]

‐‐‐‐‐‐‐ 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).