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

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

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

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

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

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

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";
-- 
2.37.3

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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				\
-- 
2.37.3

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

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

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

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

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_@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

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

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

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

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

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

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

-- 
Stefano