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@xxxxxxx>
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@xxxxxxx>
---
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
