[libnftnl PATCH 0/6] Another round of Covscan indicated fixes

[libnftnl PATCH 0/6] Another round of Covscan indicated fixes

[Phil Sutter]

This series fixes potential issues identified by a recent Covscan run.

Phil Sutter (6):
  nftnl_data_reg_snprintf: Add a missing break
  src/gen: Remove a pointless call to mnl_nlmsg_get_payload()
  src/object: Avoid returning garbage in nftnl_obj_do_parse()
  src/ruleset: Avoid reading garbage in nftnl_ruleset_cb()
  src/set_elem: Don't return garbage in nftnl_set_elems_parse()
  src/trace: Check return value of mnl_attr_parse_nested()

 src/expr/data_reg.c |  1 +
 src/gen.c           |  4 ++--
 src/object.c        |  2 +-
 src/ruleset.c       | 10 +++++-----
 src/set_elem.c      |  2 +-
 src/trace.c         |  3 ++-
 6 files changed, 12 insertions(+), 10 deletions(-)

-- 
2.13.1

[libnftnl PATCH 1/6] nftnl_data_reg_snprintf: Add a missing break

[Phil Sutter]

The code works fine as-is, but if reg_type == DATA_VALUE &&
output_format == NFTNL_OUTPUT_XML, we fall through to DATA_CHAIN case
and therefore pointlessly check output_format again.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/expr/data_reg.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/expr/data_reg.c b/src/expr/data_reg.c
index a246952f7f01b..c087d792a8af6 100644
--- a/src/expr/data_reg.c
+++ b/src/expr/data_reg.c
@@ -207,6 +207,7 @@ int nftnl_data_reg_snprintf(char *buf, size_t size,
 		default:
 			break;
 		}
+		break;
 	case DATA_VERDICT:
 	case DATA_CHAIN:
 		switch(output_format) {
-- 
2.13.1

[libnftnl PATCH 2/6] src/gen: Remove a pointless call to mnl_nlmsg_get_payload()

[Phil Sutter]

It is a common idiom in all *_nlmsg_parse() functions, but
nftnl_gen_nlmsg_parse() doesn't make use of the data pointer and the
compiler probably can't eliminate it since there could be a side-effect.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/gen.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/gen.c b/src/gen.c
index 58b3a96dbdcb9..eafb015a25345 100644
--- a/src/gen.c
+++ b/src/gen.c
@@ -143,9 +143,9 @@ static int nftnl_gen_parse_attr_cb(const struct nlattr *attr, void *data)
 int nftnl_gen_nlmsg_parse(const struct nlmsghdr *nlh, struct nftnl_gen *gen)
 {
 	struct nlattr *tb[NFTA_GEN_MAX + 1] = {};
-	struct nfgenmsg *nfg = mnl_nlmsg_get_payload(nlh);
 
-	if (mnl_attr_parse(nlh, sizeof(*nfg), nftnl_gen_parse_attr_cb, tb) < 0)
+	if (mnl_attr_parse(nlh, sizeof(struct nfgenmsg),
+			   nftnl_gen_parse_attr_cb, tb) < 0)
 		return -1;
 
 	if (tb[NFTA_GEN_ID]) {
-- 
2.13.1

[libnftnl PATCH 3/6] src/object: Avoid returning garbage in nftnl_obj_do_parse()

[Phil Sutter]

It may happen that 'perr' variable does not get initialized, so making
parameter 'err' point to it in any case is error-prone. Avoid this by
initializing 'perr' upon declaration.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/object.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/object.c b/src/object.c
index 9a4ee712a2a95..da3423bf3381a 100644
--- a/src/object.c
+++ b/src/object.c
@@ -358,7 +358,7 @@ static int nftnl_obj_do_parse(struct nftnl_obj *obj, enum nftnl_parse_type type,
 			      const void *data, struct nftnl_parse_err *err,
 			      enum nftnl_parse_input input)
 {
-	struct nftnl_parse_err perr;
+	struct nftnl_parse_err perr = {};
 	int ret;
 
 	switch (type) {
-- 
2.13.1

[libnftnl PATCH 4/6] src/ruleset: Avoid reading garbage in nftnl_ruleset_cb()

[Phil Sutter]

If nftnl_ruleset_json_parse() is called with arg == NULL, ctx.data is
left uninitialized and will later be used in nftnl_ruleset_cb(). Avoid
this by using a C99-style initializer for 'ctx' which sets all omitted
fields to zero.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/ruleset.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/ruleset.c b/src/ruleset.c
index 3de9b87283292..cf86ca68a51f3 100644
--- a/src/ruleset.c
+++ b/src/ruleset.c
@@ -519,11 +519,11 @@ static int nftnl_ruleset_json_parse(const void *json,
 	json_error_t error;
 	int i, len;
 	const char *key;
-	struct nftnl_parse_ctx ctx;
-
-	ctx.cb = cb;
-	ctx.format = type;
-	ctx.flags = 0;
+	struct nftnl_parse_ctx ctx = {
+		.cb = cb,
+		.format = type,
+		.flags = 0,
+	};
 
 	ctx.set_list = nftnl_set_list_alloc();
 	if (ctx.set_list == NULL)
-- 
2.13.1

[libnftnl PATCH 5/6] src/set_elem: Don't return garbage in nftnl_set_elems_parse()

[Phil Sutter]

This might happen if netlink message is malformed (no nested attributes
are present), so treat this as an error and return -1 instead of
garbage to caller.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/set_elem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/set_elem.c b/src/set_elem.c
index e45dbc6bfe3e4..71c279a540860 100644
--- a/src/set_elem.c
+++ b/src/set_elem.c
@@ -490,7 +490,7 @@ nftnl_set_elem_list_parse_attr_cb(const struct nlattr *attr, void *data)
 static int nftnl_set_elems_parse(struct nftnl_set *s, const struct nlattr *nest)
 {
 	struct nlattr *attr;
-	int ret;
+	int ret = -1;
 
 	mnl_attr_for_each_nested(attr, nest) {
 		if (mnl_attr_get_type(attr) != NFTA_LIST_ELEM)
-- 
2.13.1

[libnftnl PATCH 6/6] src/trace: Check return value of mnl_attr_parse_nested()

[Phil Sutter]

This is done everywhere else as well, so certainly not a bad thing here
either.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/trace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/trace.c b/src/trace.c
index bd05d3c58d2fa..b016e723a7ded 100644
--- a/src/trace.c
+++ b/src/trace.c
@@ -301,7 +301,8 @@ static int nftnl_trace_parse_verdict(const struct nlattr *attr,
 {
 	struct nlattr *tb[NFTA_VERDICT_MAX+1];
 
-	mnl_attr_parse_nested(attr, nftnl_trace_parse_verdict_cb, tb);
+	if (mnl_attr_parse_nested(attr, nftnl_trace_parse_verdict_cb, tb) < 0)
+		return -1;
 
 	if (!tb[NFTA_VERDICT_CODE])
 		abi_breakage();
-- 
2.13.1

Re: [libnftnl PATCH 1/6] nftnl_data_reg_snprintf: Add a missing break

[Pablo Neira Ayuso]

On Thu, Dec 14, 2017 at 08:40:20PM +0100, Phil Sutter wrote:
> The code works fine as-is, but if reg_type == DATA_VALUE &&
> output_format == NFTNL_OUTPUT_XML, we fall through to DATA_CHAIN case
> and therefore pointlessly check output_format again.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  src/expr/data_reg.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/expr/data_reg.c b/src/expr/data_reg.c
> index a246952f7f01b..c087d792a8af6 100644
> --- a/src/expr/data_reg.c
> +++ b/src/expr/data_reg.c
> @@ -207,6 +207,7 @@ int nftnl_data_reg_snprintf(char *buf, size_t size,
>  		default:
>  			break;
>  		}
> +		break;
>  	case DATA_VERDICT:
>  	case DATA_CHAIN:
>  		switch(output_format) {

I'm going to add a break also at the end of 'case DATA_CHAIN:', recent
gcc can spot warning on implicit fall through without comments, so
let's fix that spot too.

Applied, thanks!

Re: [libnftnl PATCH 2/6] src/gen: Remove a pointless call to mnl_nlmsg_get_payload()

[Pablo Neira Ayuso]

On Thu, Dec 14, 2017 at 08:40:21PM +0100, Phil Sutter wrote:
> It is a common idiom in all *_nlmsg_parse() functions, but
> nftnl_gen_nlmsg_parse() doesn't make use of the data pointer and the
> compiler probably can't eliminate it since there could be a side-effect.

Also applied, thanks.

Re: [libnftnl PATCH 3/6] src/object: Avoid returning garbage in nftnl_obj_do_parse()

[Pablo Neira Ayuso]

On Thu, Dec 14, 2017 at 08:40:22PM +0100, Phil Sutter wrote:
> It may happen that 'perr' variable does not get initialized, so making
> parameter 'err' point to it in any case is error-prone. Avoid this by
> initializing 'perr' upon declaration.

Applied, thanks.

Re: [libnftnl PATCH 4/6] src/ruleset: Avoid reading garbage in nftnl_ruleset_cb()

[Pablo Neira Ayuso]

On Thu, Dec 14, 2017 at 08:40:23PM +0100, Phil Sutter wrote:
> If nftnl_ruleset_json_parse() is called with arg == NULL, ctx.data is
> left uninitialized and will later be used in nftnl_ruleset_cb(). Avoid
> this by using a C99-style initializer for 'ctx' which sets all omitted
> fields to zero.

Applied, thanks Phil.

Re: [libnftnl PATCH 5/6] src/set_elem: Don't return garbage in nftnl_set_elems_parse()

[Pablo Neira Ayuso]

On Thu, Dec 14, 2017 at 08:40:24PM +0100, Phil Sutter wrote:
> This might happen if netlink message is malformed (no nested attributes
> are present), so treat this as an error and return -1 instead of
> garbage to caller.

Applied, thanks.

Re: [libnftnl PATCH 6/6] src/trace: Check return value of mnl_attr_parse_nested()

[Pablo Neira Ayuso]

On Thu, Dec 14, 2017 at 08:40:25PM +0100, Phil Sutter wrote:
> This is done everywhere else as well, so certainly not a bad thing here
> either.

Indeed. Applied, thanks Phil.