[iptables PATCH 00/15] cache evaluation phase bonus material

[iptables PATCH 00/15] cache evaluation phase bonus material

[Phil Sutter]

Play a bit with valgrind I thought. This will be easy, I thought. So
here's what this turned into:

Patches 1-4 fix bugs in the previous series "iptables: introduce cache
evaluation phase" and hence will get folded into respective commits
before pushing upstream. I left those separate to ease reviews and
provide some explanation in commit messages.

Patch 5 reveals what happens if I'm too lazy to create test cases for
use with valgrind but am not too lazy for shell scripting: In a "big
hammer turns everything into a nail" style, I hacked tests/shell for
memleak analysis.

The remaining patches fix old code, mostly to get rid of reachable
memory at zero-status program exit. This is not just cosmetics: Reducing
noise in valgrind output does a great deal to emphasize real issues.

Phil Sutter (15):
  nft: Free rule pointer in nft_cmd_free()
  nft: Add missing clear_cs() calls
  nft: Avoid use-after-free when rebuilding cache
  nft: Call nft_release_cache() in nft_fini()
  tests: shell: Implement --valgrind mode
  nft: cache: Re-establish cache consistency check
  nft: Clear all lists in nft_fini()
  nft: Fix leaks in ebt_add_policy_rule()
  nft: Fix leak when deleting rules
  ebtables: Free statically loaded extensions again
  libxtables: Introduce xtables_fini()
  nft: Use clear_cs() instead of open coding
  arptables: Fix leak in nft_arp_print_rule()
  nft: Fix leak when replacing a rule
  nft: Don't exit early after printing help texts

 configure.ac                      |  4 +--
 include/xtables.h                 |  1 +
 iptables/ip6tables-standalone.c   |  2 ++
 iptables/iptables-restore.c       | 14 ++++++---
 iptables/iptables-save.c          | 14 +++++++--
 iptables/iptables-standalone.c    |  2 ++
 iptables/nft-arp.c                |  3 ++
 iptables/nft-bridge.c             |  1 +
 iptables/nft-cache.c              | 25 +++++++++++++---
 iptables/nft-cmd.c                |  9 +++++-
 iptables/nft-ipv4.c               |  2 +-
 iptables/nft-ipv6.c               |  2 +-
 iptables/nft-shared.c             |  1 +
 iptables/nft.c                    | 37 ++++++++++++++++--------
 iptables/nft.h                    |  5 ++--
 iptables/tests/shell/run-tests.sh | 47 +++++++++++++++++++++++++++++++
 iptables/xtables-arp-standalone.c |  1 +
 iptables/xtables-arp.c            | 14 ++++-----
 iptables/xtables-eb-standalone.c  |  2 +-
 iptables/xtables-eb.c             | 20 ++++++++++++-
 iptables/xtables-monitor.c        |  2 ++
 iptables/xtables-restore.c        |  4 ++-
 iptables/xtables-save.c           |  1 +
 iptables/xtables-standalone.c     |  1 +
 iptables/xtables-translate.c      |  2 ++
 iptables/xtables.c                | 13 ++++-----
 libxtables/xtables.c              | 44 ++++++++++++++++++++++++++++-
 27 files changed, 224 insertions(+), 49 deletions(-)

-- 
2.25.1

[iptables PATCH 01/15] nft: Free rule pointer in nft_cmd_free()

[Phil Sutter]

Most commands either don't assign to obj.rule or pass it on when
creating a batch job. Check and delete commands are the exception to
that.

One could free the rule inside nft_rule_check() and nft_rule_delete() as
well, but since only the pointer is passed to them via parameter, the
pointer would remain set afterwards. So instead do that from the proper
routine. At some point, structs nft_cmd and obj_update should be merged
and consequently the functions called from nft_prepare() be given full
control over that combined struct so they can zero pointers if data is
reused or leave set to get them freed.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-cmd.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/iptables/nft-cmd.c b/iptables/nft-cmd.c
index 3c0c6a34515e4..1f46dc6c369cc 100644
--- a/iptables/nft-cmd.c
+++ b/iptables/nft-cmd.c
@@ -57,7 +57,14 @@ void nft_cmd_free(struct nft_cmd *cmd)
 	free((void *)cmd->rename);
 	free((void *)cmd->jumpto);
 
-	/* cmd->obj.rule not released here. */
+	switch (cmd->command) {
+	case NFT_COMPAT_RULE_CHECK:
+	case NFT_COMPAT_RULE_DELETE:
+		free(cmd->obj.rule);
+		break;
+	default:
+		break;
+	}
 
 	list_del(&cmd->head);
 	free(cmd);
-- 
2.25.1

[iptables PATCH 02/15] nft: Add missing clear_cs() calls

[Phil Sutter]

Previous patch "nft: split parsing from netlink commands" added a second
struct iptables_command_state to rule_find functions but missed to add a
related clear_cs() call also.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-arp.c    | 1 +
 iptables/nft-bridge.c | 1 +
 iptables/nft-shared.c | 1 +
 3 files changed, 3 insertions(+)

diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
index 748784bc49048..e9a2d9de21560 100644
--- a/iptables/nft-arp.c
+++ b/iptables/nft-arp.c
@@ -656,6 +656,7 @@ static bool nft_arp_rule_find(struct nft_handle *h, struct nftnl_rule *r,
 	ret = true;
 out:
 	h->ops->clear_cs(&this);
+	h->ops->clear_cs(cs);
 	return ret;
 }
 
diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index 80d7f91710c16..39a2f704000c7 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -789,6 +789,7 @@ static bool nft_bridge_rule_find(struct nft_handle *h, struct nftnl_rule *r,
 	ret = true;
 out:
 	h->ops->clear_cs(&this);
+	h->ops->clear_cs(cs);
 	return ret;
 }
 
diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index 6b425f8525d3a..bfc7bc2203239 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -1024,6 +1024,7 @@ bool nft_ipv46_rule_find(struct nft_handle *h, struct nftnl_rule *r,
 	ret = true;
 out:
 	h->ops->clear_cs(&this);
+	h->ops->clear_cs(cs);
 	return ret;
 }
 
-- 
2.25.1

[iptables PATCH 03/15] nft: Avoid use-after-free when rebuilding cache

[Phil Sutter]

By the time nft_action() decides to rebuild the cache, nft_cmd structs
have been freed already and therefore table and chain names in
nft_cache_req point to invalid memory.

Fix this by duplicating the strings and freeing them when releasing the
cache.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-cache.c | 14 ++++++++++----
 iptables/nft.h       |  4 ++--
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index 83af9a2f689e1..84ea97d3e54a6 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -39,7 +39,7 @@ static void cache_chain_list_insert(struct list_head *list, const char *name)
 	}
 
 	new = xtables_malloc(sizeof(*new));
-	new->name = name;
+	new->name = strdup(name);
 	list_add_tail(&new->head, pos ? &pos->head : list);
 }
 
@@ -54,7 +54,10 @@ void nft_cache_level_set(struct nft_handle *h, int level,
 	if (!cmd || !cmd->table || req->all_chains)
 		return;
 
-	req->table = cmd->table;
+	if (!req->table)
+		req->table = strdup(cmd->table);
+	else
+		assert(!strcmp(req->table, cmd->table));
 
 	if (!cmd->chain) {
 		req->all_chains = true;
@@ -663,11 +666,14 @@ void nft_release_cache(struct nft_handle *h)
 
 	if (req->level != NFT_CL_FAKE)
 		req->level = NFT_CL_TABLES;
-	req->table = NULL;
-
+	if (req->table) {
+		free(req->table);
+		req->table = NULL;
+	}
 	req->all_chains = false;
 	list_for_each_entry_safe(cc, cc_tmp, &req->chain_list, head) {
 		list_del(&cc->head);
+		free(cc->name);
 		free(cc);
 	}
 }
diff --git a/iptables/nft.h b/iptables/nft.h
index 045393da7c179..aeacc608fcb9f 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -73,12 +73,12 @@ enum obj_update_type {
 
 struct cache_chain {
 	struct list_head head;
-	const char *name;
+	char *name;
 };
 
 struct nft_cache_req {
 	enum nft_cache_level	level;
-	const char		*table;
+	char			*table;
 	bool			all_chains;
 	struct list_head	chain_list;
 };
-- 
2.25.1

[iptables PATCH 04/15] nft: Call nft_release_cache() in nft_fini()

[Phil Sutter]

In contrast to flush_chain_cache(), this will also clean up cache_req.

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

diff --git a/iptables/nft.c b/iptables/nft.c
index d2796fcd8ad26..6503259eb443e 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -855,7 +855,7 @@ void nft_fini(struct nft_handle *h)
 	list_for_each_entry_safe(cmd, next, &h->cmd_list, head)
 		nft_cmd_free(cmd);
 
-	flush_chain_cache(h, NULL);
+	nft_release_cache(h);
 	mnl_socket_close(h->nl);
 }
 
-- 
2.25.1

[iptables PATCH 05/15] tests: shell: Implement --valgrind mode

[Phil Sutter]

Wrap every call to $XT_MULTI with valgrind, or actually a wrapper script
which does the valgrind wrap and stores the log if it contains something
relevant.

Carefully name the wrapper script(s) so that test cases' checks on
$XT_MULTI name stay intact.

This mode slows down testsuite execution horribly. Luckily, it's not
meant for constant use, though.

For now, ignore commands with non-zero exit status - error paths
typically hit direct exit() calls and therefore leave reachable memory
in place.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/tests/shell/run-tests.sh | 47 +++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/iptables/tests/shell/run-tests.sh b/iptables/tests/shell/run-tests.sh
index d71c13729b3ee..2125e2cb119bb 100755
--- a/iptables/tests/shell/run-tests.sh
+++ b/iptables/tests/shell/run-tests.sh
@@ -46,6 +46,10 @@ while [ -n "$1" ]; do
 		NFT_ONLY=y
 		shift
 		;;
+	-V|--valgrind)
+		VALGRIND=y
+		shift
+		;;
 	*${RETURNCODE_SEPARATOR}+([0-9]))
 		SINGLE+=" $1"
 		VERBOSE=y
@@ -67,6 +71,49 @@ else
 	XTABLES_LEGACY_MULTI="xtables-legacy-multi"
 fi
 
+printscript() { # (cmd, tmpd)
+	cat <<EOF
+#!/bin/bash
+
+CMD="$1"
+
+# note: valgrind man page warns about --log-file with --trace-children, the
+# last child executed overwrites previous reports unless %p or %q is used.
+# Since libtool wrapper calls exec but none of the iptables tools do, this is
+# perfect for us as it effectively hides bash-related errors
+
+valgrind --log-file=$2/valgrind.log --trace-children=yes \
+	 --leak-check=full --show-leak-kinds=all \$CMD "\$@"
+RC=\$?
+
+# don't keep uninteresting logs
+if grep -q 'no leaks are possible' $2/valgrind.log; then
+	rm $2/valgrind.log
+else
+	mv $2/valgrind.log $2/valgrind_\$\$.log
+fi
+
+# drop logs for failing commands for now
+[ \$RC -eq 0 ] || rm $2/valgrind_\$\$.log
+
+exit \$RC
+EOF
+}
+
+if [ "$VALGRIND" == "y" ]; then
+	tmpd=$(mktemp -d)
+	msg_info "writing valgrind logs to $tmpd"
+	chmod a+rx $tmpd
+	printscript "$XTABLES_NFT_MULTI" "$tmpd" >${tmpd}/xtables-nft-multi
+	printscript "$XTABLES_LEGACY_MULTI" "$tmpd" >${tmpd}/xtables-legacy-multi
+	trap "rm ${tmpd}/xtables-*-multi" EXIT
+	chmod a+x ${tmpd}/xtables-nft-multi ${tmpd}/xtables-legacy-multi
+
+	XTABLES_NFT_MULTI="${tmpd}/xtables-nft-multi"
+	XTABLES_LEGACY_MULTI="${tmpd}/xtables-legacy-multi"
+
+fi
+
 find_tests() {
         if [ ! -z "$SINGLE" ] ; then
                 echo $SINGLE
-- 
2.25.1

[iptables PATCH 06/15] nft: cache: Re-establish cache consistency check

[Phil Sutter]

Restore code ensuring __nft_build_cache() returns a consistent cache in
which all ruleset elements belong to the same generation.

This check was removed by commit 200bc39965149 ("nft: cache: Fix
iptables-save segfault under stress") as it could lead to segfaults if a
partial cache fetch was done while cache's chain list was traversed.
With the new cache fetch logic, __nft_build_cache() is never called
while holding references to cache entries.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-cache.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index 84ea97d3e54a6..638b18bc7e382 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -484,12 +484,16 @@ static int fetch_rule_cache(struct nft_handle *h,
 	return 0;
 }
 
+static int flush_cache(struct nft_handle *h, struct nft_cache *c,
+		       const char *tablename);
+
 static void
 __nft_build_cache(struct nft_handle *h)
 {
 	struct nft_cache_req *req = &h->cache_req;
 	const struct builtin_table *t = NULL;
 	struct list_head *chains = NULL;
+	uint32_t genid_check;
 
 	if (h->cache_init)
 		return;
@@ -501,6 +505,7 @@ __nft_build_cache(struct nft_handle *h)
 	}
 
 	h->cache_init = true;
+retry:
 	mnl_genid_get(h, &h->nft_genid);
 
 	if (req->level >= NFT_CL_TABLES)
@@ -513,6 +518,12 @@ __nft_build_cache(struct nft_handle *h)
 		fetch_set_cache(h, t, NULL);
 	if (req->level >= NFT_CL_RULES)
 		fetch_rule_cache(h, t);
+
+	mnl_genid_get(h, &genid_check);
+	if (h->nft_genid != genid_check) {
+		flush_cache(h, h->cache, NULL);
+		goto retry;
+	}
 }
 
 static void __nft_flush_cache(struct nft_handle *h)
-- 
2.25.1

[iptables PATCH 07/15] nft: Clear all lists in nft_fini()

[Phil Sutter]

Remove and free any pending entries in obj_list and err_list as well. To
get by without having to declare list-specific cursors, use generic
list_head types and call list_entry() explicitly.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 6503259eb443e..addde1b53f37e 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -850,10 +850,16 @@ int nft_init(struct nft_handle *h, int family, const struct builtin_table *t)
 
 void nft_fini(struct nft_handle *h)
 {
-	struct nft_cmd *cmd, *next;
+	struct list_head *pos, *n;
 
-	list_for_each_entry_safe(cmd, next, &h->cmd_list, head)
-		nft_cmd_free(cmd);
+	list_for_each_safe(pos, n, &h->cmd_list)
+		nft_cmd_free(list_entry(pos, struct nft_cmd, head));
+
+	list_for_each_safe(pos, n, &h->obj_list)
+		batch_obj_del(h, list_entry(pos, struct obj_update, head));
+
+	list_for_each_safe(pos, n, &h->err_list)
+		mnl_err_list_free(list_entry(pos, struct mnl_err, head));
 
 	nft_release_cache(h);
 	mnl_socket_close(h->nl);
-- 
2.25.1

[iptables PATCH 08/15] nft: Fix leaks in ebt_add_policy_rule()

[Phil Sutter]

The function leaked memory allocated in temporary struct
iptables_command_state, clean it immediately after use.

In any of the udata-related error cases, allocated nftnl_rule would
leak, fix this by introducing a common error path to goto.

In regular code path, the allocated nftnl_rule would still leak:
batch_obj_del() does not free rules in NFT_COMPAT_RULE_APPEND jobs, as
they typically sit in cache as well. Policy rules in turn weren't added
to cache: They are created immediately before commit and never
referenced from other rules. Add them now so they are freed just like
regular rules.

Fixes: aff1162b3e4b7 ("ebtables-nft: Support user-defined chain policies")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index addde1b53f37e..c0b5e2fc524a7 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2970,27 +2970,33 @@ static int ebt_add_policy_rule(struct nftnl_chain *c, void *data)
 
 	r = nft_rule_new(h, nftnl_chain_get_str(c, NFTNL_CHAIN_NAME),
 			 nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE), &cs);
+	ebt_cs_clean(&cs);
+
 	if (!r)
 		return -1;
 
 	udata = nftnl_udata_buf_alloc(NFT_USERDATA_MAXLEN);
 	if (!udata)
-		return -1;
+		goto err_free_rule;
 
 	if (!nftnl_udata_put_u32(udata, UDATA_TYPE_EBTABLES_POLICY, 1))
-		return -1;
+		goto err_free_rule;
 
 	nftnl_rule_set_data(r, NFTNL_RULE_USERDATA,
 			    nftnl_udata_buf_data(udata),
 			    nftnl_udata_buf_len(udata));
 	nftnl_udata_buf_free(udata);
 
-	if (!batch_rule_add(h, NFT_COMPAT_RULE_APPEND, r)) {
-		nftnl_rule_free(r);
-		return -1;
-	}
+	if (!batch_rule_add(h, NFT_COMPAT_RULE_APPEND, r))
+		goto err_free_rule;
+
+	/* add the rule to chain so it is freed later */
+	nftnl_chain_rule_add_tail(r, c);
 
 	return 0;
+err_free_rule:
+	nftnl_rule_free(r);
+	return -1;
 }
 
 int ebt_set_user_chain_policy(struct nft_handle *h, const char *table,
-- 
2.25.1

[iptables PATCH 09/15] nft: Fix leak when deleting rules

[Phil Sutter]

For NFT_COMPAT_RULE_DELETE jobs, batch_obj_del() has to do the rule
freeing, they are no longer in cache.

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

diff --git a/iptables/nft.c b/iptables/nft.c
index c0b5e2fc524a7..01268f7859e9b 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2680,8 +2680,8 @@ static void batch_obj_del(struct nft_handle *h, struct obj_update *o)
 	case NFT_COMPAT_RULE_APPEND:
 	case NFT_COMPAT_RULE_INSERT:
 	case NFT_COMPAT_RULE_REPLACE:
-	case NFT_COMPAT_RULE_DELETE:
 		break;
+	case NFT_COMPAT_RULE_DELETE:
 	case NFT_COMPAT_RULE_FLUSH:
 		nftnl_rule_free(o->rule);
 		break;
-- 
2.25.1

[iptables PATCH 10/15] ebtables: Free statically loaded extensions again

[Phil Sutter]

All ebtables extensions are loaded upon program start as due to the lack
of '-m' parameters, loading on demand is not possible. Introduce
nft_fini_eb() to counteract nft_init_eb() and free dynamic memory in
matches and targets from there.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.h                   |  1 +
 iptables/xtables-eb-standalone.c |  2 +-
 iptables/xtables-eb.c            | 17 +++++++++++++++++
 iptables/xtables-restore.c       |  2 +-
 4 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/iptables/nft.h b/iptables/nft.h
index aeacc608fcb9f..bd783231156b7 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -225,6 +225,7 @@ int nft_init_arp(struct nft_handle *h, const char *pname);
 int do_commandarp(struct nft_handle *h, int argc, char *argv[], char **table, bool restore);
 /* For xtables-eb.c */
 int nft_init_eb(struct nft_handle *h, const char *pname);
+void nft_fini_eb(struct nft_handle *h);
 int ebt_get_current_chain(const char *chain);
 int do_commandeb(struct nft_handle *h, int argc, char *argv[], char **table, bool restore);
 
diff --git a/iptables/xtables-eb-standalone.c b/iptables/xtables-eb-standalone.c
index ff74ddbb37334..181cf2d0cbe11 100644
--- a/iptables/xtables-eb-standalone.c
+++ b/iptables/xtables-eb-standalone.c
@@ -53,7 +53,7 @@ int xtables_eb_main(int argc, char *argv[])
 	if (ret)
 		ret = nft_bridge_commit(&h);
 
-	nft_fini(&h);
+	nft_fini_eb(&h);
 
 	if (!ret)
 		fprintf(stderr, "ebtables: %s\n", nft_strerror(errno));
diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c
index 07ed651370913..0df1345ae5cd3 100644
--- a/iptables/xtables-eb.c
+++ b/iptables/xtables-eb.c
@@ -752,6 +752,23 @@ int nft_init_eb(struct nft_handle *h, const char *pname)
 	return 0;
 }
 
+void nft_fini_eb(struct nft_handle *h)
+{
+	struct xtables_match *match;
+	struct xtables_target *target;
+
+	for (match = xtables_matches; match; match = match->next) {
+		free(match->m);
+	}
+	for (target = xtables_targets; target; target = target->next) {
+		free(target->t);
+	}
+
+	free(opts);
+
+	nft_fini(h);
+}
+
 int do_commandeb(struct nft_handle *h, int argc, char *argv[], char **table,
 		 bool restore)
 {
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 44eaf8ab6c483..f1ffcbe246217 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -446,7 +446,7 @@ int xtables_eb_restore_main(int argc, char *argv[])
 	nft_init_eb(&h, "ebtables-restore");
 	h.noflush = noflush;
 	xtables_restore_parse(&h, &p);
-	nft_fini(&h);
+	nft_fini_eb(&h);
 
 	return 0;
 }
-- 
2.25.1

[iptables PATCH 11/15] libxtables: Introduce xtables_fini()

[Phil Sutter]

Record handles of loaded shared objects in a linked list and dlclose()
them from the newly introduced function. While functionally not
necessary, this clears up valgrind's memcheck output when also
displaying reachable memory.

Since this is an extra function that doesn't change the existing API,
increment both current and age.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 configure.ac                      |  4 +--
 include/xtables.h                 |  1 +
 iptables/ip6tables-standalone.c   |  2 ++
 iptables/iptables-restore.c       | 14 +++++++---
 iptables/iptables-save.c          | 14 ++++++++--
 iptables/iptables-standalone.c    |  2 ++
 iptables/xtables-arp-standalone.c |  1 +
 iptables/xtables-eb.c             |  1 +
 iptables/xtables-monitor.c        |  2 ++
 iptables/xtables-restore.c        |  2 ++
 iptables/xtables-save.c           |  1 +
 iptables/xtables-standalone.c     |  1 +
 iptables/xtables-translate.c      |  2 ++
 libxtables/xtables.c              | 44 ++++++++++++++++++++++++++++++-
 14 files changed, 82 insertions(+), 9 deletions(-)

diff --git a/configure.ac b/configure.ac
index 27e90703c9ada..02f6481ca52ed 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2,8 +2,8 @@
 AC_INIT([iptables], [1.8.4])
 
 # See libtool.info "Libtool's versioning system"
-libxtables_vcurrent=14
-libxtables_vage=2
+libxtables_vcurrent=15
+libxtables_vage=3
 
 AC_CONFIG_AUX_DIR([build-aux])
 AC_CONFIG_HEADERS([config.h])
diff --git a/include/xtables.h b/include/xtables.h
index 4aa084a1a2a30..5044dd08e86d3 100644
--- a/include/xtables.h
+++ b/include/xtables.h
@@ -448,6 +448,7 @@ extern struct xtables_match *xtables_matches;
 extern struct xtables_target *xtables_targets;
 
 extern void xtables_init(void);
+extern void xtables_fini(void);
 extern void xtables_set_nfproto(uint8_t);
 extern void *xtables_calloc(size_t, size_t);
 extern void *xtables_malloc(size_t);
diff --git a/iptables/ip6tables-standalone.c b/iptables/ip6tables-standalone.c
index 35d2d9a51f575..105b83ba54010 100644
--- a/iptables/ip6tables-standalone.c
+++ b/iptables/ip6tables-standalone.c
@@ -64,6 +64,8 @@ ip6tables_main(int argc, char *argv[])
 		ip6tc_free(handle);
 	}
 
+	xtables_fini();
+
 	if (!ret) {
 		if (errno == EINVAL) {
 			fprintf(stderr, "ip6tables: %s. "
diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c
index b0a51d491c508..1edad82fc8842 100644
--- a/iptables/iptables-restore.c
+++ b/iptables/iptables-restore.c
@@ -370,7 +370,7 @@ static const struct iptables_restore_cb ipt_restore_cb = {
 int
 iptables_restore_main(int argc, char *argv[])
 {
-	int c;
+	int c, ret;
 
 	iptables_globals.program_name = "iptables-restore";
 	c = xtables_init_all(&iptables_globals, NFPROTO_IPV4);
@@ -385,7 +385,10 @@ iptables_restore_main(int argc, char *argv[])
 	init_extensions4();
 #endif
 
-	return ip46tables_restore_main(&ipt_restore_cb, argc, argv);
+	ret = ip46tables_restore_main(&ipt_restore_cb, argc, argv);
+
+	xtables_fini();
+	return ret;
 }
 #endif
 
@@ -401,7 +404,7 @@ static const struct iptables_restore_cb ip6t_restore_cb = {
 int
 ip6tables_restore_main(int argc, char *argv[])
 {
-	int c;
+	int c, ret;
 
 	ip6tables_globals.program_name = "ip6tables-restore";
 	c = xtables_init_all(&ip6tables_globals, NFPROTO_IPV6);
@@ -416,6 +419,9 @@ ip6tables_restore_main(int argc, char *argv[])
 	init_extensions6();
 #endif
 
-	return ip46tables_restore_main(&ip6t_restore_cb, argc, argv);
+	ret = ip46tables_restore_main(&ip6t_restore_cb, argc, argv);
+
+	xtables_fini();
+	return ret;
 }
 #endif
diff --git a/iptables/iptables-save.c b/iptables/iptables-save.c
index c7251e35ad763..4efd66673f5de 100644
--- a/iptables/iptables-save.c
+++ b/iptables/iptables-save.c
@@ -218,6 +218,8 @@ struct iptables_save_cb ipt_save_cb = {
 int
 iptables_save_main(int argc, char *argv[])
 {
+	int ret;
+
 	iptables_globals.program_name = "iptables-save";
 	if (xtables_init_all(&iptables_globals, NFPROTO_IPV4) < 0) {
 		fprintf(stderr, "%s/%s Failed to initialize xtables\n",
@@ -230,7 +232,10 @@ iptables_save_main(int argc, char *argv[])
 	init_extensions4();
 #endif
 
-	return do_iptables_save(&ipt_save_cb, argc, argv);
+	ret = do_iptables_save(&ipt_save_cb, argc, argv);
+
+	xtables_fini();
+	return ret;
 }
 #endif /* ENABLE_IPV4 */
 
@@ -259,6 +264,8 @@ struct iptables_save_cb ip6t_save_cb = {
 int
 ip6tables_save_main(int argc, char *argv[])
 {
+	int ret;
+
 	ip6tables_globals.program_name = "ip6tables-save";
 	if (xtables_init_all(&ip6tables_globals, NFPROTO_IPV6) < 0) {
 		fprintf(stderr, "%s/%s Failed to initialize xtables\n",
@@ -271,6 +278,9 @@ ip6tables_save_main(int argc, char *argv[])
 	init_extensions6();
 #endif
 
-	return do_iptables_save(&ip6t_save_cb, argc, argv);
+	ret = do_iptables_save(&ip6t_save_cb, argc, argv);
+
+	xtables_fini();
+	return ret;
 }
 #endif /* ENABLE_IPV6 */
diff --git a/iptables/iptables-standalone.c b/iptables/iptables-standalone.c
index c211fb7399dac..8c67ea4d9a2fb 100644
--- a/iptables/iptables-standalone.c
+++ b/iptables/iptables-standalone.c
@@ -64,6 +64,8 @@ iptables_main(int argc, char *argv[])
 		iptc_free(handle);
 	}
 
+	xtables_fini();
+
 	if (!ret) {
 		if (errno == EINVAL) {
 			fprintf(stderr, "iptables: %s. "
diff --git a/iptables/xtables-arp-standalone.c b/iptables/xtables-arp-standalone.c
index eca7bb979b967..04cf7dccc8206 100644
--- a/iptables/xtables-arp-standalone.c
+++ b/iptables/xtables-arp-standalone.c
@@ -56,6 +56,7 @@ int xtables_arp_main(int argc, char *argv[])
 		ret = nft_commit(&h);
 
 	nft_fini(&h);
+	xtables_fini();
 
 	if (!ret)
 		fprintf(stderr, "arptables: %s\n", nft_strerror(errno));
diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c
index 0df1345ae5cd3..5764d1803cba7 100644
--- a/iptables/xtables-eb.c
+++ b/iptables/xtables-eb.c
@@ -767,6 +767,7 @@ void nft_fini_eb(struct nft_handle *h)
 	free(opts);
 
 	nft_fini(h);
+	xtables_fini();
 }
 
 int do_commandeb(struct nft_handle *h, int argc, char *argv[], char **table,
diff --git a/iptables/xtables-monitor.c b/iptables/xtables-monitor.c
index c2b31dbaa0795..57def83e2eea0 100644
--- a/iptables/xtables-monitor.c
+++ b/iptables/xtables-monitor.c
@@ -688,6 +688,8 @@ int xtables_monitor_main(int argc, char *argv[])
 	}
 	mnl_socket_close(nl);
 
+	xtables_fini();
+
 	return EXIT_SUCCESS;
 }
 
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index f1ffcbe246217..573597ab3a2de 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -389,6 +389,7 @@ xtables_restore_main(int family, const char *progname, int argc, char *argv[])
 	xtables_restore_parse(&h, &p);
 
 	nft_fini(&h);
+	xtables_fini();
 	fclose(p.in);
 	return 0;
 }
@@ -471,6 +472,7 @@ int xtables_arp_restore_main(int argc, char *argv[])
 	nft_init_arp(&h, "arptables-restore");
 	xtables_restore_parse(&h, &p);
 	nft_fini(&h);
+	xtables_fini();
 
 	return 0;
 }
diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index 0ce66e5d15cee..bb3d8cd336c38 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -244,6 +244,7 @@ xtables_save_main(int family, int argc, char *argv[],
 
 	ret = do_output(&h, tablename, &d);
 	nft_fini(&h);
+	xtables_fini();
 	if (dump)
 		exit(0);
 
diff --git a/iptables/xtables-standalone.c b/iptables/xtables-standalone.c
index 022d5dd44abbf..dd6fb7919d2e1 100644
--- a/iptables/xtables-standalone.c
+++ b/iptables/xtables-standalone.c
@@ -72,6 +72,7 @@ xtables_main(int family, const char *progname, int argc, char *argv[])
 		ret = nft_commit(&h);
 
 	nft_fini(&h);
+	xtables_fini();
 
 	if (!ret) {
 		if (errno == EINVAL) {
diff --git a/iptables/xtables-translate.c b/iptables/xtables-translate.c
index 76ad7eb69eca9..5aa42496b5a48 100644
--- a/iptables/xtables-translate.c
+++ b/iptables/xtables-translate.c
@@ -509,6 +509,7 @@ static int xtables_xlate_main(int family, const char *progname, int argc,
 		fprintf(stderr, "Translation not implemented\n");
 
 	nft_fini(&h);
+	xtables_fini();
 	exit(!ret);
 }
 
@@ -563,6 +564,7 @@ static int xtables_restore_xlate_main(int family, const char *progname,
 	printf("# Completed on %s", ctime(&now));
 
 	nft_fini(&h);
+	xtables_fini();
 	fclose(p.in);
 	exit(0);
 }
diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index 777c2b08e9896..7fe42580f9b70 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -206,6 +206,38 @@ struct xtables_target *xtables_targets;
 static bool xtables_fully_register_pending_match(struct xtables_match *me);
 static bool xtables_fully_register_pending_target(struct xtables_target *me);
 
+/* registry for loaded shared objects to close later */
+struct dlreg {
+	struct dlreg *next;
+	void *handle;
+};
+static struct dlreg *dlreg = NULL;
+
+static int dlreg_add(void *handle)
+{
+	struct dlreg *new = malloc(sizeof(*new));
+
+	if (!new)
+		return -1;
+
+	new->handle = handle;
+	new->next = dlreg;
+	dlreg = new;
+	return 0;
+}
+
+static void dlreg_free(void)
+{
+	struct dlreg *next;
+
+	while (dlreg) {
+		next = dlreg->next;
+		dlclose(dlreg->handle);
+		free(dlreg);
+		dlreg = next;
+	}
+}
+
 void xtables_init(void)
 {
 	xtables_libdir = getenv("XTABLES_LIBDIR");
@@ -233,6 +265,11 @@ void xtables_init(void)
 	xtables_libdir = XTABLES_LIBDIR;
 }
 
+void xtables_fini(void)
+{
+	dlreg_free();
+}
+
 void xtables_set_nfproto(uint8_t nfproto)
 {
 	switch (nfproto) {
@@ -567,6 +604,8 @@ static void *load_extension(const char *search_path, const char *af_prefix,
 			next = dir + strlen(dir);
 
 		for (prefix = all_prefixes; *prefix != NULL; ++prefix) {
+			void *handle;
+
 			snprintf(path, sizeof(path), "%.*s/%s%s.so",
 			         (unsigned int)(next - dir), dir,
 			         *prefix, name);
@@ -578,11 +617,14 @@ static void *load_extension(const char *search_path, const char *af_prefix,
 					strerror(errno));
 				return NULL;
 			}
-			if (dlopen(path, RTLD_NOW) == NULL) {
+			handle = dlopen(path, RTLD_NOW);
+			if (handle == NULL) {
 				fprintf(stderr, "%s: %s\n", path, dlerror());
 				break;
 			}
 
+			dlreg_add(handle);
+
 			if (is_target)
 				ptr = xtables_find_target(name, XTF_DONT_LOAD);
 			else
-- 
2.25.1

[iptables PATCH 12/15] nft: Use clear_cs() instead of open coding

[Phil Sutter]

In a few places, initialized struct iptables_command_state was not fully
deinitialized. Change them to call nft_clear_iptables_command_state()
which does it properly.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-ipv4.c    | 2 +-
 iptables/nft-ipv6.c    | 2 +-
 iptables/xtables-arp.c | 4 +---
 iptables/xtables.c     | 6 +-----
 4 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/iptables/nft-ipv4.c b/iptables/nft-ipv4.c
index 70634f8fad84d..69691fe28cf80 100644
--- a/iptables/nft-ipv4.c
+++ b/iptables/nft-ipv4.c
@@ -288,7 +288,7 @@ static void nft_ipv4_print_rule(struct nft_handle *h, struct nftnl_rule *r,
 	if (!(format & FMT_NONEWLINE))
 		fputc('\n', stdout);
 
-	xtables_rule_matches_free(&cs.matches);
+	nft_clear_iptables_command_state(&cs);
 }
 
 static void save_ipv4_addr(char letter, const struct in_addr *addr,
diff --git a/iptables/nft-ipv6.c b/iptables/nft-ipv6.c
index d01491bfdb689..76f2613d95c6a 100644
--- a/iptables/nft-ipv6.c
+++ b/iptables/nft-ipv6.c
@@ -217,7 +217,7 @@ static void nft_ipv6_print_rule(struct nft_handle *h, struct nftnl_rule *r,
 	if (!(format & FMT_NONEWLINE))
 		fputc('\n', stdout);
 
-	xtables_rule_matches_free(&cs.matches);
+	nft_clear_iptables_command_state(&cs);
 }
 
 static void save_ipv6_addr(char letter, const struct in6_addr *addr,
diff --git a/iptables/xtables-arp.c b/iptables/xtables-arp.c
index a0136059bb710..e64938fbf5d36 100644
--- a/iptables/xtables-arp.c
+++ b/iptables/xtables-arp.c
@@ -1019,9 +1019,7 @@ int do_commandarp(struct nft_handle *h, int argc, char *argv[], char **table,
 	free(daddrs);
 	free(dmasks);
 
-	if (cs.target)
-		free(cs.target->t);
-
+	nft_clear_iptables_command_state(&cs);
 	xtables_free_opts(1);
 
 /*	if (verbose > 1)
diff --git a/iptables/xtables.c b/iptables/xtables.c
index c180af13975f8..63a37ae867069 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -1138,11 +1138,7 @@ int do_commandx(struct nft_handle *h, int argc, char *argv[], char **table,
 
 	*table = p.table;
 
-	xtables_rule_matches_free(&cs.matches);
-	if (cs.target) {
-		free(cs.target->t);
-		cs.target->t = NULL;
-	}
+	nft_clear_iptables_command_state(&cs);
 
 	if (h->family == AF_INET) {
 		free(args.s.addr.v4);
-- 
2.25.1

[iptables PATCH 13/15] arptables: Fix leak in nft_arp_print_rule()

[Phil Sutter]

The function missed to clear struct iptables_command_state again after
use.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-arp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
index e9a2d9de21560..9a831efd07a28 100644
--- a/iptables/nft-arp.c
+++ b/iptables/nft-arp.c
@@ -604,6 +604,8 @@ nft_arp_print_rule(struct nft_handle *h, struct nftnl_rule *r,
 
 	if (!(format & FMT_NONEWLINE))
 		fputc('\n', stdout);
+
+	nft_clear_iptables_command_state(&cs);
 }
 
 static bool nft_arp_is_same(const void *data_a,
-- 
2.25.1

[iptables PATCH 14/15] nft: Fix leak when replacing a rule

[Phil Sutter]

If nft_rule_append() is called with a reference rule, it is supposed to
insert the new rule at the reference position and then remove the
reference from cache. Instead, it removed the new rule from cache again
right after inserting it. Also, it missed to free the removed rule.

Fixes: 5ca9acf51adf9 ("xtables: Fix position of replaced rules in cache")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 01268f7859e9b..3c0daa8d42529 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1429,7 +1429,8 @@ nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
 
 	if (ref) {
 		nftnl_chain_rule_insert_at(r, ref);
-		nftnl_chain_rule_del(r);
+		nftnl_chain_rule_del(ref);
+		nftnl_rule_free(ref);
 	} else {
 		c = nft_chain_find(h, table, chain);
 		if (!c) {
-- 
2.25.1

[iptables PATCH 15/15] nft: Don't exit early after printing help texts

[Phil Sutter]

Follow regular code path after handling --help option to gracefully
deinit and free stuff.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xtables-arp.c | 10 +++++-----
 iptables/xtables-eb.c  |  2 +-
 iptables/xtables.c     |  7 ++++---
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/iptables/xtables-arp.c b/iptables/xtables-arp.c
index e64938fbf5d36..8632774dfb705 100644
--- a/iptables/xtables-arp.c
+++ b/iptables/xtables-arp.c
@@ -235,7 +235,7 @@ exit_tryhelp(int status)
 }
 
 static void
-exit_printhelp(void)
+printhelp(void)
 {
 	struct xtables_target *t = NULL;
 	int i;
@@ -325,7 +325,6 @@ exit_printhelp(void)
 		printf("\n");
 		t->help();
 	}
-	exit(0);
 }
 
 static char
@@ -666,7 +665,8 @@ int do_commandarp(struct nft_handle *h, int argc, char *argv[], char **table,
 			if (!optarg)
 				optarg = argv[optind];
 
-			exit_printhelp();
+			printhelp();
+			command = CMD_NONE;
 			break;
 		case 's':
 			check_inverse(optarg, &invert, &optind, argc);
@@ -881,8 +881,6 @@ int do_commandarp(struct nft_handle *h, int argc, char *argv[], char **table,
 	if (optind < argc)
 		xtables_error(PARAMETER_PROBLEM,
 			      "unknown arguments found on commandline");
-	if (!command)
-		xtables_error(PARAMETER_PROBLEM, "no command specified");
 	if (invert)
 		xtables_error(PARAMETER_PROBLEM,
 			      "nothing appropriate following !");
@@ -1009,6 +1007,8 @@ int do_commandarp(struct nft_handle *h, int argc, char *argv[], char **table,
 			xtables_error(PARAMETER_PROBLEM, "Wrong policy `%s'\n",
 				      policy);
 		break;
+	case CMD_NONE:
+		break;
 	default:
 		/* We should never reach this... */
 		exit_tryhelp(2);
diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c
index 5764d1803cba7..375a95d1d5c75 100644
--- a/iptables/xtables-eb.c
+++ b/iptables/xtables-eb.c
@@ -1218,7 +1218,7 @@ print_zero:
 
 	if (command == 'h' && !(flags & OPT_ZERO)) {
 		print_help(cs.target, cs.matches, *table);
-		exit(0);
+		ret = 1;
 	}
 
 	/* Do the final checks */
diff --git a/iptables/xtables.c b/iptables/xtables.c
index 63a37ae867069..9d2e441e0b773 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -161,7 +161,7 @@ exit_tryhelp(int status)
 }
 
 static void
-exit_printhelp(const struct xtables_rule_match *matches)
+printhelp(const struct xtables_rule_match *matches)
 {
 	printf("%s v%s\n\n"
 "Usage: %s -[ACD] chain rule-specification [options]\n"
@@ -240,7 +240,6 @@ exit_printhelp(const struct xtables_rule_match *matches)
 "[!] --version	-V		print package version.\n");
 
 	print_extension_helps(xtables_targets, matches);
-	exit(0);
 }
 
 void
@@ -724,7 +723,9 @@ void do_parse(struct nft_handle *h, int argc, char *argv[],
 				xtables_find_match(cs->protocol,
 					XTF_TRY_LOAD, &cs->matches);
 
-			exit_printhelp(cs->matches);
+			printhelp(cs->matches);
+			p->command = CMD_NONE;
+			return;
 
 			/*
 			 * Option selection
-- 
2.25.1