| Message ID | 20231023170058.919275-3-thaller@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [nft,1/3] tests/shell: add "bogons/nft-f/zero_length_devicename2_assert" | expand |
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.
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!
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
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
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(-)