[PATCH nft 0/3] scanner: improving include handling

[PATCH nft 0/3] scanner: improving include handling

[Laurent Fasnacht]

Hello,

I'd like to submit a small patch series to improve include behaviour. It fixes bug #1243 for example.

Let me know if you have any questions/remarks,

Best,
Laurent

[PATCH nft 1/3] 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

[PATCH nft 2/3] 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 2016acd5..837bc476 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -691,7 +691,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;
@@ -706,6 +707,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 3/3] scanner: remove indescs and indescs_idx attributes from the parser, and directly use indesc_list

[Laurent Fasnacht]

parser_state.indescs (static array) and parser_state.indesc_list are in fact duplicating information.

This commit removes the static array and uses the list for everything.

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

diff --git a/include/parser.h b/include/parser.h
index 949284d9..636d1c88 100644
--- a/include/parser.h
+++ b/include/parser.h
@@ -15,8 +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;
 
 	struct list_head		*msgs;
diff --git a/src/scanner.l b/src/scanner.l
index 837bc476..4b06cb99 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -668,18 +668,23 @@ 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;
 }
 
 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
+	struct input_descriptor *indesc;
+	indesc = state->indesc;
+	if (!list_empty(&state->indesc_list)) {
+		state->indesc = list_entry(state->indesc->list.prev, struct input_descriptor, list);
+	} else {
 		state->indesc = NULL;
+	}
+	if (indesc->f) {
+		fclose(indesc->f);
+		indesc->f = NULL;
+	}
 }
 
 static void scanner_pop_buffer(yyscan_t scanner)
@@ -716,7 +721,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,
@@ -915,15 +919,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);
@@ -940,35 +943,22 @@ void *scanner_init(struct parser_state *state)
 	return scanner;
 }
 
-static void input_descriptor_destroy(const struct input_descriptor *indesc)
-{
-	if (indesc->name)
-		xfree(indesc->name);
-	xfree(indesc);
-}
 
-static void input_descriptor_list_destroy(struct parser_state *state)
+static void input_descriptor_list_destroy(yyscan_t *scanner)
 {
+	struct parser_state *state = yyget_extra(scanner);
 	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;
-		}
+		if (indesc->name)
+			xfree(indesc->name);
 		list_del(&indesc->list);
-		input_descriptor_destroy(indesc);
+		xfree(indesc);
 	}
 }
 
 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);
+	input_descriptor_list_destroy(nft->scanner);
 	yylex_destroy(nft->scanner);
 }
-- 
2.20.1

Re: [PATCH nft 1/3] scanner: move the file descriptor to be in the input_descriptor structure

[Pablo Neira Ayuso]

On Wed, Feb 05, 2020 at 12:29:55PM +0000, Laurent Fasnacht wrote:
> This prevents a static allocation of file descriptors array, thus allows
> more flexibility.

Applied, thanks.

Re: [PATCH nft 2/3] scanner: correctly compute include depth

[Pablo Neira Ayuso]

On Wed, Feb 05, 2020 at 12:30:13PM +0000, Laurent Fasnacht wrote:
> Inclusion depth was computed incorrectly for glob includes.

I get a segfault here when running your test after your patch 2/3 is
applied. I'm going to keep back this series, sorry.

Re: [PATCH nft 3/3] scanner: remove indescs and indescs_idx attributes from the parser, and directly use indesc_list

[Pablo Neira Ayuso]

On Wed, Feb 05, 2020 at 12:30:24PM +0000, Laurent Fasnacht wrote:
[...]
> @@ -915,15 +919,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));

I'd appreciate if you could split initially cleanups in separated
patches. Like adding this new variable.

> -	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);
> @@ -940,35 +943,22 @@ void *scanner_init(struct parser_state *state)
>  	return scanner;
>  }
>  
> -static void input_descriptor_destroy(const struct input_descriptor *indesc)
> -{
> -	if (indesc->name)
> -		xfree(indesc->name);
> -	xfree(indesc);
> -}

Or removing this function, actually there is no need to check if
indesc->name != NULL, so just:

        xfree(indesc->name);
        xfree(indesc);

is probably fine while you are here.

That will make it easier for us to track changes.

Thanks!

Re: [PATCH nft 0/3] scanner: improving include handling

[Pablo Neira Ayuso]

On Wed, Feb 05, 2020 at 12:29:21PM +0000, Laurent Fasnacht wrote:
> Hello,
> 
> I'd like to submit a small patch series to improve include
> behaviour. It fixes bug #1243 for example.

Thanks for addressing bugs. I'd suggest, let's start with making 1/3
and 2/3 fix the problem. Then following up with refinements is fine
too.

Thanks.