[PATCH nft 0/3] Two fixes to avoid "-Wstrict-overflow" warnings

[PATCH nft 0/3] Two fixes to avoid "-Wstrict-overflow" warnings

[Thomas Haller]

The "-Wstrict-overflow=5" warning seems useful, as it's easy to get this
wrong. I think C really should have "-fwrapv" behavior, but the reality
is that we need to be careful about signed overflow. The warning helps
with that, and luckily there are only a few places that require fixes.
Note that libparser and mini-gmp.c cannot build with "-Wstrict-overflow=5".

Maybe there are other warnings, as it depends on the optimization level
and the compiler. This works for my gcc and -O3.

Thomas Haller (3):
  nft: add NFT_ARRAY_SIZE() helper
  nfnl_osf: rework nf_osf_parse_opt() and avoid "-Wstrict-overflow"
    warning
  netlink_linearize: avoid strict-overflow warning in
    netlink_gen_bitwise()

 include/nft.h           |   2 +
 src/netlink_linearize.c |   7 +--
 src/nfnl_osf.c          | 128 ++++++++++++++++++----------------------
 3 files changed, 63 insertions(+), 74 deletions(-)

-- 
2.41.0

[PATCH nft 1/3] nft: add NFT_ARRAY_SIZE() helper

[Thomas Haller]

Add NFT_ARRAY_SIZE() macro, commonly known as ARRAY_SIZE() (or G_N_ELEMENTS()).

<nft.h> is the right place for macros and static-inline functions. It is
included in *every* C sources, as it only depends on libc headers and
<config.h>. NFT_ARRAY_SIZE() is part of the basic toolset, that should
be available everywhere.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 include/nft.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/nft.h b/include/nft.h
index 9384054c11c8..4463b5c0fa4a 100644
--- a/include/nft.h
+++ b/include/nft.h
@@ -8,4 +8,6 @@
 #include <stdint.h>
 #include <stdlib.h>
 
+#define NFT_ARRAY_SIZE(arr) (sizeof(arr)/sizeof((arr)[0]))
+
 #endif /* NFTABLES_NFT_H */
-- 
2.41.0

[PATCH nft 2/3] nfnl_osf: rework nf_osf_parse_opt() and avoid "-Wstrict-overflow" warning

[Thomas Haller]

We almost can compile everything with "-Wstrict-overflow" (which depends
on the optimization level). In a quest to make that happen, rework
nf_osf_parse_opt(). Previously, gcc-13.2.1-1.fc38.x86_64 warned:

    $ gcc -Iinclude "-DDEFAULT_INCLUDE_PATH=\"/usr/local/etc\"" -c -o tmp.o src/nfnl_osf.c -Werror -Wstrict-overflow=5 -O3
    src/nfnl_osf.c: In function ‘nfnl_osf_load_fingerprints’:
    src/nfnl_osf.c:356:5: error: assuming signed overflow does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-Werror=strict-overflow]
      356 | int nfnl_osf_load_fingerprints(struct netlink_ctx *ctx, int del)
          |     ^~~~~~~~~~~~~~~~~~~~~~~~~~
    src/nfnl_osf.c:356:5: error: assuming signed overflow does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-Werror=strict-overflow]
    src/nfnl_osf.c:356:5: error: assuming signed overflow does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-Werror=strict-overflow]
    src/nfnl_osf.c:356:5: error: assuming signed overflow does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-Werror=strict-overflow]
    src/nfnl_osf.c:356:5: error: assuming signed overflow does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-Werror=strict-overflow]
    src/nfnl_osf.c:356:5: error: assuming signed overflow does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-Werror=strict-overflow]
    cc1: all warnings being treated as errors

The previous code was needlessly confusing. Keeping track of an index
variable "i" and a "ptr" was redundant. The signed "i" variable caused a
"-Wstrict-overflow" warning, but it can be dropped completely.

While at it, there is also almost no need to ever truncate the bits that
we parse. Only the callers of the new skip_delim_trunc() required the
truncation.

Also, introduce new skip_delim() and skip_delim_trunc() methods, which
point right *after* the delimiter to the next word.  Contrary to
nf_osf_strchr(), which leaves the pointer at the end of the previous
part.

Also, the parsing code using strchr() requires that the overall buffer
(obuf[olen]) is NUL terminated. And the caller in fact ensured that too.
There is no point in having a "olen" parameter, we require the string to
be NUL terminated (which already was implicitly required).  Drop the
"olen" parameter. On the other hand, it's unclear what ensures that we
don't overflow the "opt" output buffer. Pass a "optlen" parameter and
ensure we don't overflow the buffer.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/nfnl_osf.c | 128 ++++++++++++++++++++++---------------------------
 1 file changed, 58 insertions(+), 70 deletions(-)

diff --git a/src/nfnl_osf.c b/src/nfnl_osf.c
index 38a27a3683e2..f2b50fa9fc8e 100644
--- a/src/nfnl_osf.c
+++ b/src/nfnl_osf.c
@@ -74,6 +74,33 @@ static struct nf_osf_opt IANA_opts[] = {
 	{ .kind=26, .length=1,},
 };
 
+static char *skip_delim(char *ptr, char c)
+{
+	char *tmp;
+
+	tmp = strchr(ptr, c);
+	if (tmp) {
+		tmp++;
+		while (isspace(tmp[0]))
+			tmp++;
+	}
+	return tmp;
+}
+
+static char *skip_delim_trunc(char *ptr, char c)
+{
+	char *tmp;
+
+	tmp = strchr(ptr, c);
+	if (tmp) {
+		tmp[0] = '\0';
+		tmp++;
+		while (isspace(tmp[0]))
+			tmp++;
+	}
+	return tmp;
+}
+
 static char *nf_osf_strchr(char *ptr, char c)
 {
 	char *tmp;
@@ -88,54 +115,34 @@ static char *nf_osf_strchr(char *ptr, char c)
 	return tmp;
 }
 
-static void nf_osf_parse_opt(struct nf_osf_opt *opt, __u16 *optnum, char *obuf,
-			     int olen)
+static void nf_osf_parse_opt(struct nf_osf_opt *opt, __u16 optlen, __u16 *optnum, char *obuf)
 {
-	int i, op;
-	char *ptr, wc;
-	unsigned long val;
-
-	ptr = &obuf[0];
-	i = 0;
-	while (ptr != NULL && i < olen && *ptr != 0) {
-		val = 0;
-		wc = OSF_WSS_PLAIN;
-		switch (obuf[i]) {
+	char *ptr = &obuf[0];
+
+	while (ptr && ptr[0] != '\0') {
+		char *const ptr0 = ptr;
+		unsigned long val = 0;
+		char wc = OSF_WSS_PLAIN;
+		int op;
+
+		switch (ptr0[0]) {
 		case 'N':
 			op = OSFOPT_NOP;
-			ptr = nf_osf_strchr(&obuf[i], OPTDEL);
-			if (ptr) {
-				*ptr = '\0';
-				ptr++;
-				i += (int)(ptr - &obuf[i]);
-			} else
-				i++;
+			ptr = skip_delim(&ptr0[1], OPTDEL);
 			break;
 		case 'S':
 			op = OSFOPT_SACKP;
-			ptr = nf_osf_strchr(&obuf[i], OPTDEL);
-			if (ptr) {
-				*ptr = '\0';
-				ptr++;
-				i += (int)(ptr - &obuf[i]);
-			} else
-				i++;
+			ptr = skip_delim(&ptr0[1], OPTDEL);
 			break;
 		case 'T':
 			op = OSFOPT_TS;
-			ptr = nf_osf_strchr(&obuf[i], OPTDEL);
-			if (ptr) {
-				*ptr = '\0';
-				ptr++;
-				i += (int)(ptr - &obuf[i]);
-			} else
-				i++;
+			ptr = skip_delim(&ptr0[1], OPTDEL);
 			break;
 		case 'W':
 			op = OSFOPT_WSO;
-			ptr = nf_osf_strchr(&obuf[i], OPTDEL);
+			ptr = skip_delim_trunc(&ptr0[1], OPTDEL);
 			if (ptr) {
-				switch (obuf[i + 1]) {
+				switch (ptr0[1]) {
 				case '%':
 					wc = OSF_WSS_MODULO;
 					break;
@@ -149,56 +156,37 @@ static void nf_osf_parse_opt(struct nf_osf_opt *opt, __u16 *optnum, char *obuf,
 					wc = OSF_WSS_PLAIN;
 					break;
 				}
-
-				*ptr = '\0';
-				ptr++;
-				if (wc)
-					val = strtoul(&obuf[i + 2], NULL, 10);
+				if (wc != OSF_WSS_PLAIN)
+					val = strtoul(&ptr0[2], NULL, 10);
 				else
-					val = strtoul(&obuf[i + 1], NULL, 10);
-				i += (int)(ptr - &obuf[i]);
-
-			} else
-				i++;
+					val = strtoul(&ptr0[1], NULL, 10);
+			}
 			break;
 		case 'M':
 			op = OSFOPT_MSS;
-			ptr = nf_osf_strchr(&obuf[i], OPTDEL);
+			ptr = skip_delim_trunc(&ptr0[1], OPTDEL);
 			if (ptr) {
-				if (obuf[i + 1] == '%')
+				if (ptr0[1] == '%')
 					wc = OSF_WSS_MODULO;
-				*ptr = '\0';
-				ptr++;
-				if (wc)
-					val = strtoul(&obuf[i + 2], NULL, 10);
+				if (wc != OSF_WSS_PLAIN)
+					val = strtoul(&ptr0[2], NULL, 10);
 				else
-					val = strtoul(&obuf[i + 1], NULL, 10);
-				i += (int)(ptr - &obuf[i]);
-			} else
-				i++;
+					val = strtoul(&ptr0[1], NULL, 10);
+			}
 			break;
 		case 'E':
 			op = OSFOPT_EOL;
-			ptr = nf_osf_strchr(&obuf[i], OPTDEL);
-			if (ptr) {
-				*ptr = '\0';
-				ptr++;
-				i += (int)(ptr - &obuf[i]);
-			} else
-				i++;
+			ptr = skip_delim(&ptr0[1], OPTDEL);
 			break;
 		default:
 			op = OSFOPT_EMPTY;
-			ptr = nf_osf_strchr(&obuf[i], OPTDEL);
-			if (ptr) {
-				ptr++;
-				i += (int)(ptr - &obuf[i]);
-			} else
-				i++;
+			ptr = skip_delim(&ptr0[0], OPTDEL);
 			break;
 		}
 
 		if (op != OSFOPT_EMPTY) {
+			if (*optnum >= optlen)
+				return;
 			opt[*optnum].kind = IANA_opts[op].kind;
 			opt[*optnum].length = IANA_opts[op].length;
 			opt[*optnum].wc.wc = wc;
@@ -235,7 +223,7 @@ static int osf_load_line(char *buffer, int len, int del,
 		return -EINVAL;
 	}
 
-	memset(obuf, 0, sizeof(obuf));
+	obuf[0] = '\0';
 
 	pbeg = buffer;
 	pend = nf_osf_strchr(pbeg, OSFPDEL);
@@ -320,7 +308,7 @@ static int osf_load_line(char *buffer, int len, int del,
 		pbeg = pend + 1;
 	}
 
-	nf_osf_parse_opt(f.opt, &f.opt_num, obuf, sizeof(obuf));
+	nf_osf_parse_opt(f.opt, NFT_ARRAY_SIZE(f.opt), &f.opt_num, obuf);
 
 	memset(buf, 0, sizeof(buf));
 
-- 
2.41.0

[PATCH nft 3/3] netlink_linearize: avoid strict-overflow warning in netlink_gen_bitwise()

[Thomas Haller]

With gcc-13.2.1-1.fc38.x86_64:

  $ gcc -Iinclude -c -o tmp.o src/netlink_linearize.c -Werror -Wstrict-overflow=5 -O3
  src/netlink_linearize.c: In function ‘netlink_gen_bitwise’:
  src/netlink_linearize.c:1790:1: error: assuming signed overflow does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-Werror=strict-overflow]
   1790 | }
        | ^
  cc1: all warnings being treated as errors

It also makes more sense this way, where "n" is the hight of the
"binops" stack, and we check for a non-empty stack with "n > 0" and pop
the last element with "binops[--n]".

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/netlink_linearize.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index c91211582b3d..f2514b012a9d 100644
--- a/src/netlink_linearize.c
+++ b/src/netlink_linearize.c
@@ -712,14 +712,13 @@ static void netlink_gen_bitwise(struct netlink_linearize_ctx *ctx,
 	while (left->etype == EXPR_BINOP && left->left != NULL &&
 	       (left->op == OP_AND || left->op == OP_OR || left->op == OP_XOR))
 		binops[n++] = left = left->left;
-	n--;
 
-	netlink_gen_expr(ctx, binops[n--], dreg);
+	netlink_gen_expr(ctx, binops[--n], dreg);
 
 	mpz_bitmask(mask, expr->len);
 	mpz_set_ui(xor, 0);
-	for (; n >= 0; n--) {
-		i = binops[n];
+	while (n > 0) {
+		i = binops[--n];
 		mpz_set(val, i->right->value);
 
 		switch (i->op) {
-- 
2.41.0

Re: [PATCH nft 1/3] nft: add NFT_ARRAY_SIZE() helper

[Thomas Haller]

On Wed, 2023-09-27 at 14:23 +0200, Thomas Haller wrote:
> Add NFT_ARRAY_SIZE() macro, commonly known as ARRAY_SIZE() (or
> G_N_ELEMENTS()).
> 
> <nft.h> is the right place for macros and static-inline functions. It
> is
> included in *every* C sources, as it only depends on libc headers and
> <config.h>. NFT_ARRAY_SIZE() is part of the basic toolset, that
> should
> be available everywhere.
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  include/nft.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/nft.h b/include/nft.h
> index 9384054c11c8..4463b5c0fa4a 100644
> --- a/include/nft.h
> +++ b/include/nft.h
> @@ -8,4 +8,6 @@
>  #include <stdint.h>
>  #include <stdlib.h>
>  
> +#define NFT_ARRAY_SIZE(arr) (sizeof(arr)/sizeof((arr)[0]))
> +
>  #endif /* NFTABLES_NFT_H */

oh, I just found the "array_size()" macro. Didn't expect it to be
lower-case.

Will use that in v2.

Thomas

Re: [PATCH nft 1/3] nft: add NFT_ARRAY_SIZE() helper

[Pablo Neira Ayuso]

On Wed, Sep 27, 2023 at 04:24:19PM +0200, Thomas Haller wrote:
> On Wed, 2023-09-27 at 14:23 +0200, Thomas Haller wrote:
> > Add NFT_ARRAY_SIZE() macro, commonly known as ARRAY_SIZE() (or
> > G_N_ELEMENTS()).
> > 
> > <nft.h> is the right place for macros and static-inline functions. It
> > is
> > included in *every* C sources, as it only depends on libc headers and
> > <config.h>. NFT_ARRAY_SIZE() is part of the basic toolset, that
> > should
> > be available everywhere.
> > 
> > Signed-off-by: Thomas Haller <thaller@redhat.com>
> > ---
> >  include/nft.h | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/include/nft.h b/include/nft.h
> > index 9384054c11c8..4463b5c0fa4a 100644
> > --- a/include/nft.h
> > +++ b/include/nft.h
> > @@ -8,4 +8,6 @@
> >  #include <stdint.h>
> >  #include <stdlib.h>
> >  
> > +#define NFT_ARRAY_SIZE(arr) (sizeof(arr)/sizeof((arr)[0]))
> > +
> >  #endif /* NFTABLES_NFT_H */
> 
> oh, I just found the "array_size()" macro. Didn't expect it to be
> lower-case.
> 
> Will use that in v2.

If you read this before you post v2, then for this oneliner, I'd
suggest to squash it where it is used for the first time, otherwise it
is fine, not a deal breaker.

Re: [PATCH nft 2/3] nfnl_osf: rework nf_osf_parse_opt() and avoid "-Wstrict-overflow" warning

[Pablo Neira Ayuso]

On Wed, Sep 27, 2023 at 02:23:27PM +0200, Thomas Haller wrote:
> We almost can compile everything with "-Wstrict-overflow" (which depends
> on the optimization level). In a quest to make that happen, rework
> nf_osf_parse_opt(). Previously, gcc-13.2.1-1.fc38.x86_64 warned:
> 
>     $ gcc -Iinclude "-DDEFAULT_INCLUDE_PATH=\"/usr/local/etc\"" -c -o tmp.o src/nfnl_osf.c -Werror -Wstrict-overflow=5 -O3
>     src/nfnl_osf.c: In function ‘nfnl_osf_load_fingerprints’:
>     src/nfnl_osf.c:356:5: error: assuming signed overflow does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-Werror=strict-overflow]
>       356 | int nfnl_osf_load_fingerprints(struct netlink_ctx *ctx, int del)
>           |     ^~~~~~~~~~~~~~~~~~~~~~~~~~
>     src/nfnl_osf.c:356:5: error: assuming signed overflow does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-Werror=strict-overflow]
>     src/nfnl_osf.c:356:5: error: assuming signed overflow does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-Werror=strict-overflow]
>     src/nfnl_osf.c:356:5: error: assuming signed overflow does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-Werror=strict-overflow]
>     src/nfnl_osf.c:356:5: error: assuming signed overflow does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-Werror=strict-overflow]
>     src/nfnl_osf.c:356:5: error: assuming signed overflow does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-Werror=strict-overflow]
>     cc1: all warnings being treated as errors
> 
> The previous code was needlessly confusing. Keeping track of an index
> variable "i" and a "ptr" was redundant. The signed "i" variable caused a
> "-Wstrict-overflow" warning, but it can be dropped completely.
> 
> While at it, there is also almost no need to ever truncate the bits that
> we parse. Only the callers of the new skip_delim_trunc() required the
> truncation.
> 
> Also, introduce new skip_delim() and skip_delim_trunc() methods, which
> point right *after* the delimiter to the next word.  Contrary to
> nf_osf_strchr(), which leaves the pointer at the end of the previous
> part.
> 
> Also, the parsing code using strchr() requires that the overall buffer
> (obuf[olen]) is NUL terminated. And the caller in fact ensured that too.
> There is no point in having a "olen" parameter, we require the string to
> be NUL terminated (which already was implicitly required).  Drop the
> "olen" parameter. On the other hand, it's unclear what ensures that we
> don't overflow the "opt" output buffer. Pass a "optlen" parameter and
> ensure we don't overflow the buffer.

Nice.

IIRC, this code was copied and pasted from iptables. Maybe porting
this patch there would be also good.

BTW, did you test this patch with the pf.os file that nftables ships in?

Thanks!

Re: [PATCH nft 2/3] nfnl_osf: rework nf_osf_parse_opt() and avoid "-Wstrict-overflow" warning

[Thomas Haller]

On Wed, 2023-09-27 at 18:42 +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 27, 2023 at 02:23:27PM +0200, Thomas Haller wrote:
> > We almost can compile everything with "-Wstrict-overflow" (which
> > depends
> > on the optimization level). In a quest to make that happen, rework
> > nf_osf_parse_opt(). Previously, gcc-13.2.1-1.fc38.x86_64 warned:
> > 
> >     $ gcc -Iinclude "-DDEFAULT_INCLUDE_PATH=\"/usr/local/etc\"" -c
> > -o tmp.o src/nfnl_osf.c -Werror -Wstrict-overflow=5 -O3
> >     src/nfnl_osf.c: In function ‘nfnl_osf_load_fingerprints’:
> >     src/nfnl_osf.c:356:5: error: assuming signed overflow does not
> > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> > Werror=strict-overflow]
> >       356 | int nfnl_osf_load_fingerprints(struct netlink_ctx *ctx,
> > int del)
> >           |     ^~~~~~~~~~~~~~~~~~~~~~~~~~
> >     src/nfnl_osf.c:356:5: error: assuming signed overflow does not
> > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> > Werror=strict-overflow]
> >     src/nfnl_osf.c:356:5: error: assuming signed overflow does not
> > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> > Werror=strict-overflow]
> >     src/nfnl_osf.c:356:5: error: assuming signed overflow does not
> > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> > Werror=strict-overflow]
> >     src/nfnl_osf.c:356:5: error: assuming signed overflow does not
> > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> > Werror=strict-overflow]
> >     src/nfnl_osf.c:356:5: error: assuming signed overflow does not
> > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> > Werror=strict-overflow]
> >     cc1: all warnings being treated as errors
> > 
> > The previous code was needlessly confusing. Keeping track of an
> > index
> > variable "i" and a "ptr" was redundant. The signed "i" variable
> > caused a
> > "-Wstrict-overflow" warning, but it can be dropped completely.
> > 
> > While at it, there is also almost no need to ever truncate the bits
> > that
> > we parse. Only the callers of the new skip_delim_trunc() required
> > the
> > truncation.
> > 
> > Also, introduce new skip_delim() and skip_delim_trunc() methods,
> > which
> > point right *after* the delimiter to the next word.  Contrary to
> > nf_osf_strchr(), which leaves the pointer at the end of the
> > previous
> > part.
> > 
> > Also, the parsing code using strchr() requires that the overall
> > buffer
> > (obuf[olen]) is NUL terminated. And the caller in fact ensured that
> > too.
> > There is no point in having a "olen" parameter, we require the
> > string to
> > be NUL terminated (which already was implicitly required).  Drop
> > the
> > "olen" parameter. On the other hand, it's unclear what ensures that
> > we
> > don't overflow the "opt" output buffer. Pass a "optlen" parameter
> > and
> > ensure we don't overflow the buffer.
> 
> Nice.
> 
> IIRC, this code was copied and pasted from iptables. Maybe porting
> this patch there would be also good.

I will do that, after the patch was merged (and the final version
known).

> BTW, did you test this patch with the pf.os file that nftables ships
> in?

Right. I need to point out, that I did not test this. So it might be
horribly broken. My Fedora kernel builds without CONFIG_NFT_OSF, so the
shell tests are skipped.

How can pf.os used?


Thomas

Re: [PATCH nft 3/3] netlink_linearize: avoid strict-overflow warning in netlink_gen_bitwise()

[Thomas Haller]

On Wed, 2023-09-27 at 14:23 +0200, Thomas Haller wrote:
> With gcc-13.2.1-1.fc38.x86_64:
> 
>   $ gcc -Iinclude -c -o tmp.o src/netlink_linearize.c -Werror -
> Wstrict-overflow=5 -O3
>   src/netlink_linearize.c: In function ‘netlink_gen_bitwise’:
>   src/netlink_linearize.c:1790:1: error: assuming signed overflow
> does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> Werror=strict-overflow]
>    1790 | }
>         | ^
>   cc1: all warnings being treated as errors
> 
> It also makes more sense this way, where "n" is the hight of the
> "binops" stack, and we check for a non-empty stack with "n > 0" and
> pop
> the last element with "binops[--n]".
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  src/netlink_linearize.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
> index c91211582b3d..f2514b012a9d 100644
> --- a/src/netlink_linearize.c
> +++ b/src/netlink_linearize.c
> @@ -712,14 +712,13 @@ static void netlink_gen_bitwise(struct
> netlink_linearize_ctx *ctx,
>         while (left->etype == EXPR_BINOP && left->left != NULL &&
>                (left->op == OP_AND || left->op == OP_OR || left->op
> == OP_XOR))
>                 binops[n++] = left = left->left;

I wanted to ask, what ensures that binops buffer does not overflow?


> -       n--;
>  
> -       netlink_gen_expr(ctx, binops[n--], dreg);
> +       netlink_gen_expr(ctx, binops[--n], dreg);
>  
>         mpz_bitmask(mask, expr->len);
>         mpz_set_ui(xor, 0);
> -       for (; n >= 0; n--) {
> -               i = binops[n];
> +       while (n > 0) {
> +               i = binops[--n];
>                 mpz_set(val, i->right->value);
>  
>                 switch (i->op) {

Re: [PATCH nft 2/3] nfnl_osf: rework nf_osf_parse_opt() and avoid "-Wstrict-overflow" warning

[Pablo Neira Ayuso]

On Wed, Sep 27, 2023 at 07:04:57PM +0200, Thomas Haller wrote:
> On Wed, 2023-09-27 at 18:42 +0200, Pablo Neira Ayuso wrote:
> > On Wed, Sep 27, 2023 at 02:23:27PM +0200, Thomas Haller wrote:
> > > We almost can compile everything with "-Wstrict-overflow" (which
> > > depends
> > > on the optimization level). In a quest to make that happen, rework
> > > nf_osf_parse_opt(). Previously, gcc-13.2.1-1.fc38.x86_64 warned:
> > > 
> > >     $ gcc -Iinclude "-DDEFAULT_INCLUDE_PATH=\"/usr/local/etc\"" -c
> > > -o tmp.o src/nfnl_osf.c -Werror -Wstrict-overflow=5 -O3
> > >     src/nfnl_osf.c: In function ‘nfnl_osf_load_fingerprints’:
> > >     src/nfnl_osf.c:356:5: error: assuming signed overflow does not
> > > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> > > Werror=strict-overflow]
> > >       356 | int nfnl_osf_load_fingerprints(struct netlink_ctx *ctx,
> > > int del)
> > >           |     ^~~~~~~~~~~~~~~~~~~~~~~~~~
> > >     src/nfnl_osf.c:356:5: error: assuming signed overflow does not
> > > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> > > Werror=strict-overflow]
> > >     src/nfnl_osf.c:356:5: error: assuming signed overflow does not
> > > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> > > Werror=strict-overflow]
> > >     src/nfnl_osf.c:356:5: error: assuming signed overflow does not
> > > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> > > Werror=strict-overflow]
> > >     src/nfnl_osf.c:356:5: error: assuming signed overflow does not
> > > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> > > Werror=strict-overflow]
> > >     src/nfnl_osf.c:356:5: error: assuming signed overflow does not
> > > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> > > Werror=strict-overflow]
> > >     cc1: all warnings being treated as errors
> > > 
> > > The previous code was needlessly confusing. Keeping track of an
> > > index
> > > variable "i" and a "ptr" was redundant. The signed "i" variable
> > > caused a
> > > "-Wstrict-overflow" warning, but it can be dropped completely.
> > > 
> > > While at it, there is also almost no need to ever truncate the bits
> > > that
> > > we parse. Only the callers of the new skip_delim_trunc() required
> > > the
> > > truncation.
> > > 
> > > Also, introduce new skip_delim() and skip_delim_trunc() methods,
> > > which
> > > point right *after* the delimiter to the next word.  Contrary to
> > > nf_osf_strchr(), which leaves the pointer at the end of the
> > > previous
> > > part.
> > > 
> > > Also, the parsing code using strchr() requires that the overall
> > > buffer
> > > (obuf[olen]) is NUL terminated. And the caller in fact ensured that
> > > too.
> > > There is no point in having a "olen" parameter, we require the
> > > string to
> > > be NUL terminated (which already was implicitly required).  Drop
> > > the
> > > "olen" parameter. On the other hand, it's unclear what ensures that
> > > we
> > > don't overflow the "opt" output buffer. Pass a "optlen" parameter
> > > and
> > > ensure we don't overflow the buffer.
> > 
> > Nice.
> > 
> > IIRC, this code was copied and pasted from iptables. Maybe porting
> > this patch there would be also good.
> 
> I will do that, after the patch was merged (and the final version
> known).
> 
> > BTW, did you test this patch with the pf.os file that nftables ships
> > in?
> 
> Right. I need to point out, that I did not test this. So it might be
> horribly broken. My Fedora kernel builds without CONFIG_NFT_OSF, so the
> shell tests are skipped.
> 
> How can pf.os used?

According to code, pf.os file with signatures needs to be placed here:

#define OS_SIGNATURES DEFAULT_INCLUDE_PATH "/nftables/osf/pf.os"

then, you can start matching on OS type, see 'osf' expression in
manpage. Note there is a "unknown" OS type when it does not guess the OS.

Re: [PATCH nft 3/3] netlink_linearize: avoid strict-overflow warning in netlink_gen_bitwise()

[Pablo Neira Ayuso]

On Wed, Sep 27, 2023 at 07:06:26PM +0200, Thomas Haller wrote:
> On Wed, 2023-09-27 at 14:23 +0200, Thomas Haller wrote:
> > With gcc-13.2.1-1.fc38.x86_64:
> > 
> >   $ gcc -Iinclude -c -o tmp.o src/netlink_linearize.c -Werror -
> > Wstrict-overflow=5 -O3
> >   src/netlink_linearize.c: In function ‘netlink_gen_bitwise’:
> >   src/netlink_linearize.c:1790:1: error: assuming signed overflow
> > does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-
> > Werror=strict-overflow]
> >    1790 | }
> >         | ^
> >   cc1: all warnings being treated as errors
> > 
> > It also makes more sense this way, where "n" is the hight of the
> > "binops" stack, and we check for a non-empty stack with "n > 0" and
> > pop
> > the last element with "binops[--n]".
> > 
> > Signed-off-by: Thomas Haller <thaller@redhat.com>
> > ---
> >  src/netlink_linearize.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
> > index c91211582b3d..f2514b012a9d 100644
> > --- a/src/netlink_linearize.c
> > +++ b/src/netlink_linearize.c
> > @@ -712,14 +712,13 @@ static void netlink_gen_bitwise(struct
> > netlink_linearize_ctx *ctx,
> >         while (left->etype == EXPR_BINOP && left->left != NULL &&
> >                (left->op == OP_AND || left->op == OP_OR || left->op
> > == OP_XOR))
> >                 binops[n++] = left = left->left;
> 
> I wanted to ask, what ensures that binops buffer does not overflow?

This is stacking binops, then popping them out one by one to generate
code IIRC.

binops has 16 positions, if you manage to generate a large expression
with lots of bitwise, probably you can hit the buffer overflow.

Go explore :)

Re: [PATCH nft 2/3] nfnl_osf: rework nf_osf_parse_opt() and avoid "-Wstrict-overflow" warning

[Thomas Haller]

On Wed, 2023-09-27 at 19:11 +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 27, 2023 at 07:04:57PM +0200, Thomas Haller wrote:
> > 
> > How can pf.os used?
> 
> According to code, pf.os file with signatures needs to be placed
> here:
> 
> #define OS_SIGNATURES DEFAULT_INCLUDE_PATH "/nftables/osf/pf.os"
> 
> then, you can start matching on OS type, see 'osf' expression in
> manpage. Note there is a "unknown" OS type when it does not guess the
> OS.


Sorry, I don't follow. Testing this seems very cumbersome.

I suspect, the tests "tests/shell/testcases/sets/typeof_{sets,maps}_0"
might hit the code. But that test requires kernel support.

IMO the netfilter projects should require contributors to provide tests
(as sensible). That is, tests that are simply invoked via `make check`
and don't require to build special features in the kernel
(CONFIG_NFT_OSF).

Anyway. Let's hold this patch [2/3] back for now. And patch [1/3] is
obsolete too.

I have patches that would add unit tests to the project (merely as a
place where more unit tests could be added). I will add a test there.
But that is based on top of "no recursive make", and I'd like to get
that changed first.


Thomas

Re: [PATCH nft 2/3] nfnl_osf: rework nf_osf_parse_opt() and avoid "-Wstrict-overflow" warning

[Pablo Neira Ayuso]

On Wed, Sep 27, 2023 at 07:50:27PM +0200, Thomas Haller wrote:
> On Wed, 2023-09-27 at 19:11 +0200, Pablo Neira Ayuso wrote:
> > On Wed, Sep 27, 2023 at 07:04:57PM +0200, Thomas Haller wrote:
> > > 
> > > How can pf.os used?
> > 
> > According to code, pf.os file with signatures needs to be placed
> > here:
> > 
> > #define OS_SIGNATURES DEFAULT_INCLUDE_PATH "/nftables/osf/pf.os"
> > 
> > then, you can start matching on OS type, see 'osf' expression in
> > manpage. Note there is a "unknown" OS type when it does not guess the
> > OS.
> 
> Sorry, I don't follow. Testing this seems very cumbersome.

It requires kernel support and the pf.os file in place, yes.

> I suspect, the tests "tests/shell/testcases/sets/typeof_{sets,maps}_0"
> might hit the code. But that test requires kernel support.

This requires kernel support, yes.

> IMO the netfilter projects should require contributors to provide tests
> (as sensible). That is, tests that are simply invoked via `make check`
> and don't require to build special features in the kernel
> (CONFIG_NFT_OSF).

You mean, some way to exercise userspace code without involving the
kernel at all.

> Anyway. Let's hold this patch [2/3] back for now. And patch [1/3] is
> obsolete too.

OK, as you prefer.

> I have patches that would add unit tests to the project (merely as a
> place where more unit tests could be added). I will add a test there.

We have tests/py/ as unit tests, if that might look similar to what
you have in mind? Or are you thinking of more tests/shell/ scripts?

> But that is based on top of "no recursive make", and I'd like to get
> that changed first.

I would like to make a release before such change is applied, build
infrastructure and python support was messy in the previous release.
Then we look into this, OK?

Re: [PATCH nft 2/3] nfnl_osf: rework nf_osf_parse_opt() and avoid "-Wstrict-overflow" warning

[Thomas Haller]

On Wed, 2023-09-27 at 21:16 +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 27, 2023 at 07:50:27PM +0200, Thomas Haller wrote:
> 
> 
> > IMO the netfilter projects should require contributors to provide
> > tests
> > (as sensible). That is, tests that are simply invoked via `make
> > check`
> > and don't require to build special features in the kernel
> > (CONFIG_NFT_OSF).
> 
> You mean, some way to exercise userspace code without involving the
> kernel at all.

Yes, the relevant part is parsing some strings. That should be tested
in isolation. Or just to validate the pf.os file...

> 
> > I have patches that would add unit tests to the project (merely as
> > a
> > place where more unit tests could be added). I will add a test
> > there.
> 
> We have tests/py/ as unit tests, if that might look similar to what
> you have in mind? Or are you thinking of more tests/shell/ scripts?

Those only use the public API of libnftables.so. What would be also
useful, is to statically link against the code and have more immediate
access.

Also, currently they don't unshare and cannot run rootless. That should
be fixed by extending tests/shell/run-tests.sh script. Well, you
already hack that via `./tests/shell/run-tests.sh ./tests/py/nft-
test.py`, but this should integrate better.

It's waiting on the WIP branch:
https://gitlab.freedesktop.org/thaller/nftables/-/commits/th/no-recursive-make
https://gitlab.freedesktop.org/thaller/nftables/-/blob/545f40babb90584fd188ebe80a1103b93ba49707/tests/unit/test-libnftables-static.c#L177

> 

> > But that is based on top of "no recursive make", and I'd like to
> > get
> > that changed first.
> 
> I would like to make a release before such change is applied, build
> infrastructure and python support was messy in the previous release.
> Then we look into this, OK?

Sounds great. Thank you.


Thomas

Re: [PATCH nft 2/3] nfnl_osf: rework nf_osf_parse_opt() and avoid "-Wstrict-overflow" warning

[Pablo Neira Ayuso]

On Wed, Sep 27, 2023 at 10:11:15PM +0200, Thomas Haller wrote:
> On Wed, 2023-09-27 at 21:16 +0200, Pablo Neira Ayuso wrote:
> > On Wed, Sep 27, 2023 at 07:50:27PM +0200, Thomas Haller wrote:
> > 
> > 
> > > IMO the netfilter projects should require contributors to provide
> > > tests
> > > (as sensible). That is, tests that are simply invoked via `make
> > > check`
> > > and don't require to build special features in the kernel
> > > (CONFIG_NFT_OSF).
> > 
> > You mean, some way to exercise userspace code without involving the
> > kernel at all.
> 
> Yes, the relevant part is parsing some strings. That should be tested
> in isolation. Or just to validate the pf.os file...

I understand, that would also require some sort of dump of the
parsing, to validate this is correct. I think I understand what you
mean by unit test here: You could make a program that imports this
.c file, the parse pf.os and dump an output that you could validate in
some automated fashion.

This osf support from iptables, and tests/py (which was the automated
test infrastructure it had) was only added 2012, more than 10 years
after iptables was in place.

> > > I have patches that would add unit tests to the project (merely as
> > > a
> > > place where more unit tests could be added). I will add a test
> > > there.
> > 
> > We have tests/py/ as unit tests, if that might look similar to what
> > you have in mind? Or are you thinking of more tests/shell/ scripts?
> 
> Those only use the public API of libnftables.so. What would be also
> useful, is to statically link against the code and have more immediate
> access.

I see, some internal tests for private API then it is your idea, I am
all in for more test coverage.

> Also, currently they don't unshare and cannot run rootless. That should
> be fixed by extending tests/shell/run-tests.sh script. Well, you
> already hack that via `./tests/shell/run-tests.sh ./tests/py/nft-
> test.py`, but this should integrate better.

Yes, unshare and rootless for tests/py would be good to have if I
understood this correctly.

> It's waiting on the WIP branch:
> https://gitlab.freedesktop.org/thaller/nftables/-/commits/th/no-recursive-make
> https://gitlab.freedesktop.org/thaller/nftables/-/blob/545f40babb90584fd188ebe80a1103b93ba49707/tests/unit/test-libnftables-static.c#L177
> 
> > 
> 
> > > But that is based on top of "no recursive make", and I'd like to
> > > get
> > > that changed first.
> > 
> > I would like to make a release before such change is applied, build
> > infrastructure and python support was messy in the previous release.
> > Then we look into this, OK?
> 
> Sounds great. Thank you.

OK, let's prepare for release then.