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);

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) {

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)

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;

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;