All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH nft v2 0/8] fix compiler warnings with clang
@ 2023-08-29 12:53 Thomas Haller
  2023-08-29 12:53 ` [PATCH nft v2 1/8] netlink: avoid "-Wenum-conversion" warning in dtype_map_from_kernel() Thomas Haller
                   ` (8 more replies)
  0 siblings, 9 replies; 13+ messages in thread
From: Thomas Haller @ 2023-08-29 12:53 UTC (permalink / raw)
  To: NetFilter; +Cc: Thomas Haller

Building with clang caused some compiler warnings. Fix, suppress or work
around them.

Changes to v1:
- replace patches
    "src: use "%zx" format instead of "%Zx""
    "utils: add _NFT_PRAGMA_WARNING_DISABLE()/_NFT_PRAGMA_WARNING_REENABLE helpers"
    "datatype: suppress "-Wformat-nonliteral" warning in integer_type_print()"
  with
    "include: drop "format" attribute from nft_gmp_print()"
  which is the better solution.
- let SNPRINTF_BUFFER_SIZE() not assert against truncation. Instead, the
  callers handle it.
- add bugfix "evaluate: fix check for truncation in stmt_evaluate_log_prefix()"
- add minor patch "evaluate: don't needlessly clear full string buffer in stmt_evaluate_log_prefix()"

Thomas Haller (8):
  netlink: avoid "-Wenum-conversion" warning in dtype_map_from_kernel()
  netlink: avoid "-Wenum-conversion" warning in parser_bison.y
  datatype: avoid cast-align warning with struct sockaddr result from
    getaddrinfo()
  evaluate: fix check for truncation in stmt_evaluate_log_prefix()
  src: rework SNPRINTF_BUFFER_SIZE() and handle truncation
  evaluate: don't needlessly clear full string buffer in
    stmt_evaluate_log_prefix()
  src: suppress "-Wunused-but-set-variable" warning with
    "parser_bison.c"
  include: drop "format" attribute from nft_gmp_print()

 include/nftables.h |  3 +--
 include/utils.h    | 35 ++++++++++++++++++++++++++---------
 src/Makefile.am    |  1 +
 src/datatype.c     | 14 +++++++++++---
 src/evaluate.c     | 15 ++++++++++-----
 src/meta.c         | 11 ++++++-----
 src/netlink.c      |  2 +-
 src/parser_bison.y |  4 ++--
 8 files changed, 58 insertions(+), 27 deletions(-)

-- 
2.41.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH nft v2 1/8] netlink: avoid "-Wenum-conversion" warning in dtype_map_from_kernel()
  2023-08-29 12:53 [PATCH nft v2 0/8] fix compiler warnings with clang Thomas Haller
@ 2023-08-29 12:53 ` Thomas Haller
  2023-08-29 12:53 ` [PATCH nft v2 2/8] netlink: avoid "-Wenum-conversion" warning in parser_bison.y Thomas Haller
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 13+ messages in thread
From: Thomas Haller @ 2023-08-29 12:53 UTC (permalink / raw)
  To: NetFilter; +Cc: Thomas Haller

Clang warns:

    netlink.c:806:26: error: implicit conversion from enumeration type 'enum nft_data_types' to different enumeration type 'enum datatypes' [-Werror,-Wenum-conversion]
                    return datatype_lookup(type);
                           ~~~~~~~~~~~~~~~ ^~~~

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

diff --git a/src/netlink.c b/src/netlink.c
index e1904a99d3ba..1afe162ec79b 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -803,7 +803,7 @@ static const struct datatype *dtype_map_from_kernel(enum nft_data_types type)
 	default:
 		if (type & ~TYPE_MASK)
 			return concat_type_alloc(type);
-		return datatype_lookup(type);
+		return datatype_lookup((enum datatypes) type);
 	}
 }
 
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH nft v2 2/8] netlink: avoid "-Wenum-conversion" warning in parser_bison.y
  2023-08-29 12:53 [PATCH nft v2 0/8] fix compiler warnings with clang Thomas Haller
  2023-08-29 12:53 ` [PATCH nft v2 1/8] netlink: avoid "-Wenum-conversion" warning in dtype_map_from_kernel() Thomas Haller
@ 2023-08-29 12:53 ` Thomas Haller
  2023-08-29 12:53 ` [PATCH nft v2 3/8] datatype: avoid cast-align warning with struct sockaddr result from getaddrinfo() Thomas Haller
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 13+ messages in thread
From: Thomas Haller @ 2023-08-29 12:53 UTC (permalink / raw)
  To: NetFilter; +Cc: Thomas Haller

Clang warns:

    parser_bison.y:3658:83: error: implicit conversion from enumeration type 'enum nft_nat_types' to different enumeration type 'enum nft_nat_etypes' [-Werror,-Wenum-conversion]
                                            { (yyval.stmt) = nat_stmt_alloc(&(yyloc), NFT_NAT_SNAT); }
                                                             ~~~~~~~~~~~~~~           ^~~~~~~~~~~~
    parser_bison.y:3659:83: error: implicit conversion from enumeration type 'enum nft_nat_types' to different enumeration type 'enum nft_nat_etypes' [-Werror,-Wenum-conversion]
                                            { (yyval.stmt) = nat_stmt_alloc(&(yyloc), NFT_NAT_DNAT); }
                                                             ~~~~~~~~~~~~~~           ^~~~~~~~~~~~

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

diff --git a/src/parser_bison.y b/src/parser_bison.y
index 5637829332ce..9e7f5c829c54 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -3655,8 +3655,8 @@ reject_opts		:       /* empty */
 nat_stmt		:	nat_stmt_alloc	nat_stmt_args
 			;
 
-nat_stmt_alloc		:	SNAT	{ $$ = nat_stmt_alloc(&@$, NFT_NAT_SNAT); }
-			|	DNAT	{ $$ = nat_stmt_alloc(&@$, NFT_NAT_DNAT); }
+nat_stmt_alloc		:	SNAT	{ $$ = nat_stmt_alloc(&@$, __NFT_NAT_SNAT); }
+			|	DNAT	{ $$ = nat_stmt_alloc(&@$, __NFT_NAT_DNAT); }
 			;
 
 tproxy_stmt		:	TPROXY TO stmt_expr
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH nft v2 3/8] datatype: avoid cast-align warning with struct sockaddr result from getaddrinfo()
  2023-08-29 12:53 [PATCH nft v2 0/8] fix compiler warnings with clang Thomas Haller
  2023-08-29 12:53 ` [PATCH nft v2 1/8] netlink: avoid "-Wenum-conversion" warning in dtype_map_from_kernel() Thomas Haller
  2023-08-29 12:53 ` [PATCH nft v2 2/8] netlink: avoid "-Wenum-conversion" warning in parser_bison.y Thomas Haller
@ 2023-08-29 12:53 ` Thomas Haller
  2023-08-29 12:53 ` [PATCH nft v2 4/8] evaluate: fix check for truncation in stmt_evaluate_log_prefix() Thomas Haller
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 13+ messages in thread
From: Thomas Haller @ 2023-08-29 12:53 UTC (permalink / raw)
  To: NetFilter; +Cc: Thomas Haller

With CC=clang we get

    datatype.c:625:11: error: cast from 'struct sockaddr *' to 'struct sockaddr_in *' increases required alignment from 2 to 4 [-Werror,-Wcast-align]
                    addr = ((struct sockaddr_in *)ai->ai_addr)->sin_addr;
                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    datatype.c:690:11: error: cast from 'struct sockaddr *' to 'struct sockaddr_in6 *' increases required alignment from 2 to 4 [-Werror,-Wcast-align]
                    addr = ((struct sockaddr_in6 *)ai->ai_addr)->sin6_addr;
                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    datatype.c:826:11: error: cast from 'struct sockaddr *' to 'struct sockaddr_in *' increases required alignment from 2 to 4 [-Werror,-Wcast-align]
                    port = ((struct sockaddr_in *)ai->ai_addr)->sin_port;
                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Fix that by casting to (void*) first. Also, add an assertion that the
type is as expected.

For inet_service_type_parse(), differentiate between AF_INET and
AF_INET6. It might not have been a problem in practice, because the
struct offsets of sin_port/sin6_port are identical.

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

diff --git a/src/datatype.c b/src/datatype.c
index dd6a5fbf5df8..df2e500502fa 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -622,7 +622,8 @@ static struct error_record *ipaddr_type_parse(struct parse_ctx *ctx,
 			return error(&sym->location,
 				     "Hostname resolves to multiple addresses");
 		}
-		addr = ((struct sockaddr_in *)ai->ai_addr)->sin_addr;
+		assert(ai->ai_addr->sa_family == AF_INET);
+		addr = ((struct sockaddr_in *) (void *) ai->ai_addr)->sin_addr;
 		freeaddrinfo(ai);
 	}
 
@@ -687,7 +688,9 @@ static struct error_record *ip6addr_type_parse(struct parse_ctx *ctx,
 			return error(&sym->location,
 				     "Hostname resolves to multiple addresses");
 		}
-		addr = ((struct sockaddr_in6 *)ai->ai_addr)->sin6_addr;
+
+		assert(ai->ai_addr->sa_family == AF_INET6);
+		addr = ((struct sockaddr_in6 *)(void *)ai->ai_addr)->sin6_addr;
 		freeaddrinfo(ai);
 	}
 
@@ -823,7 +826,12 @@ static struct error_record *inet_service_type_parse(struct parse_ctx *ctx,
 			return error(&sym->location, "Could not resolve service: %s",
 				     gai_strerror(err));
 
-		port = ((struct sockaddr_in *)ai->ai_addr)->sin_port;
+		if (ai->ai_addr->sa_family == AF_INET)
+			port = ((struct sockaddr_in *)(void *)ai->ai_addr)->sin_port;
+		else {
+			assert(ai->ai_addr->sa_family == AF_INET6);
+			port = ((struct sockaddr_in6 *)(void *)ai->ai_addr)->sin6_port;
+		}
 		freeaddrinfo(ai);
 	}
 
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH nft v2 4/8] evaluate: fix check for truncation in stmt_evaluate_log_prefix()
  2023-08-29 12:53 [PATCH nft v2 0/8] fix compiler warnings with clang Thomas Haller
                   ` (2 preceding siblings ...)
  2023-08-29 12:53 ` [PATCH nft v2 3/8] datatype: avoid cast-align warning with struct sockaddr result from getaddrinfo() Thomas Haller
@ 2023-08-29 12:53 ` Thomas Haller
  2023-08-29 12:53 ` [PATCH nft v2 5/8] src: rework SNPRINTF_BUFFER_SIZE() and handle truncation Thomas Haller
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 13+ messages in thread
From: Thomas Haller @ 2023-08-29 12:53 UTC (permalink / raw)
  To: NetFilter; +Cc: Thomas Haller

Fixes: e76bb3794018 ('src: allow for variables in the log prefix string')

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

diff --git a/src/evaluate.c b/src/evaluate.c
index 1ae2ef0de10c..77e3838e2692 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -4153,7 +4153,7 @@ static int stmt_evaluate_log_prefix(struct eval_ctx *ctx, struct stmt *stmt)
 		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 	}
 
-	if (len == NF_LOG_PREFIXLEN)
+	if (len == 0)
 		return stmt_error(ctx, stmt, "log prefix is too long");
 
 	expr = constant_expr_alloc(&stmt->log.prefix->location, &string_type,
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH nft v2 5/8] src: rework SNPRINTF_BUFFER_SIZE() and handle truncation
  2023-08-29 12:53 [PATCH nft v2 0/8] fix compiler warnings with clang Thomas Haller
                   ` (3 preceding siblings ...)
  2023-08-29 12:53 ` [PATCH nft v2 4/8] evaluate: fix check for truncation in stmt_evaluate_log_prefix() Thomas Haller
@ 2023-08-29 12:53 ` Thomas Haller
  2023-08-29 12:53 ` [PATCH nft v2 6/8] evaluate: don't needlessly clear full string buffer in stmt_evaluate_log_prefix() Thomas Haller
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 13+ messages in thread
From: Thomas Haller @ 2023-08-29 12:53 UTC (permalink / raw)
  To: NetFilter; +Cc: Thomas Haller

Before, the macro asserts against truncation. This is despite the
callers still checked for truncation and tried to handle it. Probably
for good reason. With stmt_evaluate_log_prefix() it's not clear that the
code ensures that truncation cannot happen, so we must not assert
against it, but handle it.

Also,

- wrap the macro in "do { ... } while(0)" to make it more
  function-like.

- evaluate macro arguments exactly once, to make it more function-like.

- take pointers to the arguments that are being modified.

- use assert() instead of abort().

- use size_t type for arguments related to the buffer size.

- drop "size". It was mostly redundant to "offset". We can know
  everything we want based on "len" and "offset" alone.

- "offset" previously was incremented before checking for truncation.
  So it would point somewhere past the buffer. This behavior does not
  seem useful. Instead, on truncation "len" will be zero (as before) and
  "offset" will point one past the buffer (one past the terminating
  NUL).

Thereby, also fix a warning from clang:

    evaluate.c:4134:9: error: variable 'size' set but not used [-Werror,-Wunused-but-set-variable]
            size_t size = 0;
                   ^

    meta.c:1006:9: error: variable 'size' set but not used [-Werror,-Wunused-but-set-variable]
            size_t size;
                   ^

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 include/utils.h | 35 ++++++++++++++++++++++++++---------
 src/evaluate.c  |  8 +++++---
 src/meta.c      | 11 ++++++-----
 3 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/include/utils.h b/include/utils.h
index cee1e5c1e8ae..efc8dec013a1 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -72,15 +72,32 @@
 #define max(_x, _y) ({				\
 	_x > _y ? _x : _y; })
 
-#define SNPRINTF_BUFFER_SIZE(ret, size, len, offset)	\
-	if (ret < 0)					\
-		abort();				\
-	offset += ret;					\
-	assert(ret < len);				\
-	if (ret > len)					\
-		ret = len;				\
-	size += ret;					\
-	len -= ret;
+#define SNPRINTF_BUFFER_SIZE(ret, len, offset)			\
+	({ \
+		const int _ret = (ret);				\
+		size_t *const _len = (len);			\
+		size_t *const _offset = (offset);		\
+		bool _not_truncated = true;			\
+		size_t _ret2;					\
+								\
+		assert(_ret >= 0);				\
+								\
+		if ((size_t) _ret >= *_len) {			\
+			/* Truncated.
+			 *
+			 * We will leave "len" at zero and increment
+			 * "offset" to point one byte after the buffer
+			 * (after the terminating NUL byte). */	\
+			_ret2 = *_len;				\
+			_not_truncated = false;			\
+		} else						\
+			_ret2 = (size_t) _ret;			\
+								\
+		*_offset += _ret2;				\
+		*_len -= _ret2;					\
+								\
+		_not_truncated;					\
+	})
 
 #define MSEC_PER_SEC	1000L
 
diff --git a/src/evaluate.c b/src/evaluate.c
index 77e3838e2692..71e94a51cc2f 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -4129,14 +4129,16 @@ static int stmt_evaluate_queue(struct eval_ctx *ctx, struct stmt *stmt)
 static int stmt_evaluate_log_prefix(struct eval_ctx *ctx, struct stmt *stmt)
 {
 	char prefix[NF_LOG_PREFIXLEN] = {}, tmp[NF_LOG_PREFIXLEN] = {};
-	int len = sizeof(prefix), offset = 0, ret;
+	size_t len = sizeof(prefix);
+	size_t offset = 0;
 	struct expr *expr;
-	size_t size = 0;
 
 	if (stmt->log.prefix->etype != EXPR_LIST)
 		return 0;
 
 	list_for_each_entry(expr, &stmt->log.prefix->expressions, list) {
+		int ret;
+
 		switch (expr->etype) {
 		case EXPR_VALUE:
 			expr_to_string(expr, tmp);
@@ -4150,7 +4152,7 @@ static int stmt_evaluate_log_prefix(struct eval_ctx *ctx, struct stmt *stmt)
 			BUG("unknown expression type %s\n", expr_name(expr));
 			break;
 		}
-		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+		SNPRINTF_BUFFER_SIZE(ret, &len, &offset);
 	}
 
 	if (len == 0)
diff --git a/src/meta.c b/src/meta.c
index 4f383269d032..ea00f2396b99 100644
--- a/src/meta.c
+++ b/src/meta.c
@@ -999,11 +999,11 @@ struct error_record *meta_key_parse(const struct location *loc,
                                     const char *str,
                                     unsigned int *value)
 {
-	int ret, len, offset = 0;
 	const char *sep = "";
+	size_t offset = 0;
 	unsigned int i;
 	char buf[1024];
-	size_t size;
+	size_t len;
 
 	for (i = 0; i < array_size(meta_templates); i++) {
 		if (!meta_templates[i].token || strcmp(meta_templates[i].token, str))
@@ -1026,9 +1026,10 @@ struct error_record *meta_key_parse(const struct location *loc,
 	}
 
 	len = (int)sizeof(buf);
-	size = sizeof(buf);
 
 	for (i = 0; i < array_size(meta_templates); i++) {
+		int ret;
+
 		if (!meta_templates[i].token)
 			continue;
 
@@ -1036,8 +1037,8 @@ struct error_record *meta_key_parse(const struct location *loc,
 			sep = ", ";
 
 		ret = snprintf(buf+offset, len, "%s%s", sep, meta_templates[i].token);
-		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
-		assert(offset < (int)sizeof(buf));
+		SNPRINTF_BUFFER_SIZE(ret, &len, &offset);
+		assert(len > 0);
 	}
 
 	return error(loc, "syntax error, unexpected %s, known keys are %s", str, buf);
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH nft v2 6/8] evaluate: don't needlessly clear full string buffer in stmt_evaluate_log_prefix()
  2023-08-29 12:53 [PATCH nft v2 0/8] fix compiler warnings with clang Thomas Haller
                   ` (4 preceding siblings ...)
  2023-08-29 12:53 ` [PATCH nft v2 5/8] src: rework SNPRINTF_BUFFER_SIZE() and handle truncation Thomas Haller
@ 2023-08-29 12:53 ` Thomas Haller
  2023-08-29 18:08   ` Pablo Neira Ayuso
  2023-08-29 12:53 ` [PATCH nft v2 7/8] src: suppress "-Wunused-but-set-variable" warning with "parser_bison.c" Thomas Haller
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 13+ messages in thread
From: Thomas Haller @ 2023-08-29 12:53 UTC (permalink / raw)
  To: NetFilter; +Cc: Thomas Haller

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

diff --git a/src/evaluate.c b/src/evaluate.c
index 71e94a51cc2f..bfe3638bfe54 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -4128,7 +4128,8 @@ static int stmt_evaluate_queue(struct eval_ctx *ctx, struct stmt *stmt)
 
 static int stmt_evaluate_log_prefix(struct eval_ctx *ctx, struct stmt *stmt)
 {
-	char prefix[NF_LOG_PREFIXLEN] = {}, tmp[NF_LOG_PREFIXLEN] = {};
+	char tmp[NF_LOG_PREFIXLEN] = {};
+	char prefix[NF_LOG_PREFIXLEN];
 	size_t len = sizeof(prefix);
 	size_t offset = 0;
 	struct expr *expr;
@@ -4136,6 +4137,8 @@ static int stmt_evaluate_log_prefix(struct eval_ctx *ctx, struct stmt *stmt)
 	if (stmt->log.prefix->etype != EXPR_LIST)
 		return 0;
 
+	prefix[0] = '\0';
+
 	list_for_each_entry(expr, &stmt->log.prefix->expressions, list) {
 		int ret;
 
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH nft v2 7/8] src: suppress "-Wunused-but-set-variable" warning with "parser_bison.c"
  2023-08-29 12:53 [PATCH nft v2 0/8] fix compiler warnings with clang Thomas Haller
                   ` (5 preceding siblings ...)
  2023-08-29 12:53 ` [PATCH nft v2 6/8] evaluate: don't needlessly clear full string buffer in stmt_evaluate_log_prefix() Thomas Haller
@ 2023-08-29 12:53 ` Thomas Haller
  2023-08-29 12:53 ` [PATCH nft v2 8/8] include: drop "format" attribute from nft_gmp_print() Thomas Haller
  2023-08-29 15:00 ` [PATCH nft v2 0/8] fix compiler warnings with clang Pablo Neira Ayuso
  8 siblings, 0 replies; 13+ messages in thread
From: Thomas Haller @ 2023-08-29 12:53 UTC (permalink / raw)
  To: NetFilter; +Cc: Thomas Haller

Clang warns:

    parser_bison.c:7606:9: error: variable 'nft_nerrs' set but not used [-Werror,-Wunused-but-set-variable]
        int yynerrs = 0;
            ^
    parser_bison.c:72:25: note: expanded from macro 'yynerrs'
    #define yynerrs         nft_nerrs
                            ^

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

diff --git a/src/Makefile.am b/src/Makefile.am
index ad22a918c120..63a4ef43dae3 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -88,6 +88,7 @@ libparser_la_CFLAGS = ${AM_CFLAGS} \
 		      -Wno-missing-prototypes \
 		      -Wno-missing-declarations \
 		      -Wno-implicit-function-declaration \
+		      -Wno-unused-but-set-variable \
 		      -Wno-nested-externs \
 		      -Wno-undef \
 		      -Wno-redundant-decls
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH nft v2 8/8] include: drop "format" attribute from nft_gmp_print()
  2023-08-29 12:53 [PATCH nft v2 0/8] fix compiler warnings with clang Thomas Haller
                   ` (6 preceding siblings ...)
  2023-08-29 12:53 ` [PATCH nft v2 7/8] src: suppress "-Wunused-but-set-variable" warning with "parser_bison.c" Thomas Haller
@ 2023-08-29 12:53 ` Thomas Haller
  2023-08-29 15:00 ` [PATCH nft v2 0/8] fix compiler warnings with clang Pablo Neira Ayuso
  8 siblings, 0 replies; 13+ messages in thread
From: Thomas Haller @ 2023-08-29 12:53 UTC (permalink / raw)
  To: NetFilter; +Cc: Thomas Haller

nft_gmp_print() passes the format string and arguments to
gmp_vfprintf(). Note that the format string is then interpreted
by gmp, which also understand special specifiers like "%Zx".

Note that with clang we get various compiler warnings:

  datatype.c:299:26: error: invalid conversion specifier 'Z' [-Werror,-Wformat-invalid-specifier]
          nft_gmp_print(octx, "0x%Zx [invalid type]", expr->value);
                                 ~^

gcc doesn't warn, because to gcc 'Z' is a deprecated alias for 'z' and
because the 3rd argument of the attribute((format())) is zero (so gcc
doesn't validate the arguments). But Z specifier in gmp expects a
"mpz_t" value and not a size_t. It's really not the same thing.

The correct solution is not to mark the function to accept a printf format
string.

Fixes: 2535ba7006f2 ('src: get rid of printf')

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

diff --git a/include/nftables.h b/include/nftables.h
index 219a10100206..b9b2b01c2689 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -236,8 +236,7 @@ void realm_table_rt_exit(struct nft_ctx *ctx);
 
 int nft_print(struct output_ctx *octx, const char *fmt, ...)
 	__attribute__((format(printf, 2, 3)));
-int nft_gmp_print(struct output_ctx *octx, const char *fmt, ...)
-	__attribute__((format(printf, 2, 0)));
+int nft_gmp_print(struct output_ctx *octx, const char *fmt, ...);
 
 int nft_optimize(struct nft_ctx *nft, struct list_head *cmds);
 
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH nft v2 0/8] fix compiler warnings with clang
  2023-08-29 12:53 [PATCH nft v2 0/8] fix compiler warnings with clang Thomas Haller
                   ` (7 preceding siblings ...)
  2023-08-29 12:53 ` [PATCH nft v2 8/8] include: drop "format" attribute from nft_gmp_print() Thomas Haller
@ 2023-08-29 15:00 ` Pablo Neira Ayuso
  2023-08-29 17:52   ` Thomas Haller
  8 siblings, 1 reply; 13+ messages in thread
From: Pablo Neira Ayuso @ 2023-08-29 15:00 UTC (permalink / raw)
  To: Thomas Haller; +Cc: NetFilter

On Tue, Aug 29, 2023 at 02:53:29PM +0200, Thomas Haller wrote:
> Building with clang caused some compiler warnings. Fix, suppress or work
> around them.

Series LGTM, applied.

The ugly macro can go away later, once meta_parse_key() is removed
when bison/flex use start conditions for this, and probably log prefix
is reviewed not to use it anymore. I still think that macro is not
looking any better after this update but this is not a deal breaker
for this series.

BTW, would you extend tests/build to check for run build tests for
clang in a follow up patch? That would help a lot to improve coverage,
and reduce chances compilation with clang breaks again in the future
before a release.

Thanks.

> Changes to v1:
> - replace patches
>     "src: use "%zx" format instead of "%Zx""
>     "utils: add _NFT_PRAGMA_WARNING_DISABLE()/_NFT_PRAGMA_WARNING_REENABLE helpers"
>     "datatype: suppress "-Wformat-nonliteral" warning in integer_type_print()"
>   with
>     "include: drop "format" attribute from nft_gmp_print()"
>   which is the better solution.
> - let SNPRINTF_BUFFER_SIZE() not assert against truncation. Instead, the
>   callers handle it.
> - add bugfix "evaluate: fix check for truncation in stmt_evaluate_log_prefix()"
> - add minor patch "evaluate: don't needlessly clear full string buffer in stmt_evaluate_log_prefix()"
> 
> Thomas Haller (8):
>   netlink: avoid "-Wenum-conversion" warning in dtype_map_from_kernel()
>   netlink: avoid "-Wenum-conversion" warning in parser_bison.y
>   datatype: avoid cast-align warning with struct sockaddr result from
>     getaddrinfo()
>   evaluate: fix check for truncation in stmt_evaluate_log_prefix()
>   src: rework SNPRINTF_BUFFER_SIZE() and handle truncation
>   evaluate: don't needlessly clear full string buffer in
>     stmt_evaluate_log_prefix()
>   src: suppress "-Wunused-but-set-variable" warning with
>     "parser_bison.c"
>   include: drop "format" attribute from nft_gmp_print()
> 
>  include/nftables.h |  3 +--
>  include/utils.h    | 35 ++++++++++++++++++++++++++---------
>  src/Makefile.am    |  1 +
>  src/datatype.c     | 14 +++++++++++---
>  src/evaluate.c     | 15 ++++++++++-----
>  src/meta.c         | 11 ++++++-----
>  src/netlink.c      |  2 +-
>  src/parser_bison.y |  4 ++--
>  8 files changed, 58 insertions(+), 27 deletions(-)
> 
> -- 
> 2.41.0
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH nft v2 0/8] fix compiler warnings with clang
  2023-08-29 15:00 ` [PATCH nft v2 0/8] fix compiler warnings with clang Pablo Neira Ayuso
@ 2023-08-29 17:52   ` Thomas Haller
  0 siblings, 0 replies; 13+ messages in thread
From: Thomas Haller @ 2023-08-29 17:52 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: NetFilter

On Tue, 2023-08-29 at 17:00 +0200, Pablo Neira Ayuso wrote:
> On Tue, Aug 29, 2023 at 02:53:29PM +0200, Thomas Haller wrote:
> > Building with clang caused some compiler warnings. Fix, suppress or
> > work
> > around them.
> 
> Series LGTM, applied.


Thank you!!

> 
> The ugly macro can go away later, once meta_parse_key() is removed
> when bison/flex use start conditions for this, and probably log
> prefix
> is reviewed not to use it anymore. I still think that macro is not
> looking any better after this update but this is not a deal breaker
> for this series.

I don't find that macro that ugly (anymore). But OK.

> 
> BTW, would you extend tests/build to check for run build tests for
> clang in a follow up patch? That would help a lot to improve
> coverage,
> and reduce chances compilation with clang breaks again in the future
> before a release.

Yes, I have several more patches, that I will send soon.


Thomas

> 
> Thanks.
> 
> > Changes to v1:
> > - replace patches
> >     "src: use "%zx" format instead of "%Zx""
> >     "utils: add
> > _NFT_PRAGMA_WARNING_DISABLE()/_NFT_PRAGMA_WARNING_REENABLE helpers"
> >     "datatype: suppress "-Wformat-nonliteral" warning in
> > integer_type_print()"
> >   with
> >     "include: drop "format" attribute from nft_gmp_print()"
> >   which is the better solution.
> > - let SNPRINTF_BUFFER_SIZE() not assert against truncation.
> > Instead, the
> >   callers handle it.
> > - add bugfix "evaluate: fix check for truncation in
> > stmt_evaluate_log_prefix()"
> > - add minor patch "evaluate: don't needlessly clear full string
> > buffer in stmt_evaluate_log_prefix()"
> > 
> > Thomas Haller (8):
> >   netlink: avoid "-Wenum-conversion" warning in
> > dtype_map_from_kernel()
> >   netlink: avoid "-Wenum-conversion" warning in parser_bison.y
> >   datatype: avoid cast-align warning with struct sockaddr result
> > from
> >     getaddrinfo()
> >   evaluate: fix check for truncation in stmt_evaluate_log_prefix()
> >   src: rework SNPRINTF_BUFFER_SIZE() and handle truncation
> >   evaluate: don't needlessly clear full string buffer in
> >     stmt_evaluate_log_prefix()
> >   src: suppress "-Wunused-but-set-variable" warning with
> >     "parser_bison.c"
> >   include: drop "format" attribute from nft_gmp_print()
> > 
> >  include/nftables.h |  3 +--
> >  include/utils.h    | 35 ++++++++++++++++++++++++++---------
> >  src/Makefile.am    |  1 +
> >  src/datatype.c     | 14 +++++++++++---
> >  src/evaluate.c     | 15 ++++++++++-----
> >  src/meta.c         | 11 ++++++-----
> >  src/netlink.c      |  2 +-
> >  src/parser_bison.y |  4 ++--
> >  8 files changed, 58 insertions(+), 27 deletions(-)
> > 
> > -- 
> > 2.41.0
> > 
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH nft v2 6/8] evaluate: don't needlessly clear full string buffer in stmt_evaluate_log_prefix()
  2023-08-29 12:53 ` [PATCH nft v2 6/8] evaluate: don't needlessly clear full string buffer in stmt_evaluate_log_prefix() Thomas Haller
@ 2023-08-29 18:08   ` Pablo Neira Ayuso
  2023-08-29 18:55     ` Thomas Haller
  0 siblings, 1 reply; 13+ messages in thread
From: Pablo Neira Ayuso @ 2023-08-29 18:08 UTC (permalink / raw)
  To: Thomas Haller; +Cc: NetFilter

Please, always add description to your next patches, in this case a
long description does not make sense, but probably there is something
else it can be said on top of the patch subject.

Thanks.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH nft v2 6/8] evaluate: don't needlessly clear full string buffer in stmt_evaluate_log_prefix()
  2023-08-29 18:08   ` Pablo Neira Ayuso
@ 2023-08-29 18:55     ` Thomas Haller
  0 siblings, 0 replies; 13+ messages in thread
From: Thomas Haller @ 2023-08-29 18:55 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: NetFilter

On Tue, 2023-08-29 at 20:08 +0200, Pablo Neira Ayuso wrote:
> Please, always add description to your next patches, in this case a
> long description does not make sense, but probably there is something
> else it can be said on top of the patch subject.
> 
> Thanks.
> 

ACK!
Thomas


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2023-08-29 18:57 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-29 12:53 [PATCH nft v2 0/8] fix compiler warnings with clang Thomas Haller
2023-08-29 12:53 ` [PATCH nft v2 1/8] netlink: avoid "-Wenum-conversion" warning in dtype_map_from_kernel() Thomas Haller
2023-08-29 12:53 ` [PATCH nft v2 2/8] netlink: avoid "-Wenum-conversion" warning in parser_bison.y Thomas Haller
2023-08-29 12:53 ` [PATCH nft v2 3/8] datatype: avoid cast-align warning with struct sockaddr result from getaddrinfo() Thomas Haller
2023-08-29 12:53 ` [PATCH nft v2 4/8] evaluate: fix check for truncation in stmt_evaluate_log_prefix() Thomas Haller
2023-08-29 12:53 ` [PATCH nft v2 5/8] src: rework SNPRINTF_BUFFER_SIZE() and handle truncation Thomas Haller
2023-08-29 12:53 ` [PATCH nft v2 6/8] evaluate: don't needlessly clear full string buffer in stmt_evaluate_log_prefix() Thomas Haller
2023-08-29 18:08   ` Pablo Neira Ayuso
2023-08-29 18:55     ` Thomas Haller
2023-08-29 12:53 ` [PATCH nft v2 7/8] src: suppress "-Wunused-but-set-variable" warning with "parser_bison.c" Thomas Haller
2023-08-29 12:53 ` [PATCH nft v2 8/8] include: drop "format" attribute from nft_gmp_print() Thomas Haller
2023-08-29 15:00 ` [PATCH nft v2 0/8] fix compiler warnings with clang Pablo Neira Ayuso
2023-08-29 17:52   ` Thomas Haller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.