[PATCH nft 0/5] fix compiler warnings with clang and "-Wextra"

[PATCH nft 0/5] fix compiler warnings with clang and "-Wextra"

[Thomas Haller]

More fixes (workarounds) for compiler warnings. Mostly by enabling
"-Wextra" with gcc.

Thomas Haller (5):
  rule: fix "const static" declaration
  utils: call abort() after BUG() macro
  src: silence "implicit-fallthrough" warnings
  xt: avoid "-Wmissing-field-initializers" for "original_opts"
  datatype: check against negative "type" argument in datatype_lookup()

 include/utils.h | 3 ++-
 src/datatype.c  | 2 +-
 src/optimize.c  | 1 +
 src/rule.c      | 4 ++--
 src/segtree.c   | 5 ++---
 src/xt.c        | 2 +-
 6 files changed, 9 insertions(+), 8 deletions(-)

-- 
2.41.0

[PATCH nft 1/5] rule: fix "const static" declaration

[Thomas Haller]

Gcc warns against this with "-Wextra":

    src/rule.c:869:1: error: static is not at beginning of declaration [-Werror=old-style-declaration]
      869 | const static struct prio_tag std_prios[] = {
          | ^~~~~
    src/rule.c:878:1: error: static is not at beginning of declaration [-Werror=old-style-declaration]
      878 | const static struct prio_tag bridge_std_prios[] = {
          | ^~~~~

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

diff --git a/src/rule.c b/src/rule.c
index 8ea606e146b2..bbea05d5b288 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -866,7 +866,7 @@ struct prio_tag {
 	const char *str;
 };
 
-const static struct prio_tag std_prios[] = {
+static const struct prio_tag std_prios[] = {
 	{ NF_IP_PRI_RAW,      "raw" },
 	{ NF_IP_PRI_MANGLE,   "mangle" },
 	{ NF_IP_PRI_NAT_DST,  "dstnat" },
@@ -875,7 +875,7 @@ const static struct prio_tag std_prios[] = {
 	{ NF_IP_PRI_NAT_SRC,  "srcnat" },
 };
 
-const static struct prio_tag bridge_std_prios[] = {
+static const struct prio_tag bridge_std_prios[] = {
 	{ NF_BR_PRI_NAT_DST_BRIDGED,  "dstnat" },
 	{ NF_BR_PRI_FILTER_BRIDGED,   "filter" },
 	{ NF_BR_PRI_NAT_DST_OTHER,    "out" },
-- 
2.41.0

[PATCH nft 2/5] utils: call abort() after BUG() macro

[Thomas Haller]

Otherwise, we get spurious warnings. The compiler should be aware that there is
no return from BUG(). Call abort() there, which is marked as __attribute__
((__noreturn__)).

    In file included from ./include/nftables.h:6,
                     from ./include/rule.h:4,
                     from src/payload.c:26:
    src/payload.c: In function 'icmp_dep_to_type':
    ./include/utils.h:39:34: error: this statement may fall through [-Werror=implicit-fallthrough=]
       39 | #define BUG(fmt, arg...)        ({ fprintf(stderr, "BUG: " fmt, ##arg); assert(0); })
          |                                 ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    src/payload.c:791:17: note: in expansion of macro 'BUG'
      791 |                 BUG("Invalid map for simple dependency");
          |                 ^~~
    src/payload.c:792:9: note: here
      792 |         case PROTO_ICMP_ECHO: return ICMP_ECHO;
          |         ^~~~

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

diff --git a/include/utils.h b/include/utils.h
index efc8dec013a1..5b8b181c1e99 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -3,6 +3,7 @@
 
 #include <asm/byteorder.h>
 #include <stdarg.h>
+#include <stdlib.h>
 #include <stdio.h>
 #include <unistd.h>
 #include <assert.h>
@@ -36,7 +37,7 @@
 #define __must_check		__attribute__((warn_unused_result))
 #define __noreturn		__attribute__((__noreturn__))
 
-#define BUG(fmt, arg...)	({ fprintf(stderr, "BUG: " fmt, ##arg); assert(0); })
+#define BUG(fmt, arg...)	({ fprintf(stderr, "BUG: " fmt, ##arg); assert(0); abort(); })
 
 #define BUILD_BUG_ON(condition)	((void)sizeof(char[1 - 2*!!(condition)]))
 #define BUILD_BUG_ON_ZERO(e)	(sizeof(char[1 - 2 * !!(e)]) - 1)
-- 
2.41.0

[PATCH nft 3/5] src: silence "implicit-fallthrough" warnings

[Thomas Haller]

Gcc with "-Wextra" warns:

    CC       segtree.lo
  segtree.c: In function 'get_set_interval_find':
  segtree.c:129:28: error: this statement may fall through [-Werror=implicit-fallthrough=]
    129 |                         if (expr_basetype(i->key)->type != TYPE_STRING)
        |                            ^
  segtree.c:134:17: note: here
    134 |                 case EXPR_PREFIX:
        |                 ^~~~

    CC       optimize.lo
  optimize.c: In function 'rule_collect_stmts':
  optimize.c:396:28: error: this statement may fall through [-Werror=implicit-fallthrough=]
    396 |                         if (stmt->expr->left->etype == EXPR_CONCAT) {
        |                            ^
  optimize.c:400:17: note: here
    400 |                 case STMT_VERDICT:
        |                 ^~~~

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/optimize.c | 1 +
 src/segtree.c  | 5 ++---
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/optimize.c b/src/optimize.c
index 0b99b6726115..9c1704831693 100644
--- a/src/optimize.c
+++ b/src/optimize.c
@@ -397,6 +397,7 @@ static int rule_collect_stmts(struct optimize_ctx *ctx, struct rule *rule)
 				clone->ops = &unsupported_stmt_ops;
 				break;
 			}
+			/* fall-through */
 		case STMT_VERDICT:
 			clone->expr = expr_get(stmt->expr);
 			break;
diff --git a/src/segtree.c b/src/segtree.c
index a265a0b30d64..bf207402c945 100644
--- a/src/segtree.c
+++ b/src/segtree.c
@@ -128,9 +128,8 @@ static struct expr *get_set_interval_find(const struct set *cache_set,
 		case EXPR_VALUE:
 			if (expr_basetype(i->key)->type != TYPE_STRING)
 				break;
-			/* string type, check if its a range (wildcard), so
-			 * fall through.
-			 */
+			/* string type, check if its a range (wildcard). */
+			/* fall-through */
 		case EXPR_PREFIX:
 		case EXPR_RANGE:
 			range_expr_value_low(val, i);
-- 
2.41.0

[PATCH nft 4/5] xt: avoid "-Wmissing-field-initializers" for "original_opts"

[Thomas Haller]

Avoid this warning with clang:

      CC       src/xt.lo
    src/xt.c:353:9: error: missing field 'has_arg' initializer [-Werror,-Wmissing-field-initializers]
            { NULL },
                   ^

The warning seems not very useful, because it's well understood that
specifying only some initializers leaves the remaining fields
initialized with the default. However, as this warning is only hit once
in the code base, it doesn't seem that we violate this style frequently.
Hence, fix it instead of disabling the warning.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/xt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/xt.c b/src/xt.c
index df7140b4fa97..a217cc7b6bd0 100644
--- a/src/xt.c
+++ b/src/xt.c
@@ -350,7 +350,7 @@ err:
 }
 
 static struct option original_opts[] = {
-	{ NULL },
+	{ },
 };
 
 static struct xtables_globals xt_nft_globals = {
-- 
2.41.0

[PATCH nft 5/5] datatype: check against negative "type" argument in datatype_lookup()

[Thomas Haller]

An enum can be either signed or unsigned (implementation defined).

datatype_lookup() checks for invalid type arguments. Also check, whether
the argument is not negative (which, depending on the compiler it may
never be).

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/datatype.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/datatype.c b/src/datatype.c
index ba1192c83595..91735ff8b360 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -87,7 +87,7 @@ const struct datatype *datatype_lookup(enum datatypes type)
 {
 	BUILD_BUG_ON(TYPE_MAX & ~TYPE_MASK);
 
-	if (type > TYPE_MAX)
+	if ((uintmax_t) type > TYPE_MAX)
 		return NULL;
 	return datatypes[type];
 }
-- 
2.41.0

Re: [PATCH nft 5/5] datatype: check against negative "type" argument in datatype_lookup()

[Pablo Neira Ayuso]

On Tue, Aug 29, 2023 at 08:54:11PM +0200, Thomas Haller wrote:
> An enum can be either signed or unsigned (implementation defined).
> 
> datatype_lookup() checks for invalid type arguments. Also check, whether
> the argument is not negative (which, depending on the compiler it may
> never be).
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  src/datatype.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/datatype.c b/src/datatype.c
> index ba1192c83595..91735ff8b360 100644
> --- a/src/datatype.c
> +++ b/src/datatype.c
> @@ -87,7 +87,7 @@ const struct datatype *datatype_lookup(enum datatypes type)
>  {
>  	BUILD_BUG_ON(TYPE_MAX & ~TYPE_MASK);
>  
> -	if (type > TYPE_MAX)
> +	if ((uintmax_t) type > TYPE_MAX)

            uint32_t ?

>  		return NULL;
>  	return datatypes[type];
>  }
> -- 
> 2.41.0
> 

Re: [PATCH nft 5/5] datatype: check against negative "type" argument in datatype_lookup()

[Pablo Neira Ayuso]

On Tue, Aug 29, 2023 at 09:10:26PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Aug 29, 2023 at 08:54:11PM +0200, Thomas Haller wrote:
> > An enum can be either signed or unsigned (implementation defined).
> > 
> > datatype_lookup() checks for invalid type arguments. Also check, whether
> > the argument is not negative (which, depending on the compiler it may
> > never be).
> > 
> > Signed-off-by: Thomas Haller <thaller@redhat.com>
> > ---
> >  src/datatype.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/datatype.c b/src/datatype.c
> > index ba1192c83595..91735ff8b360 100644
> > --- a/src/datatype.c
> > +++ b/src/datatype.c
> > @@ -87,7 +87,7 @@ const struct datatype *datatype_lookup(enum datatypes type)
> >  {
> >  	BUILD_BUG_ON(TYPE_MAX & ~TYPE_MASK);
> >  
> > -	if (type > TYPE_MAX)
> > +	if ((uintmax_t) type > TYPE_MAX)
> 
>             uint32_t ?

Another question: What warning does clang print on this one?
Description does not specify.

> >  		return NULL;
> >  	return datatypes[type];
> >  }
> > -- 
> > 2.41.0
> > 

Re: [PATCH nft 5/5] datatype: check against negative "type" argument in datatype_lookup()

[Thomas Haller]

On Tue, 2023-08-29 at 21:14 +0200, Pablo Neira Ayuso wrote:
> On Tue, Aug 29, 2023 at 09:10:26PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Aug 29, 2023 at 08:54:11PM +0200, Thomas Haller wrote:
> > > An enum can be either signed or unsigned (implementation
> > > defined).
> > > 
> > > datatype_lookup() checks for invalid type arguments. Also check,
> > > whether
> > > the argument is not negative (which, depending on the compiler it
> > > may
> > > never be).
> > > 
> > > Signed-off-by: Thomas Haller <thaller@redhat.com>
> > > ---
> > >  src/datatype.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/src/datatype.c b/src/datatype.c
> > > index ba1192c83595..91735ff8b360 100644
> > > --- a/src/datatype.c
> > > +++ b/src/datatype.c
> > > @@ -87,7 +87,7 @@ const struct datatype *datatype_lookup(enum
> > > datatypes type)
> > >  {
> > >         BUILD_BUG_ON(TYPE_MAX & ~TYPE_MASK);
> > >  
> > > -       if (type > TYPE_MAX)
> > > +       if ((uintmax_t) type > TYPE_MAX)
> > 
> >             uint32_t ?

The more straight forward way would be

    if (type < 0 || type > TYPE_MAX)

However, if the enum is unsigned, then the compiler might see that the
condition is never true and warn against that. It does warn, if "type"
were just an "unsigned int". I cannot actually reproduce a compiler
warning with the enum (for now).

The size of the enum is most likely int/unsigned (or smaller, with "-
fshort-enums" or packed). Is it on POSIX/Linux always guaranteed that
an int is 32bit? I think not, but I cannot find an architecture where
int is larger either. Also, if someone would add an enum value larger
than the 32 bit range, then the behavior is compiler dependent, but
most likely the enum type would be a 64 bit integer and
"uint"/"uint32_t" would not be the right check.

All of this is highly theoretical. But "uintmax_t" avoids all those
problems and makes fewer assumptions on what the enum actually is. Is
there a hypothetical scenario where it wouldn't work correctly?

> 
> Another question: What warning does clang print on this one?
> Description does not specify.

this one isn't about a compiler warning. Sorry, I should not have
included it in this set.


Thomas

Re: [PATCH nft 5/5] datatype: check against negative "type" argument in datatype_lookup()

[Pablo Neira Ayuso]

On Tue, Aug 29, 2023 at 09:58:53PM +0200, Thomas Haller wrote:
> On Tue, 2023-08-29 at 21:14 +0200, Pablo Neira Ayuso wrote:
> > On Tue, Aug 29, 2023 at 09:10:26PM +0200, Pablo Neira Ayuso wrote:
> > > On Tue, Aug 29, 2023 at 08:54:11PM +0200, Thomas Haller wrote:
> > > > An enum can be either signed or unsigned (implementation
> > > > defined).
> > > > 
> > > > datatype_lookup() checks for invalid type arguments. Also check,
> > > > whether
> > > > the argument is not negative (which, depending on the compiler it
> > > > may
> > > > never be).
> > > > 
> > > > Signed-off-by: Thomas Haller <thaller@redhat.com>
> > > > ---
> > > >  src/datatype.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/src/datatype.c b/src/datatype.c
> > > > index ba1192c83595..91735ff8b360 100644
> > > > --- a/src/datatype.c
> > > > +++ b/src/datatype.c
> > > > @@ -87,7 +87,7 @@ const struct datatype *datatype_lookup(enum
> > > > datatypes type)
> > > >  {
> > > >         BUILD_BUG_ON(TYPE_MAX & ~TYPE_MASK);
> > > >  
> > > > -       if (type > TYPE_MAX)
> > > > +       if ((uintmax_t) type > TYPE_MAX)
> > > 
> > >             uint32_t ?
> 
> The more straight forward way would be
> 
>     if (type < 0 || type > TYPE_MAX)
> 
> However, if the enum is unsigned, then the compiler might see that the
> condition is never true and warn against that. It does warn, if "type"
> were just an "unsigned int". I cannot actually reproduce a compiler
> warning with the enum (for now).

Then, better keep it back?

> The size of the enum is most likely int/unsigned (or smaller, with "-
> fshort-enums" or packed). Is it on POSIX/Linux always guaranteed that
> an int is 32bit? I think not, but I cannot find an architecture where
> int is larger either. Also, if someone would add an enum value larger
> than the 32 bit range, then the behavior is compiler dependent, but
> most likely the enum type would be a 64 bit integer and
> "uint"/"uint32_t" would not be the right check.

I don't expect to ever have such a large number of types. Specifically
because there are API restrictions that apply in this case.

> All of this is highly theoretical. But "uintmax_t" avoids all those
> problems and makes fewer assumptions on what the enum actually is. Is
> there a hypothetical scenario where it wouldn't work correctly?

I was trying to figure out what this is fixing.

> > Another question: What warning does clang print on this one?
> > Description does not specify.
> 
> this one isn't about a compiler warning. Sorry, I should not have
> included it in this set.

This TYPE_MAX will not ever become very large to require 64-bits.
With an implementation where enum is taken as signed, then this should
be sufficient too:

     if (type > TYPE_MAX)

If this is not fixing up anything right now, I would prefer to keep
this back.

I'll take this series except this one.

Thanks.

Re: [PATCH nft 5/5] datatype: check against negative "type" argument in datatype_lookup()

[Thomas Haller]

On Wed, 2023-08-30 at 09:46 +0200, Pablo Neira Ayuso wrote:
> On Tue, Aug 29, 2023 at 09:58:53PM +0200, Thomas Haller wrote:
> > On Tue, 2023-08-29 at 21:14 +0200, Pablo Neira Ayuso wrote:
> > > On Tue, Aug 29, 2023 at 09:10:26PM +0200, Pablo Neira Ayuso
> > > wrote:
> > > > On Tue, Aug 29, 2023 at 08:54:11PM +0200, Thomas Haller wrote:
> > > > > An enum can be either signed or unsigned (implementation
> > > > > defined).
> > > > > 
> > > > > datatype_lookup() checks for invalid type arguments. Also
> > > > > check,
> > > > > whether
> > > > > the argument is not negative (which, depending on the
> > > > > compiler it
> > > > > may
> > > > > never be).
> > > > > 
> > > > > Signed-off-by: Thomas Haller <thaller@redhat.com>
> > > > > ---
> > > > >  src/datatype.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/src/datatype.c b/src/datatype.c
> > > > > index ba1192c83595..91735ff8b360 100644
> > > > > --- a/src/datatype.c
> > > > > +++ b/src/datatype.c
> > > > > @@ -87,7 +87,7 @@ const struct datatype *datatype_lookup(enum
> > > > > datatypes type)
> > > > >  {
> > > > >         BUILD_BUG_ON(TYPE_MAX & ~TYPE_MASK);
> > > > >  
> > > > > -       if (type > TYPE_MAX)
> > > > > +       if ((uintmax_t) type > TYPE_MAX)
> > > > 
> > > >             uint32_t ?
> > 
> > The more straight forward way would be
> > 
> >     if (type < 0 || type > TYPE_MAX)
> > 
> > However, if the enum is unsigned, then the compiler might see that
> > the
> > condition is never true and warn against that. It does warn, if
> > "type"
> > were just an "unsigned int". I cannot actually reproduce a compiler
> > warning with the enum (for now).
> 
> Then, better keep it back?
> 
> > The size of the enum is most likely int/unsigned (or smaller, with
> > "-
> > fshort-enums" or packed). Is it on POSIX/Linux always guaranteed
> > that
> > an int is 32bit? I think not, but I cannot find an architecture
> > where
> > int is larger either. Also, if someone would add an enum value
> > larger
> > than the 32 bit range, then the behavior is compiler dependent, but
> > most likely the enum type would be a 64 bit integer and
> > "uint"/"uint32_t" would not be the right check.
> 
> I don't expect to ever have such a large number of types.
> Specifically
> because there are API restrictions that apply in this case.
> 
> > All of this is highly theoretical. But "uintmax_t" avoids all those
> > problems and makes fewer assumptions on what the enum actually is.
> > Is
> > there a hypothetical scenario where it wouldn't work correctly?
> 
> I was trying to figure out what this is fixing.
> 
> > > Another question: What warning does clang print on this one?
> > > Description does not specify.
> > 
> > this one isn't about a compiler warning. Sorry, I should not have
> > included it in this set.
> 
> This TYPE_MAX will not ever become very large to require 64-bits.

TYPE_MAX is not relevant.

> With an implementation where enum is taken as signed, then this
> should
> be sufficient too:
> 
>      if (type > TYPE_MAX)
> 
> If this is not fixing up anything right now, I would prefer to keep
> this back.

I don't think it suffices. The following fail the assertion (or would
access out of bounds).


diff --git c/include/datatype.h i/include/datatype.h
index 9ce7359cd340..7d3b6b20d27c 100644
--- c/include/datatype.h
+++ i/include/datatype.h
@@ -98,7 +98,8 @@ enum datatypes {
     TYPE_TIME_HOUR,
     TYPE_TIME_DAY,
     TYPE_CGROUPV2,
-    __TYPE_MAX
+    __TYPE_MAX,
+    __TYPE_FORCE_SIGNED = -1,
 };
 #define TYPE_MAX        (__TYPE_MAX - 1)
 
diff --git c/src/datatype.c i/src/datatype.c
index ba1192c83595..1ff8a4a08551 100644
--- c/src/datatype.c
+++ i/src/datatype.c
@@ -89,6 +89,7 @@ const struct datatype *datatype_lookup(enum datatypes
type)
 
     if (type > TYPE_MAX)
          return NULL;
+    assert(type != (enum datatypes) -1);
     return datatypes[type];
 }
 
diff --git c/src/libnftables.c i/src/libnftables.c
index 9c802ec95f27..7e60d1a18d39 100644
--- c/src/libnftables.c
+++ i/src/libnftables.c
@@ -203,6 +203,8 @@ struct nft_ctx *nft_ctx_new(uint32_t flags)
 #endif
     }
 
+    datatype_lookup(-1);
+
     ctx = xzalloc(sizeof(struct nft_ctx));
     nft_init(ctx);
 



If you expect that "type" is always valid, then there is no need to
check against >TYPE_MAX. If you expect that it might be invalid, it
seems prudent to also check against negative values.



Thomas

Re: [PATCH nft 5/5] datatype: check against negative "type" argument in datatype_lookup()

[Pablo Neira Ayuso]

On Wed, Aug 30, 2023 at 10:08:50AM +0200, Thomas Haller wrote:
[...]
> I don't think it suffices. The following fail the assertion (or would
> access out of bounds).
> 
> 
> diff --git c/include/datatype.h i/include/datatype.h
> index 9ce7359cd340..7d3b6b20d27c 100644
> --- c/include/datatype.h
> +++ i/include/datatype.h
> @@ -98,7 +98,8 @@ enum datatypes {
>      TYPE_TIME_HOUR,
>      TYPE_TIME_DAY,
>      TYPE_CGROUPV2,
> -    __TYPE_MAX
> +    __TYPE_MAX,
> +    __TYPE_FORCE_SIGNED = -1,

I don't expect to ever have a negative defined here.

>  };
>  #define TYPE_MAX        (__TYPE_MAX - 1)
>  
> diff --git c/src/datatype.c i/src/datatype.c
> index ba1192c83595..1ff8a4a08551 100644
> --- c/src/datatype.c
> +++ i/src/datatype.c
> @@ -89,6 +89,7 @@ const struct datatype *datatype_lookup(enum datatypes
> type)
>  
>      if (type > TYPE_MAX)
>           return NULL;
> +    assert(type != (enum datatypes) -1);
>      return datatypes[type];
>  }
>  
> diff --git c/src/libnftables.c i/src/libnftables.c
> index 9c802ec95f27..7e60d1a18d39 100644
> --- c/src/libnftables.c
> +++ i/src/libnftables.c
> @@ -203,6 +203,8 @@ struct nft_ctx *nft_ctx_new(uint32_t flags)
>  #endif
>      }
>  
> +    datatype_lookup(-1);
> +
>      ctx = xzalloc(sizeof(struct nft_ctx));
>      nft_init(ctx);
>  
> 
> 
> 
> If you expect that "type" is always valid, then there is no need to
> check against >TYPE_MAX. If you expect that it might be invalid, it
> seems prudent to also check against negative values.
> 
> 
> 
> Thomas
>