[B.A.T.M.A.N.] [PATCH 0/4] batctl: Minor cleanups related to tcpdump

[B.A.T.M.A.N.] [PATCH 0/4] batctl: Minor cleanups related to tcpdump

[Sven Eckelmann]

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

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

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

[B.A.T.M.A.N.] [PATCH 1/4] batctl: tcpdump: Reset socket state on initialization error

[Sven Eckelmann]

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

[B.A.T.M.A.N.] [PATCH 2/4] batctl: tcpdump: Free resources on SIG(TERM|INT)

[Sven Eckelmann]

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

[B.A.T.M.A.N.] [PATCH 3/4] batctl: ping: Use sig_atomic_t variable to stop ping

[Sven Eckelmann]

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

[B.A.T.M.A.N.] [PATCH 4/4] batctl: Simplify standard error messages with perror

[Sven Eckelmann]

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;
 		}
 
-- 
2.11.0

Re: [B.A.T.M.A.N.] [PATCH 0/4] batctl: Minor cleanups related to tcpdump

[Simon Wunderlich]

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

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


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