[conntrack-tools PATCH 0/8] Fixes for a recent Coverity tool run

[conntrack-tools PATCH 0/8] Fixes for a recent Coverity tool run

[Phil Sutter]

Phil Sutter (8):
  hash: Flush tables when destroying
  cache: Fix features array allocation
  Fix potential buffer overrun in snprintf() calls
  helpers: ftp: Avoid ugly casts
  read_config_yy: Drop extra argument from dlog() call
  Don't call exit() from signal handler
  Drop pointless assignments
  connntrack: Fix for memleak when parsing -j arg

 src/cache.c          |  4 ++--
 src/conntrack.c      |  2 ++
 src/hash.c           |  1 +
 src/helpers/ftp.c    | 20 +++++++++-----------
 src/helpers/ssdp.c   |  1 -
 src/main.c           |  2 +-
 src/process.c        |  2 +-
 src/queue.c          |  4 ++--
 src/read_config_yy.y |  2 +-
 src/run.c            |  2 +-
 10 files changed, 20 insertions(+), 20 deletions(-)

-- 
2.34.1

[conntrack-tools PATCH 1/8] hash: Flush tables when destroying

[Phil Sutter]

This is cosmetics only, but stops valgrind from complaining about
definitely lost memory.

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

diff --git a/src/hash.c b/src/hash.c
index fe6a047fcebe0..a0f240c21fa82 100644
--- a/src/hash.c
+++ b/src/hash.c
@@ -55,6 +55,7 @@ hashtable_create(int hashsize, int limit,
 
 void hashtable_destroy(struct hashtable *h)
 {
+	hashtable_flush(h);
 	free(h);
 }
 
-- 
2.34.1

[conntrack-tools PATCH 2/8] cache: Fix features array allocation

[Phil Sutter]

struct cache::features is of type struct cache_feature **, allocate and
populate accordingly.

Fixes: ad31f852c3454 ("initial import of the conntrack daemon to Netfilter SVN")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/cache.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/cache.c b/src/cache.c
index 79a024f8b6bb0..9bc8d0f5bf34a 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -69,12 +69,12 @@ struct cache *cache_create(const char *name, enum cache_type type,
 
 	memcpy(c->feature_type, feature_type, sizeof(feature_type));
 
-	c->features = malloc(sizeof(struct cache_feature) * j);
+	c->features = malloc(sizeof(struct cache_feature *) * j);
 	if (!c->features) {
 		free(c);
 		return NULL;
 	}
-	memcpy(c->features, feature_array, sizeof(struct cache_feature) * j);
+	memcpy(c->features, feature_array, sizeof(struct cache_feature *) * j);
 	c->num_features = j;
 
 	c->extra_offset = size;
-- 
2.34.1

[conntrack-tools PATCH 3/8] Fix potential buffer overrun in snprintf() calls

[Phil Sutter]

When consecutively printing into the same buffer at increasing offset,
reduce buffer size passed to snprintf() to not defeat its size checking.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/process.c | 2 +-
 src/queue.c   | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/process.c b/src/process.c
index 3ddad5ffa7959..08598eeae84de 100644
--- a/src/process.c
+++ b/src/process.c
@@ -84,7 +84,7 @@ void fork_process_dump(int fd)
 	int size = 0;
 
 	list_for_each_entry(this, &process_list, head) {
-		size += snprintf(buf+size, sizeof(buf),
+		size += snprintf(buf + size, sizeof(buf) - size,
 				 "PID=%u type=%s\n",
 				 this->pid,
 				 this->type < CTD_PROC_MAX ?
diff --git a/src/queue.c b/src/queue.c
index 76425b18495b5..e94dc7c45d1fd 100644
--- a/src/queue.c
+++ b/src/queue.c
@@ -69,12 +69,12 @@ void queue_stats_show(int fd)
 	int size = 0;
 	char buf[512];
 
-	size += snprintf(buf+size, sizeof(buf),
+	size += snprintf(buf + size, sizeof(buf) - size,
 			 "allocated queue nodes:\t\t%12u\n\n",
 			 qobjects_num);
 
 	list_for_each_entry(this, &queue_list, list) {
-		size += snprintf(buf+size, sizeof(buf),
+		size += snprintf(buf + size, sizeof(buf) - size,
 				 "queue %s:\n"
 				 "current elements:\t\t%12u\n"
 				 "maximum elements:\t\t%12u\n"
-- 
2.34.1

[conntrack-tools PATCH 4/8] helpers: ftp: Avoid ugly casts

[Phil Sutter]

Coverity tool complains about accessing a local variable at non-zero
offset. Avoid this by using a helper union. This should silence the
checker, although the code is still probably not Big Endian-safe.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/helpers/ftp.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/src/helpers/ftp.c b/src/helpers/ftp.c
index 29ac55c236507..2b345340a70b9 100644
--- a/src/helpers/ftp.c
+++ b/src/helpers/ftp.c
@@ -332,23 +332,21 @@ static int nf_nat_ftp_fmt_cmd(enum nf_ct_ftp_type type,
 			      char *buffer, size_t buflen,
 			      uint32_t addr, uint16_t port)
 {
+	union {
+		unsigned char c[4];
+		uint32_t d;
+	} tmp;
+
+	tmp.d = addr;
 	switch (type) {
 	case NF_CT_FTP_PORT:
 	case NF_CT_FTP_PASV:
 		return snprintf(buffer, buflen, "%u,%u,%u,%u,%u,%u",
-				((unsigned char *)&addr)[0],
-				((unsigned char *)&addr)[1],
-				((unsigned char *)&addr)[2],
-				((unsigned char *)&addr)[3],
-				port >> 8,
-				port & 0xFF);
+				tmp.c[0], tmp.c[1], tmp.c[2], tmp.c[3],
+				port >> 8, port & 0xFF);
 	case NF_CT_FTP_EPRT:
 		return snprintf(buffer, buflen, "|1|%u.%u.%u.%u|%u|",
-				((unsigned char *)&addr)[0],
-				((unsigned char *)&addr)[1],
-				((unsigned char *)&addr)[2],
-				((unsigned char *)&addr)[3],
-				port);
+				tmp.c[0], tmp.c[1], tmp.c[2], tmp.c[3], port);
 	case NF_CT_FTP_EPSV:
 		return snprintf(buffer, buflen, "|||%u|", port);
 	}
-- 
2.34.1

[conntrack-tools PATCH 5/8] read_config_yy: Drop extra argument from dlog() call

[Phil Sutter]

False priority value was never printed.

Fixes: dfb88dae65fbd ("conntrackd: change scheduler and priority via configuration file")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/read_config_yy.y | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/read_config_yy.y b/src/read_config_yy.y
index 070b349c59498..5815d6ab464e8 100644
--- a/src/read_config_yy.y
+++ b/src/read_config_yy.y
@@ -1052,7 +1052,7 @@ scheduler_line : T_PRIO T_NUMBER
 {
 	conf.sched.prio = $2;
 	if (conf.sched.prio < 0 || conf.sched.prio > 99) {
-		dlog(LOG_ERR, "`Priority' must be [0, 99]\n", $2);
+		dlog(LOG_ERR, "`Priority' must be [0, 99]\n");
 		exit(EXIT_FAILURE);
 	}
 };
-- 
2.34.1

[conntrack-tools PATCH 6/8] Don't call exit() from signal handler

[Phil Sutter]

Coverity tool complains that exit() is not signal-safe and therefore
should not be called from within a signal handler. Call _exit() instead.

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

diff --git a/src/run.c b/src/run.c
index f11a5327fe5e6..37a0eb1c6b957 100644
--- a/src/run.c
+++ b/src/run.c
@@ -67,7 +67,7 @@ void killer(int signo)
 	close_log();
 
 	sd_ct_stop();
-	exit(0);
+	_exit(0);
 }
 
 static void child(int foo)
-- 
2.34.1

[conntrack-tools PATCH 7/8] Drop pointless assignments

[Phil Sutter]

These variables are not referred to after assigning within their scope
(or until they're overwritten).

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/helpers/ssdp.c | 1 -
 src/main.c         | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/helpers/ssdp.c b/src/helpers/ssdp.c
index 0c6f563c592aa..527892cdabf8a 100644
--- a/src/helpers/ssdp.c
+++ b/src/helpers/ssdp.c
@@ -256,7 +256,6 @@ static int find_hdr(const char *name, const uint8_t *data, int data_len,
 		data += i+2;
 	}
 
-	data_len -= name_len;
 	data += name_len;
 	if (pos)
 		*pos = data;
diff --git a/src/main.c b/src/main.c
index 31e0eed950b48..de4773df8a204 100644
--- a/src/main.c
+++ b/src/main.c
@@ -319,7 +319,7 @@ int main(int argc, char *argv[])
 
 	umask(0177);
 
-	if ((ret = init_config(config_file)) == -1) {
+	if (init_config(config_file) == -1) {
 		dlog(LOG_ERR, "can't open config file `%s'", config_file);
 		exit(EXIT_FAILURE);
 	}
-- 
2.34.1

[conntrack-tools PATCH 8/8] connntrack: Fix for memleak when parsing -j arg

[Phil Sutter]

Have to free the strings allocated by split_address_and_port().

Fixes: 29b390a212214 ("conntrack: Support IPv6 NAT")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/conntrack.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/conntrack.c b/src/conntrack.c
index 679a1d27e250a..894bf3f6bf440 100644
--- a/src/conntrack.c
+++ b/src/conntrack.c
@@ -3113,6 +3113,8 @@ static void do_parse(struct ct_cmd *ct_cmd, int argc, char *argv[])
 					nfct_set_nat_details(c, tmpl->ct, &ad,
 							     port_str, family);
 				}
+				free(port_str);
+				free(nat_address);
 			}
 			break;
 		case 'w':
-- 
2.34.1

Re: [conntrack-tools PATCH 0/8] Fixes for a recent Coverity tool run

[Pablo Neira Ayuso]

LGTM

On Fri, Mar 25, 2022 at 11:49:55AM +0100, Phil Sutter wrote:
> Phil Sutter (8):
>   hash: Flush tables when destroying
>   cache: Fix features array allocation
>   Fix potential buffer overrun in snprintf() calls
>   helpers: ftp: Avoid ugly casts
>   read_config_yy: Drop extra argument from dlog() call
>   Don't call exit() from signal handler
>   Drop pointless assignments
>   connntrack: Fix for memleak when parsing -j arg
> 
>  src/cache.c          |  4 ++--
>  src/conntrack.c      |  2 ++
>  src/hash.c           |  1 +
>  src/helpers/ftp.c    | 20 +++++++++-----------
>  src/helpers/ssdp.c   |  1 -
>  src/main.c           |  2 +-
>  src/process.c        |  2 +-
>  src/queue.c          |  4 ++--
>  src/read_config_yy.y |  2 +-
>  src/run.c            |  2 +-
>  10 files changed, 20 insertions(+), 20 deletions(-)
> 
> -- 
> 2.34.1
>