[nft PATCH 00/10] Some covscan fixes

[nft PATCH 00/10] Some covscan fixes

[Phil Sutter]

This series fixes a bunch of minor issues identified by Coverity tool.

Phil Sutter (10):
  parser_bison: Fix for implicit declaration of isalnum
  parser_json: Fix for memleak in tcp option error path
  evaluate: Mark fall through case in str2hooknum()
  json: Drop pointless assignment in exthdr_expr_json()
  netlink: Avoid memleak in error path of netlink_delinearize_set()
  netlink: Avoid memleak in error path of netlink_delinearize_chain()
  netlink: Avoid memleak in error path of netlink_delinearize_table()
  netlink: Avoid memleak in error path of netlink_delinearize_obj()
  netlink_delinearize: Fix suspicious calloc() call
  rule: Fix for potential off-by-one in cmd_add_loc()

 src/evaluate.c            | 1 +
 src/json.c                | 1 -
 src/netlink.c             | 7 +++++--
 src/netlink_delinearize.c | 5 ++---
 src/parser_bison.y        | 1 +
 src/parser_json.c         | 6 +++---
 src/rule.c                | 2 +-
 7 files changed, 13 insertions(+), 10 deletions(-)

-- 
2.31.1

[nft PATCH 01/10] parser_bison: Fix for implicit declaration of isalnum

[Phil Sutter]

Have to include ctype.h to make it known.

Fixes: e76bb37940181 ("src: allow for variables in the log prefix string")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/parser_bison.y | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/parser_bison.y b/src/parser_bison.y
index 136ae105f5132..c0a9dc52963ea 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -10,6 +10,7 @@
 
 %{
 
+#include <ctype.h>
 #include <stddef.h>
 #include <stdio.h>
 #include <inttypes.h>
-- 
2.31.1

[nft PATCH 02/10] parser_json: Fix for memleak in tcp option error path

[Phil Sutter]

If 'kind' value is invalid, the function returned without freeing 'expr'
first. Fix this by performing the check before allocation.

Fixes: cb21869649208 ("json: tcp: add raw tcp option match support")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/parser_json.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/parser_json.c b/src/parser_json.c
index e6a0233ab6ce3..bb0e4169b477d 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -603,12 +603,12 @@ static struct expr *json_parse_tcp_option_expr(struct json_ctx *ctx,
 			"base", &kind, "offset", &offset, "len", &len)) {
 		uint32_t flag = 0;
 
-		expr = tcpopt_expr_alloc(int_loc, kind,
-					 TCPOPT_COMMON_KIND);
-
 		if (kind < 0 || kind > 255)
 			return NULL;
 
+		expr = tcpopt_expr_alloc(int_loc, kind,
+					 TCPOPT_COMMON_KIND);
+
 		if (offset == TCPOPT_COMMON_KIND && len == 8)
 			flag = NFT_EXTHDR_F_PRESENT;
 
-- 
2.31.1

[nft PATCH 03/10] evaluate: Mark fall through case in str2hooknum()

[Phil Sutter]

It is certainly intentional, so just mark it as such.

Fixes: b4775dec9f80b ("src: ingress inet support")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/evaluate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/evaluate.c b/src/evaluate.c
index 43f1f8a3977b2..ee79f160f8ff3 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -4145,6 +4145,7 @@ static uint32_t str2hooknum(uint32_t family, const char *hook)
 	case NFPROTO_INET:
 		if (!strcmp(hook, "ingress"))
 			return NF_INET_INGRESS;
+		/* fall through */
 	case NFPROTO_IPV4:
 	case NFPROTO_BRIDGE:
 	case NFPROTO_IPV6:
-- 
2.31.1

[nft PATCH 04/10] json: Drop pointless assignment in exthdr_expr_json()

[Phil Sutter]

The updated value of 'is_exists' is no longer read at this point.

Fixes: cb21869649208 ("json: tcp: add raw tcp option match support")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/json.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/json.c b/src/json.c
index f648ea1b8c178..f111ad678f8a0 100644
--- a/src/json.c
+++ b/src/json.c
@@ -708,7 +708,6 @@ json_t *exthdr_expr_json(const struct expr *expr, struct output_ctx *octx)
 					 "base", expr->exthdr.raw_type,
 					 "offset", expr->exthdr.offset,
 					 "len", expr->len);
-			is_exists = false;
 		}
 
 		return json_pack("{s:o}", "tcp option", root);
-- 
2.31.1

[nft PATCH 05/10] netlink: Avoid memleak in error path of netlink_delinearize_set()

[Phil Sutter]

Duplicate string 'comment' later when the function does not fail
anymore.

Fixes: 0864c2d49ee8a ("src: add comment support for set declarations")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/netlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/netlink.c b/src/netlink.c
index e91b06e3ea971..41cce3379ca50 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -867,7 +867,7 @@ struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
 		if (ud[NFTNL_UDATA_SET_DATA_TYPEOF])
 			typeof_expr_data = set_make_key(ud[NFTNL_UDATA_SET_DATA_TYPEOF]);
 		if (ud[NFTNL_UDATA_SET_COMMENT])
-			comment = xstrdup(nftnl_udata_get(ud[NFTNL_UDATA_SET_COMMENT]));
+			comment = nftnl_udata_get(ud[NFTNL_UDATA_SET_COMMENT]);
 	}
 
 	key = nftnl_set_get_u32(nls, NFTNL_SET_KEY_TYPE);
@@ -905,7 +905,7 @@ struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
 	set->handle.set.name = xstrdup(nftnl_set_get_str(nls, NFTNL_SET_NAME));
 	set->automerge	   = automerge;
 	if (comment)
-		set->comment = comment;
+		set->comment = xstrdup(comment);
 
 	init_list_head(&set_parse_ctx.stmt_list);
 
-- 
2.31.1

[nft PATCH 06/10] netlink: Avoid memleak in error path of netlink_delinearize_chain()

[Phil Sutter]

If parsing udata fails, 'chain' has to be freed before returning to
caller.

Fixes: 702ac2b72c0e8 ("src: add comment support for chains")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/netlink.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/netlink.c b/src/netlink.c
index 41cce3379ca50..1bbdf98bd2ee2 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -599,6 +599,7 @@ struct chain *netlink_delinearize_chain(struct netlink_ctx *ctx,
 		udata = nftnl_chain_get_data(nlc, NFTNL_CHAIN_USERDATA, &ulen);
 		if (nftnl_udata_parse(udata, ulen, chain_parse_udata_cb, ud) < 0) {
 			netlink_io_error(ctx, NULL, "Cannot parse userdata");
+			chain_free(chain);
 			return NULL;
 		}
 		if (ud[NFTNL_UDATA_CHAIN_COMMENT])
-- 
2.31.1

[nft PATCH 07/10] netlink: Avoid memleak in error path of netlink_delinearize_table()

[Phil Sutter]

If parsing udata fails, 'table' has to be freed before returning to
caller.

Fixes: c156232a530b3 ("src: add comment support when adding tables")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/netlink.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/netlink.c b/src/netlink.c
index 1bbdf98bd2ee2..be98bfb7f5c12 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -647,6 +647,7 @@ struct table *netlink_delinearize_table(struct netlink_ctx *ctx,
 		udata = nftnl_table_get_data(nlt, NFTNL_TABLE_USERDATA, &ulen);
 		if (nftnl_udata_parse(udata, ulen, table_parse_udata_cb, ud) < 0) {
 			netlink_io_error(ctx, NULL, "Cannot parse userdata");
+			table_free(table);
 			return NULL;
 		}
 		if (ud[NFTNL_UDATA_TABLE_COMMENT])
-- 
2.31.1

[nft PATCH 08/10] netlink: Avoid memleak in error path of netlink_delinearize_obj()

[Phil Sutter]

If parsing udata fails, 'obj' has to be freed before returning to
caller.

Fixes: 293c9b114faef ("src: add comment support for objects")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/netlink.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/netlink.c b/src/netlink.c
index be98bfb7f5c12..f2c1a4a15dee8 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -1450,6 +1450,7 @@ struct obj *netlink_delinearize_obj(struct netlink_ctx *ctx,
 		udata = nftnl_obj_get_data(nlo, NFTNL_OBJ_USERDATA, &ulen);
 		if (nftnl_udata_parse(udata, ulen, obj_parse_udata_cb, ud) < 0) {
 			netlink_io_error(ctx, NULL, "Cannot parse userdata");
+			obj_free(obj);
 			return NULL;
 		}
 		if (ud[NFTNL_UDATA_OBJ_COMMENT])
-- 
2.31.1

[nft PATCH 09/10] netlink_delinearize: Fix suspicious calloc() call

[Phil Sutter]

Parameter passed to sizeof() was wrong. While being at it, replace the
whole call with xmalloc_array() which takes care of error checking.

Fixes: 913979f882d13 ("src: add expression handler hashtable")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/netlink_delinearize.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index a71d06d7fe12f..9a1cf3c4f7d90 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -1733,9 +1733,8 @@ void expr_handler_init(void)
 	unsigned int i;
 	uint32_t hash;
 
-	expr_handle_ht = calloc(NFT_EXPR_HSIZE, sizeof(expr_handle_ht));
-	if (!expr_handle_ht)
-		memory_allocation_error();
+	expr_handle_ht = xmalloc_array(NFT_EXPR_HSIZE,
+				       sizeof(expr_handle_ht[0]));
 
 	for (i = 0; i < array_size(netlink_parsers); i++) {
 		hash = djb_hash(netlink_parsers[i].name) % NFT_EXPR_HSIZE;
-- 
2.31.1

[nft PATCH 10/10] rule: Fix for potential off-by-one in cmd_add_loc()

[Phil Sutter]

Using num_attrs as index means it must be at max one less than the
array's size at function start.

Fixes: 27362a5bfa433 ("rule: larger number of error locations")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/rule.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/rule.c b/src/rule.c
index dbbe744eee0d8..92daf2f33b76b 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1275,7 +1275,7 @@ struct cmd *cmd_alloc(enum cmd_ops op, enum cmd_obj obj,
 
 void cmd_add_loc(struct cmd *cmd, uint16_t offset, const struct location *loc)
 {
-	if (cmd->num_attrs > NFT_NLATTR_LOC_MAX)
+	if (cmd->num_attrs >= NFT_NLATTR_LOC_MAX)
 		return;
 
 	cmd->attr[cmd->num_attrs].offset = offset;
-- 
2.31.1