[PATCH 00/28] Fixes for static checkers

[PATCH 00/28] Fixes for static checkers

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2616 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).

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          | 344 ++++++++++++++++++++++--------------------------
 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           |   2 +
 test/Makefile   |   2 +-
 test/nsholder.c |   2 +-
 util.c          |   2 +-
 util.h          |   1 +
 18 files changed, 222 insertions(+), 249 deletions(-)

-- 
2.37.3

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 8675 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..dbc8864 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 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 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 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 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 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 07/28] Clean up parsing in conf_runas()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6002 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   | 95 +++++++++++++++++++++++++++++---------------------------
 2 files changed, 51 insertions(+), 47 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 dbc8864..c668eea 100644
--- a/conf.c
+++ b/conf.c
@@ -859,46 +859,50 @@ 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)) || !(*uid = pw->pw_uid))
 			return -ENOENT;
+		*gid = pw->pw_gid;
+#else
+		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)))
+			return -ENOENT;
+		*gid = gr->gr_gid;
+#else
+		return -EINVAL;
+#endif
+	}
 
 	return 0;
-#endif /* !GLIBC_NO_STATIC_NSS */
 }
 
 /**
@@ -909,10 +913,9 @@ 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;
@@ -951,21 +954,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 +1037,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,50 @@ 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)) || !(*uid = pw->pw_uid))
 			return -ENOENT;
+		*gid = pw->pw_gid;
+#else
+		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)))
+			return -ENOENT;
+		*gid = gr->gr_gid;
+#else
+		return -EINVAL;
+#endif
+	}
 
 	return 0;
-#endif /* !GLIBC_NO_STATIC_NSS */
 }
 
 /**
@@ -909,10 +913,9 @@ 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;
@@ -951,21 +954,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 +1037,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 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 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 c668eea..3fbd308 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 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 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 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 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 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 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 3fbd308..1537dbf 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 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 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 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 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 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 21/28] cppcheck: Suppress NULL pointer warning in tcp_sock_consume()

[David Gibson]

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

Recent versions of cppcheck give a warning due to the NULL buffer passed
to recv() in tcp_sock_consume().  Since this apparently works, I assume
it's actually valid, but cppcheck doesn't know that recv() can take a NULL
buffer.  So, 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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tcp.c b/tcp.c
index b694792..8ec991c 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2280,6 +2280,7 @@ 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-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,7 @@ 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-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 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 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 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 25/28] cppcheck: Remove unused objectIndex suppressions

[David Gibson]

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

I can't get these warnings to trigger on the cppcheck versions I have, so
remove them.  If we find in future we need to replace these, they should be
replaced with inline suppressions so its clear what's the section of code
at issue.

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 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 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 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 01/28] Clean up parsing of port ranges

[Stefano Brivio]

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

On Wed, 28 Sep 2022 14:33:12 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> 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,

Indeed. Just one excess whitespace:

> [...]
> +		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 */

...here. I can drop it myself.

-- 
Stefano

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

[Stefano Brivio]

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

On Wed, 28 Sep 2022 14:33:18 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> 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.

Ah, actually, I had noticed that: I usually type my UID as number, but
I can't remember GIDs... then I had half a mind of "fixing" that, and
never got to it.

> [...]
>
> +++ b/conf.c
> @@ -859,46 +859,50 @@ 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)

While I like your approach better than the existing one, I see one
detail that, albeit minor, might drive somebody mad: if the UID is
valid, but the GID isn't, we go back to conf_ugid():

		if (ret)
			err("Invalid --runas option: %s", runas);

and print the UID part, only, because we cut before the separator.

Perhaps we could simply put the separator back before returning
-EINVAL (instead of copying the buffer) -- we just have zero or one
occurrences of ':'.

-- 
Stefano

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

[Stefano Brivio]

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

On Wed, 28 Sep 2022 14:33:31 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> 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] */

Somewhat surprisingly to me, NOLINTNEXTLINE means "next line of code",
not next line altogether -- given that this still works (I wouldn't
have even tried).

-- 
Stefano

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

[Stefano Brivio]

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

On Wed, 28 Sep 2022 14:33:35 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> 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.

The former: you got rid of that case with the rework of the PID parsing.

-- 
Stefano

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

[Stefano Brivio]

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

On Wed, 28 Sep 2022 14:33:36 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> I can't get these warnings to trigger on the cppcheck versions I have, so
> remove them.  If we find in future we need to replace these, they should be
> replaced with inline suppressions so its clear what's the section of code
> at issue.

At least for TCP:

-	/* First buffer is to discard data, last one may be partially filled. */
-	iov[-1].iov_len = already_sent;

gone with commit 38fbfdbcb916 ("tcp: Get rid of iov with cached MSS, drop
sendmmsg(), add deferred flush").

-- 
Stefano

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

[Stefano Brivio]

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

On Wed, 28 Sep 2022 14:33:32 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> Recent versions of cppcheck give a warning due to the NULL buffer passed
> to recv() in tcp_sock_consume().  Since this apparently works, I assume
> it's actually valid,

Yes, given that we use MSG_TRUNC to discard socket buffers, I thought
it's cleaner to avoid supplying a data buffer altogether.

POSIX doesn't specify MSG_TRUNC, and whether the buffer can be NULL
isn't specified in Linux documentation, but it works reliably (the
kernel won't even look at it).

This was actually the first step of a long-overdue plan: if you observe
this through perf(1), you'll see some overhead in the kernel, which
looks a bit more than a reasonable expectation for pure syscall
overhead.

So, I planned to check if we can simplify the kernel path if no buffer
is passed. I'll track this somewhere next week.

Even the day we add a vhost-user back-end, this overhead is still going
to be there, as that wouldn't change anything host-side.

> but cppcheck doesn't know that recv() can take a NULL
> buffer.  So, use a suppression to get rid of the error.

valgrind didn't know either, by the way -- see the corresponding
suppression in test/valgrind.supp.

-- 
Stefano

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

[David Gibson]

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

On Wed, Sep 28, 2022 at 10:58:06PM +0200, Stefano Brivio wrote:
> On Wed, 28 Sep 2022 14:33:31 +1000
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
> 
> > 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] */
> 
> Somewhat surprisingly to me, NOLINTNEXTLINE means "next line of code",
> not next line altogether -- given that this still works (I wouldn't
> have even tried).

Yeah, I wondered about that and came to the same conclusion.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

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

On Wed, Sep 28, 2022 at 10:57:31PM +0200, Stefano Brivio wrote:
> On Wed, 28 Sep 2022 14:33:12 +1000
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
> 
> > 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,
> 
> Indeed. Just one excess whitespace:
> 
> > [...]
> > +		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 */
> 
> ...here. I can drop it myself.

Actually, I might as well respin it quickly.  That way I can add some
of the extra background information you've supplied to the commit
messages, which might be useful for posterity.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

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

On Wed, Sep 28, 2022 at 10:58:37PM +0200, Stefano Brivio wrote:
> On Wed, 28 Sep 2022 14:33:32 +1000
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
> 
> > Recent versions of cppcheck give a warning due to the NULL buffer passed
> > to recv() in tcp_sock_consume().  Since this apparently works, I assume
> > it's actually valid,
> 
> Yes, given that we use MSG_TRUNC to discard socket buffers, I thought
> it's cleaner to avoid supplying a data buffer altogether.
> 
> POSIX doesn't specify MSG_TRUNC, and whether the buffer can be NULL
> isn't specified in Linux documentation, but it works reliably (the
> kernel won't even look at it).

Ah, interesting.  I've update the comment and commit message with this
detail.

> This was actually the first step of a long-overdue plan: if you observe
> this through perf(1), you'll see some overhead in the kernel, which
> looks a bit more than a reasonable expectation for pure syscall
> overhead.
> 
> So, I planned to check if we can simplify the kernel path if no buffer
> is passed. I'll track this somewhere next week.
> 
> Even the day we add a vhost-user back-end, this overhead is still going
> to be there, as that wouldn't change anything host-side.
> 
> > but cppcheck doesn't know that recv() can take a NULL
> > buffer.  So, use a suppression to get rid of the error.
> 
> valgrind didn't know either, by the way -- see the corresponding
> suppression in test/valgrind.supp.
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

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

On Wed, Sep 28, 2022 at 10:58:29PM +0200, Stefano Brivio wrote:
11;rgb:ffff/ffff/ffff> On Wed, 28 Sep 2022 14:33:36 +1000
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
> 
> > I can't get these warnings to trigger on the cppcheck versions I have, so
> > remove them.  If we find in future we need to replace these, they should be
> > replaced with inline suppressions so its clear what's the section of code
> > at issue.
> 
> At least for TCP:
> 
> -	/* First buffer is to discard data, last one may be partially filled. */
> -	iov[-1].iov_len = already_sent;
> 
> gone with commit 38fbfdbcb916 ("tcp: Get rid of iov with cached MSS, drop
> sendmmsg(), add deferred flush").

Ah, thanks, I've added that background to the commit message.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

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

On Wed, Sep 28, 2022 at 10:58:25PM +0200, Stefano Brivio wrote:
> On Wed, 28 Sep 2022 14:33:35 +1000
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
> 
> > 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.
> 
> The former: you got rid of that case with the rework of the PID parsing.

Hmm... I couldn't manage to trip the warning even if I went back
before that rework.  I did only try cppcheck-2.7 though.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

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

On Wed, Sep 28, 2022 at 10:57:55PM +0200, Stefano Brivio wrote:
> On Wed, 28 Sep 2022 14:33:18 +1000
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
> 
> > 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.
> 
> Ah, actually, I had noticed that: I usually type my UID as number, but
> I can't remember GIDs... then I had half a mind of "fixing" that, and
> never got to it.
> 
> > [...]
> >
> > +++ b/conf.c
> > @@ -859,46 +859,50 @@ 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)
> 
> While I like your approach better than the existing one, I see one
> detail that, albeit minor, might drive somebody mad: if the UID is
> valid, but the GID isn't, we go back to conf_ugid():
> 
> 		if (ret)
> 			err("Invalid --runas option: %s", runas);
> 
> and print the UID part, only, because we cut before the separator.

Urgh, right.  This is why having worked a bit with Rust and Go, I'm
really coming to hate C-style strings: even really rudimentary parsing
usually requires either allocation & copies or modifying logically
read-only inputs.

> Perhaps we could simply put the separator back before returning
> -EINVAL (instead of copying the buffer) -- we just have zero or one
> occurrences of ':'.

That would work, although making sure we do that on every exit path
looks messier than I'd like.  I went with a different approach of
removing that message from the caller, and instead printing more
specific messages for each of the cases within conf_runas().  A little
more involved, but I think gives a marginally better UX overall.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]