[PATCH nft] evaluate: tolerate empty concatenation

[PATCH nft] evaluate: tolerate empty concatenation

[Florian Westphal]

Don't rely on a successful evaluation of set->key.
With this input, set->key fails validation but subsequent
element evaluation asserts because the context points at
the set key -- an empty concatenation.

Causes:
nft: src/evaluate.c:1681: expr_evaluate_concat: Assertion `!list_empty(&ctx->ectx.key->expressions)' failed.

After patch:
internal:0:0-0: Error: unqualified type  specified in set definition. Try "typeof expression" instead of "type datatype".
internal:0:0-0: Error: Could not parse symbolic invalid expression

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/evaluate.c                                | 10 +++++--
 ...pr_evaluate_concat_empty_concat_key_assert | 27 +++++++++++++++++++
 2 files changed, 35 insertions(+), 2 deletions(-)
 create mode 100644 tests/shell/testcases/bogons/nft-j-f/expr_evaluate_concat_empty_concat_key_assert

diff --git a/src/evaluate.c b/src/evaluate.c
index 1e7f6f53542b..a6b08cf3b1b5 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1645,6 +1645,13 @@ static int list_member_evaluate(struct eval_ctx *ctx, struct expr **expr)
 	return err;
 }
 
+static bool ctx_has_concat_key(const struct eval_ctx *ctx)
+{
+	/* Ignore empty concatenation key, set eval queued an error */
+	return ctx->ectx.key && ctx->ectx.key->etype == EXPR_CONCAT &&
+	       !list_empty(&ctx->ectx.key->expressions);
+}
+
 static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr)
 {
 	const struct datatype *dtype = ctx->ectx.dtype, *tmp;
@@ -1657,9 +1664,8 @@ static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr)
 	bool runaway = false;
 	uint32_t size = 0;
 
-	if (ctx->ectx.key && ctx->ectx.key->etype == EXPR_CONCAT) {
+	if (ctx_has_concat_key(ctx)) {
 		key_ctx = ctx->ectx.key;
-		assert(!list_empty(&ctx->ectx.key->expressions));
 		key = list_first_entry(&ctx->ectx.key->expressions, struct expr, list);
 		expressions = &ctx->ectx.key->expressions;
 	}
diff --git a/tests/shell/testcases/bogons/nft-j-f/expr_evaluate_concat_empty_concat_key_assert b/tests/shell/testcases/bogons/nft-j-f/expr_evaluate_concat_empty_concat_key_assert
new file mode 100644
index 000000000000..956ecdc99721
--- /dev/null
+++ b/tests/shell/testcases/bogons/nft-j-f/expr_evaluate_concat_empty_concat_key_assert
@@ -0,0 +1,27 @@
+{
+  "nftables": [
+    {
+      "table": { "family": "ip",
+        "name": "t",
+        "handle": 0
+      }
+    },
+    {
+      "set": {
+        "family": "ip",
+        "name": "s",
+        "table": "t",
+        "type": [
+             ],
+        "elem": [
+          {
+            "concat": [
+              "foo", "bar"
+            ]
+          }
+        ]
+      }
+    }
+  ]
+}
+
-- 
2.48.1

Re: [PATCH nft] evaluate: tolerate empty concatenation

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 674 bytes --]

Hi Florian,

On Mon, Mar 24, 2025 at 12:52:58PM +0100, Florian Westphal wrote:
> Don't rely on a successful evaluation of set->key.
> With this input, set->key fails validation but subsequent
> element evaluation asserts because the context points at
> the set key -- an empty concatenation.
> 
> Causes:
> nft: src/evaluate.c:1681: expr_evaluate_concat: Assertion `!list_empty(&ctx->ectx.key->expressions)' failed.
> 
> After patch:
> internal:0:0-0: Error: unqualified type  specified in set definition. Try "typeof expression" instead of "type datatype".
> internal:0:0-0: Error: Could not parse symbolic invalid expression

Maybe block this from the json parser itself?

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 569 bytes --]

diff --git a/src/parser_json.c b/src/parser_json.c
index 17bc38b565ae..8d5aa480ae04 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -3395,6 +3395,14 @@ static struct cmd *json_parse_cmd_add_set(struct json_ctx *ctx, json_t *root,
 		return NULL;
 	}
 
+	if (set->key->etype == EXPR_CONCAT &&
+	    list_empty(&set->key->expressions)) {
+		json_error(ctx, "Empty set type.");
+		set_free(set);
+		handle_free(&h);
+		return NULL;
+	}
+
 	if (!json_unpack(root, "{s:o}", "map", &tmp)) {
 		if (json_is_string(tmp)) {
 			const char *s = json_string_value(tmp);

Re: [PATCH nft] evaluate: tolerate empty concatenation

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 860 bytes --]

On Thu, Mar 27, 2025 at 04:33:59PM +0100, Pablo Neira Ayuso wrote:
> Hi Florian,
> 
> On Mon, Mar 24, 2025 at 12:52:58PM +0100, Florian Westphal wrote:
> > Don't rely on a successful evaluation of set->key.
> > With this input, set->key fails validation but subsequent
> > element evaluation asserts because the context points at
> > the set key -- an empty concatenation.
> > 
> > Causes:
> > nft: src/evaluate.c:1681: expr_evaluate_concat: Assertion `!list_empty(&ctx->ectx.key->expressions)' failed.
> > 
> > After patch:
> > internal:0:0-0: Error: unqualified type  specified in set definition. Try "typeof expression" instead of "type datatype".
> > internal:0:0-0: Error: Could not parse symbolic invalid expression
> 
> Maybe block this from the json parser itself?

Maybe this instead? This covers for empty concatenation in both set
key and set data.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 752 bytes --]

commit e0123be7a908d1a4b7c43d0817b9ecccf2bd1416
Author: Pablo Neira Ayuso <pablo@netfilter.org>
Date:   Thu Mar 27 16:32:16 2025 +0100

    parser_json: reject empty concatention in set key and data
    
    Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

diff --git a/src/parser_json.c b/src/parser_json.c
index 17bc38b565ae..513e0b10f028 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -1729,6 +1729,13 @@ static struct expr *json_parse_dtype_expr(struct json_ctx *ctx, json_t *root)
 			}
 			compound_expr_add(expr, i);
 		}
+
+		if (list_empty(&expr->expressions)) {
+			json_error(ctx, "Empty concatenation");
+			expr_free(expr);
+			return NULL;
+		}
+
 		return expr;
 	} else if (json_is_object(root)) {
 		const char *key;

Re: [PATCH nft] evaluate: tolerate empty concatenation

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Mon, Mar 24, 2025 at 12:52:58PM +0100, Florian Westphal wrote:
> > > Don't rely on a successful evaluation of set->key.
> > > With this input, set->key fails validation but subsequent
> > > element evaluation asserts because the context points at
> > > the set key -- an empty concatenation.
> > > 
> > > Causes:
> > > nft: src/evaluate.c:1681: expr_evaluate_concat: Assertion `!list_empty(&ctx->ectx.key->expressions)' failed.
> > > 
> > > After patch:
> > > internal:0:0-0: Error: unqualified type  specified in set definition. Try "typeof expression" instead of "type datatype".
> > > internal:0:0-0: Error: Could not parse symbolic invalid expression
> > 
> > Maybe block this from the json parser itself?
> 
> Maybe this instead? This covers for empty concatenation in both set
> key and set data.

I don't like the idea of having to keep double-error-checks in
both json and bison frontends.

I would prefer a generic solution where possible, unless
there is some other advantage such as better error reporting.

Re: [PATCH nft] evaluate: tolerate empty concatenation

[Pablo Neira Ayuso]

On Thu, Mar 27, 2025 at 04:49:10PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Mon, Mar 24, 2025 at 12:52:58PM +0100, Florian Westphal wrote:
> > > > Don't rely on a successful evaluation of set->key.
> > > > With this input, set->key fails validation but subsequent
> > > > element evaluation asserts because the context points at
> > > > the set key -- an empty concatenation.
> > > > 
> > > > Causes:
> > > > nft: src/evaluate.c:1681: expr_evaluate_concat: Assertion `!list_empty(&ctx->ectx.key->expressions)' failed.
> > > > 
> > > > After patch:
> > > > internal:0:0-0: Error: unqualified type  specified in set definition. Try "typeof expression" instead of "type datatype".
> > > > internal:0:0-0: Error: Could not parse symbolic invalid expression
> > > 
> > > Maybe block this from the json parser itself?
> > 
> > Maybe this instead? This covers for empty concatenation in both set
> > key and set data.
> 
> I don't like the idea of having to keep double-error-checks in
> both json and bison frontends.
> 
> I would prefer a generic solution where possible, unless
> there is some other advantage such as better error reporting.

I have been trying to trigger this from the bison parser, but I could
not.

I think assertion are still useful to denote something is very broken
if we get to the evaluation step with an empty concatenation.

Re: [PATCH nft] evaluate: tolerate empty concatenation

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> I think assertion are still useful to denote something is very broken
> if we get to the evaluation step with an empty concatenation.

OK, then please push put your fix then.