[PATCH v2 00/28] Fixes for static checkers

[PATCH v2 00/28] Fixes for static checkers

[David Gibson]

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

The passt tests include two static checking tools: clang-tidy and
cppcheck.  However, newer versions of those tools have introduced
extra checks, and may cause these tests to fail.

This series fixes all the clang-tidy and cppcheck warnings, either by
altering our code, or by suppressing them with relevant options to the
checkers.  With this series, the checks are now clean on both my
Fedora 36 machine (clang-tools-extra-14.0.5-1.fc36.x86_64 and
cppcheck-2.7.4-2.fc36.x86_64) and my Debian machine (bookworm with
some pieces from sid: clang-tidy 1:14.0-55.1 and cppcheck 2.9-1).

Changes since v1:
 * Fixed a whitespace error
 * Added extra background information and details to comments and
   commit messages when removing old suppressions
 * Improved conf_runas() rework to give better error messages

David Gibson (28):
  Clean up parsing of port ranges
  clang-tidy: Suppress warning about unchecked error in logfn macro
  clang-tidy: Fix spurious null pointer warning in pasta_start_ns()
  clang-tidy: Remove duplicate #include from icmp.c
  Catch failures when installing signal handlers
  Pack DHCPv6 "on wire" structures
  Clean up parsing in conf_runas()
  cppcheck: Reduce scope of some variables
  Don't shadow 'i' in conf_ports()
  Don't shadow global function names
  Stricter checking for nsholder.c
  cppcheck: Work around false positive NULL pointer dereference error
  cppcheck: Use inline suppression for ffsl()
  cppcheck: Use inline suppressions for qrap.c
  cppcheck: Use inline suppression for strtok() in conf.c
  Avoid ugly 'end' members in netlink structures
  cppcheck: Broaden suppression for unused struct members
  cppcheck: Remove localtime suppression for pcap.c
  qrap: Handle case of PATH environment variable being unset
  cppcheck: Suppress same-value-in-ternary branches warning
  cppcheck: Suppress NULL pointer warning in tcp_sock_consume()
  Regenerate seccomp.h if seccomp.sh changes
  cppcheck: Avoid errors due to zeroes in bitwise ORs
  cppcheck: Remove unused knownConditionTrueFalse suppression
  cppcheck: Remove unused objectIndex suppressions
  cppcheck: Remove unused va_list_usedBeforeStarted suppression
  Mark unused functions for cppcheck
  cppcheck: Remove unused unmatchedSuppression suppressions

 Makefile        |  26 +---
 arch.c          |   4 +-
 conf.c          | 359 +++++++++++++++++++++++-------------------------
 dhcpv6.c        |  26 ++--
 icmp.c          |   1 -
 igmp.c          |   1 +
 netlink.c       |  22 +--
 passt.c         |   7 +-
 pasta.c         |   5 +-
 qrap.c          |  18 ++-
 seccomp.sh      |   2 +
 siphash.c       |   1 +
 tap.c           |   5 +-
 tcp.c           |   5 +
 test/Makefile   |   2 +-
 test/nsholder.c |   2 +-
 util.c          |   2 +-
 util.h          |   1 +
 18 files changed, 236 insertions(+), 253 deletions(-)

-- 
2.37.3

[PATCH v2 01/28] Clean up parsing of port ranges

[David Gibson]

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

conf_ports() parses ranges of ports for the -t, -u, -T and -U options.
The code is quite difficult to the follow, to the point that clang-tidy
and cppcheck disagree on whether one of the pointers can be NULL at some
points.

Rework the code with the use of two new helper functions:
  * parse_port_range() operates a bit like strtoul(), but can parse a whole
    port range specification (e.g. '80' or '1000-1015')
  * next_chunk() does the necessary wrapping around strchr() to advance to
    just after the next given delimiter, while cleanly handling if there
    are no more delimiters

The new version is easier to follow, and also removes some cppcheck
warnings.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 conf.c | 242 ++++++++++++++++++++++++---------------------------------
 1 file changed, 102 insertions(+), 140 deletions(-)

diff --git a/conf.c b/conf.c
index 993f840..ddba07c 100644
--- a/conf.c
+++ b/conf.c
@@ -106,6 +106,66 @@ static int get_bound_ports_ns(void *arg)
 	return 0;
 }
 
+/**
+ * next_chunk - Return the next piece of a string delimited by a character
+ * @s:		String to search
+ * @c:		Delimiter character
+ *
+ * Returns: If another @c is found in @s, returns a pointer to the
+ *	    character *after* the delimiter, if no further @c is in
+ *	    @s, return NULL
+ */
+static char *next_chunk(const char *s, char c)
+{
+	char *sep = strchr(s, c);
+	return sep ? sep + 1 : NULL;
+}
+
+/**
+ * port_range - Represents a non-empty range of ports
+ * @first:	First port number in the range
+ * @last:	Last port number in the range (inclusive)
+ *
+ * Invariant:	@last >= @first
+ */
+struct port_range {
+	in_port_t first, last;
+};
+
+/**
+ * parse_port_range() - Parse a range of port numbers '<first>[-<last>]'
+ * @s:		String to parse
+ * @endptr:	Update to the character after the parsed range (similar to
+ *		strtol() etc.)
+ * @range:	Update with the parsed values on success
+ *
+ * Return: -EINVAL on parsing error, -ERANGE on out of range port
+ *	   numbers, 0 on success
+ */
+static int parse_port_range(const char *s, char **endptr,
+			    struct port_range *range)
+{
+	unsigned long first, last;
+
+	last = first = strtoul(s, endptr, 10);
+	if (*endptr == s) /* Parsed nothing */
+		return -EINVAL;
+	if (**endptr == '-') { /* we have a last value too */
+		const char *lasts = *endptr + 1;
+		last = strtoul(lasts, endptr, 10);
+		if (*endptr == lasts) /* Parsed nothing */
+			return -EINVAL;
+	}
+
+	if ((last < first) || (last >= NUM_PORTS))
+		return -ERANGE;
+
+	range->first = first;
+	range->last = last;
+
+	return 0;
+}
+
 /**
  * conf_ports() - Parse port configuration options, initialise UDP/TCP sockets
  * @c:		Execution context
@@ -119,11 +179,11 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
 		      struct port_fwd *fwd)
 {
 	char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf;
-	int start_src, end_src, start_dst, end_dst, exclude_only = 1, i;
 	uint8_t exclude[PORT_BITMAP_SIZE] = { 0 };
-	char buf[BUFSIZ], *sep, *spec, *p;
+	char buf[BUFSIZ], *spec, *p;
 	sa_family_t af = AF_UNSPEC;
-	unsigned port;
+	bool exclude_only = true;
+	unsigned i;
 
 	if (!strcmp(optarg, "none")) {
 		if (fwd->mode)
@@ -183,65 +243,29 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
 		addr = NULL;
 	}
 
-	if (strspn(spec, "0123456789-,:~") != strlen(spec))
-		goto bad;
-
 	/* Mark all exclusions first, they might be given after base ranges */
 	p = spec;
-	start_src = end_src = -1;
 	do {
-		while (*p != '~' && start_src == -1) {
-			exclude_only = 0;
-
-			if (!(p = strchr(p, ',')))
-				break;
+		struct port_range xrange;
 
-			p++;
+		if (*p != '~') {
+			/* Not an exclude range, parse later */
+			exclude_only = false;
+			continue;
 		}
-		if (!p || !*p)
-			break;
 
-		if (*p == '~')
-			p++;
-
-		errno = 0;
-		port = strtoul(p, &sep, 10);
-		if (sep == p)
-			break;
-
-		if (port >= NUM_PORTS || errno)
+		if (parse_port_range(p, &p, &xrange))
+			goto bad;
+		if ((*p != '\0')  && (*p != ',')) /* Garbage after the range */
 			goto bad;
 
-		switch (*sep) {
-		case '-':
-			if (start_src == -1)		/* ~22-... */
-				start_src = port;
-			break;
-		case ',':
-		case 0:
-			if (start_src == -1)		/* ~80 */
-				start_src = end_src = port;
-			else if (end_src == -1)		/* ~22-25 */
-				end_src = port;
-			else
-				goto bad;
-
-			if (start_src > end_src)	/* ~80-22 */
-				goto bad;
-
-			for (i = start_src; i <= end_src; i++) {
-				if (bitmap_isset(exclude, i))
-					goto overlap;
+		for (i = xrange.first; i <= xrange.last; i++) {
+			if (bitmap_isset(exclude, i))
+				goto overlap;
 
-				bitmap_set(exclude, i);
-			}
-			start_src = end_src = -1;
-			break;
-		default:
-			goto bad;
+			bitmap_set(exclude, i);
 		}
-		p = sep + 1;
-	} while (*sep);
+	} while ((p = next_chunk(p, ',')));
 
 	if (exclude_only) {
 		for (i = 0; i < PORT_EPHEMERAL_MIN; i++) {
@@ -260,109 +284,47 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
 	}
 
 	/* Now process base ranges, skipping exclusions */
-	start_src = end_src = start_dst = end_dst = -1;
 	p = spec;
 	do {
-		while (*p == '~') {
-			if (!(p = strchr(p, ',')))
-				break;
-			p++;
-		}
-		if (!p || !*p)
-			break;
+		struct port_range orig_range, mapped_range;
 
-		errno = 0;
-		port = strtoul(p, &sep, 10);
-		if (sep == p)
-			break;
+		if (*p == '~')
+			/* Exclude range, already parsed */
+			continue;
 
-		if (port >= NUM_PORTS || errno)
+		if (parse_port_range(p, &p, &orig_range))
 			goto bad;
 
-		/* -p 22
-		 *    ^ start_src	end_src == start_dst == end_dst == -1
-		 *
-		 * -p 22-25
-		 *    |  ^ end_src
-		 *     ` start_src	start_dst == end_dst == -1
-		 *
-		 * -p 80:8080
-		 *    |  ^ start_dst
-		 *     ` start_src	end_src == end_dst == -1
-		 *
-		 * -p 22-80:8022-8080
-		 *    |  |  |    ^ end_dst
-		 *    |  |   ` start_dst
-		 *    |   ` end_dst
-		 *     ` start_src
-		 */
-		switch (*sep) {
-		case '-':
-			if (start_src == -1) {		/* 22-... */
-				start_src = port;
-			} else {
-				if (!end_src)		/* 22:8022-8080 */
-					goto bad;
-				start_dst = port;	/* 22-80:8022-... */
-			}
-			break;
-		case ':':
-			if (start_src == -1)		/* 80:... */
-				start_src = end_src = port;
-			else if (end_src == -1)		/* 22-80:... */
-				end_src = port;
-			else				/* 22-80:8022:... */
-				goto bad;
-			break;
-		case ',':
-		case 0:
-			if (start_src == -1)		/* 80 */
-				start_src = end_src = port;
-			else if (end_src == -1)		/* 22-25 */
-				end_src = port;
-			else if (start_dst == -1)	/* 80:8080 */
-				start_dst = end_dst = port;
-			else if (end_dst == -1)		/* 22-80:8022-8080 */
-				end_dst = port;
-			else
-				goto bad;
-
-			if (start_src > end_src)	/* 80-22 */
+		if (*p == ':') { /* There's a range to map to as well */
+			if (parse_port_range(p + 1, &p, &mapped_range))
 				goto bad;
-
-			if (start_dst > end_dst)	/* 22-80:8080:8022 */
+			if ((mapped_range.last - mapped_range.first) !=
+			    (orig_range.last - orig_range.first))
 				goto bad;
+		} else {
+			mapped_range = orig_range;
+		}
 
-			if (end_dst != -1 &&
-			    end_dst - start_dst != end_src - start_src)
-				goto bad;		/* 22-81:8022:8080 */
-
-			for (i = start_src; i <= end_src; i++) {
-				if (bitmap_isset(fwd->map, i))
-					goto overlap;
+		if ((*p != '\0') && (*p != ',')) /* Garbage after the ranges */
+			goto bad;
 
-				if (bitmap_isset(exclude, i))
-					continue;
+		for (i = orig_range.first; i <= orig_range.last; i++) {
+			if (bitmap_isset(fwd->map, i))
+				goto overlap;
 
-				bitmap_set(fwd->map, i);
+			if (bitmap_isset(exclude, i))
+				continue;
 
-				if (start_dst != -1) {
-					/* 80:8080 or 22-80:8080:8080 */
-					fwd->delta[i] = (in_port_t)(start_dst -
-								    start_src);
-				}
+			bitmap_set(fwd->map, i);
 
-				if (optname == 't')
-					tcp_sock_init(c, 0, af, addr, i);
-				else if (optname == 'u')
-					udp_sock_init(c, 0, af, addr, i);
-			}
+			fwd->delta[i] = mapped_range.first - orig_range.first;
 
-			start_src = end_src = start_dst = end_dst = -1;
-			break;
+			if (optname == 't')
+				tcp_sock_init(c, 0, af, addr, i);
+			else if (optname == 'u')
+				udp_sock_init(c, 0, af, addr, i);
 		}
-		p = sep + 1;
-	} while (*sep);
+	} while ((p = next_chunk(p, ',')));
 
 	return 0;
 bad:
-- 
@@ -106,6 +106,66 @@ static int get_bound_ports_ns(void *arg)
 	return 0;
 }
 
+/**
+ * next_chunk - Return the next piece of a string delimited by a character
+ * @s:		String to search
+ * @c:		Delimiter character
+ *
+ * Returns: If another @c is found in @s, returns a pointer to the
+ *	    character *after* the delimiter, if no further @c is in
+ *	    @s, return NULL
+ */
+static char *next_chunk(const char *s, char c)
+{
+	char *sep = strchr(s, c);
+	return sep ? sep + 1 : NULL;
+}
+
+/**
+ * port_range - Represents a non-empty range of ports
+ * @first:	First port number in the range
+ * @last:	Last port number in the range (inclusive)
+ *
+ * Invariant:	@last >= @first
+ */
+struct port_range {
+	in_port_t first, last;
+};
+
+/**
+ * parse_port_range() - Parse a range of port numbers '<first>[-<last>]'
+ * @s:		String to parse
+ * @endptr:	Update to the character after the parsed range (similar to
+ *		strtol() etc.)
+ * @range:	Update with the parsed values on success
+ *
+ * Return: -EINVAL on parsing error, -ERANGE on out of range port
+ *	   numbers, 0 on success
+ */
+static int parse_port_range(const char *s, char **endptr,
+			    struct port_range *range)
+{
+	unsigned long first, last;
+
+	last = first = strtoul(s, endptr, 10);
+	if (*endptr == s) /* Parsed nothing */
+		return -EINVAL;
+	if (**endptr == '-') { /* we have a last value too */
+		const char *lasts = *endptr + 1;
+		last = strtoul(lasts, endptr, 10);
+		if (*endptr == lasts) /* Parsed nothing */
+			return -EINVAL;
+	}
+
+	if ((last < first) || (last >= NUM_PORTS))
+		return -ERANGE;
+
+	range->first = first;
+	range->last = last;
+
+	return 0;
+}
+
 /**
  * conf_ports() - Parse port configuration options, initialise UDP/TCP sockets
  * @c:		Execution context
@@ -119,11 +179,11 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
 		      struct port_fwd *fwd)
 {
 	char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf;
-	int start_src, end_src, start_dst, end_dst, exclude_only = 1, i;
 	uint8_t exclude[PORT_BITMAP_SIZE] = { 0 };
-	char buf[BUFSIZ], *sep, *spec, *p;
+	char buf[BUFSIZ], *spec, *p;
 	sa_family_t af = AF_UNSPEC;
-	unsigned port;
+	bool exclude_only = true;
+	unsigned i;
 
 	if (!strcmp(optarg, "none")) {
 		if (fwd->mode)
@@ -183,65 +243,29 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
 		addr = NULL;
 	}
 
-	if (strspn(spec, "0123456789-,:~") != strlen(spec))
-		goto bad;
-
 	/* Mark all exclusions first, they might be given after base ranges */
 	p = spec;
-	start_src = end_src = -1;
 	do {
-		while (*p != '~' && start_src == -1) {
-			exclude_only = 0;
-
-			if (!(p = strchr(p, ',')))
-				break;
+		struct port_range xrange;
 
-			p++;
+		if (*p != '~') {
+			/* Not an exclude range, parse later */
+			exclude_only = false;
+			continue;
 		}
-		if (!p || !*p)
-			break;
 
-		if (*p == '~')
-			p++;
-
-		errno = 0;
-		port = strtoul(p, &sep, 10);
-		if (sep == p)
-			break;
-
-		if (port >= NUM_PORTS || errno)
+		if (parse_port_range(p, &p, &xrange))
+			goto bad;
+		if ((*p != '\0')  && (*p != ',')) /* Garbage after the range */
 			goto bad;
 
-		switch (*sep) {
-		case '-':
-			if (start_src == -1)		/* ~22-... */
-				start_src = port;
-			break;
-		case ',':
-		case 0:
-			if (start_src == -1)		/* ~80 */
-				start_src = end_src = port;
-			else if (end_src == -1)		/* ~22-25 */
-				end_src = port;
-			else
-				goto bad;
-
-			if (start_src > end_src)	/* ~80-22 */
-				goto bad;
-
-			for (i = start_src; i <= end_src; i++) {
-				if (bitmap_isset(exclude, i))
-					goto overlap;
+		for (i = xrange.first; i <= xrange.last; i++) {
+			if (bitmap_isset(exclude, i))
+				goto overlap;
 
-				bitmap_set(exclude, i);
-			}
-			start_src = end_src = -1;
-			break;
-		default:
-			goto bad;
+			bitmap_set(exclude, i);
 		}
-		p = sep + 1;
-	} while (*sep);
+	} while ((p = next_chunk(p, ',')));
 
 	if (exclude_only) {
 		for (i = 0; i < PORT_EPHEMERAL_MIN; i++) {
@@ -260,109 +284,47 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
 	}
 
 	/* Now process base ranges, skipping exclusions */
-	start_src = end_src = start_dst = end_dst = -1;
 	p = spec;
 	do {
-		while (*p == '~') {
-			if (!(p = strchr(p, ',')))
-				break;
-			p++;
-		}
-		if (!p || !*p)
-			break;
+		struct port_range orig_range, mapped_range;
 
-		errno = 0;
-		port = strtoul(p, &sep, 10);
-		if (sep == p)
-			break;
+		if (*p == '~')
+			/* Exclude range, already parsed */
+			continue;
 
-		if (port >= NUM_PORTS || errno)
+		if (parse_port_range(p, &p, &orig_range))
 			goto bad;
 
-		/* -p 22
-		 *    ^ start_src	end_src == start_dst == end_dst == -1
-		 *
-		 * -p 22-25
-		 *    |  ^ end_src
-		 *     ` start_src	start_dst == end_dst == -1
-		 *
-		 * -p 80:8080
-		 *    |  ^ start_dst
-		 *     ` start_src	end_src == end_dst == -1
-		 *
-		 * -p 22-80:8022-8080
-		 *    |  |  |    ^ end_dst
-		 *    |  |   ` start_dst
-		 *    |   ` end_dst
-		 *     ` start_src
-		 */
-		switch (*sep) {
-		case '-':
-			if (start_src == -1) {		/* 22-... */
-				start_src = port;
-			} else {
-				if (!end_src)		/* 22:8022-8080 */
-					goto bad;
-				start_dst = port;	/* 22-80:8022-... */
-			}
-			break;
-		case ':':
-			if (start_src == -1)		/* 80:... */
-				start_src = end_src = port;
-			else if (end_src == -1)		/* 22-80:... */
-				end_src = port;
-			else				/* 22-80:8022:... */
-				goto bad;
-			break;
-		case ',':
-		case 0:
-			if (start_src == -1)		/* 80 */
-				start_src = end_src = port;
-			else if (end_src == -1)		/* 22-25 */
-				end_src = port;
-			else if (start_dst == -1)	/* 80:8080 */
-				start_dst = end_dst = port;
-			else if (end_dst == -1)		/* 22-80:8022-8080 */
-				end_dst = port;
-			else
-				goto bad;
-
-			if (start_src > end_src)	/* 80-22 */
+		if (*p == ':') { /* There's a range to map to as well */
+			if (parse_port_range(p + 1, &p, &mapped_range))
 				goto bad;
-
-			if (start_dst > end_dst)	/* 22-80:8080:8022 */
+			if ((mapped_range.last - mapped_range.first) !=
+			    (orig_range.last - orig_range.first))
 				goto bad;
+		} else {
+			mapped_range = orig_range;
+		}
 
-			if (end_dst != -1 &&
-			    end_dst - start_dst != end_src - start_src)
-				goto bad;		/* 22-81:8022:8080 */
-
-			for (i = start_src; i <= end_src; i++) {
-				if (bitmap_isset(fwd->map, i))
-					goto overlap;
+		if ((*p != '\0') && (*p != ',')) /* Garbage after the ranges */
+			goto bad;
 
-				if (bitmap_isset(exclude, i))
-					continue;
+		for (i = orig_range.first; i <= orig_range.last; i++) {
+			if (bitmap_isset(fwd->map, i))
+				goto overlap;
 
-				bitmap_set(fwd->map, i);
+			if (bitmap_isset(exclude, i))
+				continue;
 
-				if (start_dst != -1) {
-					/* 80:8080 or 22-80:8080:8080 */
-					fwd->delta[i] = (in_port_t)(start_dst -
-								    start_src);
-				}
+			bitmap_set(fwd->map, i);
 
-				if (optname == 't')
-					tcp_sock_init(c, 0, af, addr, i);
-				else if (optname == 'u')
-					udp_sock_init(c, 0, af, addr, i);
-			}
+			fwd->delta[i] = mapped_range.first - orig_range.first;
 
-			start_src = end_src = start_dst = end_dst = -1;
-			break;
+			if (optname == 't')
+				tcp_sock_init(c, 0, af, addr, i);
+			else if (optname == 'u')
+				udp_sock_init(c, 0, af, addr, i);
 		}
-		p = sep + 1;
-	} while (*sep);
+	} while ((p = next_chunk(p, ',')));
 
 	return 0;
 bad:
-- 
2.37.3

[PATCH v2 02/28] clang-tidy: Suppress warning about unchecked error in logfn macro

[David Gibson]

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

clang-tidy complains that we're not checking the result of vfprintf in
logfn().  There's not really anything we can do if this fails here, so just
suppress the error with a cast to void.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util.c b/util.c
index 8d26561..6b86ead 100644
--- a/util.c
+++ b/util.c
@@ -57,7 +57,7 @@ void name(const char *format, ...) {					\
 	if (setlogmask(0) & LOG_MASK(LOG_DEBUG) ||			\
 	    setlogmask(0) == LOG_MASK(LOG_EMERG)) {			\
 		va_start(args, format);					\
-		vfprintf(stderr, format, args); 			\
+		(void)vfprintf(stderr, format, args); 			\
 		va_end(args);						\
 		if (format[strlen(format)] != '\n')			\
 			fprintf(stderr, "\n");				\
-- 
@@ -57,7 +57,7 @@ void name(const char *format, ...) {					\
 	if (setlogmask(0) & LOG_MASK(LOG_DEBUG) ||			\
 	    setlogmask(0) == LOG_MASK(LOG_EMERG)) {			\
 		va_start(args, format);					\
-		vfprintf(stderr, format, args); 			\
+		(void)vfprintf(stderr, format, args); 			\
 		va_end(args);						\
 		if (format[strlen(format)] != '\n')			\
 			fprintf(stderr, "\n");				\
-- 
2.37.3

[PATCH v2 03/28] clang-tidy: Fix spurious null pointer warning in pasta_start_ns()

[David Gibson]

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

clang-tidy isn't quite clever enough to figure out that getenv("SHELL")
will return the same thing both times here, which makes it conclude that
shell could be NULL, causing problems later.

It's a bit ugly that we call getenv() twice in any case, so rework this in
a way that clang-tidy can figure out shell won't be NULL.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 pasta.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/pasta.c b/pasta.c
index e233762..1dd8267 100644
--- a/pasta.c
+++ b/pasta.c
@@ -184,7 +184,7 @@ void pasta_start_ns(struct ctx *c, int argc, char *argv[])
 	struct pasta_setup_ns_arg arg = {
 		.argv = argv,
 	};
-	char *shell = getenv("SHELL") ? getenv("SHELL") : "/bin/sh";
+	char *shell = getenv("SHELL");
 	char *sh_argv[] = { shell, NULL };
 	char *bash_argv[] = { shell, "-l", NULL };
 	char ns_fn_stack[NS_FN_STACK_SIZE];
@@ -193,6 +193,9 @@ void pasta_start_ns(struct ctx *c, int argc, char *argv[])
 	if (!c->debug)
 		c->quiet = 1;
 
+	if (!shell)
+		shell = "/bin/sh";
+
 	if (argc == 0) {
 		if (strstr(shell, "/bash")) {
 			arg.argv = bash_argv;
-- 
@@ -184,7 +184,7 @@ void pasta_start_ns(struct ctx *c, int argc, char *argv[])
 	struct pasta_setup_ns_arg arg = {
 		.argv = argv,
 	};
-	char *shell = getenv("SHELL") ? getenv("SHELL") : "/bin/sh";
+	char *shell = getenv("SHELL");
 	char *sh_argv[] = { shell, NULL };
 	char *bash_argv[] = { shell, "-l", NULL };
 	char ns_fn_stack[NS_FN_STACK_SIZE];
@@ -193,6 +193,9 @@ void pasta_start_ns(struct ctx *c, int argc, char *argv[])
 	if (!c->debug)
 		c->quiet = 1;
 
+	if (!shell)
+		shell = "/bin/sh";
+
 	if (argc == 0) {
 		if (strstr(shell, "/bash")) {
 			arg.argv = bash_argv;
-- 
2.37.3

[PATCH v2 04/28] clang-tidy: Remove duplicate #include from icmp.c

[David Gibson]

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

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 icmp.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/icmp.c b/icmp.c
index 39a8694..29170aa 100644
--- a/icmp.c
+++ b/icmp.c
@@ -35,7 +35,6 @@
 #include "util.h"
 #include "passt.h"
 #include "tap.h"
-#include "packet.h"
 #include "icmp.h"
 
 #define ICMP_ECHO_TIMEOUT	60 /* s, timeout for ICMP socket activity */
-- 
@@ -35,7 +35,6 @@
 #include "util.h"
 #include "passt.h"
 #include "tap.h"
-#include "packet.h"
 #include "icmp.h"
 
 #define ICMP_ECHO_TIMEOUT	60 /* s, timeout for ICMP socket activity */
-- 
2.37.3

[PATCH v2 05/28] Catch failures when installing signal handlers

[David Gibson]

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

Stop ignoring the return codes from sigaction() and signal().  Unlikely to
happen in practice, but if it ever did it could lead to really hard to
debug problems.  So, take clang-tidy's advice and check for errors here.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 passt.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/passt.c b/passt.c
index f0ed897..4796c89 100644
--- a/passt.c
+++ b/passt.c
@@ -201,8 +201,10 @@ int main(int argc, char **argv)
 	name = basename(argv0);
 	if (strstr(name, "pasta")) {
 		sa.sa_handler = pasta_child_handler;
-		sigaction(SIGCHLD, &sa, NULL);
-		signal(SIGPIPE, SIG_IGN);
+		if (sigaction(SIGCHLD, &sa, NULL) || signal(SIGPIPE, SIG_IGN)) {
+			err("Couldn't install signal handlers");
+			exit(EXIT_FAILURE);
+		}
 
 		c.mode = MODE_PASTA;
 		log_name = "pasta";
-- 
@@ -201,8 +201,10 @@ int main(int argc, char **argv)
 	name = basename(argv0);
 	if (strstr(name, "pasta")) {
 		sa.sa_handler = pasta_child_handler;
-		sigaction(SIGCHLD, &sa, NULL);
-		signal(SIGPIPE, SIG_IGN);
+		if (sigaction(SIGCHLD, &sa, NULL) || signal(SIGPIPE, SIG_IGN)) {
+			err("Couldn't install signal handlers");
+			exit(EXIT_FAILURE);
+		}
 
 		c.mode = MODE_PASTA;
 		log_name = "pasta";
-- 
2.37.3

[PATCH v2 06/28] Pack DHCPv6 "on wire" structures

[David Gibson]

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

dhcpv6.c contains a number of structures which represent actual DHCPv6
packets as they appear on the wire, which will break if the structures
don't have exactly the in-memory layout we expect.

Therefore, we should mark these structures as ((packed)).  The contents of
them means this is unlikely to change the layout in practice - and since
it was working, presumably didn't on any arch we were testing on.  However
it's not impossible for the compiler on some arch to insert unexpected
padding in one of these structures, so we should be explicit.

clang-tidy warned about this since we were using memcmp() to compare some
of these structures, which it thought might not have a unique
representation.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 dhcpv6.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/dhcpv6.c b/dhcpv6.c
index fbae88d..8a1d4a6 100644
--- a/dhcpv6.c
+++ b/dhcpv6.c
@@ -62,7 +62,7 @@ struct opt_hdr {
 #define   STR_NOTONLINK		"Prefix not appropriate for link."
 
 	uint16_t l;
-};
+} __attribute__((packed));
 
 #if __BYTE_ORDER == __BIG_ENDIAN
 # define OPT_SIZE_CONV(x)	(x)
@@ -82,7 +82,7 @@ struct opt_hdr {
 struct opt_client_id {
 	struct opt_hdr hdr;
 	uint8_t duid[128];
-};
+} __attribute__((packed));
 
 /**
  * struct opt_server_id - DHCPv6 Server Identifier option
@@ -100,7 +100,7 @@ struct opt_server_id {
 	uint16_t duid_hw;
 	uint32_t duid_time;
 	uint8_t duid_lladdr[ETH_ALEN];
-};
+} __attribute__ ((packed));
 
 #if __BYTE_ORDER == __BIG_ENDIAN
 #define SERVER_ID {						\
@@ -128,7 +128,7 @@ struct opt_ia_na {
 	uint32_t iaid;
 	uint32_t t1;
 	uint32_t t2;
-};
+} __attribute__((packed));
 
 /**
  * struct opt_ia_ta - Identity Association for Temporary Addresses Option
@@ -138,7 +138,7 @@ struct opt_ia_na {
 struct opt_ia_ta {
 	struct opt_hdr hdr;
 	uint32_t iaid;
-};
+} __attribute__((packed));
 
 /**
  * struct opt_ia_addr - IA Address Option
@@ -152,7 +152,7 @@ struct opt_ia_addr {
 	struct in6_addr addr;
 	uint32_t pref_lifetime;
 	uint32_t valid_lifetime;
-};
+} __attribute__((packed));
 
 /**
  * struct opt_status_code - Status Code Option (used for NotOnLink error only)
@@ -164,7 +164,7 @@ struct opt_status_code {
 	struct opt_hdr hdr;
 	uint16_t code;
 	char status_msg[sizeof(STR_NOTONLINK) - 1];
-};
+} __attribute__((packed));
 
 /**
  * struct opt_dns_servers - DNS Recursive Name Server option (RFC 3646)
@@ -174,7 +174,7 @@ struct opt_status_code {
 struct opt_dns_servers {
 	struct opt_hdr hdr;
 	struct in6_addr addr[MAXNS];
-};
+} __attribute__((packed));
 
 /**
  * struct opt_dns_servers - Domain Search List option (RFC 3646)
@@ -184,7 +184,7 @@ struct opt_dns_servers {
 struct opt_dns_search {
 	struct opt_hdr hdr;
 	char list[MAXDNSRCH * NS_MAXDNAME];
-};
+} __attribute__((packed));
 
 /**
  * struct msg_hdr - DHCPv6 client/server message header
@@ -332,7 +332,7 @@ static struct opt_hdr *dhcpv6_ia_notonlink(const struct pool *p,
 					   struct in6_addr *la)
 {
 	char buf[INET6_ADDRSTRLEN];
-	struct in6_addr *req_addr;
+	struct in6_addr req_addr;
 	struct opt_hdr *ia, *h;
 	size_t offset;
 	int ia_type;
@@ -352,10 +352,10 @@ ia_ta:
 			if (ntohs(h->l) != OPT_VSIZE(ia_addr))
 				return NULL;
 
-			req_addr = &opt_addr->addr;
-			if (!IN6_ARE_ADDR_EQUAL(la, req_addr)) {
+			memcpy(&req_addr, &opt_addr->addr, sizeof(req_addr));
+			if (!IN6_ARE_ADDR_EQUAL(la, &req_addr)) {
 				info("DHCPv6: requested address %s not on link",
-				     inet_ntop(AF_INET6, req_addr,
+				     inet_ntop(AF_INET6, &req_addr,
 					       buf, sizeof(buf)));
 				return ia;
 			}
-- 
@@ -62,7 +62,7 @@ struct opt_hdr {
 #define   STR_NOTONLINK		"Prefix not appropriate for link."
 
 	uint16_t l;
-};
+} __attribute__((packed));
 
 #if __BYTE_ORDER == __BIG_ENDIAN
 # define OPT_SIZE_CONV(x)	(x)
@@ -82,7 +82,7 @@ struct opt_hdr {
 struct opt_client_id {
 	struct opt_hdr hdr;
 	uint8_t duid[128];
-};
+} __attribute__((packed));
 
 /**
  * struct opt_server_id - DHCPv6 Server Identifier option
@@ -100,7 +100,7 @@ struct opt_server_id {
 	uint16_t duid_hw;
 	uint32_t duid_time;
 	uint8_t duid_lladdr[ETH_ALEN];
-};
+} __attribute__ ((packed));
 
 #if __BYTE_ORDER == __BIG_ENDIAN
 #define SERVER_ID {						\
@@ -128,7 +128,7 @@ struct opt_ia_na {
 	uint32_t iaid;
 	uint32_t t1;
 	uint32_t t2;
-};
+} __attribute__((packed));
 
 /**
  * struct opt_ia_ta - Identity Association for Temporary Addresses Option
@@ -138,7 +138,7 @@ struct opt_ia_na {
 struct opt_ia_ta {
 	struct opt_hdr hdr;
 	uint32_t iaid;
-};
+} __attribute__((packed));
 
 /**
  * struct opt_ia_addr - IA Address Option
@@ -152,7 +152,7 @@ struct opt_ia_addr {
 	struct in6_addr addr;
 	uint32_t pref_lifetime;
 	uint32_t valid_lifetime;
-};
+} __attribute__((packed));
 
 /**
  * struct opt_status_code - Status Code Option (used for NotOnLink error only)
@@ -164,7 +164,7 @@ struct opt_status_code {
 	struct opt_hdr hdr;
 	uint16_t code;
 	char status_msg[sizeof(STR_NOTONLINK) - 1];
-};
+} __attribute__((packed));
 
 /**
  * struct opt_dns_servers - DNS Recursive Name Server option (RFC 3646)
@@ -174,7 +174,7 @@ struct opt_status_code {
 struct opt_dns_servers {
 	struct opt_hdr hdr;
 	struct in6_addr addr[MAXNS];
-};
+} __attribute__((packed));
 
 /**
  * struct opt_dns_servers - Domain Search List option (RFC 3646)
@@ -184,7 +184,7 @@ struct opt_dns_servers {
 struct opt_dns_search {
 	struct opt_hdr hdr;
 	char list[MAXDNSRCH * NS_MAXDNAME];
-};
+} __attribute__((packed));
 
 /**
  * struct msg_hdr - DHCPv6 client/server message header
@@ -332,7 +332,7 @@ static struct opt_hdr *dhcpv6_ia_notonlink(const struct pool *p,
 					   struct in6_addr *la)
 {
 	char buf[INET6_ADDRSTRLEN];
-	struct in6_addr *req_addr;
+	struct in6_addr req_addr;
 	struct opt_hdr *ia, *h;
 	size_t offset;
 	int ia_type;
@@ -352,10 +352,10 @@ ia_ta:
 			if (ntohs(h->l) != OPT_VSIZE(ia_addr))
 				return NULL;
 
-			req_addr = &opt_addr->addr;
-			if (!IN6_ARE_ADDR_EQUAL(la, req_addr)) {
+			memcpy(&req_addr, &opt_addr->addr, sizeof(req_addr));
+			if (!IN6_ARE_ADDR_EQUAL(la, &req_addr)) {
 				info("DHCPv6: requested address %s not on link",
-				     inet_ntop(AF_INET6, req_addr,
+				     inet_ntop(AF_INET6, &req_addr,
 					       buf, sizeof(buf)));
 				return ia;
 			}
-- 
2.37.3

[PATCH v2 07/28] Clean up parsing in conf_runas()

[David Gibson]

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

conf_runas() handles several of the different possible cases for the
--runas argument in a slightly odd order.  Although it can parse both
numeric UIDs/GIDs and user/group names, it can't parse a numeric UID
combined with a group name or vice versa.  That's not obviously useful, but
it's slightly surprising gap to have.

Rework the parsing to be more systematic: first split the option into
user and (optional) group parts, then separately parse each part as either
numeric or a name.  As a bonus this removes some clang-tidy warnings.

While we're there also add cppcheck suppressions for getpwnam() and
getgrnam().  It complains about those because they're not reentrant.
passt is single threaded though, and is always likely to be during
this initialization code, even if we multithread later.

There were some existing suppressions for these in the cppcheck
invocation but they're no longer up to date.  Replace them with inline
suppressions which, being next to the code, are more likely to stay
correct.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 Makefile |   3 +-
 conf.c   | 110 ++++++++++++++++++++++++++++++-------------------------
 2 files changed, 62 insertions(+), 51 deletions(-)

diff --git a/Makefile b/Makefile
index 59967eb..e0933f3 100644
--- a/Makefile
+++ b/Makefile
@@ -282,12 +282,12 @@ cppcheck: $(SRCS) $(HEADERS)
 	$(SYSTEM_INCLUDES:%=--config-exclude=%)				\
 	$(SYSTEM_INCLUDES:%=--suppress=*:%/*)				\
 	$(SYSTEM_INCLUDES:%=--suppress=unmatchedSuppression:%/*)	\
+	--inline-suppr							\
 	--suppress=objectIndex:tcp.c --suppress=objectIndex:udp.c	\
 	--suppress=va_list_usedBeforeStarted:util.c			\
 	--suppress=unusedFunction					\
 	--suppress=knownConditionTrueFalse:conf.c			\
 	--suppress=strtokCalled:conf.c --suppress=strtokCalled:qrap.c	\
-	--suppress=getpwnamCalled:passt.c				\
 	--suppress=localtimeCalled:pcap.c				\
 	--suppress=unusedStructMember:pcap.c				\
 	--suppress=funcArgNamesDifferent:util.h				\
@@ -295,7 +295,6 @@ cppcheck: $(SRCS) $(HEADERS)
 									\
 	--suppress=unmatchedSuppression:conf.c				\
 	--suppress=unmatchedSuppression:dhcp.c				\
-	--suppress=unmatchedSuppression:passt.c				\
 	--suppress=unmatchedSuppression:pcap.c				\
 	--suppress=unmatchedSuppression:qrap.c				\
 	--suppress=unmatchedSuppression:tcp.c				\
diff --git a/conf.c b/conf.c
index ddba07c..41afccd 100644
--- a/conf.c
+++ b/conf.c
@@ -859,46 +859,60 @@ dns6:
  *
  * Return: 0 on success, negative error code on failure
  */
-static int conf_runas(const char *opt, unsigned int *uid, unsigned int *gid)
+static int conf_runas(char *opt, unsigned int *uid, unsigned int *gid)
 {
-	char ubuf[LOGIN_NAME_MAX], gbuf[LOGIN_NAME_MAX], *endptr;
-	struct passwd *pw;
-	struct group *gr;
+	const char *uopt, *gopt = NULL;
+	char *sep = strchr(opt, ':');
+	char *endptr;
 
-	/* NOLINTNEXTLINE(cert-err34-c): 2 if conversion succeeds */
-	if (sscanf(opt, "%u:%u", uid, gid) == 2 && *uid && *gid)
-		return 0;
-
-	*uid = strtol(opt, &endptr, 0);
-	if (!*endptr && (*gid = *uid))
-		return 0;
-
-#ifdef GLIBC_NO_STATIC_NSS
-	(void)ubuf;
-	(void)gbuf;
-	(void)pw;
-	(void)gr;
-
-	return -EINVAL;
-#else
-	/* NOLINTNEXTLINE(cert-err34-c): 2 if conversion succeeds */
-	if (sscanf(opt, "%" STR(LOGIN_NAME_MAX) "[^:]:"
-			"%" STR(LOGIN_NAME_MAX) "s", ubuf, gbuf) == 2) {
-		if (!(pw = getpwnam(ubuf)) || !(*uid = pw->pw_uid))
-			return -ENOENT;
+	if (sep) {
+		*sep = '\0';
+		gopt = sep + 1;
+	}
+	uopt = opt;
 
-		if (!(gr = getgrnam(gbuf)) || !(*gid = gr->gr_gid))
+	*gid = *uid = strtol(uopt, &endptr, 0);
+	if (*endptr) {
+#ifndef GLIBC_NO_STATIC_NSS
+		/* Not numeric, look up as a username */
+		struct passwd *pw;
+		/* cppcheck-suppress getpwnamCalled */
+		if (!(pw = getpwnam(uopt))) {
+			err("No such user '%s' in --runas", uopt);
 			return -ENOENT;
+		}
+		if (!(*uid = pw->pw_uid)) {
+			err("Refusing to run as UID 0 user '%s'", uopt);
+			return -EPERM;
+		}
+		*gid = pw->pw_gid;
+#else
+		err("Bad user ID '%s' in --runas (no NSS support)", uopt);
+		return -EINVAL;
+#endif
+	}
 
+	if (!gopt)
 		return 0;
-	}
 
-	pw = getpwnam(ubuf);
-	if (!pw || !(*uid = pw->pw_uid) || !(*gid = pw->pw_gid))
-		return -ENOENT;
+	*gid = strtol(gopt, &endptr, 0);
+	if (*endptr) {
+#ifndef GLIBC_NO_STATIC_NSS
+		/* Not numeric, look up as a group name */
+		struct group *gr;
+		/* cppcheck-suppress getgrnamCalled */
+		if (!(gr = getgrnam(gopt))) {
+			err("No such group '%s' in --runas", gopt);
+			return -ENOENT;
+		}
+		*gid = gr->gr_gid;
+#else
+		err("Bad group ID '%s' in --runas (no NSS support)", gopt);
+		return -EINVAL;
+#endif
+	}
 
 	return 0;
-#endif /* !GLIBC_NO_STATIC_NSS */
 }
 
 /**
@@ -909,20 +923,16 @@ static int conf_runas(const char *opt, unsigned int *uid, unsigned int *gid)
  *
  * Return: 0 on success, negative error code on failure
  */
-static int conf_ugid(const char *runas, uid_t *uid, gid_t *gid)
+static int conf_ugid(char *runas, uid_t *uid, gid_t *gid)
 {
 	const char root_uid_map[] = "         0          0 4294967295";
-	struct passwd *pw;
 	char buf[BUFSIZ];
 	int ret;
 	int fd;
 
 	/* If user has specified --runas, that takes precedence... */
 	if (runas) {
-		ret = conf_runas(runas, uid, gid);
-		if (ret)
-			err("Invalid --runas option: %s", runas);
-		return ret;
+		return conf_runas(runas, uid, gid);
 	}
 
 	/* ...otherwise default to current user and group... */
@@ -951,21 +961,23 @@ static int conf_ugid(const char *runas, uid_t *uid, gid_t *gid)
 
 	/* ...otherwise use nobody:nobody */
 	warn("Don't run as root. Changing to nobody...");
+	{
 #ifndef GLIBC_NO_STATIC_NSS
-	pw = getpwnam("nobody");
-	if (!pw) {
-		perror("getpwnam");
-		exit(EXIT_FAILURE);
-	}
+		struct passwd *pw;
+		/* cppcheck-suppress getpwnamCalled */
+		pw = getpwnam("nobody");
+		if (!pw) {
+			perror("getpwnam");
+			exit(EXIT_FAILURE);
+		}
 
-	*uid = pw->pw_uid;
-	*gid = pw->pw_gid;
+		*uid = pw->pw_uid;
+		*gid = pw->pw_gid;
 #else
-	(void)pw;
-
-	/* Common value for 'nobody', not really specified */
-	*uid = *gid = 65534;
+		/* Common value for 'nobody', not really specified */
+		*uid = *gid = 65534;
 #endif
+	}
 	return 0;
 }
 
@@ -1032,8 +1044,8 @@ void conf(struct ctx *c, int argc, char **argv)
 	struct fqdn *dnss = c->dns_search;
 	uint32_t *dns4 = c->ip4.dns;
 	int name, ret, mask, b, i;
-	const char *runas = NULL;
 	unsigned int ifi = 0;
+	char *runas = NULL;
 	uid_t uid;
 	gid_t gid;
 
-- 
@@ -859,46 +859,60 @@ dns6:
  *
  * Return: 0 on success, negative error code on failure
  */
-static int conf_runas(const char *opt, unsigned int *uid, unsigned int *gid)
+static int conf_runas(char *opt, unsigned int *uid, unsigned int *gid)
 {
-	char ubuf[LOGIN_NAME_MAX], gbuf[LOGIN_NAME_MAX], *endptr;
-	struct passwd *pw;
-	struct group *gr;
+	const char *uopt, *gopt = NULL;
+	char *sep = strchr(opt, ':');
+	char *endptr;
 
-	/* NOLINTNEXTLINE(cert-err34-c): 2 if conversion succeeds */
-	if (sscanf(opt, "%u:%u", uid, gid) == 2 && *uid && *gid)
-		return 0;
-
-	*uid = strtol(opt, &endptr, 0);
-	if (!*endptr && (*gid = *uid))
-		return 0;
-
-#ifdef GLIBC_NO_STATIC_NSS
-	(void)ubuf;
-	(void)gbuf;
-	(void)pw;
-	(void)gr;
-
-	return -EINVAL;
-#else
-	/* NOLINTNEXTLINE(cert-err34-c): 2 if conversion succeeds */
-	if (sscanf(opt, "%" STR(LOGIN_NAME_MAX) "[^:]:"
-			"%" STR(LOGIN_NAME_MAX) "s", ubuf, gbuf) == 2) {
-		if (!(pw = getpwnam(ubuf)) || !(*uid = pw->pw_uid))
-			return -ENOENT;
+	if (sep) {
+		*sep = '\0';
+		gopt = sep + 1;
+	}
+	uopt = opt;
 
-		if (!(gr = getgrnam(gbuf)) || !(*gid = gr->gr_gid))
+	*gid = *uid = strtol(uopt, &endptr, 0);
+	if (*endptr) {
+#ifndef GLIBC_NO_STATIC_NSS
+		/* Not numeric, look up as a username */
+		struct passwd *pw;
+		/* cppcheck-suppress getpwnamCalled */
+		if (!(pw = getpwnam(uopt))) {
+			err("No such user '%s' in --runas", uopt);
 			return -ENOENT;
+		}
+		if (!(*uid = pw->pw_uid)) {
+			err("Refusing to run as UID 0 user '%s'", uopt);
+			return -EPERM;
+		}
+		*gid = pw->pw_gid;
+#else
+		err("Bad user ID '%s' in --runas (no NSS support)", uopt);
+		return -EINVAL;
+#endif
+	}
 
+	if (!gopt)
 		return 0;
-	}
 
-	pw = getpwnam(ubuf);
-	if (!pw || !(*uid = pw->pw_uid) || !(*gid = pw->pw_gid))
-		return -ENOENT;
+	*gid = strtol(gopt, &endptr, 0);
+	if (*endptr) {
+#ifndef GLIBC_NO_STATIC_NSS
+		/* Not numeric, look up as a group name */
+		struct group *gr;
+		/* cppcheck-suppress getgrnamCalled */
+		if (!(gr = getgrnam(gopt))) {
+			err("No such group '%s' in --runas", gopt);
+			return -ENOENT;
+		}
+		*gid = gr->gr_gid;
+#else
+		err("Bad group ID '%s' in --runas (no NSS support)", gopt);
+		return -EINVAL;
+#endif
+	}
 
 	return 0;
-#endif /* !GLIBC_NO_STATIC_NSS */
 }
 
 /**
@@ -909,20 +923,16 @@ static int conf_runas(const char *opt, unsigned int *uid, unsigned int *gid)
  *
  * Return: 0 on success, negative error code on failure
  */
-static int conf_ugid(const char *runas, uid_t *uid, gid_t *gid)
+static int conf_ugid(char *runas, uid_t *uid, gid_t *gid)
 {
 	const char root_uid_map[] = "         0          0 4294967295";
-	struct passwd *pw;
 	char buf[BUFSIZ];
 	int ret;
 	int fd;
 
 	/* If user has specified --runas, that takes precedence... */
 	if (runas) {
-		ret = conf_runas(runas, uid, gid);
-		if (ret)
-			err("Invalid --runas option: %s", runas);
-		return ret;
+		return conf_runas(runas, uid, gid);
 	}
 
 	/* ...otherwise default to current user and group... */
@@ -951,21 +961,23 @@ static int conf_ugid(const char *runas, uid_t *uid, gid_t *gid)
 
 	/* ...otherwise use nobody:nobody */
 	warn("Don't run as root. Changing to nobody...");
+	{
 #ifndef GLIBC_NO_STATIC_NSS
-	pw = getpwnam("nobody");
-	if (!pw) {
-		perror("getpwnam");
-		exit(EXIT_FAILURE);
-	}
+		struct passwd *pw;
+		/* cppcheck-suppress getpwnamCalled */
+		pw = getpwnam("nobody");
+		if (!pw) {
+			perror("getpwnam");
+			exit(EXIT_FAILURE);
+		}
 
-	*uid = pw->pw_uid;
-	*gid = pw->pw_gid;
+		*uid = pw->pw_uid;
+		*gid = pw->pw_gid;
 #else
-	(void)pw;
-
-	/* Common value for 'nobody', not really specified */
-	*uid = *gid = 65534;
+		/* Common value for 'nobody', not really specified */
+		*uid = *gid = 65534;
 #endif
+	}
 	return 0;
 }
 
@@ -1032,8 +1044,8 @@ void conf(struct ctx *c, int argc, char **argv)
 	struct fqdn *dnss = c->dns_search;
 	uint32_t *dns4 = c->ip4.dns;
 	int name, ret, mask, b, i;
-	const char *runas = NULL;
 	unsigned int ifi = 0;
+	char *runas = NULL;
 	uid_t uid;
 	gid_t gid;
 
-- 
2.37.3

[PATCH v2 08/28] cppcheck: Reduce scope of some variables

[David Gibson]

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

Minor style improvement suggested by cppcheck.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 arch.c    | 4 +++-
 netlink.c | 3 +--
 tap.c     | 5 +++--
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch.c b/arch.c
index 10eb24a..a2c4562 100644
--- a/arch.c
+++ b/arch.c
@@ -25,7 +25,7 @@
 #ifdef __x86_64__
 void arch_avx2_exec(char **argv)
 {
-	char exe[PATH_MAX] = { 0 }, new_path[PATH_MAX + sizeof(".avx2")], *p;
+	char exe[PATH_MAX] = { 0 }, *p;
 
 	if (readlink("/proc/self/exe", exe, PATH_MAX - 1) < 0) {
 		perror("readlink /proc/self/exe");
@@ -37,6 +37,8 @@ void arch_avx2_exec(char **argv)
 		return;
 
 	if (__builtin_cpu_supports("avx2")) {
+		char new_path[PATH_MAX + sizeof(".avx2")];
+
 		snprintf(new_path, PATH_MAX + sizeof(".avx2"), "%s.avx2", exe);
 		execve(new_path, argv, environ);
 		perror("Can't run AVX2 build, using non-AVX2 version");
diff --git a/netlink.c b/netlink.c
index 8ad6a0c..6f7ada8 100644
--- a/netlink.c
+++ b/netlink.c
@@ -147,7 +147,6 @@ unsigned int nl_get_ext_if(sa_family_t af)
 	};
 	struct nlmsghdr *nh;
 	struct rtattr *rta;
-	struct rtmsg *rtm;
 	char buf[BUFSIZ];
 	ssize_t n;
 	size_t na;
@@ -158,7 +157,7 @@ unsigned int nl_get_ext_if(sa_family_t af)
 	nh = (struct nlmsghdr *)buf;
 
 	for ( ; NLMSG_OK(nh, n); nh = NLMSG_NEXT(nh, n)) {
-		rtm = (struct rtmsg *)NLMSG_DATA(nh);
+		struct rtmsg *rtm = (struct rtmsg *)NLMSG_DATA(nh);
 
 		if (rtm->rtm_dst_len || rtm->rtm_family != af)
 			continue;
diff --git a/tap.c b/tap.c
index 4d7422f..5540c18 100644
--- a/tap.c
+++ b/tap.c
@@ -782,12 +782,12 @@ restart:
  */
 static void tap_sock_unix_init(struct ctx *c)
 {
-	int fd = socket(AF_UNIX, SOCK_STREAM, 0), ex;
+	int fd = socket(AF_UNIX, SOCK_STREAM, 0);
 	struct epoll_event ev = { 0 };
 	struct sockaddr_un addr = {
 		.sun_family = AF_UNIX,
 	};
-	int i, ret;
+	int i;
 
 	if (fd < 0) {
 		perror("UNIX socket");
@@ -802,6 +802,7 @@ static void tap_sock_unix_init(struct ctx *c)
 
 	for (i = 1; i < UNIX_SOCK_MAX; i++) {
 		char *path = addr.sun_path;
+		int ex, ret;
 
 		if (*c->sock_path)
 			memcpy(path, c->sock_path, UNIX_PATH_MAX);
-- 
@@ -782,12 +782,12 @@ restart:
  */
 static void tap_sock_unix_init(struct ctx *c)
 {
-	int fd = socket(AF_UNIX, SOCK_STREAM, 0), ex;
+	int fd = socket(AF_UNIX, SOCK_STREAM, 0);
 	struct epoll_event ev = { 0 };
 	struct sockaddr_un addr = {
 		.sun_family = AF_UNIX,
 	};
-	int i, ret;
+	int i;
 
 	if (fd < 0) {
 		perror("UNIX socket");
@@ -802,6 +802,7 @@ static void tap_sock_unix_init(struct ctx *c)
 
 	for (i = 1; i < UNIX_SOCK_MAX; i++) {
 		char *path = addr.sun_path;
+		int ex, ret;
 
 		if (*c->sock_path)
 			memcpy(path, c->sock_path, UNIX_PATH_MAX);
-- 
2.37.3

[PATCH v2 09/28] Don't shadow 'i' in conf_ports()

[David Gibson]

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

The counter 'i' is used in a number of places in conf_ports(), but in one
of those we unnecessarily shadow it in an inner scope.  We could re-use the
same 'i' every time, but each use is logically separate, so instead remove
the outer declaration and declare it locally in each of the clauses where
we need it.

While we're there change it from a signed to unsigned int, since it's used
to iterate over port numbers which are generally treated as unsigned.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 conf.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/conf.c b/conf.c
index 41afccd..2581730 100644
--- a/conf.c
+++ b/conf.c
@@ -183,7 +183,6 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
 	char buf[BUFSIZ], *spec, *p;
 	sa_family_t af = AF_UNSPEC;
 	bool exclude_only = true;
-	unsigned i;
 
 	if (!strcmp(optarg, "none")) {
 		if (fwd->mode)
@@ -200,7 +199,7 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
 	}
 
 	if (!strcmp(optarg, "all")) {
-		int i;
+		unsigned i;
 
 		if (fwd->mode || c->mode != MODE_PASST)
 			return -EINVAL;
@@ -247,6 +246,7 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
 	p = spec;
 	do {
 		struct port_range xrange;
+		unsigned i;
 
 		if (*p != '~') {
 			/* Not an exclude range, parse later */
@@ -268,6 +268,8 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
 	} while ((p = next_chunk(p, ',')));
 
 	if (exclude_only) {
+		unsigned i;
+
 		for (i = 0; i < PORT_EPHEMERAL_MIN; i++) {
 			if (bitmap_isset(exclude, i))
 				continue;
@@ -287,6 +289,7 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
 	p = spec;
 	do {
 		struct port_range orig_range, mapped_range;
+		unsigned i;
 
 		if (*p == '~')
 			/* Exclude range, already parsed */
-- 
@@ -183,7 +183,6 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
 	char buf[BUFSIZ], *spec, *p;
 	sa_family_t af = AF_UNSPEC;
 	bool exclude_only = true;
-	unsigned i;
 
 	if (!strcmp(optarg, "none")) {
 		if (fwd->mode)
@@ -200,7 +199,7 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
 	}
 
 	if (!strcmp(optarg, "all")) {
-		int i;
+		unsigned i;
 
 		if (fwd->mode || c->mode != MODE_PASST)
 			return -EINVAL;
@@ -247,6 +246,7 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
 	p = spec;
 	do {
 		struct port_range xrange;
+		unsigned i;
 
 		if (*p != '~') {
 			/* Not an exclude range, parse later */
@@ -268,6 +268,8 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
 	} while ((p = next_chunk(p, ',')));
 
 	if (exclude_only) {
+		unsigned i;
+
 		for (i = 0; i < PORT_EPHEMERAL_MIN; i++) {
 			if (bitmap_isset(exclude, i))
 				continue;
@@ -287,6 +289,7 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg,
 	p = spec;
 	do {
 		struct port_range orig_range, mapped_range;
+		unsigned i;
 
 		if (*p == '~')
 			/* Exclude range, already parsed */
-- 
2.37.3

[PATCH v2 10/28] Don't shadow global function names

[David Gibson]

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

cppcheck points out that qrap's main shadows the global err() function with
a local.  Rename it to rc to avoid the clash.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 qrap.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/qrap.c b/qrap.c
index da96b22..a7d0645 100644
--- a/qrap.c
+++ b/qrap.c
@@ -115,7 +115,7 @@ void usage(const char *name)
  */
 int main(int argc, char **argv)
 {
-	int i, s, qemu_argc = 0, addr_map = 0, has_dev = 0, retry_on_reset, err;
+	int i, s, qemu_argc = 0, addr_map = 0, has_dev = 0, retry_on_reset, rc;
 	struct timeval tv = { .tv_sec = 0, .tv_usec = (long)(500 * 1000) };
 	char *qemu_argv[ARG_MAX], dev_str[ARG_MAX];
 	struct sockaddr_un addr = {
@@ -281,13 +281,13 @@ retry:
 		errno = 0;
 
 		if (connect(s, (const struct sockaddr *)&addr, sizeof(addr))) {
-			err = errno;
+			rc = errno;
 			perror("connect");
 		} else if (send(s, &probe, sizeof(probe), 0) != sizeof(probe)) {
-			err = errno;
+			rc = errno;
 			perror("send");
 		} else if (recv(s, &probe_r, 1, MSG_PEEK) <= 0) {
-			err = errno;
+			rc = errno;
 			perror("recv");
 		} else {
 			break;
@@ -315,7 +315,7 @@ retry:
 		 * this FIXME will probably remain until the tool itself is
 		 * obsoleted.
 		 */
-		if (retry_on_reset && err == ECONNRESET) {
+		if (retry_on_reset && rc == ECONNRESET) {
 			retry_on_reset--;
 			usleep(50 * 1000);
 			goto retry;
-- 
@@ -115,7 +115,7 @@ void usage(const char *name)
  */
 int main(int argc, char **argv)
 {
-	int i, s, qemu_argc = 0, addr_map = 0, has_dev = 0, retry_on_reset, err;
+	int i, s, qemu_argc = 0, addr_map = 0, has_dev = 0, retry_on_reset, rc;
 	struct timeval tv = { .tv_sec = 0, .tv_usec = (long)(500 * 1000) };
 	char *qemu_argv[ARG_MAX], dev_str[ARG_MAX];
 	struct sockaddr_un addr = {
@@ -281,13 +281,13 @@ retry:
 		errno = 0;
 
 		if (connect(s, (const struct sockaddr *)&addr, sizeof(addr))) {
-			err = errno;
+			rc = errno;
 			perror("connect");
 		} else if (send(s, &probe, sizeof(probe), 0) != sizeof(probe)) {
-			err = errno;
+			rc = errno;
 			perror("send");
 		} else if (recv(s, &probe_r, 1, MSG_PEEK) <= 0) {
-			err = errno;
+			rc = errno;
 			perror("recv");
 		} else {
 			break;
@@ -315,7 +315,7 @@ retry:
 		 * this FIXME will probably remain until the tool itself is
 		 * obsoleted.
 		 */
-		if (retry_on_reset && err == ECONNRESET) {
+		if (retry_on_reset && rc == ECONNRESET) {
 			retry_on_reset--;
 			usleep(50 * 1000);
 			goto retry;
-- 
2.37.3

[PATCH v2 11/28] Stricter checking for nsholder.c

[David Gibson]

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

Add the -Wextra -pedantic and -std=c99 flags when compiling the nsholder
test helper to get extra compiler checks, like we already use for the
main source code.

While we're there, fix some %d (signed) printf descriptors being used
for unsigned values (uid_t and gid_t).  Pointed out by cppcheck.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 test/Makefile   | 2 +-
 test/nsholder.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/Makefile b/test/Makefile
index 08f0c2d..91498ff 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -63,7 +63,7 @@ LOCAL_ASSETS = mbuto.img QEMU_EFI.fd \
 
 ASSETS = $(DOWNLOAD_ASSETS) $(LOCAL_ASSETS)
 
-CFLAGS = -Wall -Werror
+CFLAGS = -Wall -Werror -Wextra -pedantic -std=c99
 
 assets: $(ASSETS)
 
diff --git a/test/nsholder.c b/test/nsholder.c
index aac901b..010a051 100644
--- a/test/nsholder.c
+++ b/test/nsholder.c
@@ -53,7 +53,7 @@ static void hold(int fd, const struct sockaddr_un *addr)
 	if (rc < 0)
 		die("listen(): %s\n", strerror(errno));
 
-	printf("nsholder: local PID=%d  local UID=%d  local GID=%d\n",
+	printf("nsholder: local PID=%d  local UID=%u  local GID=%u\n",
 	       getpid(), getuid(), getgid());
 	do {
 		int afd = accept(fd, NULL, NULL);
-- 
@@ -53,7 +53,7 @@ static void hold(int fd, const struct sockaddr_un *addr)
 	if (rc < 0)
 		die("listen(): %s\n", strerror(errno));
 
-	printf("nsholder: local PID=%d  local UID=%d  local GID=%d\n",
+	printf("nsholder: local PID=%d  local UID=%u  local GID=%u\n",
 	       getpid(), getuid(), getgid());
 	do {
 		int afd = accept(fd, NULL, NULL);
-- 
2.37.3

[PATCH v2 12/28] cppcheck: Work around false positive NULL pointer dereference error

[David Gibson]

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

Some versions of cppcheck could errneously report a NULL pointer deference
inside a sizeof().  This is now fixed in cppcheck upstream[0].  For systems
using an affected version, add a suppression to work around the bug.  Also
add an unmatchedSuppression suppression so the suppression itself doesn't
cause a warning if you *do* have a fixed cppcheck.

[0] https://github.com/danmar/cppcheck/pull/4471

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 tcp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tcp.c b/tcp.c
index e45dfda..b694792 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1734,6 +1734,7 @@ static int tcp_update_seqack_wnd(const struct ctx *c, struct tcp_conn *conn,
 {
 	uint32_t prev_wnd_to_tap = conn->wnd_to_tap << conn->ws_to_tap;
 	uint32_t prev_ack_to_tap = conn->seq_ack_to_tap;
+	/* cppcheck-suppress [ctunullpointer, unmatchedSuppression] */
 	socklen_t sl = sizeof(*tinfo);
 	struct tcp_info tinfo_new;
 	uint32_t new_wnd_to_tap = prev_wnd_to_tap;
-- 
@@ -1734,6 +1734,7 @@ static int tcp_update_seqack_wnd(const struct ctx *c, struct tcp_conn *conn,
 {
 	uint32_t prev_wnd_to_tap = conn->wnd_to_tap << conn->ws_to_tap;
 	uint32_t prev_ack_to_tap = conn->seq_ack_to_tap;
+	/* cppcheck-suppress [ctunullpointer, unmatchedSuppression] */
 	socklen_t sl = sizeof(*tinfo);
 	struct tcp_info tinfo_new;
 	uint32_t new_wnd_to_tap = prev_wnd_to_tap;
-- 
2.37.3

[PATCH v2 13/28] cppcheck: Use inline suppression for ffsl()

[David Gibson]

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

We define our own ffsl() as a weak symbol, in case our C library doesn't
include it.  On glibc systems which *do* include it, this causes a cppcheck
warning because unsurprisingly our version doesn't pick the same argument
names.  Convert the suppression for this into an inline suppression.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 Makefile | 2 --
 util.h   | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index e0933f3..d5bfce4 100644
--- a/Makefile
+++ b/Makefile
@@ -290,7 +290,6 @@ cppcheck: $(SRCS) $(HEADERS)
 	--suppress=strtokCalled:conf.c --suppress=strtokCalled:qrap.c	\
 	--suppress=localtimeCalled:pcap.c				\
 	--suppress=unusedStructMember:pcap.c				\
-	--suppress=funcArgNamesDifferent:util.h				\
 	--suppress=unusedStructMember:dhcp.c				\
 									\
 	--suppress=unmatchedSuppression:conf.c				\
@@ -300,6 +299,5 @@ cppcheck: $(SRCS) $(HEADERS)
 	--suppress=unmatchedSuppression:tcp.c				\
 	--suppress=unmatchedSuppression:udp.c				\
 	--suppress=unmatchedSuppression:util.c				\
-	--suppress=unmatchedSuppression:util.h				\
 	$(filter -D%,$(FLAGS) $(CFLAGS))				\
 	.
diff --git a/util.h b/util.h
index 1003303..0c3c994 100644
--- a/util.h
+++ b/util.h
@@ -217,6 +217,7 @@ struct ipv6_opt_hdr {
 	 */
 } __attribute__((packed));	/* required for some archs */
 
+/* cppcheck-suppress funcArgNamesDifferent */
 __attribute__ ((weak)) int ffsl(long int i) { return __builtin_ffsl(i); }
 void __openlog(const char *ident, int option, int facility);
 void passt_vsyslog(int pri, const char *format, va_list ap);
-- 
@@ -217,6 +217,7 @@ struct ipv6_opt_hdr {
 	 */
 } __attribute__((packed));	/* required for some archs */
 
+/* cppcheck-suppress funcArgNamesDifferent */
 __attribute__ ((weak)) int ffsl(long int i) { return __builtin_ffsl(i); }
 void __openlog(const char *ident, int option, int facility);
 void passt_vsyslog(int pri, const char *format, va_list ap);
-- 
2.37.3

[PATCH v2 14/28] cppcheck: Use inline suppressions for qrap.c

[David Gibson]

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

qrap.c uses several old-fashioned functions that cppcheck complains about.
Since it's headed for obselesence anyway, just suppress these rather than
attempting to modernize the code.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 Makefile | 3 +--
 qrap.c   | 3 +++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index d5bfce4..d465d87 100644
--- a/Makefile
+++ b/Makefile
@@ -287,7 +287,7 @@ cppcheck: $(SRCS) $(HEADERS)
 	--suppress=va_list_usedBeforeStarted:util.c			\
 	--suppress=unusedFunction					\
 	--suppress=knownConditionTrueFalse:conf.c			\
-	--suppress=strtokCalled:conf.c --suppress=strtokCalled:qrap.c	\
+	--suppress=strtokCalled:conf.c					\
 	--suppress=localtimeCalled:pcap.c				\
 	--suppress=unusedStructMember:pcap.c				\
 	--suppress=unusedStructMember:dhcp.c				\
@@ -295,7 +295,6 @@ cppcheck: $(SRCS) $(HEADERS)
 	--suppress=unmatchedSuppression:conf.c				\
 	--suppress=unmatchedSuppression:dhcp.c				\
 	--suppress=unmatchedSuppression:pcap.c				\
-	--suppress=unmatchedSuppression:qrap.c				\
 	--suppress=unmatchedSuppression:tcp.c				\
 	--suppress=unmatchedSuppression:udp.c				\
 	--suppress=unmatchedSuppression:util.c				\
diff --git a/qrap.c b/qrap.c
index a7d0645..3138386 100644
--- a/qrap.c
+++ b/qrap.c
@@ -179,12 +179,14 @@ int main(int argc, char **argv)
 			char env_path[ARG_MAX + 1], *p, command[ARG_MAX];
 
 			strncpy(env_path, getenv("PATH"), ARG_MAX);
+			/* cppcheck-suppress strtokCalled */
 			p = strtok(env_path, ":");
 			while (p) {
 				snprintf(command, ARG_MAX, "%s/%s", p, argv[2]);
 				if (!access(command, X_OK))
 					goto valid_args;
 
+				/* cppcheck-suppress strtokCalled */
 				p = strtok(NULL, ":");
 			}
 		}
@@ -317,6 +319,7 @@ retry:
 		 */
 		if (retry_on_reset && rc == ECONNRESET) {
 			retry_on_reset--;
+			/* cppcheck-suppress usleepCalled */
 			usleep(50 * 1000);
 			goto retry;
 		}
-- 
@@ -179,12 +179,14 @@ int main(int argc, char **argv)
 			char env_path[ARG_MAX + 1], *p, command[ARG_MAX];
 
 			strncpy(env_path, getenv("PATH"), ARG_MAX);
+			/* cppcheck-suppress strtokCalled */
 			p = strtok(env_path, ":");
 			while (p) {
 				snprintf(command, ARG_MAX, "%s/%s", p, argv[2]);
 				if (!access(command, X_OK))
 					goto valid_args;
 
+				/* cppcheck-suppress strtokCalled */
 				p = strtok(NULL, ":");
 			}
 		}
@@ -317,6 +319,7 @@ retry:
 		 */
 		if (retry_on_reset && rc == ECONNRESET) {
 			retry_on_reset--;
+			/* cppcheck-suppress usleepCalled */
 			usleep(50 * 1000);
 			goto retry;
 		}
-- 
2.37.3

[PATCH v2 15/28] cppcheck: Use inline suppression for strtok() in conf.c

[David Gibson]

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

strtok() is non-reentrant and old-fashioned, so cppcheck would complains
about its use in conf.c if it weren't suppressed.  We're single threaded
and strtok() is convenient though, so it's not really worth reworking at
this time.  Convert this to an inline suppression so it's adjacent to the
code its annotating.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 Makefile | 1 -
 conf.c   | 2 ++
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index d465d87..7c0b7e9 100644
--- a/Makefile
+++ b/Makefile
@@ -287,7 +287,6 @@ cppcheck: $(SRCS) $(HEADERS)
 	--suppress=va_list_usedBeforeStarted:util.c			\
 	--suppress=unusedFunction					\
 	--suppress=knownConditionTrueFalse:conf.c			\
-	--suppress=strtokCalled:conf.c					\
 	--suppress=localtimeCalled:pcap.c				\
 	--suppress=unusedStructMember:pcap.c				\
 	--suppress=unusedStructMember:dhcp.c				\
diff --git a/conf.c b/conf.c
index 2581730..7545b34 100644
--- a/conf.c
+++ b/conf.c
@@ -410,10 +410,12 @@ static void get_dns(struct ctx *c)
 			if (end)
 				*end = 0;
 
+			/* cppcheck-suppress strtokCalled */
 			if (!strtok(line, " \t"))
 				continue;
 
 			while (s - c->dns_search < ARRAY_SIZE(c->dns_search) - 1
+			       /* cppcheck-suppress strtokCalled */
 			       && (p = strtok(NULL, " \t"))) {
 				strncpy(s->n, p, sizeof(c->dns_search[0]));
 				s++;
-- 
@@ -410,10 +410,12 @@ static void get_dns(struct ctx *c)
 			if (end)
 				*end = 0;
 
+			/* cppcheck-suppress strtokCalled */
 			if (!strtok(line, " \t"))
 				continue;
 
 			while (s - c->dns_search < ARRAY_SIZE(c->dns_search) - 1
+			       /* cppcheck-suppress strtokCalled */
 			       && (p = strtok(NULL, " \t"))) {
 				strncpy(s->n, p, sizeof(c->dns_search[0]));
 				s++;
-- 
2.37.3

[PATCH v2 16/28] Avoid ugly 'end' members in netlink structures

[David Gibson]

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

We use a number of complex structures to format messages to send to
netlink.  In some cases we add imaginary 'end' members not because they
actually mean something on the wire, but so that we can use offsetof() on
the member to determine the relevant size.

Adding extra things to the structures for this is kinda nasty.  We can use
a different construct with offsetof and sizeof to avoid them.  As a bonus
this removes some cppcheck warnings about unused struct members.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 netlink.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/netlink.c b/netlink.c
index 6f7ada8..15ce213 100644
--- a/netlink.c
+++ b/netlink.c
@@ -206,7 +206,6 @@ void nl_route(int ns, unsigned int ifi, sa_family_t af, void *gw)
 				uint32_t d;
 				struct rtattr rta_gw;
 				uint32_t a;
-				uint8_t end;
 			} r4;
 		} set;
 	} req = {
@@ -234,7 +233,8 @@ void nl_route(int ns, unsigned int ifi, sa_family_t af, void *gw)
 		if (af == AF_INET6) {
 			size_t rta_len = RTA_LENGTH(sizeof(req.set.r6.d));
 
-			req.nlh.nlmsg_len = sizeof(req);
+			req.nlh.nlmsg_len = offsetof(struct req_t, set.r6)
+				+ sizeof(req.set.r6);
 
 			req.set.r6.rta_dst.rta_type = RTA_DST;
 			req.set.r6.rta_dst.rta_len = rta_len;
@@ -245,7 +245,8 @@ void nl_route(int ns, unsigned int ifi, sa_family_t af, void *gw)
 		} else {
 			size_t rta_len = RTA_LENGTH(sizeof(req.set.r4.d));
 
-			req.nlh.nlmsg_len = offsetof(struct req_t, set.r4.end);
+			req.nlh.nlmsg_len = offsetof(struct req_t, set.r4)
+				+ sizeof(req.set.r4);
 
 			req.set.r4.rta_dst.rta_type = RTA_DST;
 			req.set.r4.rta_dst.rta_len = rta_len;
@@ -312,8 +313,6 @@ void nl_addr(int ns, unsigned int ifi, sa_family_t af,
 				uint32_t l;
 				struct rtattr rta_a;
 				uint32_t a;
-
-				uint8_t end;
 			} a4;
 			struct {
 				struct rtattr rta_l;
@@ -343,7 +342,8 @@ void nl_addr(int ns, unsigned int ifi, sa_family_t af,
 		if (af == AF_INET6) {
 			size_t rta_len = RTA_LENGTH(sizeof(req.set.a6.l));
 
-			req.nlh.nlmsg_len = sizeof(req);
+			req.nlh.nlmsg_len = offsetof(struct req_t, set.a6)
+				+ sizeof(req.set.a6);
 
 			memcpy(&req.set.a6.l, addr, sizeof(req.set.a6.l));
 			req.set.a6.rta_l.rta_len = rta_len;
@@ -354,7 +354,8 @@ void nl_addr(int ns, unsigned int ifi, sa_family_t af,
 		} else {
 			size_t rta_len = RTA_LENGTH(sizeof(req.set.a4.l));
 
-			req.nlh.nlmsg_len = offsetof(struct req_t, set.a4.end);
+			req.nlh.nlmsg_len = offsetof(struct req_t, set.a4)
+				+ sizeof(req.set.a4);
 
 			req.set.a4.l = req.set.a4.a = *(uint32_t *)addr;
 			req.set.a4.rta_l.rta_len = rta_len;
@@ -425,7 +426,6 @@ void nl_link(int ns, unsigned int ifi, void *mac, int up, int mtu)
 			unsigned char mac[ETH_ALEN];
 			struct {
 				unsigned int mtu;
-				uint8_t end;
 			} mtu;
 		} set;
 	} req = {
@@ -457,7 +457,8 @@ void nl_link(int ns, unsigned int ifi, void *mac, int up, int mtu)
 	}
 
 	if (mtu) {
-		req.nlh.nlmsg_len = offsetof(struct req_t, set.mtu.end);
+		req.nlh.nlmsg_len = offsetof(struct req_t, set.mtu)
+			+ sizeof(req.set.mtu);
 		req.set.mtu.mtu = mtu;
 		req.rta.rta_type = IFLA_MTU;
 		req.rta.rta_len = RTA_LENGTH(sizeof(unsigned int));
-- 
@@ -206,7 +206,6 @@ void nl_route(int ns, unsigned int ifi, sa_family_t af, void *gw)
 				uint32_t d;
 				struct rtattr rta_gw;
 				uint32_t a;
-				uint8_t end;
 			} r4;
 		} set;
 	} req = {
@@ -234,7 +233,8 @@ void nl_route(int ns, unsigned int ifi, sa_family_t af, void *gw)
 		if (af == AF_INET6) {
 			size_t rta_len = RTA_LENGTH(sizeof(req.set.r6.d));
 
-			req.nlh.nlmsg_len = sizeof(req);
+			req.nlh.nlmsg_len = offsetof(struct req_t, set.r6)
+				+ sizeof(req.set.r6);
 
 			req.set.r6.rta_dst.rta_type = RTA_DST;
 			req.set.r6.rta_dst.rta_len = rta_len;
@@ -245,7 +245,8 @@ void nl_route(int ns, unsigned int ifi, sa_family_t af, void *gw)
 		} else {
 			size_t rta_len = RTA_LENGTH(sizeof(req.set.r4.d));
 
-			req.nlh.nlmsg_len = offsetof(struct req_t, set.r4.end);
+			req.nlh.nlmsg_len = offsetof(struct req_t, set.r4)
+				+ sizeof(req.set.r4);
 
 			req.set.r4.rta_dst.rta_type = RTA_DST;
 			req.set.r4.rta_dst.rta_len = rta_len;
@@ -312,8 +313,6 @@ void nl_addr(int ns, unsigned int ifi, sa_family_t af,
 				uint32_t l;
 				struct rtattr rta_a;
 				uint32_t a;
-
-				uint8_t end;
 			} a4;
 			struct {
 				struct rtattr rta_l;
@@ -343,7 +342,8 @@ void nl_addr(int ns, unsigned int ifi, sa_family_t af,
 		if (af == AF_INET6) {
 			size_t rta_len = RTA_LENGTH(sizeof(req.set.a6.l));
 
-			req.nlh.nlmsg_len = sizeof(req);
+			req.nlh.nlmsg_len = offsetof(struct req_t, set.a6)
+				+ sizeof(req.set.a6);
 
 			memcpy(&req.set.a6.l, addr, sizeof(req.set.a6.l));
 			req.set.a6.rta_l.rta_len = rta_len;
@@ -354,7 +354,8 @@ void nl_addr(int ns, unsigned int ifi, sa_family_t af,
 		} else {
 			size_t rta_len = RTA_LENGTH(sizeof(req.set.a4.l));
 
-			req.nlh.nlmsg_len = offsetof(struct req_t, set.a4.end);
+			req.nlh.nlmsg_len = offsetof(struct req_t, set.a4)
+				+ sizeof(req.set.a4);
 
 			req.set.a4.l = req.set.a4.a = *(uint32_t *)addr;
 			req.set.a4.rta_l.rta_len = rta_len;
@@ -425,7 +426,6 @@ void nl_link(int ns, unsigned int ifi, void *mac, int up, int mtu)
 			unsigned char mac[ETH_ALEN];
 			struct {
 				unsigned int mtu;
-				uint8_t end;
 			} mtu;
 		} set;
 	} req = {
@@ -457,7 +457,8 @@ void nl_link(int ns, unsigned int ifi, void *mac, int up, int mtu)
 	}
 
 	if (mtu) {
-		req.nlh.nlmsg_len = offsetof(struct req_t, set.mtu.end);
+		req.nlh.nlmsg_len = offsetof(struct req_t, set.mtu)
+			+ sizeof(req.set.mtu);
 		req.set.mtu.mtu = mtu;
 		req.rta.rta_type = IFLA_MTU;
 		req.rta.rta_len = RTA_LENGTH(sizeof(unsigned int));
-- 
2.37.3

[PATCH v2 17/28] cppcheck: Broaden suppression for unused struct members

[David Gibson]

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

In a number of places in passt we use structures to represent over the wire
or in-file data with a fixed layout.  After initialization we don't access
the fields individually and just write the structure as a whole to its
destination.

Unfortunately cppcheck doesn't cope with this pattern and thinks all the
structure members are unused.  We already have suppressions for this in
pcap.c and dhcp.c   However, it also appears in dhcp.c and netlink.c at
least.  Since this is likely to be common, it seems wiser to just suppress
the error globally.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 Makefile | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 7c0b7e9..e3ca17f 100644
--- a/Makefile
+++ b/Makefile
@@ -286,10 +286,9 @@ cppcheck: $(SRCS) $(HEADERS)
 	--suppress=objectIndex:tcp.c --suppress=objectIndex:udp.c	\
 	--suppress=va_list_usedBeforeStarted:util.c			\
 	--suppress=unusedFunction					\
+	--suppress=unusedStructMember					\
 	--suppress=knownConditionTrueFalse:conf.c			\
 	--suppress=localtimeCalled:pcap.c				\
-	--suppress=unusedStructMember:pcap.c				\
-	--suppress=unusedStructMember:dhcp.c				\
 									\
 	--suppress=unmatchedSuppression:conf.c				\
 	--suppress=unmatchedSuppression:dhcp.c				\
-- 
@@ -286,10 +286,9 @@ cppcheck: $(SRCS) $(HEADERS)
 	--suppress=objectIndex:tcp.c --suppress=objectIndex:udp.c	\
 	--suppress=va_list_usedBeforeStarted:util.c			\
 	--suppress=unusedFunction					\
+	--suppress=unusedStructMember					\
 	--suppress=knownConditionTrueFalse:conf.c			\
 	--suppress=localtimeCalled:pcap.c				\
-	--suppress=unusedStructMember:pcap.c				\
-	--suppress=unusedStructMember:dhcp.c				\
 									\
 	--suppress=unmatchedSuppression:conf.c				\
 	--suppress=unmatchedSuppression:dhcp.c				\
-- 
2.37.3

[PATCH v2 18/28] cppcheck: Remove localtime suppression for pcap.c

[David Gibson]

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

Since bf95322f "conf: Make the argument to --pcap option mandatory" we
no longer call localtime() in pcap.c, so we no longer need the matching
cppcheck suppression.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 Makefile | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Makefile b/Makefile
index e3ca17f..deee529 100644
--- a/Makefile
+++ b/Makefile
@@ -288,7 +288,6 @@ cppcheck: $(SRCS) $(HEADERS)
 	--suppress=unusedFunction					\
 	--suppress=unusedStructMember					\
 	--suppress=knownConditionTrueFalse:conf.c			\
-	--suppress=localtimeCalled:pcap.c				\
 									\
 	--suppress=unmatchedSuppression:conf.c				\
 	--suppress=unmatchedSuppression:dhcp.c				\
-- 
@@ -288,7 +288,6 @@ cppcheck: $(SRCS) $(HEADERS)
 	--suppress=unusedFunction					\
 	--suppress=unusedStructMember					\
 	--suppress=knownConditionTrueFalse:conf.c			\
-	--suppress=localtimeCalled:pcap.c				\
 									\
 	--suppress=unmatchedSuppression:conf.c				\
 	--suppress=unmatchedSuppression:dhcp.c				\
-- 
2.37.3

[PATCH v2 19/28] qrap: Handle case of PATH environment variable being unset

[David Gibson]

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

clang-tidy warns that in passing getenv("PATH") to strncpy() we could be
passing a NULL pointer.  While it's unusual for PATH to be unset, it's not
impossible and this would indeed cause getenv() to return NULL.

Handle this case by never recognizing argv[2] as a qemu binary name if
PATH is not set.  This is... no flakier than the detection of whether
it's a binary name already is.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 qrap.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/qrap.c b/qrap.c
index 3138386..a9a0fc1 100644
--- a/qrap.c
+++ b/qrap.c
@@ -173,12 +173,13 @@ int main(int argc, char **argv)
 	char probe_r;
 
 	if (argc >= 3) {
+		const char *path = getenv("PATH");
 		errno = 0;
 		fd = strtol(argv[1], NULL, 0);
-		if (fd >= 3 && fd < INT_MAX && !errno) {
+		if (fd >= 3 && fd < INT_MAX && !errno && path) {
 			char env_path[ARG_MAX + 1], *p, command[ARG_MAX];
 
-			strncpy(env_path, getenv("PATH"), ARG_MAX);
+			strncpy(env_path, path, ARG_MAX);
 			/* cppcheck-suppress strtokCalled */
 			p = strtok(env_path, ":");
 			while (p) {
-- 
@@ -173,12 +173,13 @@ int main(int argc, char **argv)
 	char probe_r;
 
 	if (argc >= 3) {
+		const char *path = getenv("PATH");
 		errno = 0;
 		fd = strtol(argv[1], NULL, 0);
-		if (fd >= 3 && fd < INT_MAX && !errno) {
+		if (fd >= 3 && fd < INT_MAX && !errno && path) {
 			char env_path[ARG_MAX + 1], *p, command[ARG_MAX];
 
-			strncpy(env_path, getenv("PATH"), ARG_MAX);
+			strncpy(env_path, path, ARG_MAX);
 			/* cppcheck-suppress strtokCalled */
 			p = strtok(env_path, ":");
 			while (p) {
-- 
2.37.3

[PATCH v2 20/28] cppcheck: Suppress same-value-in-ternary branches warning

[David Gibson]

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

TIMER_INTERVAL is the minimum of two separately defined intervals which
happen to have the same value at present.  This results in an expression
which has the same value in both branches of a ternary operator, which
cppcheck warngs about.  This is logically sound in this case, so suppress
the error (we appear to already have a similar suppression for clang-tidy).

Also add an unmatchedSuppression suppression, since only some cppcheck
versions complain about this instance.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 passt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/passt.c b/passt.c
index 4796c89..2c4a986 100644
--- a/passt.c
+++ b/passt.c
@@ -305,6 +305,7 @@ int main(int argc, char **argv)
 
 loop:
 	/* NOLINTNEXTLINE(bugprone-branch-clone): intervals can be the same */
+	/* cppcheck-suppress [duplicateValueTernary, unmatchedSuppression] */
 	nfds = epoll_wait(c.epollfd, events, EPOLL_EVENTS, TIMER_INTERVAL);
 	if (nfds == -1 && errno != EINTR) {
 		perror("epoll_wait");
-- 
@@ -305,6 +305,7 @@ int main(int argc, char **argv)
 
 loop:
 	/* NOLINTNEXTLINE(bugprone-branch-clone): intervals can be the same */
+	/* cppcheck-suppress [duplicateValueTernary, unmatchedSuppression] */
 	nfds = epoll_wait(c.epollfd, events, EPOLL_EVENTS, TIMER_INTERVAL);
 	if (nfds == -1 && errno != EINTR) {
 		perror("epoll_wait");
-- 
2.37.3

[PATCH v2 21/28] cppcheck: Suppress NULL pointer warning in tcp_sock_consume()

[David Gibson]

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

Recent versions of cppcheck give a warning due to the NULL buffer passed
to recv() in tcp_sock_consume().  In fact this is safe, because we're using
MSG_TRUNC which means the kernel will ignore the buffer.  Use a suppression
to get rid of the error. Also add an unmatchedSuppression suppression since
only some cppcheck versions complain about this.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 tcp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tcp.c b/tcp.c
index b694792..d223125 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2280,6 +2280,10 @@ static int tcp_sock_consume(struct tcp_conn *conn, uint32_t ack_seq)
 	if (SEQ_LE(ack_seq, conn->seq_ack_from_tap))
 		return 0;
 
+	/* cppcheck doesn't know about MSG_TRUNC which means the
+	 * kernel doesn't care that the buffer is NULL
+	 */
+	/* cppcheck-suppress [nullPointer, unmatchedSuppression] */
 	if (recv(conn->sock, NULL, ack_seq - conn->seq_ack_from_tap,
 		 MSG_DONTWAIT | MSG_TRUNC) < 0)
 		return -errno;
-- 
@@ -2280,6 +2280,10 @@ static int tcp_sock_consume(struct tcp_conn *conn, uint32_t ack_seq)
 	if (SEQ_LE(ack_seq, conn->seq_ack_from_tap))
 		return 0;
 
+	/* cppcheck doesn't know about MSG_TRUNC which means the
+	 * kernel doesn't care that the buffer is NULL
+	 */
+	/* cppcheck-suppress [nullPointer, unmatchedSuppression] */
 	if (recv(conn->sock, NULL, ack_seq - conn->seq_ack_from_tap,
 		 MSG_DONTWAIT | MSG_TRUNC) < 0)
 		return -errno;
-- 
2.37.3

[PATCH v2 22/28] Regenerate seccomp.h if seccomp.sh changes

[David Gibson]

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

seccomp.sh generates seccomp.h, so if we change it, we should re-build
seccomp.h as well.  Add this to the make dependencies so it happens.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index deee529..1aa7b6f 100644
--- a/Makefile
+++ b/Makefile
@@ -102,8 +102,8 @@ all: $(BIN) $(MANPAGES) docs
 static: FLAGS += -static -DGLIBC_NO_STATIC_NSS
 static: clean all
 
-seccomp.h: $(PASST_SRCS) $(PASST_HEADERS)
-	@ EXTRA_SYSCALLS=$(EXTRA_SYSCALLS) ./seccomp.sh $^
+seccomp.h: seccomp.sh $(PASST_SRCS) $(PASST_HEADERS)
+	@ EXTRA_SYSCALLS=$(EXTRA_SYSCALLS) ./seccomp.sh $(PASST_SRCS) $(PASST_HEADERS)
 
 passt: $(PASST_SRCS) $(HEADERS)
 	$(CC) $(FLAGS) $(CFLAGS) $(PASST_SRCS) -o passt $(LDFLAGS)
-- 
@@ -102,8 +102,8 @@ all: $(BIN) $(MANPAGES) docs
 static: FLAGS += -static -DGLIBC_NO_STATIC_NSS
 static: clean all
 
-seccomp.h: $(PASST_SRCS) $(PASST_HEADERS)
-	@ EXTRA_SYSCALLS=$(EXTRA_SYSCALLS) ./seccomp.sh $^
+seccomp.h: seccomp.sh $(PASST_SRCS) $(PASST_HEADERS)
+	@ EXTRA_SYSCALLS=$(EXTRA_SYSCALLS) ./seccomp.sh $(PASST_SRCS) $(PASST_HEADERS)
 
 passt: $(PASST_SRCS) $(HEADERS)
 	$(CC) $(FLAGS) $(CFLAGS) $(PASST_SRCS) -o passt $(LDFLAGS)
-- 
2.37.3

[PATCH v2 23/28] cppcheck: Avoid errors due to zeroes in bitwise ORs

[David Gibson]

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

Recent versions of cppcheck give warnings if using a bitwise OR (|) where
some of the arguments are zero.  We're triggering these warnings in our
generated seccomp.h header, because BPF_LD and BPF_W are zero-valued.
However putting these defines in makes the generate code clearer, even
though they could be left out without changing the values.  So, add
cppcheck suppressions to the generated code.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 seccomp.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/seccomp.sh b/seccomp.sh
index 17def4d..31ea8da 100755
--- a/seccomp.sh
+++ b/seccomp.sh
@@ -26,9 +26,11 @@ HEADER="/* This file was automatically generated by $(basename ${0}) */
 # Prefix for each profile: check that 'arch' in seccomp_data is matching
 PRE='
 struct sock_filter filter_(a)PROFILE@[] = {
+	/* cppcheck-suppress badBitmaskCheck */
 	BPF_STMT(BPF_LD | BPF_W | BPF_ABS,
 		 (offsetof(struct seccomp_data, arch))),
 	BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, PASST_AUDIT_ARCH, 0, @KILL@),
+	/* cppcheck-suppress badBitmaskCheck */
 	BPF_STMT(BPF_LD | BPF_W | BPF_ABS,
 		 (offsetof(struct seccomp_data, nr))),
 
-- 
@@ -26,9 +26,11 @@ HEADER="/* This file was automatically generated by $(basename ${0}) */
 # Prefix for each profile: check that 'arch' in seccomp_data is matching
 PRE='
 struct sock_filter filter_(a)PROFILE@[] = {
+	/* cppcheck-suppress badBitmaskCheck */
 	BPF_STMT(BPF_LD | BPF_W | BPF_ABS,
 		 (offsetof(struct seccomp_data, arch))),
 	BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, PASST_AUDIT_ARCH, 0, @KILL@),
+	/* cppcheck-suppress badBitmaskCheck */
 	BPF_STMT(BPF_LD | BPF_W | BPF_ABS,
 		 (offsetof(struct seccomp_data, nr))),
 
-- 
2.37.3

[PATCH v2 24/28] cppcheck: Remove unused knownConditionTrueFalse suppression

[David Gibson]

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

I can't get this warning to trigger, so I think this suppression must be
out of date.  Whether that's because we've changed our code to no longer
have the problem, or because cppcheck itself has been updated to remove a
false positive I don't know.

If we find that we do need a suppression like this for some cppcheck
version, we should replace it with an inline suppression so it's clear
what exactly is triggering the warning.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 Makefile | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Makefile b/Makefile
index 1aa7b6f..15be57c 100644
--- a/Makefile
+++ b/Makefile
@@ -287,9 +287,7 @@ cppcheck: $(SRCS) $(HEADERS)
 	--suppress=va_list_usedBeforeStarted:util.c			\
 	--suppress=unusedFunction					\
 	--suppress=unusedStructMember					\
-	--suppress=knownConditionTrueFalse:conf.c			\
 									\
-	--suppress=unmatchedSuppression:conf.c				\
 	--suppress=unmatchedSuppression:dhcp.c				\
 	--suppress=unmatchedSuppression:pcap.c				\
 	--suppress=unmatchedSuppression:tcp.c				\
-- 
@@ -287,9 +287,7 @@ cppcheck: $(SRCS) $(HEADERS)
 	--suppress=va_list_usedBeforeStarted:util.c			\
 	--suppress=unusedFunction					\
 	--suppress=unusedStructMember					\
-	--suppress=knownConditionTrueFalse:conf.c			\
 									\
-	--suppress=unmatchedSuppression:conf.c				\
 	--suppress=unmatchedSuppression:dhcp.c				\
 	--suppress=unmatchedSuppression:pcap.c				\
 	--suppress=unmatchedSuppression:tcp.c				\
-- 
2.37.3

[PATCH v2 25/28] cppcheck: Remove unused objectIndex suppressions

[David Gibson]

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

These warnings no longer on cppcheck.  The one in tcp.c was removed by
38fbfdbcb916 ("tcp: Get rid of iov with cached MSS, drop sendmmsg(),
add deferred flush"), I'm not sure exactly where the udp.c one was
fixed (or if it was an unneeded suppression in the first place), but
it no longer seems to be needed in any case.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 Makefile | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/Makefile b/Makefile
index 15be57c..6df559e 100644
--- a/Makefile
+++ b/Makefile
@@ -283,15 +283,12 @@ cppcheck: $(SRCS) $(HEADERS)
 	$(SYSTEM_INCLUDES:%=--suppress=*:%/*)				\
 	$(SYSTEM_INCLUDES:%=--suppress=unmatchedSuppression:%/*)	\
 	--inline-suppr							\
-	--suppress=objectIndex:tcp.c --suppress=objectIndex:udp.c	\
 	--suppress=va_list_usedBeforeStarted:util.c			\
 	--suppress=unusedFunction					\
 	--suppress=unusedStructMember					\
 									\
 	--suppress=unmatchedSuppression:dhcp.c				\
 	--suppress=unmatchedSuppression:pcap.c				\
-	--suppress=unmatchedSuppression:tcp.c				\
-	--suppress=unmatchedSuppression:udp.c				\
 	--suppress=unmatchedSuppression:util.c				\
 	$(filter -D%,$(FLAGS) $(CFLAGS))				\
 	.
-- 
@@ -283,15 +283,12 @@ cppcheck: $(SRCS) $(HEADERS)
 	$(SYSTEM_INCLUDES:%=--suppress=*:%/*)				\
 	$(SYSTEM_INCLUDES:%=--suppress=unmatchedSuppression:%/*)	\
 	--inline-suppr							\
-	--suppress=objectIndex:tcp.c --suppress=objectIndex:udp.c	\
 	--suppress=va_list_usedBeforeStarted:util.c			\
 	--suppress=unusedFunction					\
 	--suppress=unusedStructMember					\
 									\
 	--suppress=unmatchedSuppression:dhcp.c				\
 	--suppress=unmatchedSuppression:pcap.c				\
-	--suppress=unmatchedSuppression:tcp.c				\
-	--suppress=unmatchedSuppression:udp.c				\
 	--suppress=unmatchedSuppression:util.c				\
 	$(filter -D%,$(FLAGS) $(CFLAGS))				\
 	.
-- 
2.37.3

[PATCH v2 26/28] cppcheck: Remove unused va_list_usedBeforeStarted suppression

[David Gibson]

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

I can't get this warning to trigger, even without the suppression, so
remove it.  If it shows up again on some cppcheck version, we can replace
it with inline suppressions so it's clear where the issue lay.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 Makefile | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Makefile b/Makefile
index 6df559e..7b3f8e1 100644
--- a/Makefile
+++ b/Makefile
@@ -283,12 +283,10 @@ cppcheck: $(SRCS) $(HEADERS)
 	$(SYSTEM_INCLUDES:%=--suppress=*:%/*)				\
 	$(SYSTEM_INCLUDES:%=--suppress=unmatchedSuppression:%/*)	\
 	--inline-suppr							\
-	--suppress=va_list_usedBeforeStarted:util.c			\
 	--suppress=unusedFunction					\
 	--suppress=unusedStructMember					\
 									\
 	--suppress=unmatchedSuppression:dhcp.c				\
 	--suppress=unmatchedSuppression:pcap.c				\
-	--suppress=unmatchedSuppression:util.c				\
 	$(filter -D%,$(FLAGS) $(CFLAGS))				\
 	.
-- 
@@ -283,12 +283,10 @@ cppcheck: $(SRCS) $(HEADERS)
 	$(SYSTEM_INCLUDES:%=--suppress=*:%/*)				\
 	$(SYSTEM_INCLUDES:%=--suppress=unmatchedSuppression:%/*)	\
 	--inline-suppr							\
-	--suppress=va_list_usedBeforeStarted:util.c			\
 	--suppress=unusedFunction					\
 	--suppress=unusedStructMember					\
 									\
 	--suppress=unmatchedSuppression:dhcp.c				\
 	--suppress=unmatchedSuppression:pcap.c				\
-	--suppress=unmatchedSuppression:util.c				\
 	$(filter -D%,$(FLAGS) $(CFLAGS))				\
 	.
-- 
2.37.3

[PATCH v2 27/28] Mark unused functions for cppcheck

[David Gibson]

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

We have a couple of functions that are unused (for now) by design.
Although at least one has a flag so that gcc doesn't warn, cppcheck has its
own warnings about this.  Add specific inline suppressions for these rather
than a blanket suppression in the Makefile.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 Makefile  | 1 -
 igmp.c    | 1 +
 siphash.c | 1 +
 3 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 7b3f8e1..bb349c7 100644
--- a/Makefile
+++ b/Makefile
@@ -283,7 +283,6 @@ cppcheck: $(SRCS) $(HEADERS)
 	$(SYSTEM_INCLUDES:%=--suppress=*:%/*)				\
 	$(SYSTEM_INCLUDES:%=--suppress=unmatchedSuppression:%/*)	\
 	--inline-suppr							\
-	--suppress=unusedFunction					\
 	--suppress=unusedStructMember					\
 									\
 	--suppress=unmatchedSuppression:dhcp.c				\
diff --git a/igmp.c b/igmp.c
index 2f3a9d1..da7e83d 100644
--- a/igmp.c
+++ b/igmp.c
@@ -13,4 +13,5 @@
  */
 
 /* TO BE IMPLEMENTED */
+/* cppcheck-suppress unusedFunction */
 __attribute__((__unused__)) static void unused(void) { }
diff --git a/siphash.c b/siphash.c
index ec38848..37a6d73 100644
--- a/siphash.c
+++ b/siphash.c
@@ -177,6 +177,7 @@ uint64_t siphash_20b(const uint8_t *in, const uint64_t *k)
  *
  * Return: the 64-bit hash output
  */
+/* cppcheck-suppress unusedFunction */
 uint32_t siphash_32b(const uint8_t *in, const uint64_t *k)
 {
 	uint64_t *in64 = (uint64_t *)in;
-- 
@@ -177,6 +177,7 @@ uint64_t siphash_20b(const uint8_t *in, const uint64_t *k)
  *
  * Return: the 64-bit hash output
  */
+/* cppcheck-suppress unusedFunction */
 uint32_t siphash_32b(const uint8_t *in, const uint64_t *k)
 {
 	uint64_t *in64 = (uint64_t *)in;
-- 
2.37.3

[PATCH v2 28/28] cppcheck: Remove unused unmatchedSuppression suppressions

[David Gibson]

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

It's unclear what original suppressions these unmatchedSuppression
suppressions were supposed to go with.  They don't trigger any warnings on
the current code that I can tell, so remove them.  If we find a problem
with some cppcheck versions in future, replace them with inline
suppressions so it's clearer exactly where the issue is originating.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 Makefile | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/Makefile b/Makefile
index bb349c7..7a0b829 100644
--- a/Makefile
+++ b/Makefile
@@ -284,8 +284,5 @@ cppcheck: $(SRCS) $(HEADERS)
 	$(SYSTEM_INCLUDES:%=--suppress=unmatchedSuppression:%/*)	\
 	--inline-suppr							\
 	--suppress=unusedStructMember					\
-									\
-	--suppress=unmatchedSuppression:dhcp.c				\
-	--suppress=unmatchedSuppression:pcap.c				\
 	$(filter -D%,$(FLAGS) $(CFLAGS))				\
 	.
-- 
@@ -284,8 +284,5 @@ cppcheck: $(SRCS) $(HEADERS)
 	$(SYSTEM_INCLUDES:%=--suppress=unmatchedSuppression:%/*)	\
 	--inline-suppr							\
 	--suppress=unusedStructMember					\
-									\
-	--suppress=unmatchedSuppression:dhcp.c				\
-	--suppress=unmatchedSuppression:pcap.c				\
 	$(filter -D%,$(FLAGS) $(CFLAGS))				\
 	.
-- 
2.37.3

Re: [PATCH v2 00/28] Fixes for static checkers

[Stefano Brivio]

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

On Thu, 29 Sep 2022 13:38:04 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> The passt tests include two static checking tools: clang-tidy and
> cppcheck.  However, newer versions of those tools have introduced
> extra checks, and may cause these tests to fail.
> 
> This series fixes all the clang-tidy and cppcheck warnings, either by
> altering our code, or by suppressing them with relevant options to the
> checkers.  With this series, the checks are now clean on both my
> Fedora 36 machine (clang-tools-extra-14.0.5-1.fc36.x86_64 and
> cppcheck-2.7.4-2.fc36.x86_64) and my Debian machine (bookworm with
> some pieces from sid: clang-tidy 1:14.0-55.1 and cppcheck 2.9-1).
> 
> Changes since v1:
>  * Fixed a whitespace error
>  * Added extra background information and details to comments and
>    commit messages when removing old suppressions
>  * Improved conf_runas() rework to give better error messages

Applied, together with your previous batch (#7, v2) of test fixes.

-- 
Stefano