All of lore.kernel.org
 help / color / mirror / Atom feed
* [iptables PATCH 0/7] A number of ASAN-identified fixes
@ 2024-02-01 13:50 Phil Sutter
  2024-02-01 13:50 ` [iptables PATCH 1/7] tests: iptables-test: Increase non-fast mode strictness Phil Sutter
                   ` (7 more replies)
  0 siblings, 8 replies; 9+ messages in thread
From: Phil Sutter @ 2024-02-01 13:50 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Pablo Neira Ayuso

Many thanks to Pablo for pointing me at ASAN reporting issues with the
code valgrind had missed so far. This series fixes all reports so make
check passes also when compiled with -fsanitize=address.

Patch 1 is not quite related but rather a shortcoming in
iptables-test.py I decided to fix while being at it.

The remaining patches are actual fixes apart from patch 4 which pulls
out unrelated (but sensible) changes from patch 5.

Phil Sutter (7):
  tests: iptables-test: Increase non-fast mode strictness
  nft: ruleparse: Add missing braces around ternary
  libxtables: Fix memleak of matches' udata
  xtables-eb: Eliminate 'opts' define
  xshared: Fix for memleak in option merging with ebtables
  xshared: Introduce xtables_clear_args()
  ebtables: Fix for memleak with change counters command

 extensions/libxt_NFLOG.t     |  2 +-
 extensions/libxt_TCPMSS.t    |  2 +-
 iptables-test.py             |  6 +++++-
 iptables/ip6tables.c         |  5 +----
 iptables/iptables.c          |  5 +----
 iptables/nft-cmd.c           |  1 +
 iptables/nft-ruleparse.c     |  2 +-
 iptables/xshared.c           | 20 +++++++++++++++++---
 iptables/xshared.h           |  2 ++
 iptables/xtables-eb.c        | 29 +++++++++++++++++------------
 iptables/xtables-translate.c | 12 +-----------
 iptables/xtables.c           |  5 +----
 libxtables/xtables.c         | 10 ++++++----
 13 files changed, 55 insertions(+), 46 deletions(-)

-- 
2.43.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [iptables PATCH 1/7] tests: iptables-test: Increase non-fast mode strictness
  2024-02-01 13:50 [iptables PATCH 0/7] A number of ASAN-identified fixes Phil Sutter
@ 2024-02-01 13:50 ` Phil Sutter
  2024-02-01 13:50 ` [iptables PATCH 2/7] nft: ruleparse: Add missing braces around ternary Phil Sutter
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Phil Sutter @ 2024-02-01 13:50 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Pablo Neira Ayuso

The simple search for the rule in save output accepted arbitrary leading
and trailing rule parts. This was partly desired as it allowed to omit
the leading '-A' flag or ignore the mandatory '-j CONTINUE' in ebtables
rules, though it could hide bugs.

Introduction of fast mode mitigated this due to the way how it searches
for multiple rules at the same time, but there are cases which fast mode
does not support yet (e.g. test cases containing variant-specific rule
output).

Given save output format will never contain the rule in first or last
line, so enclosing the searched rule in newline characters is sufficient
to make the search apply to full lines only. The only drawback is having
to add '-A' and '-j CONTINUE' parts if needed.

The hidden bugs this revealed were:
- Long --nflog-prefix strings are not cut to 64 chars with iptables-nft
- The TCPMSS rule supposed to fail with legacy only must specify an
  expected save output

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 extensions/libxt_NFLOG.t  | 2 +-
 extensions/libxt_TCPMSS.t | 2 +-
 iptables-test.py          | 6 +++++-
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/extensions/libxt_NFLOG.t b/extensions/libxt_NFLOG.t
index 25f332ae16b6b..0cd81c643b2d5 100644
--- a/extensions/libxt_NFLOG.t
+++ b/extensions/libxt_NFLOG.t
@@ -15,7 +15,7 @@
 -j NFLOG --nflog-size 4294967296;;FAIL
 -j NFLOG --nflog-size -1;;FAIL
 -j NFLOG --nflog-prefix xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx;=;OK
--j NFLOG --nflog-prefix xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx;-j NFLOG --nflog-prefix xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx;OK
+-j NFLOG --nflog-prefix xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx;-j NFLOG --nflog-prefix xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx;OK;LEGACY;=
 -j NFLOG --nflog-threshold 1;=;OK
 # ERROR: line 13 (should fail: iptables -A INPUT -j NFLOG --nflog-threshold 0
 # -j NFLOG --nflog-threshold 0;;FAIL
diff --git a/extensions/libxt_TCPMSS.t b/extensions/libxt_TCPMSS.t
index fbfbfcf88d81a..b3639cc17a935 100644
--- a/extensions/libxt_TCPMSS.t
+++ b/extensions/libxt_TCPMSS.t
@@ -1,6 +1,6 @@
 :FORWARD,OUTPUT,POSTROUTING
 *mangle
 -j TCPMSS;;FAIL
--p tcp -j TCPMSS --set-mss 42;;FAIL;LEGACY
+-p tcp -j TCPMSS --set-mss 42;=;FAIL;LEGACY
 -p tcp -m tcp --tcp-flags FIN,SYN,RST,ACK SYN -j TCPMSS --set-mss 42;=;OK
 -p tcp -m tcp --tcp-flags FIN,SYN,RST,ACK SYN -j TCPMSS --clamp-mss-to-pmtu;=;OK
diff --git a/iptables-test.py b/iptables-test.py
index 179e366e02961..cefe42335d25d 100755
--- a/iptables-test.py
+++ b/iptables-test.py
@@ -143,7 +143,8 @@ STDERR_IS_TTY = sys.stderr.isatty()
         return -1
 
     # find the rule
-    matching = out.find(rule_save.encode('utf-8'))
+    matching = out.find("\n-A {}\n".format(rule_save).encode('utf-8'))
+
     if matching < 0:
         if res == "OK":
             reason = "cannot find: " + iptables + " -I " + rule
@@ -470,6 +471,9 @@ STDERR_IS_TTY = sys.stderr.isatty()
             else:
                 rule_save = chain + " " + item[1]
 
+            if iptables == EBTABLES and rule_save.find('-j') < 0:
+                rule_save += " -j CONTINUE"
+
             res = item[2].rstrip()
             if len(item) > 3:
                 variant = item[3].rstrip()
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [iptables PATCH 2/7] nft: ruleparse: Add missing braces around ternary
  2024-02-01 13:50 [iptables PATCH 0/7] A number of ASAN-identified fixes Phil Sutter
  2024-02-01 13:50 ` [iptables PATCH 1/7] tests: iptables-test: Increase non-fast mode strictness Phil Sutter
@ 2024-02-01 13:50 ` Phil Sutter
  2024-02-01 13:50 ` [iptables PATCH 3/7] libxtables: Fix memleak of matches' udata Phil Sutter
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Phil Sutter @ 2024-02-01 13:50 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Pablo Neira Ayuso

The expression evaluated the sum before the ternay, consequently not
adding target->size if tgsize was zero.

Identified by ASAN for a simple rule using standard target:
| # ebtables -A INPUT -s de:ad:be:ef:0:00 -j RETURN
| # ebtables -D INPUT -s de:ad:be:ef:0:00 -j RETURN
| =================================================================
| ==18925==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000000120 at pc 0x7f627a4c75c5 bp 0x7ffe882b5180 sp 0x7ffe882b4928
| READ of size 8 at 0x603000000120 thread T0
| [...]

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

diff --git a/iptables/nft-ruleparse.c b/iptables/nft-ruleparse.c
index 0bbdf44fafe03..3b1cbe4fa1499 100644
--- a/iptables/nft-ruleparse.c
+++ b/iptables/nft-ruleparse.c
@@ -94,7 +94,7 @@ __nft_create_target(struct nft_xt_ctx *ctx, const char *name, size_t tgsize)
 	if (!target)
 		return NULL;
 
-	size = XT_ALIGN(sizeof(*target->t)) + tgsize ?: target->size;
+	size = XT_ALIGN(sizeof(*target->t)) + (tgsize ?: target->size);
 
 	target->t = xtables_calloc(1, size);
 	target->t->u.target_size = size;
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [iptables PATCH 3/7] libxtables: Fix memleak of matches' udata
  2024-02-01 13:50 [iptables PATCH 0/7] A number of ASAN-identified fixes Phil Sutter
  2024-02-01 13:50 ` [iptables PATCH 1/7] tests: iptables-test: Increase non-fast mode strictness Phil Sutter
  2024-02-01 13:50 ` [iptables PATCH 2/7] nft: ruleparse: Add missing braces around ternary Phil Sutter
@ 2024-02-01 13:50 ` Phil Sutter
  2024-02-01 13:50 ` [iptables PATCH 4/7] xtables-eb: Eliminate 'opts' define Phil Sutter
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Phil Sutter @ 2024-02-01 13:50 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Pablo Neira Ayuso

If the extension specifies a non-zero udata_size, field 'udata' points
to an allocated buffer which needs to be freed upon extension deinit.

Interestingly, this bug was identified by ASAN and missed by valgrind.

Fixes: 2dba676b68ef8 ("extensions: support for per-extension instance "global" variable space")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 libxtables/xtables.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index b4339e8d31275..856bfae804ea9 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -1420,6 +1420,10 @@ void xtables_rule_matches_free(struct xtables_rule_match **matches)
 			free(matchp->match->m);
 			matchp->match->m = NULL;
 		}
+		if (matchp->match->udata_size) {
+			free(matchp->match->udata);
+			matchp->match->udata = NULL;
+		}
 		if (matchp->match == matchp->match->next) {
 			free(matchp->match);
 			matchp->match = NULL;
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [iptables PATCH 4/7] xtables-eb: Eliminate 'opts' define
  2024-02-01 13:50 [iptables PATCH 0/7] A number of ASAN-identified fixes Phil Sutter
                   ` (2 preceding siblings ...)
  2024-02-01 13:50 ` [iptables PATCH 3/7] libxtables: Fix memleak of matches' udata Phil Sutter
@ 2024-02-01 13:50 ` Phil Sutter
  2024-02-01 13:50 ` [iptables PATCH 5/7] xshared: Fix for memleak in option merging with ebtables Phil Sutter
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Phil Sutter @ 2024-02-01 13:50 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Pablo Neira Ayuso

It is more harm than good as it hides assignments to xt_params->opts
field and does funny things if statements actually use xt_params->opts
instead of the define.

Replace it by local variables where sensible (cf. command_match() and
command_jump() in xshared.c).

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

diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c
index 250169c35d521..d197b37e81e9e 100644
--- a/iptables/xtables-eb.c
+++ b/iptables/xtables-eb.c
@@ -141,7 +141,6 @@ struct xtables_globals ebtables_globals = {
 	.compat_rev		= nft_compatible_revision,
 };
 
-#define opts ebtables_globals.opts
 #define prog_name ebtables_globals.program_name
 #define prog_vers ebtables_globals.program_version
 
@@ -281,6 +280,7 @@ static int list_rules(struct nft_handle *h, const char *chain, const char *table
 /* This code is very similar to iptables/xtables.c:command_match() */
 static void ebt_load_match(const char *name)
 {
+	struct option *opts = xt_params->opts;
 	struct xtables_match *m;
 	size_t size;
 
@@ -305,10 +305,12 @@ static void ebt_load_match(const char *name)
 					     m->extra_opts, &m->option_offset);
 	if (opts == NULL)
 		xtables_error(OTHER_PROBLEM, "Can't alloc memory");
+	xt_params->opts = opts;
 }
 
 static void ebt_load_watcher(const char *name)
 {
+	struct option *opts = xt_params->opts;
 	struct xtables_target *watcher;
 	size_t size;
 
@@ -337,11 +339,12 @@ static void ebt_load_watcher(const char *name)
 					     &watcher->option_offset);
 	if (opts == NULL)
 		xtables_error(OTHER_PROBLEM, "Can't alloc memory");
+	xt_params->opts = opts;
 }
 
 static void ebt_load_match_extensions(void)
 {
-	opts = ebt_original_options;
+	xt_params->opts = ebt_original_options;
 	ebt_load_match("802_3");
 	ebt_load_match("arp");
 	ebt_load_match("ip");
@@ -358,7 +361,7 @@ static void ebt_load_match_extensions(void)
 
 	/* assign them back so do_parse() may
 	 * reset opts to orig_opts upon each call */
-	xt_params->orig_opts = opts;
+	xt_params->orig_opts = xt_params->opts;
 }
 
 void ebt_add_match(struct xtables_match *m,
@@ -528,6 +531,7 @@ int nft_init_eb(struct nft_handle *h, const char *pname)
 
 void nft_fini_eb(struct nft_handle *h)
 {
+	struct option *opts = xt_params->opts;
 	struct xtables_match *match;
 	struct xtables_target *target;
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [iptables PATCH 5/7] xshared: Fix for memleak in option merging with ebtables
  2024-02-01 13:50 [iptables PATCH 0/7] A number of ASAN-identified fixes Phil Sutter
                   ` (3 preceding siblings ...)
  2024-02-01 13:50 ` [iptables PATCH 4/7] xtables-eb: Eliminate 'opts' define Phil Sutter
@ 2024-02-01 13:50 ` Phil Sutter
  2024-02-01 13:50 ` [iptables PATCH 6/7] xshared: Introduce xtables_clear_args() Phil Sutter
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Phil Sutter @ 2024-02-01 13:50 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Pablo Neira Ayuso

The crucial difference in ebtables is that all extensions are loaded up
front instead of while parsing -m/-j flags. Since this loading of all
extensions before every call to do_parse() is pointless overhead (cf.
ebtables-restore), other tools' mechanism of freeing all merged options
in xtables_free_opts() after handling each command and resetting
xt_params->opts at the start of the parser loop is problematic.

Fixed commit entailed a hack to defeat the xt_params->opts happening at
start of do_parse() by assigning to xt_params->orig_opts after loading
all extensions. This approach caused a memleak though since
xtables_free_opts() called from xtables_merge_options() will free the
opts pointer only if it differs from orig_opts.

Resolve this via a different approach which eliminates the
xt_params->opts reset at the start of do_parse():

Make xt_params->opts be NULL until the first extension is loaded. Option
merging in command_match() and command_jump() tolerates a NULL pointer
there after minimal adjustment. Deinit in xtables_free_opts() is already
fine as it (re)turns xt_params->opts to a NULL pointer. With do_parse()
expecting that and falling back to xt_params->orig_opts, no explicit
initialization is required anymore and thus ebtables' init is not
mangled by accident.

A critical part is that do_parse() checks xt_params->opts pointer upon
each call to getopt_long() as it may get assigned while parsing.

Fixes: 58d364c7120b5 ("ebtables: Use do_parse() from xshared")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xshared.c    | 12 +++++++++---
 iptables/xtables-eb.c | 25 +++++++++++++------------
 libxtables/xtables.c  |  6 ++----
 3 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/iptables/xshared.c b/iptables/xshared.c
index 43fa929df7676..7d073891ed5c3 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -798,6 +798,9 @@ static void command_match(struct iptables_command_state *cs, bool invert)
 	else if (m->extra_opts != NULL)
 		opts = xtables_merge_options(xt_params->orig_opts, opts,
 					     m->extra_opts, &m->option_offset);
+	else
+		return;
+
 	if (opts == NULL)
 		xtables_error(OTHER_PROBLEM, "can't alloc memory!");
 	xt_params->opts = opts;
@@ -856,10 +859,13 @@ void command_jump(struct iptables_command_state *cs, const char *jumpto)
 		opts = xtables_options_xfrm(xt_params->orig_opts, opts,
 					    cs->target->x6_options,
 					    &cs->target->option_offset);
-	else
+	else if (cs->target->extra_opts != NULL)
 		opts = xtables_merge_options(xt_params->orig_opts, opts,
 					     cs->target->extra_opts,
 					     &cs->target->option_offset);
+	else
+		return;
+
 	if (opts == NULL)
 		xtables_error(OTHER_PROBLEM, "can't alloc memory!");
 	xt_params->opts = opts;
@@ -1484,10 +1490,10 @@ void do_parse(int argc, char *argv[],
 	   demand-load a protocol. */
 	opterr = 0;
 
-	xt_params->opts = xt_params->orig_opts;
 	while ((cs->c = getopt_long(argc, argv,
 				    optstring_lookup(afinfo->family),
-				    xt_params->opts, NULL)) != -1) {
+				    xt_params->opts ?: xt_params->orig_opts,
+				    NULL)) != -1) {
 		switch (cs->c) {
 			/*
 			 * Command selection
diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c
index d197b37e81e9e..51c699defb047 100644
--- a/iptables/xtables-eb.c
+++ b/iptables/xtables-eb.c
@@ -298,11 +298,14 @@ static void ebt_load_match(const char *name)
 	xs_init_match(m);
 
 	if (m->x6_options != NULL)
-		opts = xtables_options_xfrm(opts, NULL,
+		opts = xtables_options_xfrm(xt_params->orig_opts, opts,
 					    m->x6_options, &m->option_offset);
 	else if (m->extra_opts != NULL)
-		opts = xtables_merge_options(opts, NULL,
+		opts = xtables_merge_options(xt_params->orig_opts, opts,
 					     m->extra_opts, &m->option_offset);
+	else
+		return;
+
 	if (opts == NULL)
 		xtables_error(OTHER_PROBLEM, "Can't alloc memory");
 	xt_params->opts = opts;
@@ -332,11 +335,16 @@ static void ebt_load_watcher(const char *name)
 	xs_init_target(watcher);
 
 	if (watcher->x6_options != NULL)
-		opts = xtables_options_xfrm(opts, NULL, watcher->x6_options,
+		opts = xtables_options_xfrm(xt_params->orig_opts, opts,
+					    watcher->x6_options,
 					    &watcher->option_offset);
 	else if (watcher->extra_opts != NULL)
-		opts = xtables_merge_options(opts, NULL, watcher->extra_opts,
+		opts = xtables_merge_options(xt_params->orig_opts, opts,
+					     watcher->extra_opts,
 					     &watcher->option_offset);
+	else
+		return;
+
 	if (opts == NULL)
 		xtables_error(OTHER_PROBLEM, "Can't alloc memory");
 	xt_params->opts = opts;
@@ -344,7 +352,6 @@ static void ebt_load_watcher(const char *name)
 
 static void ebt_load_match_extensions(void)
 {
-	xt_params->opts = ebt_original_options;
 	ebt_load_match("802_3");
 	ebt_load_match("arp");
 	ebt_load_match("ip");
@@ -358,10 +365,6 @@ static void ebt_load_match_extensions(void)
 
 	ebt_load_watcher("log");
 	ebt_load_watcher("nflog");
-
-	/* assign them back so do_parse() may
-	 * reset opts to orig_opts upon each call */
-	xt_params->orig_opts = xt_params->opts;
 }
 
 void ebt_add_match(struct xtables_match *m,
@@ -531,7 +534,6 @@ int nft_init_eb(struct nft_handle *h, const char *pname)
 
 void nft_fini_eb(struct nft_handle *h)
 {
-	struct option *opts = xt_params->opts;
 	struct xtables_match *match;
 	struct xtables_target *target;
 
@@ -542,8 +544,7 @@ void nft_fini_eb(struct nft_handle *h)
 		free(target->t);
 	}
 
-	if (opts != ebt_original_options)
-		free(opts);
+	free(xt_params->opts);
 
 	nft_fini(h);
 	xtables_fini();
diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index 856bfae804ea9..ae3ff25a3a876 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -111,10 +111,8 @@ void basic_exit_err(enum xtables_exittype status, const char *msg, ...)
 
 void xtables_free_opts(int unused)
 {
-	if (xt_params->opts != xt_params->orig_opts) {
-		free(xt_params->opts);
-		xt_params->opts = NULL;
-	}
+	free(xt_params->opts);
+	xt_params->opts = NULL;
 }
 
 struct option *xtables_merge_options(struct option *orig_opts,
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [iptables PATCH 6/7] xshared: Introduce xtables_clear_args()
  2024-02-01 13:50 [iptables PATCH 0/7] A number of ASAN-identified fixes Phil Sutter
                   ` (4 preceding siblings ...)
  2024-02-01 13:50 ` [iptables PATCH 5/7] xshared: Fix for memleak in option merging with ebtables Phil Sutter
@ 2024-02-01 13:50 ` Phil Sutter
  2024-02-01 13:50 ` [iptables PATCH 7/7] ebtables: Fix for memleak with change counters command Phil Sutter
  2024-02-06 23:15 ` [iptables PATCH 0/7] A number of ASAN-identified fixes Phil Sutter
  7 siblings, 0 replies; 9+ messages in thread
From: Phil Sutter @ 2024-02-01 13:50 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Pablo Neira Ayuso

Perform struct xtables_args object deinit in a common place, even though
it merely consists of freeing any IP addresses and masks.

This fixes for a memleak in arptables-translate as the check for
h->family didn't catch the value NFPROTO_ARP.

Fixes: 5b7324e0675e3 ("nft-arp: add arptables-translate")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/ip6tables.c         |  5 +----
 iptables/iptables.c          |  5 +----
 iptables/xshared.c           |  8 ++++++++
 iptables/xshared.h           |  2 ++
 iptables/xtables-translate.c | 12 +-----------
 iptables/xtables.c           |  5 +----
 6 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index 4b5d4ac6878b7..f9ae18aed8041 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -892,10 +892,7 @@ int do_command6(int argc, char *argv[], char **table,
 		e = NULL;
 	}
 
-	free(saddrs);
-	free(smasks);
-	free(daddrs);
-	free(dmasks);
+	xtables_clear_args(&args);
 	xtables_free_opts(1);
 
 	return ret;
diff --git a/iptables/iptables.c b/iptables/iptables.c
index 5ae28fe04a5f5..8eb043e9b736e 100644
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -887,10 +887,7 @@ int do_command4(int argc, char *argv[], char **table,
 		e = NULL;
 	}
 
-	free(saddrs);
-	free(smasks);
-	free(daddrs);
-	free(dmasks);
+	xtables_clear_args(&args);
 	xtables_free_opts(1);
 
 	return ret;
diff --git a/iptables/xshared.c b/iptables/xshared.c
index 7d073891ed5c3..0b2724a3e5162 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -2185,3 +2185,11 @@ make_delete_mask(const struct xtables_rule_match *matches,
 
 	return mask;
 }
+
+void xtables_clear_args(struct xtables_args *args)
+{
+	free(args->s.addr.ptr);
+	free(args->s.mask.ptr);
+	free(args->d.addr.ptr);
+	free(args->d.mask.ptr);
+}
diff --git a/iptables/xshared.h b/iptables/xshared.h
index 2a9cdf45f581a..7d4035ec03e52 100644
--- a/iptables/xshared.h
+++ b/iptables/xshared.h
@@ -333,4 +333,6 @@ unsigned char *make_delete_mask(const struct xtables_rule_match *matches,
 
 void iface_to_mask(const char *ifname, unsigned char *mask);
 
+void xtables_clear_args(struct xtables_args *args);
+
 #endif /* IPTABLES_XSHARED_H */
diff --git a/iptables/xtables-translate.c b/iptables/xtables-translate.c
index ad44311230323..8ebe523c447f2 100644
--- a/iptables/xtables-translate.c
+++ b/iptables/xtables-translate.c
@@ -349,17 +349,7 @@ static int do_command_xlate(struct nft_handle *h, int argc, char *argv[],
 
 	h->ops->clear_cs(&cs);
 
-	if (h->family == AF_INET) {
-		free(args.s.addr.v4);
-		free(args.s.mask.v4);
-		free(args.d.addr.v4);
-		free(args.d.mask.v4);
-	} else if (h->family == AF_INET6) {
-		free(args.s.addr.v6);
-		free(args.s.mask.v6);
-		free(args.d.addr.v6);
-		free(args.d.mask.v6);
-	}
+	xtables_clear_args(&args);
 	xtables_free_opts(1);
 
 	return ret;
diff --git a/iptables/xtables.c b/iptables/xtables.c
index 22d6ea58376fc..5d73481c25761 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -264,10 +264,7 @@ int do_commandx(struct nft_handle *h, int argc, char *argv[], char **table,
 
 	h->ops->clear_cs(&cs);
 
-	free(args.s.addr.ptr);
-	free(args.s.mask.ptr);
-	free(args.d.addr.ptr);
-	free(args.d.mask.ptr);
+	xtables_clear_args(&args);
 	xtables_free_opts(1);
 
 	return ret;
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [iptables PATCH 7/7] ebtables: Fix for memleak with change counters command
  2024-02-01 13:50 [iptables PATCH 0/7] A number of ASAN-identified fixes Phil Sutter
                   ` (5 preceding siblings ...)
  2024-02-01 13:50 ` [iptables PATCH 6/7] xshared: Introduce xtables_clear_args() Phil Sutter
@ 2024-02-01 13:50 ` Phil Sutter
  2024-02-06 23:15 ` [iptables PATCH 0/7] A number of ASAN-identified fixes Phil Sutter
  7 siblings, 0 replies; 9+ messages in thread
From: Phil Sutter @ 2024-02-01 13:50 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Pablo Neira Ayuso

Just like with check command, change counters command creates a
temporary rule from rulespec on command line for a search by spec in
rule cache. It is not used anymore afterwards, so nft_cmd_free() should
free it.

Fixes: f340b7b6816be ("ebtables: Implement --change-counters command")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-cmd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/iptables/nft-cmd.c b/iptables/nft-cmd.c
index 8372d171b00c4..b38da9bdc1c0b 100644
--- a/iptables/nft-cmd.c
+++ b/iptables/nft-cmd.c
@@ -65,6 +65,7 @@ void nft_cmd_free(struct nft_cmd *cmd)
 	switch (cmd->command) {
 	case NFT_COMPAT_RULE_CHECK:
 	case NFT_COMPAT_RULE_DELETE:
+	case NFT_COMPAT_RULE_CHANGE_COUNTERS:
 		if (cmd->obj.rule)
 			nftnl_rule_free(cmd->obj.rule);
 		break;
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [iptables PATCH 0/7] A number of ASAN-identified fixes
  2024-02-01 13:50 [iptables PATCH 0/7] A number of ASAN-identified fixes Phil Sutter
                   ` (6 preceding siblings ...)
  2024-02-01 13:50 ` [iptables PATCH 7/7] ebtables: Fix for memleak with change counters command Phil Sutter
@ 2024-02-06 23:15 ` Phil Sutter
  7 siblings, 0 replies; 9+ messages in thread
From: Phil Sutter @ 2024-02-06 23:15 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Pablo Neira Ayuso

On Thu, Feb 01, 2024 at 02:50:50PM +0100, Phil Sutter wrote:
> Many thanks to Pablo for pointing me at ASAN reporting issues with the
> code valgrind had missed so far. This series fixes all reports so make
> check passes also when compiled with -fsanitize=address.
> 
> Patch 1 is not quite related but rather a shortcoming in
> iptables-test.py I decided to fix while being at it.
> 
> The remaining patches are actual fixes apart from patch 4 which pulls
> out unrelated (but sensible) changes from patch 5.
> 
> Phil Sutter (7):
>   tests: iptables-test: Increase non-fast mode strictness
>   nft: ruleparse: Add missing braces around ternary
>   libxtables: Fix memleak of matches' udata
>   xtables-eb: Eliminate 'opts' define
>   xshared: Fix for memleak in option merging with ebtables
>   xshared: Introduce xtables_clear_args()
>   ebtables: Fix for memleak with change counters command

Series applied.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2024-02-06 23:15 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-01 13:50 [iptables PATCH 0/7] A number of ASAN-identified fixes Phil Sutter
2024-02-01 13:50 ` [iptables PATCH 1/7] tests: iptables-test: Increase non-fast mode strictness Phil Sutter
2024-02-01 13:50 ` [iptables PATCH 2/7] nft: ruleparse: Add missing braces around ternary Phil Sutter
2024-02-01 13:50 ` [iptables PATCH 3/7] libxtables: Fix memleak of matches' udata Phil Sutter
2024-02-01 13:50 ` [iptables PATCH 4/7] xtables-eb: Eliminate 'opts' define Phil Sutter
2024-02-01 13:50 ` [iptables PATCH 5/7] xshared: Fix for memleak in option merging with ebtables Phil Sutter
2024-02-01 13:50 ` [iptables PATCH 6/7] xshared: Introduce xtables_clear_args() Phil Sutter
2024-02-01 13:50 ` [iptables PATCH 7/7] ebtables: Fix for memleak with change counters command Phil Sutter
2024-02-06 23:15 ` [iptables PATCH 0/7] A number of ASAN-identified fixes Phil Sutter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.