[iptables PATCH] xshared: fix memory leak in should_load_proto

[iptables PATCH] xshared: fix memory leak in should_load_proto

[Christian Marangi]

With the help of a Coverity Scan, it was pointed out that it's present a
memory leak in the corner case where find_proto is not NULL in the
function should_load_proto. find_proto return a struct xtables_match
pointer from xtables_find_match that is allocated but never freed.

Correctly free the found proto in the corner case where find_proto
succeed.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 iptables/xshared.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/iptables/xshared.c b/iptables/xshared.c
index 17aed04e..0beacfdc 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -113,11 +113,16 @@ find_proto(const char *pname, enum xtables_tryload tryload,
  */
 static bool should_load_proto(struct iptables_command_state *cs)
 {
+	struct xtables_match *proto;
+
 	if (cs->protocol == NULL)
 		return false;
-	if (find_proto(cs->protocol, XTF_DONT_LOAD,
-	    cs->options & OPT_NUMERIC, NULL) == NULL)
+	proto = find_proto(cs->protocol, XTF_DONT_LOAD,
+			   cs->options & OPT_NUMERIC, NULL);
+	if (proto == NULL)
 		return true;
+
+	free(proto);
 	return !cs->proto_used;
 }
 
-- 
2.39.2

Re: [iptables PATCH] xshared: fix memory leak in should_load_proto

[Phil Sutter]

Hi Christian,

On Mon, May 29, 2023 at 07:18:46PM +0200, Christian Marangi wrote:
> With the help of a Coverity Scan, it was pointed out that it's present a
> memory leak in the corner case where find_proto is not NULL in the
> function should_load_proto. find_proto return a struct xtables_match
> pointer from xtables_find_match that is allocated but never freed.
> 
> Correctly free the found proto in the corner case where find_proto
> succeed.

We have a '--valgrind' mode in shell testsuite and this has not been
identified yet. So assuming your finding is correct, we lack a proper
test case. Can you provide a reproducer (ideally as shell test-case)?

Also I wonder if we can provide a Fixes: tag. The code in question is
very old, so maybe hard to find.

Thanks, Phil

Re: [iptables PATCH] xshared: fix memory leak in should_load_proto

[Jan Engelhardt]

On Monday 2023-05-29 19:18, Christian Marangi wrote:

>With the help of a Coverity Scan, it was pointed out that it's present a
>memory leak in the corner case where find_proto is not NULL in the
>function should_load_proto. find_proto return a struct xtables_match
>pointer from xtables_find_match that is allocated but never freed.
>
>Correctly free the found proto in the corner case where find_proto
>succeed.
>
>@@ -113,11 +113,16 @@ find_proto(const char *pname, enum xtables_tryload tryload,
>  */
> static bool should_load_proto(struct iptables_command_state *cs)
> {
>+	struct xtables_match *proto;
>+
> 	if (cs->protocol == NULL)
> 		return false;
>-	if (find_proto(cs->protocol, XTF_DONT_LOAD,
>-	    cs->options & OPT_NUMERIC, NULL) == NULL)
>+	proto = find_proto(cs->protocol, XTF_DONT_LOAD,
>+			   cs->options & OPT_NUMERIC, NULL);
>+	if (proto == NULL)
> 		return true;
>+
>+	free(proto);
> 	return !cs->proto_used;
> }

After 13 years, the code I once wrote feels weird. In essence, find_proto is
called twice, but that should not be necessary because cs->proto_used already
tracks whether this was done.
[e.g. use `iptables -A INPUT -p tcp --dport 25 --unrecognized` to trigger]

Could someone cross check my alternative proposal I have below?


>From de6b99bbb29c148831ad072d1764012ee79a7883 Mon Sep 17 00:00:00 2001
From: Jan Engelhardt <jengelh@inai.de>
Date: Tue, 30 May 2023 14:59:30 +0200
Subject: [PATCH] iptables: dissolve should_load_proto

cs->proto_used already tells whether -p foo was turned into an
implicit -m foo once, so I do not think should_load_proto() has a
reason to exist.

Signed-off-by: Jan Engelhardt <jengelh@inai.de>
---
 iptables/xshared.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/iptables/xshared.c b/iptables/xshared.c
index ac51fac5..55fe8961 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -111,22 +111,15 @@ find_proto(const char *pname, enum xtables_tryload tryload,
  *   [think of ip6tables-restore!]
  * - the protocol extension can be successively loaded
  */
-static bool should_load_proto(struct iptables_command_state *cs)
-{
-	if (cs->protocol == NULL)
-		return false;
-	if (find_proto(cs->protocol, XTF_DONT_LOAD,
-	    cs->options & OPT_NUMERIC, NULL) == NULL)
-		return true;
-	return !cs->proto_used;
-}
-
 static struct xtables_match *load_proto(struct iptables_command_state *cs)
 {
-	if (!should_load_proto(cs))
+	if (cs->protocol == NULL)
 		return NULL;
+	if (cs->proto_used)
+		return NULL;
+	cs->proto_used = true;
 	return find_proto(cs->protocol, XTF_TRY_LOAD,
-			  cs->options & OPT_NUMERIC, &cs->matches);
+	       cs->options & OPT_NUMERIC, &cs->matches);
 }
 
 static int command_default(struct iptables_command_state *cs,
@@ -157,13 +150,10 @@ static int command_default(struct iptables_command_state *cs,
 		return 0;
 	}
 
-	/* Try loading protocol */
 	m = load_proto(cs);
 	if (m != NULL) {
 		size_t size;
 
-		cs->proto_used = 1;
-
 		size = XT_ALIGN(sizeof(struct xt_entry_match)) + m->size;
 
 		m->m = xtables_calloc(1, size);
-- 
2.40.1

Re: [iptables PATCH] xshared: fix memory leak in should_load_proto

[Phil Sutter]

Hi Jan,

On Tue, May 30, 2023 at 06:11:09PM +0200, Jan Engelhardt wrote:
[...]
> After 13 years, the code I once wrote feels weird. In essence, find_proto is
> called twice, but that should not be necessary because cs->proto_used already
> tracks whether this was done.
> [e.g. use `iptables -A INPUT -p tcp --dport 25 --unrecognized` to trigger]
> 
> Could someone cross check my alternative proposal I have below?

Thanks for the fix! Patch applied with minor subject and whitespace
fixes.