[PATCH nft] evalute: don't BUG on unexpected base datatype

[PATCH nft] evalute: don't BUG on unexpected base datatype

[Florian Westphal]

Included bogo will cause a crash but this is the evaluation
stage where we can just emit an error instead.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 I wonder if we should just replace all BUGs in evaluate.c
 with expr_error() calls, it avoids constant whack-a-mole.

 src/evaluate.c                                        |  3 ++-
 .../bogons/nft-f/invalid_basetype_verdict_assert      | 11 +++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)
 create mode 100644 tests/shell/testcases/bogons/nft-f/invalid_basetype_verdict_assert

diff --git a/src/evaluate.c b/src/evaluate.c
index 9c7f23cb080e..de054f82d55f 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -482,7 +482,8 @@ static int expr_evaluate_value(struct eval_ctx *ctx, struct expr **expr)
 			return -1;
 		break;
 	default:
-		BUG("invalid basetype %s\n", expr_basetype(*expr)->name);
+		return expr_error(ctx->msgs, *expr, "Unexpected basetype %s",
+				  expr_basetype(*expr)->name);
 	}
 	return 0;
 }
diff --git a/tests/shell/testcases/bogons/nft-f/invalid_basetype_verdict_assert b/tests/shell/testcases/bogons/nft-f/invalid_basetype_verdict_assert
new file mode 100644
index 000000000000..f85ce7fe342c
--- /dev/null
+++ b/tests/shell/testcases/bogons/nft-f/invalid_basetype_verdict_assert
@@ -0,0 +1,11 @@
+table ip t {
+	map m {
+		type ipv4_addr . inet_service : ipv4_addr .  verdict
+		elements = { 10.0.0.1 . 42 : 10.1.1.1 . 0 }
+	}
+
+	chain c {
+		type nat hook prerouting priority dstnat; policy accept;
+		dnat ip to ip saddr . tcp dport map @m
+	}
+}
-- 
2.49.0

Re: [PATCH nft] evalute: don't BUG on unexpected base datatype

[Phil Sutter]

On Fri, Jun 13, 2025 at 04:46:06PM +0200, Florian Westphal wrote:
> Included bogo will cause a crash but this is the evaluation
> stage where we can just emit an error instead.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  I wonder if we should just replace all BUGs in evaluate.c
>  with expr_error() calls, it avoids constant whack-a-mole.

I guess the expectation was that bison catches these but I fear JSON
parser has weakened that quite a bit.

I wish libnftables to well-behave in error cases unless critical ones
like ENOMEM.

Cheers, Phil

Re: [PATCH nft] evalute: don't BUG on unexpected base datatype

[Pablo Neira Ayuso]

On Fri, Jun 13, 2025 at 05:40:20PM +0200, Phil Sutter wrote:
> On Fri, Jun 13, 2025 at 04:46:06PM +0200, Florian Westphal wrote:
> > Included bogo will cause a crash but this is the evaluation
> > stage where we can just emit an error instead.
> > 
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  I wonder if we should just replace all BUGs in evaluate.c
> >  with expr_error() calls, it avoids constant whack-a-mole.

I think that can help uncover bugs, or are those json induced bugs?

> I guess the expectation was that bison catches these but I fear JSON
> parser has weakened that quite a bit.

It would be good to harden json parser to reject trivial non-sense, no
need to postpone this to the evaluation phase. If fuzzer can help in
that regard. I understand some issues can be more easily identified
from the evaluation step. I am not telling to only handle this from
the parser, I mean "it depends" on the issue.

> I wish libnftables to well-behave in error cases unless critical ones
> like ENOMEM.

Yes, I guess that's completary to my request above.

Re: [PATCH nft] evalute: don't BUG on unexpected base datatype

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > >  I wonder if we should just replace all BUGs in evaluate.c
> > >  with expr_error() calls, it avoids constant whack-a-mole.
> 
> I think that can help uncover bugs, or are those json induced bugs?

This one is bison, see included bogon input.
I meant in general.

How do these BUGs help at all?

Re: [PATCH nft] evalute: don't BUG on unexpected base datatype

[Pablo Neira Ayuso]

Hi Florian,

On Mon, Jun 16, 2025 at 11:16:18AM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > >  I wonder if we should just replace all BUGs in evaluate.c
> > > >  with expr_error() calls, it avoids constant whack-a-mole.
> > 
> > I think that can help uncover bugs, or are those json induced bugs?
> 
> This one is bison, see included bogon input.

For this particular case I think it makes sense to turn it into error.

Nitpick for this patch: I'd suggest this error:

+               return expr_error(ctx->msgs, *expr, "Unexpected datatype %s",
+                                 (*expr)->dtype->name);

Which results in:

[...]
ruleset:3:61-67: Error: Unexpected datatype verdict
               type ipv4_addr . inet_service : ipv4_addr .  verdict
                                                            ^^^^^^^
> I meant in general.

It's not so straight forward, I have quickly browsed the existing
BUG() calls and, unlike this one, they are mostly trapping nonsense,
I guess we can find a way to keep them around while not invoking
assert().

> How do these BUGs help at all?

Maybe you can turn them into error record adding "BUG:" prefix if
you prefer, I guess that will not crash a daemon process which is your
concern?

ie. add expr_bug() to highlight an input is triggering a buggy internal
state so this can still be reported without crashing the daemon
process?