[PATCH nft 1/3] tests/shell: add "bogons/nft-f/zero_length_devicename2_assert"

[PATCH nft 1/3] tests/shell: add "bogons/nft-f/zero_length_devicename2_assert"

[Thomas Haller]

This is copied from "bogons/nft-f/zero_length_devicename_assert" and
adjusted.

- `device""lo"` looks odd. In this file use `device ""` to only check
  against empty strings, without oddity.

- "ip" type has no hook ingress in filter. If the device name would be
  valid, the file would still be rejected. Use "netdev".

The purpose is to add a test for a file that would otherwise pass,
except having an empty device name. Without oddities.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 .../testcases/bogons/nft-f/zero_length_devicename2_assert    | 5 +++++
 1 file changed, 5 insertions(+)
 create mode 100644 tests/shell/testcases/bogons/nft-f/zero_length_devicename2_assert

diff --git a/tests/shell/testcases/bogons/nft-f/zero_length_devicename2_assert b/tests/shell/testcases/bogons/nft-f/zero_length_devicename2_assert
new file mode 100644
index 000000000000..fe416f8536e9
--- /dev/null
+++ b/tests/shell/testcases/bogons/nft-f/zero_length_devicename2_assert
@@ -0,0 +1,5 @@
+table netdev x {
+        chain Main_Ingress1 {
+                type filter hook ingress device "" priority -1
+	}
+}
-- 
2.41.0

[PATCH nft 2/3] tests/shell: cover long interface name in "0042chain_variable_0" test

[Thomas Haller]

IFNAMSIZ is 16. Adjust "0042chain_variable_0" to use an interface name
with the maximum allowed bytes length.

Instead of adding an entirely different test, adjust an existing one to
use another interface name. The aspect for testing for a long interface
name is not special enough, to warrant a separate test. We can cover it
by extending an existing test.

Note that the length check in "parser_bison.y" is wrong. The test checks
still for the wrong behavior and that "d23456789012345x" is accepted.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 .../testcases/chains/0042chain_variable_0     | 36 ++++++++++++++++---
 .../chains/dumps/0042chain_variable_0.nft     |  4 +--
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/tests/shell/testcases/chains/0042chain_variable_0 b/tests/shell/testcases/chains/0042chain_variable_0
index 1ea44e85c71f..739dc05a1777 100755
--- a/tests/shell/testcases/chains/0042chain_variable_0
+++ b/tests/shell/testcases/chains/0042chain_variable_0
@@ -2,7 +2,8 @@
 
 set -e
 
-ip link add name dummy0 type dummy
+ip link add name d23456789012345 type dummy
+
 
 EXPECTED="define if_main = \"lo\"
 
@@ -14,22 +15,50 @@ table netdev filter1 {
 
 $NFT -f - <<< $EXPECTED
 
+
+EXPECTED="define if_main = \"lo\"
+
+table netdev filter2 {
+	chain Main_Ingress2 {
+		type filter hook ingress devices = { \$if_main, d23456789012345x } priority -500; policy accept;
+	}
+}"
+
+rc=0
+$NFT -f - <<< $EXPECTED || rc=$?
+test "$rc" = 0
+cat <<EOF | $DIFF -u <($NFT list ruleset) -
+table netdev filter1 {
+	chain Main_Ingress1 {
+		type filter hook ingress device "lo" priority -500; policy accept;
+	}
+}
+table netdev filter2 {
+	chain Main_Ingress2 {
+		type filter hook ingress devices = { d23456789012345, lo } priority -500; policy accept;
+	}
+}
+EOF
+
+
 EXPECTED="define if_main = \"lo\"
 
 table netdev filter2 {
 	chain Main_Ingress2 {
-		type filter hook ingress devices = { \$if_main, dummy0 } priority -500; policy accept;
+		type filter hook ingress devices = { \$if_main, d23456789012345 } priority -500; policy accept;
 	}
 }"
 
 $NFT -f - <<< $EXPECTED
 
+
 if [ "$NFT_TEST_HAVE_netdev_egress" = n ] ; then
 	echo "Skip parts of the test due to NFT_TEST_HAVE_netdev_egress=n"
 	exit 77
 fi
 
-EXPECTED="define if_main = { lo, dummy0 }
+
+EXPECTED="define if_main = { lo, d23456789012345 }
 define lan_interfaces = { lo }
 
 table netdev filter3 {
@@ -43,4 +72,3 @@ table netdev filter3 {
 
 $NFT -f - <<< $EXPECTED
 
-
diff --git a/tests/shell/testcases/chains/dumps/0042chain_variable_0.nft b/tests/shell/testcases/chains/dumps/0042chain_variable_0.nft
index 5ec230d0bcfa..84a908d33dee 100644
--- a/tests/shell/testcases/chains/dumps/0042chain_variable_0.nft
+++ b/tests/shell/testcases/chains/dumps/0042chain_variable_0.nft
@@ -5,12 +5,12 @@ table netdev filter1 {
 }
 table netdev filter2 {
 	chain Main_Ingress2 {
-		type filter hook ingress devices = { dummy0, lo } priority -500; policy accept;
+		type filter hook ingress devices = { d23456789012345, lo } priority -500; policy accept;
 	}
 }
 table netdev filter3 {
 	chain Main_Ingress3 {
-		type filter hook ingress devices = { dummy0, lo } priority -500; policy accept;
+		type filter hook ingress devices = { d23456789012345, lo } priority -500; policy accept;
 	}
 
 	chain Main_Egress3 {
-- 
2.41.0

[PATCH nft 3/3] parser_bison: fix length check for ifname in ifname_expr_alloc()

[Thomas Haller]

IFNAMSIZ is 16, and the allowed byte length of the name is one less than
that. Fix the length check and adjust a test for covering the longest
allowed interface name.

This is obviously a change in behavior, because previously interface
names with length 16 were accepted and were silently truncated along the
way. Now they are rejected as invalid.

Fixes: fa52bc225806 ('parser: reject zero-length interface names')
Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/parser_bison.y                                | 3 ++-
 tests/shell/testcases/chains/0042chain_variable_0 | 7 +------
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/src/parser_bison.y b/src/parser_bison.y
index f0652ba651c6..9bfc3cdb2d12 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -16,6 +16,7 @@
 #include <stdio.h>
 #include <inttypes.h>
 #include <syslog.h>
+#include <net/if.h>
 #include <netinet/ip.h>
 #include <netinet/tcp.h>
 #include <netinet/if_ether.h>
@@ -158,7 +159,7 @@ static struct expr *ifname_expr_alloc(const struct location *location,
 		return NULL;
 	}
 
-	if (length > 16) {
+	if (length >= IFNAMSIZ) {
 		xfree(name);
 		erec_queue(error(location, "interface name too long"), queue);
 		return NULL;
diff --git a/tests/shell/testcases/chains/0042chain_variable_0 b/tests/shell/testcases/chains/0042chain_variable_0
index 739dc05a1777..a4b929f7344c 100755
--- a/tests/shell/testcases/chains/0042chain_variable_0
+++ b/tests/shell/testcases/chains/0042chain_variable_0
@@ -26,18 +26,13 @@ table netdev filter2 {
 
 rc=0
 $NFT -f - <<< $EXPECTED || rc=$?
-test "$rc" = 0
+test "$rc" = 1
 cat <<EOF | $DIFF -u <($NFT list ruleset) -
 table netdev filter1 {
 	chain Main_Ingress1 {
 		type filter hook ingress device "lo" priority -500; policy accept;
 	}
 }
-table netdev filter2 {
-	chain Main_Ingress2 {
-		type filter hook ingress devices = { d23456789012345, lo } priority -500; policy accept;
-	}
-}
 EOF
 
 
-- 
2.41.0

Re: [PATCH nft 3/3] parser_bison: fix length check for ifname in ifname_expr_alloc()

[Pablo Neira Ayuso]

On Mon, Oct 23, 2023 at 07:00:47PM +0200, Thomas Haller wrote:
> IFNAMSIZ is 16, and the allowed byte length of the name is one less than
> that. Fix the length check and adjust a test for covering the longest
> allowed interface name.
> 
> This is obviously a change in behavior, because previously interface
> names with length 16 were accepted and were silently truncated along the
> way. Now they are rejected as invalid.
> 
> Fixes: fa52bc225806 ('parser: reject zero-length interface names')
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  src/parser_bison.y                                | 3 ++-
>  tests/shell/testcases/chains/0042chain_variable_0 | 7 +------
>  2 files changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/src/parser_bison.y b/src/parser_bison.y
> index f0652ba651c6..9bfc3cdb2d12 100644
> --- a/src/parser_bison.y
> +++ b/src/parser_bison.y
> @@ -16,6 +16,7 @@
>  #include <stdio.h>
>  #include <inttypes.h>
>  #include <syslog.h>
> +#include <net/if.h>
>  #include <netinet/ip.h>
>  #include <netinet/tcp.h>
>  #include <netinet/if_ether.h>
> @@ -158,7 +159,7 @@ static struct expr *ifname_expr_alloc(const struct location *location,
>  		return NULL;
>  	}
>  
> -	if (length > 16) {
> +	if (length >= IFNAMSIZ) {
>  		xfree(name);
>  		erec_queue(error(location, "interface name too long"), queue);
>  		return NULL;
> diff --git a/tests/shell/testcases/chains/0042chain_variable_0 b/tests/shell/testcases/chains/0042chain_variable_0
> index 739dc05a1777..a4b929f7344c 100755
> --- a/tests/shell/testcases/chains/0042chain_variable_0
> +++ b/tests/shell/testcases/chains/0042chain_variable_0
> @@ -26,18 +26,13 @@ table netdev filter2 {
>  
>  rc=0
>  $NFT -f - <<< $EXPECTED || rc=$?
> -test "$rc" = 0
> +test "$rc" = 1
>  cat <<EOF | $DIFF -u <($NFT list ruleset) -
>  table netdev filter1 {
>  	chain Main_Ingress1 {
>  		type filter hook ingress device "lo" priority -500; policy accept;
>  	}
>  }
> -table netdev filter2 {
> -	chain Main_Ingress2 {
> -		type filter hook ingress devices = { d23456789012345, lo } priority -500; policy accept;
> -	}
> -}

Please, do not remove it, fix this test.

Re: [PATCH nft 3/3] parser_bison: fix length check for ifname in ifname_expr_alloc()

[Pablo Neira Ayuso]

On Mon, Oct 23, 2023 at 07:15:10PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Oct 23, 2023 at 07:00:47PM +0200, Thomas Haller wrote:
> > IFNAMSIZ is 16, and the allowed byte length of the name is one less than
> > that. Fix the length check and adjust a test for covering the longest
> > allowed interface name.
> > 
> > This is obviously a change in behavior, because previously interface
> > names with length 16 were accepted and were silently truncated along the
> > way. Now they are rejected as invalid.
> > 
> > Fixes: fa52bc225806 ('parser: reject zero-length interface names')
> > Signed-off-by: Thomas Haller <thaller@redhat.com>
> > ---
> >  src/parser_bison.y                                | 3 ++-
> >  tests/shell/testcases/chains/0042chain_variable_0 | 7 +------
> >  2 files changed, 3 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/parser_bison.y b/src/parser_bison.y
> > index f0652ba651c6..9bfc3cdb2d12 100644
> > --- a/src/parser_bison.y
> > +++ b/src/parser_bison.y
> > @@ -16,6 +16,7 @@
> >  #include <stdio.h>
> >  #include <inttypes.h>
> >  #include <syslog.h>
> > +#include <net/if.h>
> >  #include <netinet/ip.h>
> >  #include <netinet/tcp.h>
> >  #include <netinet/if_ether.h>
> > @@ -158,7 +159,7 @@ static struct expr *ifname_expr_alloc(const struct location *location,
> >  		return NULL;
> >  	}
> >  
> > -	if (length > 16) {
> > +	if (length >= IFNAMSIZ) {
> >  		xfree(name);
> >  		erec_queue(error(location, "interface name too long"), queue);
> >  		return NULL;
> > diff --git a/tests/shell/testcases/chains/0042chain_variable_0 b/tests/shell/testcases/chains/0042chain_variable_0
> > index 739dc05a1777..a4b929f7344c 100755
> > --- a/tests/shell/testcases/chains/0042chain_variable_0
> > +++ b/tests/shell/testcases/chains/0042chain_variable_0
> > @@ -26,18 +26,13 @@ table netdev filter2 {
> >  
> >  rc=0
> >  $NFT -f - <<< $EXPECTED || rc=$?
> > -test "$rc" = 0
> > +test "$rc" = 1
> >  cat <<EOF | $DIFF -u <($NFT list ruleset) -
> >  table netdev filter1 {
> >  	chain Main_Ingress1 {
> >  		type filter hook ingress device "lo" priority -500; policy accept;
> >  	}
> >  }
> > -table netdev filter2 {
> > -	chain Main_Ingress2 {
> > -		type filter hook ingress devices = { d23456789012345, lo } priority -500; policy accept;
> > -	}
> > -}
> 
> Please, do not remove it, fix this test.

Or maybe I am missing the reason for this change? It seems this
d23456789012345 was added to your previous patch in this series.

Thanks!

Re: [PATCH nft 3/3] parser_bison: fix length check for ifname in ifname_expr_alloc()

[Thomas Haller]

On Mon, 2023-10-23 at 19:16 +0200, Pablo Neira Ayuso wrote:
> On Mon, Oct 23, 2023 at 07:15:10PM +0200, Pablo Neira Ayuso wrote:
> > On Mon, Oct 23, 2023 at 07:00:47PM +0200, Thomas Haller wrote:
> > > IFNAMSIZ is 16, and the allowed byte length of the name is one
> > > less than
> > > that. Fix the length check and adjust a test for covering the
> > > longest
> > > allowed interface name.
> > > 
> > > This is obviously a change in behavior, because previously
> > > interface
> > > names with length 16 were accepted and were silently truncated
> > > along the
> > > way. Now they are rejected as invalid.
> > > 
> > > Fixes: fa52bc225806 ('parser: reject zero-length interface
> > > names')
> > > Signed-off-by: Thomas Haller <thaller@redhat.com>
> > > ---
> > >  src/parser_bison.y                                | 3 ++-
> > >  tests/shell/testcases/chains/0042chain_variable_0 | 7 +------
> > >  2 files changed, 3 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/src/parser_bison.y b/src/parser_bison.y
> > > index f0652ba651c6..9bfc3cdb2d12 100644
> > > --- a/src/parser_bison.y
> > > +++ b/src/parser_bison.y
> > > @@ -16,6 +16,7 @@
> > >  #include <stdio.h>
> > >  #include <inttypes.h>
> > >  #include <syslog.h>
> > > +#include <net/if.h>
> > >  #include <netinet/ip.h>
> > >  #include <netinet/tcp.h>
> > >  #include <netinet/if_ether.h>
> > > @@ -158,7 +159,7 @@ static struct expr *ifname_expr_alloc(const
> > > struct location *location,
> > >                 return NULL;
> > >         }
> > >  
> > > -       if (length > 16) {
> > > +       if (length >= IFNAMSIZ) {
> > >                 xfree(name);
> > >                 erec_queue(error(location, "interface name too
> > > long"), queue);
> > >                 return NULL;
> > > diff --git a/tests/shell/testcases/chains/0042chain_variable_0
> > > b/tests/shell/testcases/chains/0042chain_variable_0
> > > index 739dc05a1777..a4b929f7344c 100755
> > > --- a/tests/shell/testcases/chains/0042chain_variable_0
> > > +++ b/tests/shell/testcases/chains/0042chain_variable_0
> > > @@ -26,18 +26,13 @@ table netdev filter2 {
> > >  
> > >  rc=0
> > >  $NFT -f - <<< $EXPECTED || rc=$?
> > > -test "$rc" = 0
> > > +test "$rc" = 1
> > >  cat <<EOF | $DIFF -u <($NFT list ruleset) -
> > >  table netdev filter1 {
> > >         chain Main_Ingress1 {
> > >                 type filter hook ingress device "lo" priority -
> > > 500; policy accept;
> > >         }
> > >  }
> > > -table netdev filter2 {
> > > -       chain Main_Ingress2 {
> > > -               type filter hook ingress devices = {
> > > d23456789012345, lo } priority -500; policy accept;
> > > -       }
> > > -}
> > 
> > Please, do not remove it, fix this test.
> 
> Or maybe I am missing the reason for this change? It seems this
> d23456789012345 was added to your previous patch in this series.
> 
> 

The point is to add a test for the bug first.

Followed by the fix. And the change to the test shows how the fix
fixes.


Thomas

Re: [PATCH nft 1/3] tests/shell: add "bogons/nft-f/zero_length_devicename2_assert"

[Pablo Neira Ayuso]

Series aplied, thanks.

After all these updates I see this failure:

W: [FAILED]     1/1 testcases/sets/elem_opts_compat_0

I: results: [OK] 0 [SKIPPED] 0 [FAILED] 1 [TOTAL] 1

when running tests.

Re: [PATCH nft 1/3] tests/shell: add "bogons/nft-f/zero_length_devicename2_assert"

[Thomas Haller]

On Tue, 2023-10-24 at 11:57 +0200, Pablo Neira Ayuso wrote:
> Series aplied, thanks.

Thank you.

> 
> After all these updates I see this failure:
> 
> W: [FAILED]     1/1 testcases/sets/elem_opts_compat_0
> 
> I: results: [OK] 0 [SKIPPED] 0 [FAILED] 1 [TOTAL] 1
> 
> when running tests.
> 

Hm. I don't get such failure (Kernel 6.5.6-200.fc38.x86_64).
But regardless of that, I don't think that my patches had anything to
do with that test, do they?


Can you provide more information? Can you bisect the failure?

Could you share:

  make && ./tests/shell/run-tests.sh ./tests/shell/testcases/sets/elem_opts_compat_0 -x -k
  grep -R ^ /tmp/nft-test.latest.*/


Thomas

Re: [PATCH nft 1/3] tests/shell: add "bogons/nft-f/zero_length_devicename2_assert"

[Pablo Neira Ayuso]

On Tue, Oct 24, 2023 at 12:15:22PM +0200, Thomas Haller wrote:
> On Tue, 2023-10-24 at 11:57 +0200, Pablo Neira Ayuso wrote:
> > Series aplied, thanks.
> 
> Thank you.
> 
> > 
> > After all these updates I see this failure:
> > 
> > W: [FAILED]     1/1 testcases/sets/elem_opts_compat_0
> > 
> > I: results: [OK] 0 [SKIPPED] 0 [FAILED] 1 [TOTAL] 1
> > 
> > when running tests.
> > 
> 
> Hm. I don't get such failure (Kernel 6.5.6-200.fc38.x86_64).
> But regardless of that, I don't think that my patches had anything to
> do with that test, do they?
> 
> 
> Can you provide more information? Can you bisect the failure?
> 
> Could you share:
> 
>   make && ./tests/shell/run-tests.sh ./tests/shell/testcases/sets/elem_opts_compat_0 -x -k
>   grep -R ^ /tmp/nft-test.latest.*/

/tmp/nft-test.latest.root/times:2.3
/tmp/nft-test.latest.root/times:3067.20
/tmp/nft-test.latest.root/times:3069.50
/tmp/nft-test.latest.root/test-testcases-sets-elem_opts_compat_0.1/times:0.12
/tmp/nft-test.latest.root/test-testcases-sets-elem_opts_compat_0.1/times:3069.38
/tmp/nft-test.latest.root/test-testcases-sets-elem_opts_compat_0.1/times:3069.50
/tmp/nft-test.latest.root/test-testcases-sets-elem_opts_compat_0.1/ruleset-after:table ip t {
/tmp/nft-test.latest.root/test-testcases-sets-elem_opts_compat_0.1/ruleset-after:       set s {
/tmp/nft-test.latest.root/test-testcases-sets-elem_opts_compat_0.1/ruleset-after:               type inet_service
/tmp/nft-test.latest.root/test-testcases-sets-elem_opts_compat_0.1/ruleset-after:               counter
/tmp/nft-test.latest.root/test-testcases-sets-elem_opts_compat_0.1/ruleset-after:               timeout 30s
/tmp/nft-test.latest.root/test-testcases-sets-elem_opts_compat_0.1/ruleset-after:       }
/tmp/nft-test.latest.root/test-testcases-sets-elem_opts_compat_0.1/ruleset-after:}
/tmp/nft-test.latest.root/test-testcases-sets-elem_opts_compat_0.1/rc-failed:2
/tmp/nft-test.latest.root/test-testcases-sets-elem_opts_compat_0.1/name:testcases/sets/elem_opts_compat_0
/tmp/nft-test.latest.root/test-testcases-sets-elem_opts_compat_0.1/testout.log:Command: testcases/sets/elem_opts_compat_0 
/tmp/nft-test.latest.root/test-testcases-sets-elem_opts_compat_0.1/testout.log:testcases/sets/elem_opts_compat_0: 20: Syntax error: redirection unexpected
/tmp/nft-test.latest.root/test-testcases-sets-elem_opts_compat_0.1/rc-failed-exit:2

Re: [PATCH nft 1/3] tests/shell: add "bogons/nft-f/zero_length_devicename2_assert"

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> /tmp/nft-test.latest.root/test-testcases-sets-elem_opts_compat_0.1/testout.log:testcases/sets/elem_opts_compat_0: 20: Syntax error: redirection unexpected
> /tmp/nft-test.latest.root/test-testcases-sets-elem_opts_compat_0.1/rc-failed-exit:2

The test case is broken, it uses /bin/sh but needs /bin/bash.

Re: [PATCH nft 1/3] tests/shell: add "bogons/nft-f/zero_length_devicename2_assert"

[Pablo Neira Ayuso]

On Tue, Oct 24, 2023 at 12:29:03PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > /tmp/nft-test.latest.root/test-testcases-sets-elem_opts_compat_0.1/testout.log:testcases/sets/elem_opts_compat_0: 20: Syntax error: redirection unexpected
> > /tmp/nft-test.latest.root/test-testcases-sets-elem_opts_compat_0.1/rc-failed-exit:2
> 
> The test case is broken, it uses /bin/sh but needs /bin/bash.

Nice, it works now :)

http://git.netfilter.org/nftables/commit/?id=d3e941668be1d3922832fd70788fb248fd11f6c7

Re: [PATCH nft 1/3] tests/shell: add "bogons/nft-f/zero_length_devicename2_assert"

[Phil Sutter]

On Tue, Oct 24, 2023 at 12:29:03PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > /tmp/nft-test.latest.root/test-testcases-sets-elem_opts_compat_0.1/testout.log:testcases/sets/elem_opts_compat_0: 20: Syntax error: redirection unexpected
> > /tmp/nft-test.latest.root/test-testcases-sets-elem_opts_compat_0.1/rc-failed-exit:2
> 
> The test case is broken, it uses /bin/sh but needs /bin/bash.

Gna, I keep forgetting that here-strings are a Bashism, sorry for the
mess.

Cheers, Phil