[nft PATCH 0/5] Fix some covscan findings

[nft PATCH 0/5] Fix some covscan findings

[Phil Sutter]

All these are rather minor issues, no big deal.

Phil Sutter (5):
  optimize: Clarify chain_optimize() array allocations
  optimize: Do not return garbage from stack
  netlink: Fix for potential NULL-pointer deref
  meta: parse_iso_date() returns boolean
  mnl: dump_nf_hooks() leaks memory in error path

 src/meta.c     |  2 +-
 src/mnl.c      | 11 +++++++++--
 src/netlink.c  |  3 ++-
 src/optimize.c |  9 +++++----
 4 files changed, 17 insertions(+), 8 deletions(-)

-- 
2.38.0

[nft PATCH 1/5] optimize: Clarify chain_optimize() array allocations

[Phil Sutter]

Arguments passed to sizeof() where deemed suspicious by covscan due to
the different type. Consistently specify size of an array 'a' using
'sizeof(*a) * nmemb'.

For the statement arrays in stmt_matrix, even use xzalloc_array() since
the item count is fixed and therefore can't be zero.

Fixes: fb298877ece27 ("src: add ruleset optimization infrastructure")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/optimize.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/optimize.c b/src/optimize.c
index 32aed866eb49f..12cae00da4ab4 100644
--- a/src/optimize.c
+++ b/src/optimize.c
@@ -1113,10 +1113,11 @@ static int chain_optimize(struct nft_ctx *nft, struct list_head *rules)
 		ctx->num_rules++;
 	}
 
-	ctx->rule = xzalloc(sizeof(ctx->rule) * ctx->num_rules);
-	ctx->stmt_matrix = xzalloc(sizeof(struct stmt *) * ctx->num_rules);
+	ctx->rule = xzalloc(sizeof(*ctx->rule) * ctx->num_rules);
+	ctx->stmt_matrix = xzalloc(sizeof(*ctx->stmt_matrix) * ctx->num_rules);
 	for (i = 0; i < ctx->num_rules; i++)
-		ctx->stmt_matrix[i] = xzalloc(sizeof(struct stmt *) * MAX_STMTS);
+		ctx->stmt_matrix[i] = xzalloc_array(MAX_STMTS,
+						    sizeof(**ctx->stmt_matrix));
 
 	merge = xzalloc(sizeof(*merge) * ctx->num_rules);
 
-- 
2.38.0

[nft PATCH 2/5] optimize: Do not return garbage from stack

[Phil Sutter]

From: Phil Sutter <psutter@redhat.com>

If input does not contain a single 'add' command (unusual, but
possible), 'ret' value was not initialized by nft_optimize() before
returning its value.

Fixes: fb298877ece27 ("src: add ruleset optimization infrastructure")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/optimize.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/optimize.c b/src/optimize.c
index 12cae00da4ab4..289c442dc915e 100644
--- a/src/optimize.c
+++ b/src/optimize.c
@@ -1217,7 +1217,7 @@ static int cmd_optimize(struct nft_ctx *nft, struct cmd *cmd)
 int nft_optimize(struct nft_ctx *nft, struct list_head *cmds)
 {
 	struct cmd *cmd;
-	int ret;
+	int ret = 0;
 
 	list_for_each_entry(cmd, cmds, list) {
 		switch (cmd->op) {
-- 
2.38.0

[nft PATCH 3/5] netlink: Fix for potential NULL-pointer deref

[Phil Sutter]

If memory allocation fails, calloc() returns NULL which was not checked
for. The code seems to expect zero array size though, so simply
replacing this call by one of the x*calloc() ones won't work. So guard
the call also by a check for 'len'.

Fixes: db0697ce7f602 ("src: support for flowtable listing")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/netlink.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/netlink.c b/src/netlink.c
index 51de9c9c8edb2..efae125148b8c 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -1790,7 +1790,8 @@ netlink_delinearize_flowtable(struct netlink_ctx *ctx,
 	while (dev_array[len])
 		len++;
 
-	flowtable->dev_array = calloc(1, len * sizeof(char *));
+	if (len)
+		flowtable->dev_array = xmalloc(len * sizeof(char *));
 	for (i = 0; i < len; i++)
 		flowtable->dev_array[i] = xstrdup(dev_array[i]);
 
-- 
2.38.0

[nft PATCH 4/5] meta: parse_iso_date() returns boolean

[Phil Sutter]

Returning ts if 'ts == (time_t) -1' signals success to caller despite
failure.

Fixes: 4460b839b945a ("meta: fix compiler warning in date_type_parse()")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/meta.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/meta.c b/src/meta.c
index bd8a41ba3a032..013e8cbaf38a5 100644
--- a/src/meta.c
+++ b/src/meta.c
@@ -433,7 +433,7 @@ static bool parse_iso_date(uint64_t *tstamp, const char *sym)
 	cur_tm = localtime(&ts);
 
 	if (ts == (time_t) -1 || cur_tm == NULL)
-		return ts;
+		return false;
 
 	/* Substract tm_gmtoff to get the current time */
 	*tstamp = ts - cur_tm->tm_gmtoff;
-- 
2.38.0

[nft PATCH 5/5] mnl: dump_nf_hooks() leaks memory in error path

[Phil Sutter]

Have to free the basehook object before returning to caller.

Fixes: 4694f7230195b ("src: add support for base hook dumping")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/mnl.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/mnl.c b/src/mnl.c
index 62b0b59c2da8a..46d86f0fd9502 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -2217,16 +2217,23 @@ static int dump_nf_hooks(const struct nlmsghdr *nlh, void *_data)
 		struct nlattr *nested[NFNLA_HOOK_INFO_MAX + 1] = {};
 		uint32_t type;
 
-		if (mnl_attr_parse_nested(tb[NFNLA_HOOK_CHAIN_INFO], dump_nf_chain_info_cb, nested) < 0)
+		if (mnl_attr_parse_nested(tb[NFNLA_HOOK_CHAIN_INFO],
+					  dump_nf_chain_info_cb, nested) < 0) {
+			basehook_free(hook);
 			return -1;
+		}
 
 		type = ntohl(mnl_attr_get_u32(nested[NFNLA_HOOK_INFO_TYPE]));
 		if (type == NFNL_HOOK_TYPE_NFTABLES) {
 			struct nlattr *info[NFNLA_CHAIN_MAX + 1] = {};
 			const char *tablename, *chainname;
 
-			if (mnl_attr_parse_nested(nested[NFNLA_HOOK_INFO_DESC], dump_nf_attr_chain_cb, info) < 0)
+			if (mnl_attr_parse_nested(nested[NFNLA_HOOK_INFO_DESC],
+						  dump_nf_attr_chain_cb,
+						  info) < 0) {
+				basehook_free(hook);
 				return -1;
+			}
 
 			tablename = mnl_attr_get_str(info[NFNLA_CHAIN_TABLE]);
 			chainname = mnl_attr_get_str(info[NFNLA_CHAIN_NAME]);
-- 
2.38.0

Re: [nft PATCH 0/5] Fix some covscan findings

[Phil Sutter]

On Thu, Jan 12, 2023 at 06:28:18PM +0100, Phil Sutter wrote:
> All these are rather minor issues, no big deal.

Series applied after fixing up my email address in one of them.