[PATCH nft 0/8] fix compiler warnings with clang

[PATCH nft 0/8] fix compiler warnings with clang

[Thomas Haller]

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

Thomas Haller (8):
  netlink: avoid "-Wenum-conversion" warning in dtype_map_from_kernel()
  netlink: avoid "-Wenum-conversion" warning in parser_bison.y
  src: use "%zx" format instead of "%Zx"
  datatype: avoid cast-align warning with struct sockaddr result from
    getaddrinfo()
  src: rework SNPRINTF_BUFFER_SIZE() and avoid
    "-Wunused-but-set-variable"
  src: suppress "-Wunused-but-set-variable" warning with
    "parser_bison.c"
  utils: add _NFT_PRAGMA_WARNING_DISABLE()/_NFT_PRAGMA_WARNING_REENABLE
    helpers
  datatype: suppress "-Wformat-nonliteral" warning in
    integer_type_print()

 include/utils.h    | 73 ++++++++++++++++++++++++++++++++++++++++------
 src/Makefile.am    |  1 +
 src/datatype.c     | 22 ++++++++++----
 src/evaluate.c     | 11 ++++---
 src/intervals.c    | 10 +++----
 src/meta.c         | 10 +++----
 src/netlink.c      |  2 +-
 src/parser_bison.y |  4 +--
 8 files changed, 99 insertions(+), 34 deletions(-)

-- 
2.41.0

[PATCH nft 1/8] netlink: avoid "-Wenum-conversion" warning in dtype_map_from_kernel()

[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

[PATCH nft 2/8] netlink: avoid "-Wenum-conversion" warning in parser_bison.y

[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

[PATCH nft 3/8] src: use "%zx" format instead of "%Zx"

[Thomas Haller]

"%zu" is C99. On the other hand, `man printf` comments about "Z":

  A nonstandard synonym for z that predates the appearance of z.  Do not use in code.

This fixes a compiler warning with clang:

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

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/datatype.c  |  6 +++---
 src/intervals.c | 10 +++++-----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/datatype.c b/src/datatype.c
index dd6a5fbf5df8..a91b9cb793d5 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -296,7 +296,7 @@ void symbol_table_print(const struct symbol_table *tbl,
 
 static void invalid_type_print(const struct expr *expr, struct output_ctx *octx)
 {
-	nft_gmp_print(octx, "0x%Zx [invalid type]", expr->value);
+	nft_gmp_print(octx, "0x%zx [invalid type]", expr->value);
 }
 
 const struct datatype invalid_type = {
@@ -436,7 +436,7 @@ const struct datatype bitmask_type = {
 	.type		= TYPE_BITMASK,
 	.name		= "bitmask",
 	.desc		= "bitmask",
-	.basefmt	= "0x%Zx",
+	.basefmt	= "0x%zx",
 	.basetype	= &integer_type,
 };
 
@@ -486,7 +486,7 @@ const struct datatype integer_type = {
 
 static void xinteger_type_print(const struct expr *expr, struct output_ctx *octx)
 {
-	nft_gmp_print(octx, "0x%Zx", expr->value);
+	nft_gmp_print(octx, "0x%zx", expr->value);
 }
 
 /* Alias of integer_type to print raw payload expressions in hexadecimal. */
diff --git a/src/intervals.c b/src/intervals.c
index 85de0199c373..a24f2ca69cb4 100644
--- a/src/intervals.c
+++ b/src/intervals.c
@@ -70,7 +70,7 @@ struct set_automerge_ctx {
 static void purge_elem(struct set_automerge_ctx *ctx, struct expr *i)
 {
 	if (ctx->debug_mask & NFT_DEBUG_SEGTREE) {
-		pr_gmp_debug("remove: [%Zx-%Zx]\n",
+		pr_gmp_debug("remove: [%zx-%zx]\n",
 			     i->key->left->value,
 			     i->key->right->value);
 	}
@@ -255,7 +255,7 @@ int set_automerge(struct list_head *msgs, struct cmd *cmd, struct set *set,
 			list_move_tail(&i->list, &existing_set->init->expressions);
 		} else if (existing_set) {
 			if (debug_mask & NFT_DEBUG_SEGTREE) {
-				pr_gmp_debug("add: [%Zx-%Zx]\n",
+				pr_gmp_debug("add: [%zx-%zx]\n",
 					     i->key->left->value, i->key->right->value);
 			}
 			clone = expr_clone(i);
@@ -509,13 +509,13 @@ int set_delete(struct list_head *msgs, struct cmd *cmd, struct set *set,
 
 	if (debug_mask & NFT_DEBUG_SEGTREE) {
 		list_for_each_entry(i, &init->expressions, list)
-			pr_gmp_debug("remove: [%Zx-%Zx]\n",
+			pr_gmp_debug("remove: [%zx-%zx]\n",
 				     i->key->left->value, i->key->right->value);
 		list_for_each_entry(i, &add->expressions, list)
-			pr_gmp_debug("add: [%Zx-%Zx]\n",
+			pr_gmp_debug("add: [%zx-%zx]\n",
 				     i->key->left->value, i->key->right->value);
 		list_for_each_entry(i, &existing_set->init->expressions, list)
-			pr_gmp_debug("existing: [%Zx-%Zx]\n",
+			pr_gmp_debug("existing: [%zx-%zx]\n",
 				     i->key->left->value, i->key->right->value);
 	}
 
-- 
2.41.0

[PATCH nft 4/8] datatype: avoid cast-align warning with struct sockaddr result from getaddrinfo()

[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 a91b9cb793d5..4d0e44eeb500 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

[PATCH nft 5/8] src: rework SNPRINTF_BUFFER_SIZE() and avoid "-Wunused-but-set-variable"

[Thomas Haller]

SNPRINTF_BUFFER_SIZE() causes a warning with 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;
                   ^

Fix that, and rework SNPRINTF_BUFFER_SIZE().

- before and now, the macro asserts against truncation. Remove error
  handling related to truncation in the callers.

- 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 unused, and, unless the string was truncated,
  it was identical to "offset".

- "offset" previously was incremented before checking for truncation.
  So it would point somewhere past the buffer. This behavior is not
  useful, because we assert against truncation. Now, in case of
  truncation, "len" will be zero and "offset" will be the original "len"
  (that is, point after the buffer and one byte after the terminating NUL).

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

diff --git a/include/utils.h b/include/utils.h
index cee1e5c1e8ae..873147fb54ec 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -72,15 +72,29 @@
 #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)			\
+	do { \
+		const int _ret = (ret);				\
+		size_t *const _len = (len);			\
+		size_t *const _offset = (offset);		\
+		size_t _ret2;					\
+								\
+		assert(_ret >= 0);				\
+								\
+		if ((size_t) _ret >= *_len) {			\
+			/* Fail an assertion on truncation.
+			 *
+			 * Anyway, we would set "len" to zero and "offset" one
+			 * after the buffer size (past the terminating NUL
+			 * byte). */				\
+			assert((size_t) _ret < *_len);		\
+			_ret2 = *_len;				\
+		} else						\
+			_ret2 = (size_t) _ret;			\
+								\
+		*_offset += _ret2;				\
+		*_len -= _ret2;					\
+	} while (0)
 
 #define MSEC_PER_SEC	1000L
 
diff --git a/src/evaluate.c b/src/evaluate.c
index 1ae2ef0de10c..f8cd7b7afda3 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,12 +4152,9 @@ 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 == NF_LOG_PREFIXLEN)
-		return stmt_error(ctx, stmt, "log prefix is too long");
-
 	expr = constant_expr_alloc(&stmt->log.prefix->location, &string_type,
 				   BYTEORDER_HOST_ENDIAN,
 				   strlen(prefix) * BITS_PER_BYTE, prefix);
diff --git a/src/meta.c b/src/meta.c
index 4f383269d032..6dada9719e5b 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,7 @@ 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);
 	}
 
 	return error(loc, "syntax error, unexpected %s, known keys are %s", str, buf);
-- 
2.41.0

[PATCH nft 6/8] src: suppress "-Wunused-but-set-variable" warning with "parser_bison.c"

[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

[PATCH nft 7/8] utils: add _NFT_PRAGMA_WARNING_DISABLE()/_NFT_PRAGMA_WARNING_REENABLE helpers

[Thomas Haller]

Taken from libnl3 ([1]), which is LGPL-2.1-only licensed.

[1] https://github.com/thom311/libnl/blob/main/include/base/nl-base-utils.h#L44
---
 include/utils.h | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/include/utils.h b/include/utils.h
index 873147fb54ec..9475bbbee6a0 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -9,6 +9,47 @@
 #include <list.h>
 #include <gmputil.h>
 
+/*****************************************************************************/
+
+#define _NFT_STRINGIFY_ARG(contents) #contents
+#define _NFT_STRINGIFY(macro_or_string) _NFT_STRINGIFY_ARG(macro_or_string)
+
+/*****************************************************************************/
+
+#if defined(__GNUC__)
+#define _NFT_PRAGMA_WARNING_DO(warning) \
+	_NFT_STRINGIFY(GCC diagnostic ignored warning)
+#elif defined(__clang__)
+#define _NFT_PRAGMA_WARNING_DO(warning) \
+	_NFT_STRINGIFY(clang diagnostic ignored warning)
+#endif
+
+#if defined(__GNUC__) && \
+	(__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6))
+#define _NFT_PRAGMA_WARNING_DISABLE(warning)                 \
+	_Pragma("GCC diagnostic push")                       \
+		_Pragma(_NFT_PRAGMA_WARNING_DO("-Wpragmas")) \
+			_Pragma(_NFT_PRAGMA_WARNING_DO(warning))
+#elif defined(__clang__)
+#define _NFT_PRAGMA_WARNING_DISABLE(warning)                                \
+	_Pragma("clang diagnostic push")                                    \
+		_Pragma(_NFT_PRAGMA_WARNING_DO("-Wunknown-warning-option")) \
+			_Pragma(_NFT_PRAGMA_WARNING_DO(warning))
+#else
+#define _NFT_PRAGMA_WARNING_DISABLE(warning)
+#endif
+
+#if defined(__GNUC__) && \
+	(__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6))
+#define _NFT_PRAGMA_WARNING_REENABLE _Pragma("GCC diagnostic pop")
+#elif defined(__clang__)
+#define _NFT_PRAGMA_WARNING_REENABLE _Pragma("clang diagnostic pop")
+#else
+#define _NFT_PRAGMA_WARNING_REENABLE
+#endif
+
+/*****************************************************************************/
+
 #ifdef HAVE_VISIBILITY_HIDDEN
 #       define __visible        __attribute__((visibility("default")))
 #       define EXPORT_SYMBOL(x) typeof(x) (x) __visible;
-- 
2.41.0

[PATCH nft 8/8] datatype: suppress "-Wformat-nonliteral" warning in integer_type_print()

[Thomas Haller]

    datatype.c:455:22: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
            nft_gmp_print(octx, fmt, expr->value);
                                ^~~

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

diff --git a/src/datatype.c b/src/datatype.c
index 4d0e44eeb500..12fe7141709d 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -452,7 +452,9 @@ static void integer_type_print(const struct expr *expr, struct output_ctx *octx)
 		}
 	} while ((dtype = dtype->basetype));
 
+	_NFT_PRAGMA_WARNING_DISABLE("-Wformat-nonliteral")
 	nft_gmp_print(octx, fmt, expr->value);
+	_NFT_PRAGMA_WARNING_REENABLE
 }
 
 static struct error_record *integer_type_parse(struct parse_ctx *ctx,
-- 
2.41.0

Re: [PATCH nft 8/8] datatype: suppress "-Wformat-nonliteral" warning in integer_type_print()

[Pablo Neira Ayuso]

On Mon, Aug 28, 2023 at 04:43:58PM +0200, Thomas Haller wrote:
>     datatype.c:455:22: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
>             nft_gmp_print(octx, fmt, expr->value);
>                                 ^~~
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  src/datatype.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/datatype.c b/src/datatype.c
> index 4d0e44eeb500..12fe7141709d 100644
> --- a/src/datatype.c
> +++ b/src/datatype.c
> @@ -452,7 +452,9 @@ static void integer_type_print(const struct expr *expr, struct output_ctx *octx)
>  		}
>  	} while ((dtype = dtype->basetype));
>  
> +	_NFT_PRAGMA_WARNING_DISABLE("-Wformat-nonliteral")

Maybe simply -Wno-format-nonliteral turn off in Clang so there is no
need for this PRAGMA in order to simplify things.

>  	nft_gmp_print(octx, fmt, expr->value);
> +	_NFT_PRAGMA_WARNING_REENABLE
>  }
>  
>  static struct error_record *integer_type_parse(struct parse_ctx *ctx,
> -- 
> 2.41.0
> 

Re: [PATCH nft 5/8] src: rework SNPRINTF_BUFFER_SIZE() and avoid "-Wunused-but-set-variable"

[Pablo Neira Ayuso]

On Mon, Aug 28, 2023 at 04:43:55PM +0200, Thomas Haller wrote:
> SNPRINTF_BUFFER_SIZE() causes a warning with 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;
>                    ^
> 
> Fix that, and rework SNPRINTF_BUFFER_SIZE().
> 
> - before and now, the macro asserts against truncation. Remove error
>   handling related to truncation in the callers.
> 
> - 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 unused, and, unless the string was truncated,
>   it was identical to "offset".
> 
> - "offset" previously was incremented before checking for truncation.
>   So it would point somewhere past the buffer. This behavior is not
>   useful, because we assert against truncation. Now, in case of
>   truncation, "len" will be zero and "offset" will be the original "len"
>   (that is, point after the buffer and one byte after the terminating NUL).
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  include/utils.h | 32 +++++++++++++++++++++++---------
>  src/evaluate.c  | 11 +++++------
>  src/meta.c      | 10 +++++-----
>  3 files changed, 33 insertions(+), 20 deletions(-)
> 
> diff --git a/include/utils.h b/include/utils.h
> index cee1e5c1e8ae..873147fb54ec 100644
> --- a/include/utils.h
> +++ b/include/utils.h
> @@ -72,15 +72,29 @@
>  #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)			\
> +	do { \
> +		const int _ret = (ret);				\
> +		size_t *const _len = (len);			\
> +		size_t *const _offset = (offset);		\
> +		size_t _ret2;					\
> +								\
> +		assert(_ret >= 0);				\
> +								\
> +		if ((size_t) _ret >= *_len) {			\
> +			/* Fail an assertion on truncation.
> +			 *
> +			 * Anyway, we would set "len" to zero and "offset" one
> +			 * after the buffer size (past the terminating NUL
> +			 * byte). */				\
> +			assert((size_t) _ret < *_len);		\
> +			_ret2 = *_len;				\
> +		} else						\
> +			_ret2 = (size_t) _ret;			\
> +								\
> +		*_offset += _ret2;				\
> +		*_len -= _ret2;					\
> +	} while (0)

This macro is something I made myself, which I am particularly not
proud of it, but it getting slightly more complicated.

Probably it time to turn this into a real function?

>  #define MSEC_PER_SEC	1000L
>  
> diff --git a/src/evaluate.c b/src/evaluate.c
> index 1ae2ef0de10c..f8cd7b7afda3 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,12 +4152,9 @@ 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 == NF_LOG_PREFIXLEN)
> -		return stmt_error(ctx, stmt, "log prefix is too long");

No error anymore?

Not directly related, but are you sure tests we have are sufficient to
cover for all these updates?

> -
>  	expr = constant_expr_alloc(&stmt->log.prefix->location, &string_type,
>  				   BYTEORDER_HOST_ENDIAN,
>  				   strlen(prefix) * BITS_PER_BYTE, prefix);
> diff --git a/src/meta.c b/src/meta.c
> index 4f383269d032..6dada9719e5b 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,7 @@ 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);
>  	}
>  
>  	return error(loc, "syntax error, unexpected %s, known keys are %s", str, buf);
> -- 
> 2.41.0
> 

Re: [PATCH nft 8/8] datatype: suppress "-Wformat-nonliteral" warning in integer_type_print()

[Thomas Haller]

On Mon, 2023-08-28 at 17:08 +0200, Pablo Neira Ayuso wrote:
> On Mon, Aug 28, 2023 at 04:43:58PM +0200, Thomas Haller wrote:
> > 
> > +       _NFT_PRAGMA_WARNING_DISABLE("-Wformat-nonliteral")
> 
> Maybe simply -Wno-format-nonliteral turn off in Clang so there is no
> need for this PRAGMA in order to simplify things.

"-Wformat-nonliteral" seems a useful warning. I would rather not
disable it at a larger scale.

Gcc also supports "-Wformat-nonliteral" warning, but for some reason it
does not warn here (also not, when I pass "-Wformat=2"). I don't
understand why that is.


Thomas

Re: [PATCH nft 5/8] src: rework SNPRINTF_BUFFER_SIZE() and avoid "-Wunused-but-set-variable"

[Thomas Haller]

On Mon, 2023-08-28 at 17:13 +0200, Pablo Neira Ayuso wrote:
> On Mon, Aug 28, 2023 at 04:43:55PM +0200, Thomas Haller wrote:
> > SNPRINTF_BUFFER_SIZE() causes a warning with 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;
> >                    ^
> > 
> > Fix that, and rework SNPRINTF_BUFFER_SIZE().
> > 
> > - before and now, the macro asserts against truncation. Remove
> > error
> >   handling related to truncation in the callers.
> > 
> > - 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 unused, and, unless the string was truncated,
> >   it was identical to "offset".
> > 
> > - "offset" previously was incremented before checking for
> > truncation.
> >   So it would point somewhere past the buffer. This behavior is not
> >   useful, because we assert against truncation. Now, in case of
> >   truncation, "len" will be zero and "offset" will be the original
> > "len"
> >   (that is, point after the buffer and one byte after the
> > terminating NUL).
> > 
> > Signed-off-by: Thomas Haller <thaller@redhat.com>
> > ---
> >  include/utils.h | 32 +++++++++++++++++++++++---------
> >  src/evaluate.c  | 11 +++++------
> >  src/meta.c      | 10 +++++-----
> >  3 files changed, 33 insertions(+), 20 deletions(-)
> > 
> > diff --git a/include/utils.h b/include/utils.h
> > index cee1e5c1e8ae..873147fb54ec 100644
> > --- a/include/utils.h
> > +++ b/include/utils.h
> > @@ -72,15 +72,29 @@
> >  #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)                 \
> > +       do { \
> > +               const int _ret = (ret);                         \
> > +               size_t *const _len = (len);                     \
> > +               size_t *const _offset = (offset);               \
> > +               size_t _ret2;                                   \
> > +                                                               \
> > +               assert(_ret >= 0);                              \
> > +                                                               \
> > +               if ((size_t) _ret >= *_len) {                   \
> > +                       /* Fail an assertion on truncation.
> > +                        *
> > +                        * Anyway, we would set "len" to zero and
> > "offset" one
> > +                        * after the buffer size (past the
> > terminating NUL
> > +                        * byte). */                            \
> > +                       assert((size_t) _ret < *_len);          \
> > +                       _ret2 = *_len;                          \
> > +               } else                                          \
> > +                       _ret2 = (size_t) _ret;                  \
> > +                                                               \
> > +               *_offset += _ret2;                              \
> > +               *_len -= _ret2;                                 \
> > +       } while (0)
> 
> This macro is something I made myself, which I am particularly not
> proud of it, but it getting slightly more complicated.

IMO it just got simpler. E.g. it is now function-like; you can easier
see which arguments are modified (we take a pointer to them); one
argument got dropped.

The remaining relevant parts (assertions and truncation check aside) is
literally

   offset += ret;
   len -= ret;


> 
> Probably it time to turn this into a real function?

as it now behaves function-like, it can be easily converted to an
inline function. Only difference is that upon assertion failure, we no
longer see the location of the caller. It does not seem an improvement.


> 
> >  #define MSEC_PER_SEC   1000L
> >  
> > diff --git a/src/evaluate.c b/src/evaluate.c
> > index 1ae2ef0de10c..f8cd7b7afda3 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,12 +4152,9 @@ 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 == NF_LOG_PREFIXLEN)
> > -               return stmt_error(ctx, stmt, "log prefix is too
> > long");
> 
> No error anymore?
> 
> Not directly related, but are you sure tests we have are sufficient
> to
> cover for all these updates 

No. I am not aware of test coverage.

SNPRINTF_BUFFER_SIZE() rejects truncation of the string by asserting
against it. That behavior is part of the API of that function. Error
checking after an assert seems unnecessary.

The check "if (len == NF_LOG_PREFIXLEN)" seems wrong anyway. After
truncation, "len" would be zero. The code previously checked whether
nothing was appended, but the error string didn't match that situation.


Maybe SNPRINTF_BUFFER_SIZE() should not assert against truncation?


Thomas

Re: [PATCH nft 8/8] datatype: suppress "-Wformat-nonliteral" warning in integer_type_print()

[Pablo Neira Ayuso]

On Mon, Aug 28, 2023 at 05:33:01PM +0200, Thomas Haller wrote:
> On Mon, 2023-08-28 at 17:08 +0200, Pablo Neira Ayuso wrote:
> > On Mon, Aug 28, 2023 at 04:43:58PM +0200, Thomas Haller wrote:
> > > 
> > > +       _NFT_PRAGMA_WARNING_DISABLE("-Wformat-nonliteral")
> > 
> > Maybe simply -Wno-format-nonliteral turn off in Clang so there is no
> > need for this PRAGMA in order to simplify things.
> 
> "-Wformat-nonliteral" seems a useful warning. I would rather not
> disable it at a larger scale.
> 
> Gcc also supports "-Wformat-nonliteral" warning, but for some reason it
> does not warn here (also not, when I pass "-Wformat=2"). I don't
> understand why that is.

Can you see any other way to fix this clang compilation eror without
this pragma? What makes clang unhappy with this code?

Re: [PATCH nft 5/8] src: rework SNPRINTF_BUFFER_SIZE() and avoid "-Wunused-but-set-variable"

[Pablo Neira Ayuso]

On Mon, Aug 28, 2023 at 05:49:53PM +0200, Thomas Haller wrote:
> On Mon, 2023-08-28 at 17:13 +0200, Pablo Neira Ayuso wrote:
[...]
> > > diff --git a/include/utils.h b/include/utils.h
> > > index cee1e5c1e8ae..873147fb54ec 100644
> > > --- a/include/utils.h
> > > +++ b/include/utils.h
> > > @@ -72,15 +72,29 @@
> > >  #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)                 \
> > > +       do { \
> > > +               const int _ret = (ret);                         \
> > > +               size_t *const _len = (len);                     \
> > > +               size_t *const _offset = (offset);               \
> > > +               size_t _ret2;                                   \
> > > +                                                               \
> > > +               assert(_ret >= 0);                              \
> > > +                                                               \
> > > +               if ((size_t) _ret >= *_len) {                   \
> > > +                       /* Fail an assertion on truncation.
> > > +                        *
> > > +                        * Anyway, we would set "len" to zero and
> > > "offset" one
> > > +                        * after the buffer size (past the
> > > terminating NUL
> > > +                        * byte). */                            \
> > > +                       assert((size_t) _ret < *_len);          \
> > > +                       _ret2 = *_len;                          \
> > > +               } else                                          \
> > > +                       _ret2 = (size_t) _ret;                  \
> > > +                                                               \
> > > +               *_offset += _ret2;                              \
> > > +               *_len -= _ret2;                                 \
> > > +       } while (0)
> > 
> > This macro is something I made myself, which I am particularly not
> > proud of it, but it getting slightly more complicated.
> 
> IMO it just got simpler. E.g. it is now function-like; you can easier
> see which arguments are modified (we take a pointer to them); one
> argument got dropped.
>
> The remaining relevant parts (assertions and truncation check aside) is
> literally
> 
>    offset += ret;
>    len -= ret;
> 
> > 
> > Probably it time to turn this into a real function?
> 
> as it now behaves function-like, it can be easily converted to an
> inline function. Only difference is that upon assertion failure, we no
> longer see the location of the caller. It does not seem an improvement.

No need for inline in this case, this is not performance critical, it
will increase size of binary, better let the compiler decide. What is
left to turn this into a real function? It looks like a candidate for
the new nftutils.c file to me.

> > >  #define MSEC_PER_SEC   1000L
> > >  
> > > diff --git a/src/evaluate.c b/src/evaluate.c
> > > index 1ae2ef0de10c..f8cd7b7afda3 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,12 +4152,9 @@ 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 == NF_LOG_PREFIXLEN)
> > > -               return stmt_error(ctx, stmt, "log prefix is too
> > > long");
> > 
> > No error anymore?
> > 
> > Not directly related, but are you sure tests we have are sufficient
> > to
> > cover for all these updates 
> 
> No. I am not aware of test coverage.

There is:

        tests/py

which are unitary tests, there is a README file, it tests control
plane only: it dump the output from the kernel to test listing path.
They also cover json support.

        tests/shell

are tests coverage in the form of shell scripts, some validates that
the output is correct via dump file, some others do not. They are more
flexible that tests/py.

There are also selftests in the netfilter folders, see:

        tools/testing/selftests/netfilter/

For libnftnl, there is a number of library API tests.

This is what we have by now.

> SNPRINTF_BUFFER_SIZE() rejects truncation of the string by asserting
> against it. That behavior is part of the API of that function. Error
> checking after an assert seems unnecessary.
> 
> The check "if (len == NF_LOG_PREFIXLEN)" seems wrong anyway. After
> truncation, "len" would be zero. The code previously checked whether
> nothing was appended, but the error string didn't match that situation.
> 
> Maybe SNPRINTF_BUFFER_SIZE() should not assert against truncation?

IIRC, the goal for this function was to handle snprintf() and all its
corner cases. If there is no need for it or a better way to do this,
this is welcome.

Re: [PATCH nft 8/8] datatype: suppress "-Wformat-nonliteral" warning in integer_type_print()

[Thomas Haller]

On Mon, 2023-08-28 at 17:54 +0200, Pablo Neira Ayuso wrote:
> On Mon, Aug 28, 2023 at 05:33:01PM +0200, Thomas Haller wrote:
> > On Mon, 2023-08-28 at 17:08 +0200, Pablo Neira Ayuso wrote:
> > > On Mon, Aug 28, 2023 at 04:43:58PM +0200, Thomas Haller wrote:
> > > > 
> > > > +       _NFT_PRAGMA_WARNING_DISABLE("-Wformat-nonliteral")
> > > 
> > > Maybe simply -Wno-format-nonliteral turn off in Clang so there is
> > > no
> > > need for this PRAGMA in order to simplify things.
> > 
> > "-Wformat-nonliteral" seems a useful warning. I would rather not
> > disable it at a larger scale.
> > 
> > Gcc also supports "-Wformat-nonliteral" warning, but for some
> > reason it
> > does not warn here (also not, when I pass "-Wformat=2"). I don't
> > understand why that is.
> 
> Can you see any other way to fix this clang compilation eror without
> this pragma? What makes clang unhappy with this code?
> 


the compiler warning happens, because the argument isn't a string
literal. But the deeper cause is something else entirely (and the
patches wrong).


Those "%Zu" format strings aren't intended for libc's printf. Instead,
it's intended for gmp_vfprintf(), which implements a different meaning
for "Z".

The patch "src: use "%zx" format instead of "%Zx"" should be dropped.
Likewise, "datatype: suppress "-Wformat-nonliteral" warning in
integer_type_print()".


Instead, the format attribute from

int nft_gmp_print(struct output_ctx *octx, const char *fmt, ...)
       __attribute__((format(printf, 2, 0)));

needs to be dropped. This is not the "printf" format.


Will do in v2.


Thomas

Re: [PATCH nft 5/8] src: rework SNPRINTF_BUFFER_SIZE() and avoid "-Wunused-but-set-variable"

[Thomas Haller]

On Mon, 2023-08-28 at 18:04 +0200, Pablo Neira Ayuso wrote:
> On Mon, Aug 28, 2023 at 05:49:53PM +0200, Thomas Haller wrote:
> > On Mon, 2023-08-28 at 17:13 +0200, Pablo Neira Ayuso wrote:
> 
> > SNPRINTF_BUFFER_SIZE() rejects truncation of the string by
> > asserting
> > against it. That behavior is part of the API of that function.
> > Error
> > checking after an assert seems unnecessary.
> > 
> > The check "if (len == NF_LOG_PREFIXLEN)" seems wrong anyway. After
> > truncation, "len" would be zero. The code previously checked
> > whether
> > nothing was appended, but the error string didn't match that
> > situation.
> > 
> > Maybe SNPRINTF_BUFFER_SIZE() should not assert against truncation?
> 
> IIRC, the goal for this function was to handle snprintf() and all its
> corner cases. If there is no need for it or a better way to do this,
> this is welcome.
> 

I think the macro is sensible (at least, after some cleanup).

It makes a choice, that the caller must ensure a priori that the buffer
is long enough (by asserting).

By looking at the callers, it's not clear to me, whether the callers
can always ensure that.  For meta_key_parse(), it seems the maximum
string is limited by meta_templates. But for stmt_evaluate_log_prefix()
it may be possible to craft user-input that triggers the assertion,
isn't it?

Maybe the macro and the callers should anticipate and handle
truncation?


Thomas

Re: [PATCH nft 5/8] src: rework SNPRINTF_BUFFER_SIZE() and avoid "-Wunused-but-set-variable"

[Pablo Neira Ayuso]

On Mon, Aug 28, 2023 at 06:45:00PM +0200, Thomas Haller wrote:
> On Mon, 2023-08-28 at 18:04 +0200, Pablo Neira Ayuso wrote:
> > On Mon, Aug 28, 2023 at 05:49:53PM +0200, Thomas Haller wrote:
> > > On Mon, 2023-08-28 at 17:13 +0200, Pablo Neira Ayuso wrote:
> > 
> > > SNPRINTF_BUFFER_SIZE() rejects truncation of the string by
> > > asserting
> > > against it. That behavior is part of the API of that function.
> > > Error
> > > checking after an assert seems unnecessary.
> > > 
> > > The check "if (len == NF_LOG_PREFIXLEN)" seems wrong anyway. After
> > > truncation, "len" would be zero. The code previously checked
> > > whether
> > > nothing was appended, but the error string didn't match that
> > > situation.
> > > 
> > > Maybe SNPRINTF_BUFFER_SIZE() should not assert against truncation?
> > 
> > IIRC, the goal for this function was to handle snprintf() and all its
> > corner cases. If there is no need for it or a better way to do this,
> > this is welcome.
> > 
> 
> I think the macro is sensible (at least, after some cleanup).
> 
> It makes a choice, that the caller must ensure a priori that the buffer
> is long enough (by asserting).
> 
> By looking at the callers, it's not clear to me, whether the callers
> can always ensure that.  For meta_key_parse(), it seems the maximum
> string is limited by meta_templates. But for stmt_evaluate_log_prefix()
> it may be possible to craft user-input that triggers the assertion,
> isn't it?
> 
> Maybe the macro and the callers should anticipate and handle
> truncation?

This should not silently truncate strings, instead bail out to user in
case the string is too long.

#define NF_LOG_PREFIXLEN       128

Maximum length as specified by uapi/linux/netfilter/nf_log.h

Currently in userspace, if I specify a string longest than that, I can
see ASAN complains on incorrect memory management from userspace
(without your patchset), so this code is currently broken (by me
mostly likely since I was the last one to touch those bits).

While at this, it would be good to fix it and add a test to cover
maximum prefix length.

Regarding meta_key_parse(), these days the preferred approach is to
use start conditions in flex. This function should go away, it was an
early attempt to reduce tokens by using STRING from bison, which
turned out to be flawed.

It is not worth to fix meta_key_parse(), flex start conditions and
bison parser should be used.

This macro is hiding behind a bit of technical debt.

Re: [PATCH nft 5/8] src: rework SNPRINTF_BUFFER_SIZE() and avoid "-Wunused-but-set-variable"

[Thomas Haller]

On Mon, 2023-08-28 at 21:53 +0200, Pablo Neira Ayuso wrote:
> On Mon, Aug 28, 2023 at 06:45:00PM +0200, Thomas Haller wrote:
> > On Mon, 2023-08-28 at 18:04 +0200, Pablo Neira Ayuso wrote:
> > > On Mon, Aug 28, 2023 at 05:49:53PM +0200, Thomas Haller wrote:
> > > > On Mon, 2023-08-28 at 17:13 +0200, Pablo Neira Ayuso wrote:
> > > 
> > > > SNPRINTF_BUFFER_SIZE() rejects truncation of the string by
> > > > asserting
> > > > against it. That behavior is part of the API of that function.
> > > > Error
> > > > checking after an assert seems unnecessary.
> > > > 
> > > > The check "if (len == NF_LOG_PREFIXLEN)" seems wrong anyway.
> > > > After
> > > > truncation, "len" would be zero. The code previously checked
> > > > whether
> > > > nothing was appended, but the error string didn't match that
> > > > situation.
> > > > 
> > > > Maybe SNPRINTF_BUFFER_SIZE() should not assert against
> > > > truncation?
> > > 
> > > IIRC, the goal for this function was to handle snprintf() and all
> > > its
> > > corner cases. If there is no need for it or a better way to do
> > > this,
> > > this is welcome.
> > > 
> > 
> > I think the macro is sensible (at least, after some cleanup).
> > 
> > It makes a choice, that the caller must ensure a priori that the
> > buffer
> > is long enough (by asserting).
> > 
> > By looking at the callers, it's not clear to me, whether the
> > callers
> > can always ensure that.  For meta_key_parse(), it seems the maximum
> > string is limited by meta_templates. But for
> > stmt_evaluate_log_prefix()
> > it may be possible to craft user-input that triggers the assertion,
> > isn't it?
> > 
> > Maybe the macro and the callers should anticipate and handle
> > truncation?
> 
> This should not silently truncate strings, instead bail out to user
> in
> case the string is too long.

v2 of the patchset is supposed to do that. wdyt?


Thanks
Thomas