[nft PATCH 0/5] Some fixes for JSON support

[nft PATCH 0/5] Some fixes for JSON support

[Phil Sutter]

The first patch is actually a generic libnftables fix, it prevents a
segfault when reporting parser errors.

The remaining fixes were found when trying to parse a ruleset prepared
by firewalld (JSON API usage is WiP there). Seems like increasing test
coverage really is in order.

Phil Sutter (5):
  libnftables: Print errors before freeing commands
  parser_json: Duplicate chain name when parsing jump verdict
  parser_json: Use xstrdup() when parsing rule comment
  json: Fix memleaks in echo support
  parser_json: Respect base chain priority

 src/libnftables.c |  4 ++--
 src/parser_json.c | 34 ++++++++++++++++++++++------------
 2 files changed, 24 insertions(+), 14 deletions(-)

-- 
2.20.1

[nft PATCH 1/5] libnftables: Print errors before freeing commands

[Phil Sutter]

Commands may contain data printed by an error record, so make sure
cmd_free() is not called before erec_print_list() has returned.

Fixes: 778de37d82e7b ("libnftables: Keep cmds list outside of parser_state")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/libnftables.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/libnftables.c b/src/libnftables.c
index bd79cd6091d25..2271d270fd574 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -398,11 +398,11 @@ int nft_run_cmd_from_buffer(struct nft_ctx *nft, const char *buf)
 	if (nft_netlink(nft, &cmds, &msgs, nft->nf_sock) != 0)
 		rc = -1;
 err:
+	erec_print_list(&nft->output, &msgs, nft->debug_mask);
 	list_for_each_entry_safe(cmd, next, &cmds, list) {
 		list_del(&cmd->list);
 		cmd_free(cmd);
 	}
-	erec_print_list(&nft->output, &msgs, nft->debug_mask);
 	iface_cache_release();
 	if (nft->scanner) {
 		scanner_destroy(nft->scanner);
@@ -442,11 +442,11 @@ int nft_run_cmd_from_filename(struct nft_ctx *nft, const char *filename)
 	if (nft_netlink(nft, &cmds, &msgs, nft->nf_sock) != 0)
 		rc = -1;
 err:
+	erec_print_list(&nft->output, &msgs, nft->debug_mask);
 	list_for_each_entry_safe(cmd, next, &cmds, list) {
 		list_del(&cmd->list);
 		cmd_free(cmd);
 	}
-	erec_print_list(&nft->output, &msgs, nft->debug_mask);
 	iface_cache_release();
 	if (nft->scanner) {
 		scanner_destroy(nft->scanner);
-- 
2.20.1

[nft PATCH 2/5] parser_json: Duplicate chain name when parsing jump verdict

[Phil Sutter]

Since verdict expression frees the chain name, pass a newly allocated
string to it. Otherwise double free happens because json_decref() frees
the string property value as well.

Fixes: d1057a5feb5fd ("JSON: Simplify verdict statement parsing")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/parser_json.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/parser_json.c b/src/parser_json.c
index d00cf422c314e..78214f6519f2b 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -1075,7 +1075,8 @@ static struct expr *json_parse_verdict_expr(struct json_ctx *ctx,
 			return NULL;
 
 		return verdict_expr_alloc(int_loc,
-					  verdict_tbl[i].verdict, chain);
+					  verdict_tbl[i].verdict,
+					  chain ? xstrdup(chain) : NULL);
 	}
 	json_error(ctx, "Unknown verdict '%s'.", type);
 	return NULL;
-- 
2.20.1

[nft PATCH 3/5] parser_json: Use xstrdup() when parsing rule comment

[Phil Sutter]

Use xstrdup() instead of plain strdup() for consistency (and implicit
ENOMEM checking).

Fixes: 586ad210368b7 ("libnftables: Implement JSON parser")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/parser_json.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/parser_json.c b/src/parser_json.c
index 78214f6519f2b..6755d39c34f0a 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -2461,7 +2461,7 @@ static struct cmd *json_parse_cmd_add_rule(struct json_ctx *ctx, json_t *root,
 
 	json_unpack(root, "{s:s}", "comment", &comment);
 	if (comment)
-		rule->comment = strdup(comment);
+		rule->comment = xstrdup(comment);
 
 	json_array_foreach(tmp, index, value) {
 		struct stmt *stmt;
-- 
2.20.1

[nft PATCH 4/5] json: Fix memleaks in echo support

[Phil Sutter]

When extracting netlink message data for populating JSON objects with
handles, allocated nftnl objects were not freed. Though since freeing
these objects also frees retrieved string attributes, copy them using
strdupa() which takes care of memory deallocation upon function return.
This is ideal since these strings are used only to find the right JSON
object to insert the handle into.

Fixes: bb32d8db9a125 ("JSON: Add support for echo option")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/parser_json.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/src/parser_json.c b/src/parser_json.c
index 6755d39c34f0a..c92113ba516c2 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -1,3 +1,4 @@
+#define _GNU_SOURCE
 #include <errno.h>
 #include <stdint.h> /* needed by gmputil.h */
 #include <string.h>
@@ -3485,8 +3486,9 @@ static int json_update_table(struct netlink_mon_handler *monh,
 
 	nlt = netlink_table_alloc(nlh);
 	family = family2str(nftnl_table_get_u32(nlt, NFTNL_TABLE_FAMILY));
-	name = nftnl_table_get_str(nlt, NFTNL_TABLE_NAME);
+	name = strdupa(nftnl_table_get_str(nlt, NFTNL_TABLE_NAME));
 	handle = nftnl_table_get_u64(nlt, NFTNL_TABLE_HANDLE);
+	nftnl_table_free(nlt);
 
 	json_array_foreach(array, index, value) {
 		if (json_unpack(value, "{s:{s:o}}", "add", "table", &value) ||
@@ -3512,9 +3514,10 @@ static int json_update_chain(struct netlink_mon_handler *monh,
 
 	nlc = netlink_chain_alloc(nlh);
 	family = family2str(nftnl_chain_get_u32(nlc, NFTNL_CHAIN_FAMILY));
-	table = nftnl_chain_get_str(nlc, NFTNL_CHAIN_TABLE);
-	name = nftnl_chain_get_str(nlc, NFTNL_CHAIN_NAME);
+	table = strdupa(nftnl_chain_get_str(nlc, NFTNL_CHAIN_TABLE));
+	name = strdupa(nftnl_chain_get_str(nlc, NFTNL_CHAIN_NAME));
 	handle = nftnl_chain_get_u64(nlc, NFTNL_CHAIN_HANDLE);
+	nftnl_chain_free(nlc);
 
 	json_array_foreach(array, index, value) {
 		if (json_unpack(value, "{s:{s:o}}", "add", "chain", &value) ||
@@ -3540,9 +3543,10 @@ static int json_update_rule(struct netlink_mon_handler *monh,
 
 	nlr = netlink_rule_alloc(nlh);
 	family = family2str(nftnl_rule_get_u32(nlr, NFTNL_RULE_FAMILY));
-	table = nftnl_rule_get_str(nlr, NFTNL_RULE_TABLE);
-	chain = nftnl_rule_get_str(nlr, NFTNL_RULE_CHAIN);
+	table = strdupa(nftnl_rule_get_str(nlr, NFTNL_RULE_TABLE));
+	chain = strdupa(nftnl_rule_get_str(nlr, NFTNL_RULE_CHAIN));
 	handle = nftnl_rule_get_u64(nlr, NFTNL_RULE_HANDLE);
+	nftnl_rule_free(nlr);
 
 	json_array_foreach(array, index, value) {
 		if (json_unpack(value, "{s:{s:o}}", "add", "rule", &value) ||
@@ -3574,13 +3578,16 @@ static int json_update_set(struct netlink_mon_handler *monh,
 
 	nls = netlink_set_alloc(nlh);
 	flags = nftnl_set_get_u32(nls, NFTNL_SET_FLAGS);
-	if (flags & NFT_SET_ANONYMOUS)
+	if (flags & NFT_SET_ANONYMOUS) {
+		nftnl_set_free(nls);
 		return MNL_CB_OK;
+	}
 
 	family = family2str(nftnl_set_get_u32(nls, NFTNL_SET_FAMILY));
-	table = nftnl_set_get_str(nls, NFTNL_SET_TABLE);
-	name = nftnl_set_get_str(nls, NFTNL_SET_NAME);
+	table = strdupa(nftnl_set_get_str(nls, NFTNL_SET_TABLE));
+	name = strdupa(nftnl_set_get_str(nls, NFTNL_SET_NAME));
 	handle = nftnl_set_get_u64(nls, NFTNL_SET_HANDLE);
+	nftnl_set_free(nls);
 
 	json_array_foreach(array, index, value) {
 		if (json_unpack(value, "{s:{s:o}}", "add", "set", &value) ||
@@ -3605,10 +3612,11 @@ static int json_update_obj(struct netlink_mon_handler *monh,
 
 	nlo = netlink_obj_alloc(nlh);
 	family = family2str(nftnl_obj_get_u32(nlo, NFTNL_OBJ_FAMILY));
-	table = nftnl_obj_get_str(nlo, NFTNL_OBJ_TABLE);
-	name = nftnl_obj_get_str(nlo, NFTNL_OBJ_NAME);
+	table = strdupa(nftnl_obj_get_str(nlo, NFTNL_OBJ_TABLE));
+	name = strdupa(nftnl_obj_get_str(nlo, NFTNL_OBJ_NAME));
 	type = obj_type_name(nftnl_obj_get_u32(nlo, NFTNL_OBJ_TYPE));
 	handle = nftnl_obj_get_u64(nlo, NFTNL_OBJ_HANDLE);
+	nftnl_obj_free(nlo);
 
 	json_array_foreach(array, index, value) {
 		if (json_unpack(value, "{s:{s:o}}", "add", type, &value) ||
-- 
2.20.1

[nft PATCH 5/5] parser_json: Respect base chain priority

[Phil Sutter]

Priority value was parsed but not assigned to allocated chain object.

Fixes: 586ad210368b7 ("libnftables: Implement JSON parser")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/parser_json.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/parser_json.c b/src/parser_json.c
index c92113ba516c2..7b190bc78103e 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -2395,6 +2395,7 @@ static struct cmd *json_parse_cmd_add_chain(struct json_ctx *ctx, json_t *root,
 	chain = chain_alloc(NULL);
 	chain->flags |= CHAIN_F_BASECHAIN;
 	chain->type = xstrdup(type);
+	chain->priority.num = prio;
 	chain->hookstr = chain_hookname_lookup(hookstr);
 	if (!chain->hookstr) {
 		json_error(ctx, "Invalid chain hook '%s'.", hookstr);
-- 
2.20.1

Re: [nft PATCH 0/5] Some fixes for JSON support

[Pablo Neira Ayuso]

On Tue, Feb 26, 2019 at 10:13:37PM +0100, Phil Sutter wrote:
> The first patch is actually a generic libnftables fix, it prevents a
> segfault when reporting parser errors.
> 
> The remaining fixes were found when trying to parse a ruleset prepared
> by firewalld (JSON API usage is WiP there). Seems like increasing test
> coverage really is in order.

Series applied, thanks.

I have one question regarding patch 4/5 though, will follow up on
this.

Re: [nft PATCH 4/5] json: Fix memleaks in echo support

[Pablo Neira Ayuso]

On Tue, Feb 26, 2019 at 10:13:41PM +0100, Phil Sutter wrote:
> When extracting netlink message data for populating JSON objects with
> handles, allocated nftnl objects were not freed. Though since freeing
> these objects also frees retrieved string attributes, copy them using
> strdupa() which takes care of memory deallocation upon function return.
> This is ideal since these strings are used only to find the right JSON
> object to insert the handle into.
> 
> Fixes: bb32d8db9a125 ("JSON: Add support for echo option")
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  src/parser_json.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/src/parser_json.c b/src/parser_json.c
> index 6755d39c34f0a..c92113ba516c2 100644
> --- a/src/parser_json.c
> +++ b/src/parser_json.c
> @@ -1,3 +1,4 @@
> +#define _GNU_SOURCE
>  #include <errno.h>
>  #include <stdint.h> /* needed by gmputil.h */
>  #include <string.h>
> @@ -3485,8 +3486,9 @@ static int json_update_table(struct netlink_mon_handler *monh,
>  
>  	nlt = netlink_table_alloc(nlh);
>  	family = family2str(nftnl_table_get_u32(nlt, NFTNL_TABLE_FAMILY));
> -	name = nftnl_table_get_str(nlt, NFTNL_TABLE_NAME);
> +	name = strdupa(nftnl_table_get_str(nlt, NFTNL_TABLE_NAME));

Hm, I still don't see why we need this strdupa() call...

>  	handle = nftnl_table_get_u64(nlt, NFTNL_TABLE_HANDLE);
> +	nftnl_table_free(nlt);

The table name attribute is owned by the nlt object.

This is used by obj_info_matches() which doesn't take this?

Re: [nft PATCH 4/5] json: Fix memleaks in echo support

[Phil Sutter]

Hi Pablo,

On Wed, Feb 27, 2019 at 11:29:20AM +0100, Pablo Neira Ayuso wrote:
> On Tue, Feb 26, 2019 at 10:13:41PM +0100, Phil Sutter wrote:
> > When extracting netlink message data for populating JSON objects with
> > handles, allocated nftnl objects were not freed. Though since freeing
> > these objects also frees retrieved string attributes, copy them using
> > strdupa() which takes care of memory deallocation upon function return.
> > This is ideal since these strings are used only to find the right JSON
> > object to insert the handle into.
> > 
> > Fixes: bb32d8db9a125 ("JSON: Add support for echo option")
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  src/parser_json.c | 28 ++++++++++++++++++----------
> >  1 file changed, 18 insertions(+), 10 deletions(-)
> > 
> > diff --git a/src/parser_json.c b/src/parser_json.c
> > index 6755d39c34f0a..c92113ba516c2 100644
> > --- a/src/parser_json.c
> > +++ b/src/parser_json.c
> > @@ -1,3 +1,4 @@
> > +#define _GNU_SOURCE
> >  #include <errno.h>
> >  #include <stdint.h> /* needed by gmputil.h */
> >  #include <string.h>
> > @@ -3485,8 +3486,9 @@ static int json_update_table(struct netlink_mon_handler *monh,
> >  
> >  	nlt = netlink_table_alloc(nlh);
> >  	family = family2str(nftnl_table_get_u32(nlt, NFTNL_TABLE_FAMILY));
> > -	name = nftnl_table_get_str(nlt, NFTNL_TABLE_NAME);
> > +	name = strdupa(nftnl_table_get_str(nlt, NFTNL_TABLE_NAME));
> 
> Hm, I still don't see why we need this strdupa() call...
> 
> >  	handle = nftnl_table_get_u64(nlt, NFTNL_TABLE_HANDLE);
> > +	nftnl_table_free(nlt);
> 
> The table name attribute is owned by the nlt object.
> 
> This is used by obj_info_matches() which doesn't take this?

Strictly it is not necessary, but the call to nftnl_table_free() would
have to be delayed till after the search. Since the function either
returns from the loop or after it, I would either duplicate the free
call or have to introduce some return value handling.

The original problem is that I first extract the table name using
nftnl_table_get_str(), then two lines later free the nftnl_table object
(which makes the extracted const char *name point at freed memory).

The semantics I follow in all those update functions is:

1) parse netlink message into nftnl object
2) extract required info from nftnl object
3) free nftnl object again (since it's not used anymore afterwards)
4) lookup JSON object with data from (2) and insert handle (also from
(2)).

Using strdupa() allows for this without the need for a final cleanup
step.

Cheers, Phil

Re: [nft PATCH 4/5] json: Fix memleaks in echo support

[Pablo Neira Ayuso]

On Wed, Feb 27, 2019 at 12:15:26PM +0100, Phil Sutter wrote:
> Hi Pablo,
> 
> On Wed, Feb 27, 2019 at 11:29:20AM +0100, Pablo Neira Ayuso wrote:
> > On Tue, Feb 26, 2019 at 10:13:41PM +0100, Phil Sutter wrote:
> > > When extracting netlink message data for populating JSON objects with
> > > handles, allocated nftnl objects were not freed. Though since freeing
> > > these objects also frees retrieved string attributes, copy them using
> > > strdupa() which takes care of memory deallocation upon function return.
> > > This is ideal since these strings are used only to find the right JSON
> > > object to insert the handle into.
> > > 
> > > Fixes: bb32d8db9a125 ("JSON: Add support for echo option")
> > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > ---
> > >  src/parser_json.c | 28 ++++++++++++++++++----------
> > >  1 file changed, 18 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/src/parser_json.c b/src/parser_json.c
> > > index 6755d39c34f0a..c92113ba516c2 100644
> > > --- a/src/parser_json.c
> > > +++ b/src/parser_json.c
> > > @@ -1,3 +1,4 @@
> > > +#define _GNU_SOURCE
> > >  #include <errno.h>
> > >  #include <stdint.h> /* needed by gmputil.h */
> > >  #include <string.h>
> > > @@ -3485,8 +3486,9 @@ static int json_update_table(struct netlink_mon_handler *monh,
> > >  
> > >  	nlt = netlink_table_alloc(nlh);
> > >  	family = family2str(nftnl_table_get_u32(nlt, NFTNL_TABLE_FAMILY));
> > > -	name = nftnl_table_get_str(nlt, NFTNL_TABLE_NAME);
> > > +	name = strdupa(nftnl_table_get_str(nlt, NFTNL_TABLE_NAME));
> > 
> > Hm, I still don't see why we need this strdupa() call...
> > 
> > >  	handle = nftnl_table_get_u64(nlt, NFTNL_TABLE_HANDLE);
> > > +	nftnl_table_free(nlt);
> > 
> > The table name attribute is owned by the nlt object.
> > 
> > This is used by obj_info_matches() which doesn't take this?
> 
> Strictly it is not necessary, but the call to nftnl_table_free() would
> have to be delayed till after the search. Since the function either
> returns from the loop or after it, I would either duplicate the free
> call or have to introduce some return value handling.
>
> The original problem is that I first extract the table name using
> nftnl_table_get_str(), then two lines later free the nftnl_table object
> (which makes the extracted const char *name point at freed memory).
>
> The semantics I follow in all those update functions is:
> 
> 1) parse netlink message into nftnl object
> 2) extract required info from nftnl object
> 3) free nftnl object again (since it's not used anymore afterwards)
> 4) lookup JSON object with data from (2) and insert handle (also from
> (2)).
> 
> Using strdupa() allows for this without the need for a final cleanup
> step.

OK, so the object is released and you just clone the fields that you
need around for the lookup, that's fine with me.

Not related to this patch: IIRC this echo support is not using the
nlmsg_seq to correlate the command and the result that we obtain,
right? Telling this because this should work with a batch that
contains several requests to create rules, then use this sequence
number to correlate things the reply with the original rule creation
command.

Re: [nft PATCH 4/5] json: Fix memleaks in echo support

[Phil Sutter]

On Wed, Feb 27, 2019 at 11:29:26PM +0100, Pablo Neira Ayuso wrote:
[...]
> Not related to this patch: IIRC this echo support is not using the
> nlmsg_seq to correlate the command and the result that we obtain,
> right? Telling this because this should work with a batch that
> contains several requests to create rules, then use this sequence
> number to correlate things the reply with the original rule creation
> command.

I think you once suggested that and I promised to look into it. :)

So I just did and while nlmsg_seq would serve nice to clearly identify
which command a returned handle belongs to, I don't know how to map the
command back to the JSON object from which it was created.

Cheers, Phil

Re: [nft PATCH 4/5] json: Fix memleaks in echo support

[Pablo Neira Ayuso]

On Thu, Feb 28, 2019 at 11:30:26AM +0100, Phil Sutter wrote:
> On Wed, Feb 27, 2019 at 11:29:26PM +0100, Pablo Neira Ayuso wrote:
> [...]
> > Not related to this patch: IIRC this echo support is not using the
> > nlmsg_seq to correlate the command and the result that we obtain,
> > right? Telling this because this should work with a batch that
> > contains several requests to create rules, then use this sequence
> > number to correlate things the reply with the original rule creation
> > command.
> 
> I think you once suggested that and I promised to look into it. :)
> 
> So I just did and while nlmsg_seq would serve nice to clearly identify
> which command a returned handle belongs to, I don't know how to map the
> command back to the JSON object from which it was created.

If you have a structure that stores commands in a list or such, the
idea would be to store the sequence number in the command object.
You can just keep this in the radar, no need to work on this right now.

Thanks!