Hi,
here are just some random cleanups/fixes for things I've noticed while having a look at https://www.open-mesh.org/issues/298.
batctl: tcpdump: Reset socket state on initialization error batctl: tcpdump: Free resources on SIG(TERM|INT) batctl: ping: Use sig_atomic_t variable to stop ping batctl: Simplify standard error messages with perror
Kind regards, Sven
The raw dump_if sockets for tcpdump are closed+freed when an error happens during initialization. But only fully initialized sockets were correctly cleaned up. The socket causing the error was not handled by the standard cleanup code.
Moving the dump_if initialization code in a separate function makes it possible to also clean up the current dump_if without increasing the code complexity.
Signed-off-by: Sven Eckelmann sven@narfation.org --- tcpdump.c | 143 ++++++++++++++++++++++++++++++++++---------------------------- 1 file changed, 78 insertions(+), 65 deletions(-)
diff --git a/tcpdump.c b/tcpdump.c index c7e0cbc..586a2ca 100644 --- a/tcpdump.c +++ b/tcpdump.c @@ -1113,9 +1113,84 @@ static void parse_wifi_hdr(unsigned char *packet_buff, ssize_t buff_len, int rea parse_eth_hdr(packet_buff, buff_len, read_opt, time_printed); }
-int tcpdump(int argc, char **argv) +static struct dump_if *create_dump_interface(char *iface) { + struct dump_if *dump_if; struct ifreq req; + int res; + + dump_if = malloc(sizeof(struct dump_if)); + if (!dump_if) + return NULL; + + memset(dump_if, 0, sizeof(struct dump_if)); + + dump_if->dev = iface; + if (strlen(dump_if->dev) > IFNAMSIZ - 1) { + fprintf(stderr, "Error - interface name too long: %s\n", dump_if->dev); + goto free_dumpif; + } + + dump_if->raw_sock = socket(PF_PACKET, SOCK_RAW, htons(ETH_P_ALL)); + if (dump_if->raw_sock < 0) { + fprintf(stderr, "Error - can't create raw socket: %s\n", strerror(errno)); + goto free_dumpif; + } + + 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) { + fprintf(stderr, "Error - can't create raw socket (SIOCGIFHWADDR): %s\n", strerror(errno)); + goto close_socket; + } + + dump_if->hw_type = req.ifr_hwaddr.sa_family; + + switch (dump_if->hw_type) { + case ARPHRD_ETHER: + case ARPHRD_IEEE80211_PRISM: + case ARPHRD_IEEE80211_RADIOTAP: + break; + default: + fprintf(stderr, "Error - interface '%s' is of unknown type: %i\n", dump_if->dev, dump_if->hw_type); + goto close_socket; + } + + 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); + if (res < 0) { + fprintf(stderr, "Error - can't create raw socket (SIOCGIFINDEX): %s\n", strerror(errno)); + goto close_socket; + } + + dump_if->addr.sll_family = AF_PACKET; + dump_if->addr.sll_protocol = htons(ETH_P_ALL); + dump_if->addr.sll_ifindex = req.ifr_ifindex; + + res = bind(dump_if->raw_sock, (struct sockaddr *)&dump_if->addr, sizeof(struct sockaddr_ll)); + if (res < 0) { + fprintf(stderr, "Error - can't bind raw socket: %s\n", strerror(errno)); + goto close_socket; + } + + return dump_if; + +close_socket: + close(dump_if->raw_sock); +free_dumpif: + free(dump_if); + + return NULL; +} + +int tcpdump(int argc, char **argv) +{ struct timeval tv; struct dump_if *dump_if, *dump_if_tmp; struct list_head dump_if_list; @@ -1172,71 +1247,9 @@ int tcpdump(int argc, char **argv) FD_ZERO(&wait_sockets);
while (argc > found_args) { - - dump_if = malloc(sizeof(struct dump_if)); - memset(dump_if, 0, sizeof(struct dump_if)); - dump_if->raw_sock = -1; - - dump_if->dev = argv[found_args]; - - if (strlen(dump_if->dev) > IFNAMSIZ - 1) { - fprintf(stderr, "Error - interface name too long: %s\n", dump_if->dev); - goto out; - } - - dump_if->raw_sock = socket(PF_PACKET, SOCK_RAW, htons(ETH_P_ALL)); - - if (dump_if->raw_sock < 0) { - fprintf(stderr, "Error - can't create raw socket: %s\n", strerror(errno)); - goto out; - } - - 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) { - fprintf(stderr, "Error - can't create raw socket (SIOCGIFHWADDR): %s\n", strerror(errno)); - close(dump_if->raw_sock); - goto out; - } - - dump_if->hw_type = req.ifr_hwaddr.sa_family; - - switch (dump_if->hw_type) { - case ARPHRD_ETHER: - case ARPHRD_IEEE80211_PRISM: - case ARPHRD_IEEE80211_RADIOTAP: - break; - default: - fprintf(stderr, "Error - interface '%s' is of unknown type: %i\n", dump_if->dev, dump_if->hw_type); - goto out; - } - - 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); - - if (res < 0) { - fprintf(stderr, "Error - can't create raw socket (SIOCGIFINDEX): %s\n", strerror(errno)); - close(dump_if->raw_sock); + dump_if = create_dump_interface(argv[found_args]); + if (!dump_if) goto out; - } - - dump_if->addr.sll_family = AF_PACKET; - dump_if->addr.sll_protocol = htons(ETH_P_ALL); - dump_if->addr.sll_ifindex = req.ifr_ifindex; - - res = bind(dump_if->raw_sock, (struct sockaddr *)&dump_if->addr, sizeof(struct sockaddr_ll)); - - if (res < 0) { - fprintf(stderr, "Error - can't bind raw socket: %s\n", strerror(errno)); - close(dump_if->raw_sock); - goto out; - }
if (dump_if->raw_sock > max_sock) max_sock = dump_if->raw_sock;
The cleanup code for dump interfaces and bathosts is only called on errors during the initialization of the dump interfaces. But it should also be used when the program exits normally.
Signed-off-by: Sven Eckelmann sven@narfation.org --- tcpdump.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/tcpdump.c b/tcpdump.c index 586a2ca..92171a3 100644 --- a/tcpdump.c +++ b/tcpdump.c @@ -39,6 +39,7 @@ #include <netinet/icmp6.h> #include <netinet/if_ether.h> #include <net/ethernet.h> +#include <signal.h> #include <stddef.h> #include <stdint.h> #include <sys/select.h> @@ -1189,6 +1190,20 @@ free_dumpif: return NULL; }
+static volatile sig_atomic_t is_aborted = 0; + +static void sig_handler(int sig) +{ + switch (sig) { + case SIGINT: + case SIGTERM: + is_aborted = 1; + break; + default: + break; + } +} + int tcpdump(int argc, char **argv) { struct timeval tv; @@ -1242,6 +1257,9 @@ int tcpdump(int argc, char **argv)
bat_hosts_init(read_opt);
+ signal(SIGINT, sig_handler); + signal(SIGTERM, sig_handler); + /* init interfaces list */ INIT_LIST_HEAD(&dump_if_list); FD_ZERO(&wait_sockets); @@ -1259,7 +1277,7 @@ int tcpdump(int argc, char **argv) found_args++; }
- while (1) { + while (!is_aborted) {
memcpy(&tmp_wait_sockets, &wait_sockets, sizeof(fd_set));
The variable used in the signal handlers of ping is currently only a char. It is shared with the main routine which checks if the ping should be stopped or not. But the standard defines that such a variable must be volatile sig_atomic_t or a lock-free atomic object.
The volatile definition avoids that the compiler must not assume that this object can be cached. sig_atomic_t will make sure that this object is atomically accessible in asynchronous interrupts. Not using these types could result in an "unstoppable" ping.
Signed-off-by: Sven Eckelmann sven@narfation.org --- ping.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ping.c b/ping.c index 4fef663..7e5c6fc 100644 --- a/ping.c +++ b/ping.c @@ -45,7 +45,7 @@ #include "icmp_helper.h"
-char is_aborted = 0; +static volatile sig_atomic_t is_aborted = 0;
static void ping_usage(void)
The error string of errno can either be printed via fprintf + strerror or by simply using the helper function perror. Using the latter can simplify the code slightly by reducing the line length.
Signed-off-by: Sven Eckelmann sven@narfation.org --- bat-hosts.c | 2 +- icmp_helper.c | 7 ++++--- ioctl.c | 8 ++++---- tcpdump.c | 10 +++++----- 4 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/bat-hosts.c b/bat-hosts.c index 015fbcb..b530243 100644 --- a/bat-hosts.c +++ b/bat-hosts.c @@ -128,7 +128,7 @@ static void parse_hosts_file(struct hashtable_t **hash, const char path[], int r
if (!bat_host) { if (read_opt & USE_BAT_HOSTS) - fprintf(stderr, "Error - could not allocate memory: %s\n", strerror(errno)); + perror("Error - could not allocate memory"); goto out; }
diff --git a/icmp_helper.c b/icmp_helper.c index fb461d5..0eea5c4 100644 --- a/icmp_helper.c +++ b/icmp_helper.c @@ -30,6 +30,7 @@ #include <netinet/ether.h> #include <stdbool.h> #include <stdint.h> +#include <stdio.h> #include <stdlib.h> #include <string.h> #include <sys/ioctl.h> @@ -190,7 +191,7 @@ static int icmp_interface_add(const char *ifname, const uint8_t mac[ETH_ALEN])
iface->sock = socket(PF_PACKET, SOCK_RAW, htons(ETH_P_ALL)); if (iface->sock < 0) { - fprintf(stderr, "Error - can't create raw socket: %s\n", strerror(errno)); + perror("Error - can't create raw socket"); ret = -errno; goto free_iface; } @@ -201,7 +202,7 @@ static int icmp_interface_add(const char *ifname, const uint8_t mac[ETH_ALEN])
ret = ioctl(iface->sock, SIOCGIFINDEX, &req); if (ret < 0) { - fprintf(stderr, "Error - can't create raw socket (SIOCGIFINDEX): %s\n", strerror(errno)); + perror("Error - can't create raw socket (SIOCGIFINDEX)"); ret = -errno; goto close_sock; } @@ -214,7 +215,7 @@ static int icmp_interface_add(const char *ifname, const uint8_t mac[ETH_ALEN])
ret = bind(iface->sock, (struct sockaddr *)&sll, sizeof(struct sockaddr_ll)); if (ret < 0) { - fprintf(stderr, "Error - can't bind raw socket: %s\n", strerror(errno)); + perror("Error - can't bind raw socket"); ret = -errno; goto close_sock; } diff --git a/ioctl.c b/ioctl.c index b1db5e4..d95fc8d 100644 --- a/ioctl.c +++ b/ioctl.c @@ -48,7 +48,7 @@ static int statistics_custom_get(int fd, struct ifreq *ifr) ifr->ifr_data = (void *)&drvinfo; err = ioctl(fd, SIOCETHTOOL, ifr); if (err < 0) { - fprintf(stderr, "Error - can't open driver information: %s\n", strerror(errno)); + perror("Error - can't open driver information"); goto out; }
@@ -72,7 +72,7 @@ static int statistics_custom_get(int fd, struct ifreq *ifr) ifr->ifr_data = (void *)strings; err = ioctl(fd, SIOCETHTOOL, ifr); if (err < 0) { - fprintf(stderr, "Error - can't get stats strings information: %s\n", strerror(errno)); + perror("Error - can't get stats strings information"); goto out; }
@@ -81,7 +81,7 @@ static int statistics_custom_get(int fd, struct ifreq *ifr) ifr->ifr_data = (void *) stats; err = ioctl(fd, SIOCETHTOOL, ifr); if (err < 0) { - fprintf(stderr, "Error - can't get stats information: %s\n", strerror(errno)); + perror("Error - can't get stats information"); goto out; }
@@ -110,7 +110,7 @@ int ioctl_statistics_get(char *mesh_iface)
fd = socket(AF_INET, SOCK_DGRAM, 0); if (fd < 0) { - fprintf(stderr, "Error - can't open socket: %s\n", strerror(errno)); + perror("Error - can't open socket"); goto out; }
diff --git a/tcpdump.c b/tcpdump.c index 92171a3..3000343 100644 --- a/tcpdump.c +++ b/tcpdump.c @@ -1134,7 +1134,7 @@ static struct dump_if *create_dump_interface(char *iface)
dump_if->raw_sock = socket(PF_PACKET, SOCK_RAW, htons(ETH_P_ALL)); if (dump_if->raw_sock < 0) { - fprintf(stderr, "Error - can't create raw socket: %s\n", strerror(errno)); + perror("Error - can't create raw socket"); goto free_dumpif; }
@@ -1144,7 +1144,7 @@ static struct dump_if *create_dump_interface(char *iface)
res = ioctl(dump_if->raw_sock, SIOCGIFHWADDR, &req); if (res < 0) { - fprintf(stderr, "Error - can't create raw socket (SIOCGIFHWADDR): %s\n", strerror(errno)); + perror("Error - can't create raw socket (SIOCGIFHWADDR)"); goto close_socket; }
@@ -1166,7 +1166,7 @@ static struct dump_if *create_dump_interface(char *iface)
res = ioctl(dump_if->raw_sock, SIOCGIFINDEX, &req); if (res < 0) { - fprintf(stderr, "Error - can't create raw socket (SIOCGIFINDEX): %s\n", strerror(errno)); + perror("Error - can't create raw socket (SIOCGIFINDEX)"); goto close_socket; }
@@ -1176,7 +1176,7 @@ static struct dump_if *create_dump_interface(char *iface)
res = bind(dump_if->raw_sock, (struct sockaddr *)&dump_if->addr, sizeof(struct sockaddr_ll)); if (res < 0) { - fprintf(stderr, "Error - can't bind raw socket: %s\n", strerror(errno)); + perror("Error - can't bind raw socket"); goto close_socket; }
@@ -1290,7 +1290,7 @@ int tcpdump(int argc, char **argv) continue;
if (res < 0) { - fprintf(stderr, "Error - can't select on raw socket: %s\n", strerror(errno)); + perror("Error - can't select on raw socket"); continue; }
On Sunday, January 22, 2017 1:07:10 PM CET Sven Eckelmann wrote:
Hi,
here are just some random cleanups/fixes for things I've noticed while having a look at https://www.open-mesh.org/issues/298.
batctl: tcpdump: Reset socket state on initialization error batctl: tcpdump: Free resources on SIG(TERM|INT) batctl: ping: Use sig_atomic_t variable to stop ping batctl: Simplify standard error messages with perror
Merged this series into dfe9da9..fae430b.
Thanks, Simon
b.a.t.m.a.n@lists.open-mesh.org
-
Simon Wunderlich -
Sven Eckelmann