[B.A.T.M.A.N.] [PATCH 00/10] batctl: Fixes and minor cleanup

[B.A.T.M.A.N.] [PATCH 00/10] batctl: Fixes and minor cleanup

[Sven Eckelmann]

From: Sven Eckelmann <sven@narfation.org>

Hi,

today I had the unpleasent experience to receive [1] to a lot of PDF files
which seem to contain some reports of the Klocwork static code analysis tool.
Most of the stuff was nonsense about memory/resource leaks (even when it
stored in some kind of structure one line before). I gathered the only
interesting portions and prepared some patches for them.

The last two patches are actually not really bug fixes but are about code
which completely killed the static analyzer. I didn't like the code in
the last patch (looked to complex for something relatively easy) and thus
I've just replaced it.

WARNING: these things were only compile tested

Kind regards,
	Sven


[1] not unpleasent due to some issue reports. But going through several
    hundred false positives wasn't my goal for today. Someone at seems to
    get paid by false-positives which this Klocworks analyzer produces
    (and there are really some braindead ones - even clang's static analyzer
    looks like a genius against some of these reports) 

Sven Eckelmann (10):
  batctl: Print dummy value when localtime failed
  batctl: Handle failure during hash_iterator allocation
  batctl: Handle allocation error for path_buff
  batctl: Handle nlmsg_alloc errors
  batctl: Handle nl_socket_alloc errors
  batctl: Handle nl_cb_alloc errors
  batctl: Free nl_sock on genl_ctrl_resolve error
  batctl: Free nl_sock when if_nametoindex failed
  batctl: tcpdump: Fix types for for TT v1
  batctl: Simplify concatenation of pathnames

 bat-hosts.c |  4 +---
 functions.c |  8 ++------
 hash.c      |  3 +++
 netlink.c   | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++--------
 sys.c       | 15 +++++++++++++++
 tcpdump.c   |  9 +++++++--
 6 files changed, 83 insertions(+), 19 deletions(-)

-- 
2.11.0

[B.A.T.M.A.N.] [PATCH 01/10] batctl: Print dummy value when localtime failed

[Sven Eckelmann]

localtime can return NULL when the local time could not be calculated.
Accessing this NULL pointer is not allowed.

Fixes: 05f27bfcd302 ("add arp packets , change output")
Signed-off-by: Sven Eckelmann <sven.eckelmann@openmesh.com>
---
 tcpdump.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tcpdump.c b/tcpdump.c
index 4ede76b..5eb99cf 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -104,7 +104,11 @@ static int print_time(void)
 	gettimeofday(&tv, NULL);
 	tm = localtime(&tv.tv_sec);
 
-	printf("%02d:%02d:%02d.%06ld ", tm->tm_hour, tm->tm_min, tm->tm_sec, tv.tv_usec);
+	if (tm)
+		printf("%02d:%02d:%02d.%06ld ", tm->tm_hour, tm->tm_min, tm->tm_sec, tv.tv_usec);
+	else
+		printf("00:00:00.000000 ");
+
 	return 1;
 }
 
-- 
2.11.0

[B.A.T.M.A.N.] [PATCH 02/10] batctl: Handle failure during hash_iterator allocation

[Sven Eckelmann]

The iterator functions should not try to start the iteration when the
iterator could not be allocated.

Signed-off-by: Sven Eckelmann <sven.eckelmann@openmesh.com>
---
 hash.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hash.c b/hash.c
index f4a7af7..0e4d1e3 100644
--- a/hash.c
+++ b/hash.c
@@ -120,6 +120,9 @@ struct hash_it_t *hash_iterate(struct hashtable_t *hash,
 
 	if (iter_in == NULL) {
 		iter = debugMalloc(sizeof(struct hash_it_t), 301);
+		if (!iter)
+			return NULL;
+
 		iter->index =  -1;
 		iter->bucket = NULL;
 		iter->prev_bucket = NULL;
-- 
2.11.0

[B.A.T.M.A.N.] [PATCH 03/10] batctl: Handle allocation error for path_buff

[Sven Eckelmann]

Fixes: 5a1af99276b0 ("batctl: adapt batctl to new sysfs interface handling")
Fixes: 306fcb4480c9 ("batctl: support for multiple mesh clouds")
Fixes: af115c9acf44 ("batctl: support new gateway sysfs API")
Fixes: 2c2cb260ad2e ("batctl: list supported and configured routing algorithms")
Signed-off-by: Sven Eckelmann <sven.eckelmann@openmesh.com>
---
 sys.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/sys.c b/sys.c
index 65b438c..c49b428 100644
--- a/sys.c
+++ b/sys.c
@@ -153,6 +153,11 @@ int handle_loglevel(char *mesh_iface, int argc, char **argv)
 	}
 
 	path_buff = malloc(PATH_BUFF_LEN);
+	if (!path_buff) {
+		fprintf(stderr, "Error - could not allocate path buffer: out of memory ?\n");
+		return EXIT_FAILURE;
+	}
+
 	snprintf(path_buff, PATH_BUFF_LEN, SYS_BATIF_PATH_FMT, mesh_iface);
 
 	if (argc != 1) {
@@ -255,6 +260,11 @@ int handle_sys_setting(char *mesh_iface, int setting, int argc, char **argv)
 
 	/* prepare the classic path */
 	path_buff = malloc(PATH_BUFF_LEN);
+	if (!path_buff) {
+		fprintf(stderr, "Error - could not allocate path buffer: out of memory ?\n");
+		return EXIT_FAILURE;
+	}
+
 	snprintf(path_buff, PATH_BUFF_LEN, SYS_BATIF_PATH_FMT, mesh_iface);
 
 	/* if the specified interface is a VLAN then change the path to point
@@ -329,6 +339,11 @@ int handle_gw_setting(char *mesh_iface, int argc, char **argv)
 	}
 
 	path_buff = malloc(PATH_BUFF_LEN);
+	if (!path_buff) {
+		fprintf(stderr, "Error - could not allocate path buffer: out of memory ?\n");
+		return EXIT_FAILURE;
+	}
+
 	snprintf(path_buff, PATH_BUFF_LEN, SYS_BATIF_PATH_FMT, mesh_iface);
 
 	if (argc == 1) {
-- 
2.11.0

[B.A.T.M.A.N.] [PATCH 04/10] batctl: Handle nlmsg_alloc errors

[Sven Eckelmann]

nlmsg_alloc may return NULL on errors. The processing has to be aborted
when this happens.

Fixes: d8dd1ff1a0fe ("batctl: Use netlink to replace some of debugfs")
Signed-off-by: Sven Eckelmann <sven.eckelmann@openmesh.com>
---
 netlink.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/netlink.c b/netlink.c
index 7fb1ee1..e3d7892 100644
--- a/netlink.c
+++ b/netlink.c
@@ -302,6 +302,11 @@ static char *netlink_get_info(int ifindex, uint8_t nl_cmd, const char *header)
 		return NULL;
 
 	msg = nlmsg_alloc();
+	if (!msg) {
+		nl_socket_free(sock);
+		return NULL;
+	}
+
 	genlmsg_put(msg, NL_AUTO_PID, NL_AUTO_SEQ, family, 0, 0,
 		    BATADV_CMD_GET_MESH_INFO, 1);
 
@@ -399,6 +404,11 @@ int netlink_print_routing_algos(void)
 		return -EOPNOTSUPP;
 
 	msg = nlmsg_alloc();
+	if (!msg) {
+		last_err = -ENOMEM;
+		goto err_free_sock;
+	}
+
 	genlmsg_put(msg, NL_AUTO_PID, NL_AUTO_SEQ, family, 0, NLM_F_DUMP,
 		    BATADV_CMD_GET_ROUTING_ALGOS, 1);
 
@@ -415,6 +425,8 @@ int netlink_print_routing_algos(void)
 	nl_cb_err(cb, NL_CB_CUSTOM, print_error, NULL);
 
 	nl_recvmsgs(sock, cb);
+
+err_free_sock:
 	nl_socket_free(sock);
 
 	if (!last_err)
@@ -1131,6 +1143,9 @@ static int netlink_print_common(char *mesh_iface, char *orig_iface,
 								 header);
 
 		msg = nlmsg_alloc();
+		if (!msg)
+			continue;
+
 		genlmsg_put(msg, NL_AUTO_PID, NL_AUTO_SEQ, family, 0,
 			    NLM_F_DUMP, nl_cmd, 1);
 
-- 
2.11.0

[B.A.T.M.A.N.] [PATCH 05/10] batctl: Handle nl_socket_alloc errors

[Sven Eckelmann]

nl_socket_alloc may return NULL on errors. The processing has to be aborted
when this happens.

Fixes: d8dd1ff1a0fe ("batctl: Use netlink to replace some of debugfs")
Signed-off-by: Sven Eckelmann <sven.eckelmann@openmesh.com>
---
 netlink.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/netlink.c b/netlink.c
index e3d7892..d7aeb9a 100644
--- a/netlink.c
+++ b/netlink.c
@@ -295,6 +295,9 @@ static char *netlink_get_info(int ifindex, uint8_t nl_cmd, const char *header)
 	};
 
 	sock = nl_socket_alloc();
+	if (!sock)
+		return NULL;
+
 	genl_connect(sock);
 
 	family = genl_ctrl_resolve(sock, BATADV_NL_NAME);
@@ -397,6 +400,9 @@ int netlink_print_routing_algos(void)
 	};
 
 	sock = nl_socket_alloc();
+	if (!sock)
+		return -ENOMEM;
+
 	genl_connect(sock);
 
 	family = genl_ctrl_resolve(sock, BATADV_NL_NAME);
@@ -1104,6 +1110,9 @@ static int netlink_print_common(char *mesh_iface, char *orig_iface,
 	int family;
 
 	sock = nl_socket_alloc();
+	if (!sock)
+		return -ENOMEM;
+
 	genl_connect(sock);
 
 	family = genl_ctrl_resolve(sock, BATADV_NL_NAME);
-- 
2.11.0

[B.A.T.M.A.N.] [PATCH 06/10] batctl: Handle nl_cb_alloc errors

[Sven Eckelmann]

nl_cb_alloc may return NULL on errors. The processing has to be aborted
when this happens.

Fixes: d8dd1ff1a0fe ("batctl: Use netlink to replace some of debugfs")
Signed-off-by: Sven Eckelmann <sven.eckelmann@openmesh.com>
---
 netlink.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/netlink.c b/netlink.c
index d7aeb9a..b95063a 100644
--- a/netlink.c
+++ b/netlink.c
@@ -320,11 +320,15 @@ static char *netlink_get_info(int ifindex, uint8_t nl_cmd, const char *header)
 	nlmsg_free(msg);
 
 	cb = nl_cb_alloc(NL_CB_DEFAULT);
+	if (!cb)
+		goto err_free_sock;
+
 	nl_cb_set(cb, NL_CB_VALID, NL_CB_CUSTOM, info_callback, &opts);
 	nl_cb_err(cb, NL_CB_CUSTOM, print_error, NULL);
 
 	nl_recvmsgs(sock, cb);
 
+err_free_sock:
 	nl_socket_free(sock);
 
 	return opts.remaining_header;
@@ -425,6 +429,11 @@ int netlink_print_routing_algos(void)
 	opts.remaining_header = strdup("Available routing algorithms:\n");
 
 	cb = nl_cb_alloc(NL_CB_DEFAULT);
+	if (!cb) {
+		last_err = -ENOMEM;
+		goto err_free_sock;
+	}
+
 	nl_cb_set(cb, NL_CB_VALID, NL_CB_CUSTOM, netlink_print_common_cb,
 		  &opts);
 	nl_cb_set(cb, NL_CB_FINISH, NL_CB_CUSTOM, stop_callback, NULL);
@@ -1134,9 +1143,14 @@ static int netlink_print_common(char *mesh_iface, char *orig_iface,
 		}
 	}
 
+	cb = nl_cb_alloc(NL_CB_DEFAULT);
+	if (!cb) {
+		last_err = -ENOMEM;
+		goto err_free_sock;
+	}
+
 	bat_hosts_init(read_opt);
 
-	cb = nl_cb_alloc(NL_CB_DEFAULT);
 	nl_cb_set(cb, NL_CB_VALID, NL_CB_CUSTOM, netlink_print_common_cb, &opts);
 	nl_cb_set(cb, NL_CB_FINISH, NL_CB_CUSTOM, stop_callback, NULL);
 	nl_cb_err(cb, NL_CB_CUSTOM, print_error, NULL);
@@ -1181,6 +1195,7 @@ static int netlink_print_common(char *mesh_iface, char *orig_iface,
 
 	bat_hosts_free();
 
+err_free_sock:
 	nl_socket_free(sock);
 
 	return last_err;
-- 
2.11.0

[B.A.T.M.A.N.] [PATCH 07/10] batctl: Free nl_sock on genl_ctrl_resolve error

[Sven Eckelmann]

genl_ctrl_resolve may return NULL on errors. The code must then free the
socket which was used to start the genl_ctrl_resolve and stop the function
with an error code.

Fixes: d8dd1ff1a0fe ("batctl: Use netlink to replace some of debugfs")
Signed-off-by: Sven Eckelmann <sven.eckelmann@openmesh.com>
---
 netlink.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/netlink.c b/netlink.c
index b95063a..de81d08 100644
--- a/netlink.c
+++ b/netlink.c
@@ -301,8 +301,10 @@ static char *netlink_get_info(int ifindex, uint8_t nl_cmd, const char *header)
 	genl_connect(sock);
 
 	family = genl_ctrl_resolve(sock, BATADV_NL_NAME);
-	if (family < 0)
+	if (family < 0) {
+		nl_socket_free(sock);
 		return NULL;
+	}
 
 	msg = nlmsg_alloc();
 	if (!msg) {
@@ -410,8 +412,10 @@ int netlink_print_routing_algos(void)
 	genl_connect(sock);
 
 	family = genl_ctrl_resolve(sock, BATADV_NL_NAME);
-	if (family < 0)
-		return -EOPNOTSUPP;
+	if (family < 0) {
+		last_err = -EOPNOTSUPP;
+		goto err_free_sock;
+	}
 
 	msg = nlmsg_alloc();
 	if (!msg) {
@@ -1125,8 +1129,10 @@ static int netlink_print_common(char *mesh_iface, char *orig_iface,
 	genl_connect(sock);
 
 	family = genl_ctrl_resolve(sock, BATADV_NL_NAME);
-	if (family < 0)
-		return -EOPNOTSUPP;
+	if (family < 0) {
+		last_err = -EOPNOTSUPP;
+		goto err_free_sock;
+	}
 
 	ifindex = if_nametoindex(mesh_iface);
 	if (!ifindex) {
-- 
2.11.0

[B.A.T.M.A.N.] [PATCH 08/10] batctl: Free nl_sock when if_nametoindex failed

[Sven Eckelmann]

The if_nametoindex can return an error. The code must then free the
previously allocated nl_sock and stop the function with an error code.

Fixes: d8dd1ff1a0fe ("batctl: Use netlink to replace some of debugfs")
Signed-off-by: Sven Eckelmann <sven.eckelmann@openmesh.com>
---
 netlink.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/netlink.c b/netlink.c
index de81d08..7b97809 100644
--- a/netlink.c
+++ b/netlink.c
@@ -1137,7 +1137,8 @@ static int netlink_print_common(char *mesh_iface, char *orig_iface,
 	ifindex = if_nametoindex(mesh_iface);
 	if (!ifindex) {
 		fprintf(stderr, "Interface %s is unknown\n", mesh_iface);
-		return -ENODEV;
+		last_err = -ENODEV;
+		goto err_free_sock;
 	}
 
 	if (orig_iface) {
@@ -1145,7 +1146,8 @@ static int netlink_print_common(char *mesh_iface, char *orig_iface,
 		if (!hardifindex) {
 			fprintf(stderr, "Interface %s is unknown\n",
 				orig_iface);
-			return -ENODEV;
+			last_err = -ENODEV;
+			goto err_free_sock;
 		}
 	}
 
-- 
2.11.0

[B.A.T.M.A.N.] [PATCH 09/10] batctl: tcpdump: Fix types for for TT v1

[Sven Eckelmann]

The num_entry and num_vlan variables are accessed (printed) as u16
variables and not like integers. They should therefore also be stored like
that.

Signed-off-by: Sven Eckelmann <sven.eckelmann@openmesh.com>
---
 tcpdump.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tcpdump.c b/tcpdump.c
index 5eb99cf..a1b057b 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -158,7 +158,8 @@ static void batctl_tvlv_parse_tt_v1(void *buff, ssize_t buff_len)
 {
 	struct batadv_tvlv_tt_data *tvlv = buff;
 	struct batadv_tvlv_tt_vlan_data *vlan;
-	int i, num_vlan, num_entry;
+	int i;
+	unsigned short num_vlan, num_entry;
 	const char *type;
 	size_t vlan_len;
 
-- 
2.11.0

[B.A.T.M.A.N.] [PATCH 10/10] batctl: Simplify concatenation of pathnames

[Sven Eckelmann]

The combination of strncpy and strncat is hard to read and it is rather
easy to introduce some kind of problems when using that. The usage of
snprintf simplifies it slightly.

Signed-off-by: Sven Eckelmann <sven.eckelmann@openmesh.com>
---
 bat-hosts.c | 4 +---
 functions.c | 8 ++------
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/bat-hosts.c b/bat-hosts.c
index b530243..54b0a18 100644
--- a/bat-hosts.c
+++ b/bat-hosts.c
@@ -194,9 +194,7 @@ void bat_hosts_init(int read_opt)
 			if (!homedir)
 				continue;
 
-			strncpy(confdir, homedir, CONF_DIR_LEN);
-			confdir[CONF_DIR_LEN - 1] = '\0';
-			strncat(confdir, &bat_hosts_path[i][1], CONF_DIR_LEN - strlen(confdir) - 1);
+			snprintf(confdir, CONF_DIR_LEN, "%s%s", homedir, &bat_hosts_path[i][1]);
 		} else {
 			strncpy(confdir, bat_hosts_path[i], CONF_DIR_LEN);
 			confdir[CONF_DIR_LEN - 1] = '\0';
diff --git a/functions.c b/functions.c
index 868e0ae..8bcf52d 100644
--- a/functions.c
+++ b/functions.c
@@ -208,9 +208,7 @@ int read_file(const char *dir, const char *fname, int read_opt,
 	if (read_opt & USE_BAT_HOSTS)
 		bat_hosts_init(read_opt);
 
-	strncpy(full_path, dir, sizeof(full_path));
-	full_path[sizeof(full_path) - 1] = '\0';
-	strncat(full_path, fname, sizeof(full_path) - strlen(full_path) - 1);
+	snprintf(full_path, sizeof(full_path), "%s%s", dir, fname);
 
 open:
 	line = 0;
@@ -349,9 +347,7 @@ int write_file(const char *dir, const char *fname, const char *arg1,
 	char full_path[500];
 	ssize_t write_len;
 
-	strncpy(full_path, dir, sizeof(full_path));
-	full_path[sizeof(full_path) - 1] = '\0';
-	strncat(full_path, fname, sizeof(full_path) - strlen(full_path) - 1);
+	snprintf(full_path, sizeof(full_path), "%s%s", dir, fname);
 
 	fd = open(full_path, O_WRONLY);
 
-- 
2.11.0

Re: [B.A.T.M.A.N.] [PATCH 00/10] batctl: Fixes and minor cleanup

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 893 bytes --]

On Thursday, November 23, 2017 3:04:34 PM CET Sven Eckelmann wrote:
> From: Sven Eckelmann <sven@narfation.org>
> 
> Hi,
> 
> today I had the unpleasent experience to receive [1] to a lot of PDF files
> which seem to contain some reports of the Klocwork static code analysis
> tool. Most of the stuff was nonsense about memory/resource leaks (even when
> it stored in some kind of structure one line before). I gathered the only
> interesting portions and prepared some patches for them.
> 
> The last two patches are actually not really bug fixes but are about code
> which completely killed the static analyzer. I didn't like the code in
> the last patch (looked to complex for something relatively easy) and thus
> I've just replaced it.
> 
> WARNING: these things were only compile tested
> 
> Kind regards,
> 	Sven

I'ved picked up the series into 6116097..cfaec23

Thank you!
      Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]