[B.A.T.M.A.N.] [PATCH 1/6] batctl: Don't try to close negative file descriptors

[B.A.T.M.A.N.] [PATCH 1/6] batctl: Don't try to close negative file descriptors

[Sven Eckelmann]

Valid file descriptors are defined as being >= 0. Error codes returned by
the socket functions are defined as being < 0. This isn't checked correctly
through out the code and instead 0 is used as "not valid" file descriptor.

This can lead to functions like close being called with an error code as
argument.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 functions.c  | 4 ++--
 ping.c       | 4 ++--
 tcpdump.c    | 3 ++-
 traceroute.c | 4 ++--
 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/functions.c b/functions.c
index b9ccaff..0f96cb8 100644
--- a/functions.c
+++ b/functions.c
@@ -335,7 +335,7 @@ out:
 int write_file(const char *dir, const char *fname, const char *arg1,
 	       const char *arg2)
 {
-	int fd = 0, res = EXIT_FAILURE;
+	int fd = -1, res = EXIT_FAILURE;
 	char full_path[500];
 	ssize_t write_len;
 
@@ -363,7 +363,7 @@ int write_file(const char *dir, const char *fname, const char *arg1,
 	res = EXIT_SUCCESS;
 
 out:
-	if (fd)
+	if (fd >= 0)
 		close(fd);
 	return res;
 }
diff --git a/ping.c b/ping.c
index c52ad13..6642188 100644
--- a/ping.c
+++ b/ping.c
@@ -79,7 +79,7 @@ int ping(char *mesh_iface, int argc, char **argv)
 	struct bat_host *bat_host, *rr_host;
 	ssize_t read_len;
 	fd_set read_socket;
-	int ret = EXIT_FAILURE, ping_fd = 0, res, optchar, found_args = 1;
+	int ret = EXIT_FAILURE, ping_fd = -1, res, optchar, found_args = 1;
 	int loop_count = -1, loop_interval = 0, timeout = 1, rr = 0, i;
 	unsigned int seq_counter = 0, packets_out = 0, packets_in = 0, packets_loss;
 	char *dst_string, *mac_string, *rr_string;
@@ -353,7 +353,7 @@ sleep:
 
 out:
 	bat_hosts_free();
-	if (ping_fd)
+	if (ping_fd >= 0)
 		close(ping_fd);
 	return ret;
 }
diff --git a/tcpdump.c b/tcpdump.c
index 94e2a84..10b18f2 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -846,6 +846,7 @@ int tcpdump(int argc, char **argv)
 
 		dump_if = malloc(sizeof(struct dump_if));
 		memset(dump_if, 0, sizeof(struct dump_if));
+		dump_if->raw_sock = -1;
 		INIT_LIST_HEAD(&dump_if->list);
 
 		dump_if->dev = argv[found_args];
@@ -971,7 +972,7 @@ int tcpdump(int argc, char **argv)
 
 out:
 	list_for_each_entry_safe(dump_if, dump_if_tmp, &dump_if_list, list) {
-		if (dump_if->raw_sock)
+		if (dump_if->raw_sock >= 0)
 			close(dump_if->raw_sock);
 
 		list_del((struct list_head *)&dump_if_list, &dump_if->list, &dump_if_list);
diff --git a/traceroute.c b/traceroute.c
index ce78c5d..22b90f2 100644
--- a/traceroute.c
+++ b/traceroute.c
@@ -63,7 +63,7 @@ int traceroute(char *mesh_iface, int argc, char **argv)
 	fd_set read_socket;
 	ssize_t read_len;
 	char *dst_string, *mac_string, *return_mac, dst_reached = 0;
-	int ret = EXIT_FAILURE, res, trace_fd = 0, i;
+	int ret = EXIT_FAILURE, res, trace_fd = -1, i;
 	int found_args = 1, optchar, seq_counter = 0, read_opt = USE_BAT_HOSTS;
 	double time_delta[NUM_PACKETS];
 	char *debugfs_mnt;
@@ -241,7 +241,7 @@ read_packet:
 
 out:
 	bat_hosts_free();
-	if (trace_fd)
+	if (trace_fd >= 0)
 		close(trace_fd);
 	return ret;
 }
-- 
2.0.0.rc2

[B.A.T.M.A.N.] [PATCH 2/6] batctl: Force null termination of string after strncpy

[Sven Eckelmann]

strncpy doesn't terminate the string with a '\0' character when the length
of the destination memory location was shorter than the source string.
Accessing it again with string related functions isn't safe after such a
semi-failed copy and the caller has to handle it. The easiest way is to
always set the last character in the destination buffer to '\0' after the
strncpy was called.

Also the length provided as argument of strncpy should not be the length of
the source buffer but the maximum number of bytes in the destination buffer.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 bat-hosts.c | 6 ++++--
 bisect_iv.c | 2 ++
 debugfs.c   | 1 +
 functions.c | 8 ++++----
 tcpdump.c   | 2 ++
 5 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/bat-hosts.c b/bat-hosts.c
index 30ff848..1d62bb8 100644
--- a/bat-hosts.c
+++ b/bat-hosts.c
@@ -108,7 +108,8 @@ static void parse_hosts_file(struct hashtable_t **hash, const char path[], int r
 			if (read_opt & USE_BAT_HOSTS)
 				fprintf(stderr, "Warning - mac already known (changing name from '%s' to '%s'): %s\n",
 					bat_host->name, name, mac_str);
-			strncpy(bat_host->name, name, HOST_NAME_MAX_LEN - 1);
+			strncpy(bat_host->name, name, HOST_NAME_MAX_LEN);
+			bat_host->name[HOST_NAME_MAX_LEN - 1] = '\0';
 			continue;
 		}
 
@@ -132,7 +133,8 @@ static void parse_hosts_file(struct hashtable_t **hash, const char path[], int r
 		}
 
 		memcpy(&bat_host->mac_addr, mac_addr, sizeof(struct ether_addr));
-		strncpy(bat_host->name, name, HOST_NAME_MAX_LEN - 1);
+		strncpy(bat_host->name, name, HOST_NAME_MAX_LEN);
+		bat_host->name[HOST_NAME_MAX_LEN - 1] = '\0';
 
 		hash_add(*hash, bat_host);
 
diff --git a/bisect_iv.c b/bisect_iv.c
index 0c25664..e9e7326 100644
--- a/bisect_iv.c
+++ b/bisect_iv.c
@@ -91,6 +91,7 @@ static struct bat_node *node_get(char *name)
 	}
 
 	strncpy(bat_node->name, name, NAME_LEN);
+	bat_node->name[NAME_LEN] = '\0';
 	INIT_LIST_HEAD_FIRST(bat_node->orig_event_list);
 	INIT_LIST_HEAD_FIRST(bat_node->rt_table_list);
 	memset(bat_node->loop_magic, 0, sizeof(bat_node->loop_magic));
@@ -1438,6 +1439,7 @@ static int get_orig_addr(char *orig_name, char *orig_addr)
 
 copy_name:
 	strncpy(orig_addr, orig_name_tmp, NAME_LEN);
+	orig_addr[NAME_LEN - 1] = '\0';
 	return 1;
 
 err:
diff --git a/debugfs.c b/debugfs.c
index 8033f8b..8dd78b1 100644
--- a/debugfs.c
+++ b/debugfs.c
@@ -149,6 +149,7 @@ char *debugfs_mount(const char *mountpoint)
 
 	/* save the mountpoint */
 	strncpy(debugfs_mountpoint, mountpoint, sizeof(debugfs_mountpoint));
+	debugfs_mountpoint[sizeof(debugfs_mountpoint) - 1] = '\0';
 	debugfs_found = 1;
 
 	return debugfs_mountpoint;
diff --git a/functions.c b/functions.c
index 0f96cb8..84f0d14 100644
--- a/functions.c
+++ b/functions.c
@@ -199,8 +199,8 @@ 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, strlen(dir));
-	full_path[strlen(dir)] = '\0';
+	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);
 
 open:
@@ -339,8 +339,8 @@ int write_file(const char *dir, const char *fname, const char *arg1,
 	char full_path[500];
 	ssize_t write_len;
 
-	strncpy(full_path, dir, strlen(dir));
-	full_path[strlen(dir)] = '\0';
+	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);
 
 	fd = open(full_path, O_WRONLY);
diff --git a/tcpdump.c b/tcpdump.c
index 10b18f2..e84617e 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -865,6 +865,7 @@ int tcpdump(int argc, char **argv)
 
 		memset(&req, 0, sizeof (struct ifreq));
 		strncpy(req.ifr_name, dump_if->dev, IFNAMSIZ);
+		req.ifr_name[sizeof(req.ifr_name) - 1] = '\0';
 
 		res = ioctl(dump_if->raw_sock, SIOCGIFHWADDR, &req);
 		if (res < 0) {
@@ -887,6 +888,7 @@ int tcpdump(int argc, char **argv)
 
 		memset(&req, 0, sizeof (struct ifreq));
 		strncpy(req.ifr_name, dump_if->dev, IFNAMSIZ);
+		req.ifr_name[sizeof(req.ifr_name) - 1] = '\0';
 
 		res = ioctl(dump_if->raw_sock, SIOCGIFINDEX, &req);
 
-- 
2.0.0.rc2

Re: [B.A.T.M.A.N.] [PATCH 2/6] batctl: Force null termination of string after strncpy

[Marek Lindner]

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

On Saturday 24 May 2014 14:16:40 Sven Eckelmann wrote:
> strncpy doesn't terminate the string with a '\0' character when the length
> of the destination memory location was shorter than the source string.
> Accessing it again with string related functions isn't safe after such a
> semi-failed copy and the caller has to handle it. The easiest way is to
> always set the last character in the destination buffer to '\0' after the
> strncpy was called.
> 
> Also the length provided as argument of strncpy should not be the length of
> the source buffer but the maximum number of bytes in the destination buffer.
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
>  bat-hosts.c | 6 ++++--
>  bisect_iv.c | 2 ++
>  debugfs.c   | 1 +
>  functions.c | 8 ++++----
>  tcpdump.c   | 2 ++
>  5 files changed, 13 insertions(+), 6 deletions(-)

Applied with a slight modification in revision 4faf653.

Thanks,
Marek

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

[B.A.T.M.A.N.] [PATCH 3/6] batctl: Use strncpy instead of strcpy for string copy

[Sven Eckelmann]

The data used in strcpy is partially provided by the user. This can be larger
than the destination buffer and thus overwrite data after the actual string
buffer. This can easily be avoided by using strncpy.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 debugfs.c | 4 +++-
 ioctl.c   | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/debugfs.c b/debugfs.c
index 8dd78b1..7bac044 100644
--- a/debugfs.c
+++ b/debugfs.c
@@ -74,7 +74,9 @@ const char *debugfs_find_mountpoint(void)
 	while (*ptr) {
 		if (debugfs_valid_mountpoint(*ptr) == 0) {
 			debugfs_found = 1;
-			strcpy(debugfs_mountpoint, *ptr);
+			strncpy(debugfs_mountpoint, *ptr,
+				sizeof(debugfs_mountpoint));
+			debugfs_mountpoint[sizeof(debugfs_mountpoint) - 1] = 0;
 			return debugfs_mountpoint;
 		}
 		ptr++;
diff --git a/ioctl.c b/ioctl.c
index 1f827e8..d3d182f 100644
--- a/ioctl.c
+++ b/ioctl.c
@@ -105,7 +105,8 @@ int ioctl_statistics_get(char *mesh_iface)
 	int fd = -1, ret = EXIT_FAILURE;
 
 	memset(&ifr, 0, sizeof(ifr));
-	strcpy(ifr.ifr_name, mesh_iface);
+	strncpy(ifr.ifr_name, mesh_iface, sizeof(ifr.ifr_name));
+	ifr.ifr_name[sizeof(ifr.ifr_name) - 1] = '\0';
 
 	fd = socket(AF_INET, SOCK_DGRAM, 0);
 	if (fd < 0) {
-- 
2.0.0.rc2

Re: [B.A.T.M.A.N.] [PATCH 3/6] batctl: Use strncpy instead of strcpy for string copy

[Marek Lindner]

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

On Saturday 24 May 2014 14:16:41 Sven Eckelmann wrote:
> The data used in strcpy is partially provided by the user. This can be
> larger than the destination buffer and thus overwrite data after the actual
> string buffer. This can easily be avoided by using strncpy.
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
>  debugfs.c | 4 +++-
>  ioctl.c   | 3 ++-
>  2 files changed, 5 insertions(+), 2 deletions(-)

Applied with minor modification in revision a9e88d8.

Thanks,
Marek

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

[B.A.T.M.A.N.] [PATCH 4/6] batctl: Return success only with valid line_ptr in read_file

[Sven Eckelmann]

The read_file function is rather complex and cluttered with different
functionality. One of it is to provide a line_ptr of a single line to the
caller. The caller trusts the return code for a SUCCESS of this function
and tries to access the line_ptr. But a failed getline may lead to an
NULL-line_ptr. The caller tries to dereference this NULL pointer and
causes an segfault.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 functions.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/functions.c b/functions.c
index 84f0d14..251e616 100644
--- a/functions.c
+++ b/functions.c
@@ -320,7 +320,8 @@ written:
 		goto open;
 	}
 
-	res = EXIT_SUCCESS;
+	if (line_ptr)
+		res = EXIT_SUCCESS;
 
 out:
 	if (fp)
-- 
2.0.0.rc2

Re: [B.A.T.M.A.N.] [PATCH 4/6] batctl: Return success only with valid line_ptr in read_file

[Marek Lindner]

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

On Saturday 24 May 2014 14:16:42 Sven Eckelmann wrote:
> The read_file function is rather complex and cluttered with different
> functionality. One of it is to provide a line_ptr of a single line to the
> caller. The caller trusts the return code for a SUCCESS of this function
> and tries to access the line_ptr. But a failed getline may lead to an
> NULL-line_ptr. The caller tries to dereference this NULL pointer and
> causes an segfault.
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
>  functions.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Applied in revision 51b3e92.

Thanks,
Marek

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

[B.A.T.M.A.N.] [PATCH 5/6] batctl: Initialize complete ping packet before write

[Sven Eckelmann]

The data structure for ping packets is not completely initialized by the
userspace because the kernel sets the originator address of the packet. This
is not only a bad practice but also irritate tools like valgrind.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 ping.c       | 1 +
 traceroute.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/ping.c b/ping.c
index 6642188..bdca222 100644
--- a/ping.c
+++ b/ping.c
@@ -176,6 +176,7 @@ int ping(char *mesh_iface, int argc, char **argv)
 
 	packet_len = sizeof(struct batadv_icmp_packet);
 
+	memset(&icmp_packet_out, 0, sizeof(icmp_packet_out));
 	memcpy(&icmp_packet_out.dst, dst_mac, ETH_ALEN);
 	icmp_packet_out.packet_type = BATADV_ICMP;
 	icmp_packet_out.version = BATADV_COMPAT_VERSION;
diff --git a/traceroute.c b/traceroute.c
index 22b90f2..4ebfec2 100644
--- a/traceroute.c
+++ b/traceroute.c
@@ -133,6 +133,7 @@ int traceroute(char *mesh_iface, int argc, char **argv)
 		goto out;
 	}
 
+	memset(&icmp_packet_out, 0, sizeof(icmp_packet_out));
 	memcpy(&icmp_packet_out.dst, dst_mac, ETH_ALEN);
 	icmp_packet_out.version = BATADV_COMPAT_VERSION;
 	icmp_packet_out.packet_type = BATADV_ICMP;
-- 
2.0.0.rc2

Re: [B.A.T.M.A.N.] [PATCH 5/6] batctl: Initialize complete ping packet before write

[Marek Lindner]

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

On Saturday 24 May 2014 14:16:43 Sven Eckelmann wrote:
> The data structure for ping packets is not completely initialized by the
> userspace because the kernel sets the originator address of the packet. This
> is not only a bad practice but also irritate tools like valgrind.
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
>  ping.c       | 1 +
>  traceroute.c | 1 +
>  2 files changed, 2 insertions(+)

Applied in revision 7e53633.

Thanks,
Marek

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

[B.A.T.M.A.N.] [PATCH 6/6] batctl: Don't provide uninitialized parameter to read_file

[Sven Eckelmann]

The debug table function uses a parameter to filter out old originator nodes
when read_file processes the file. The variable is by default not initialized
and still given to this function. This is bad practice and irritates checker
tools.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 debug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/debug.c b/debug.c
index e7f463d..dfcf3c3 100644
--- a/debug.c
+++ b/debug.c
@@ -107,7 +107,7 @@ int handle_debug_table(char *mesh_iface, int debug_table, int argc, char **argv)
 	char full_path[MAX_PATH+1];
 	char *debugfs_mnt;
 	char *orig_iface = NULL;
-	float orig_timeout;
+	float orig_timeout = 0.0f;
 	float watch_interval = 1;
 	opterr = 0;
 
-- 
2.0.0.rc2

Re: [B.A.T.M.A.N.] [PATCH 6/6] batctl: Don't provide uninitialized parameter to read_file

[Marek Lindner]

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

On Saturday 24 May 2014 14:16:44 Sven Eckelmann wrote:
> The debug table function uses a parameter to filter out old originator nodes
> when read_file processes the file. The variable is by default not
> initialized and still given to this function. This is bad practice and
> irritates checker tools.
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
>  debug.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied in revision cc9e48b.

Thanks,
Marek

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

Re: [B.A.T.M.A.N.] [PATCH 1/6] batctl: Don't try to close negative file descriptors

[Marek Lindner]

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

On Saturday 24 May 2014 14:16:39 Sven Eckelmann wrote:
> Valid file descriptors are defined as being >= 0. Error codes returned by
> the socket functions are defined as being < 0. This isn't checked correctly
> through out the code and instead 0 is used as "not valid" file descriptor.
> 
> This can lead to functions like close being called with an error code as
> argument.
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
>  functions.c  | 4 ++--
>  ping.c       | 4 ++--
>  tcpdump.c    | 3 ++-
>  traceroute.c | 4 ++--
>  4 files changed, 8 insertions(+), 7 deletions(-)

Applied in revision 2656408.

Thanks,
Marek

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