[PATCH 0/9] Some more static checker fixes

[PATCH 0/9] Some more static checker fixes

[David Gibson]

This series includes a number of fixes related to the static checkers:

 * Fedora 40 has updated to Cppcheck 2.14.1 which introduces some new
   warnings.  Fix them.

 * Jon's recent patch caused a small cppcheck regression.  I assume
   neither Jon nor Stefano is using sufficiently recent cppcheck
   versions to catch it.  Fix that too.

 * We were disabling the bugprone-macro parentheses check in
   clang-tidy.  I don't think that's a good idea.  Re-enable it and
   fix existing warnings.

 * It might also be a good idea to enable the
   bugprone-narrowing-conversions check.  Fix a number of issues
   across the tree which, amongst other things trigger that warning.
   There are lots of other places that trigger the warning which I
   haven't fixed yet, so don't enable it yet.

David Gibson (9):
  tcp: Make pointer const in tcp_revert_seq
  udp: Make rport calculation more local
  cppcheck: Suppress constParameterCallback errors
  Remove pointless macro parameters in CALL_PROTO_HANDLER
  clang-tidy: Enable the bugprone-macro-parentheses check
  util: Use unsigned indices for bits in bitmaps
  conf: Safer parsing of MAC addresses
  lineread: Use ssize_t for line lengths
  util: Use 'long' to represent millisecond durations

 Makefile     |  3 +--
 conf.c       | 55 +++++++++++++++++++++++++++++++++++-----------------
 lineread.c   | 10 ++++------
 lineread.h   |  7 ++++---
 passt.c      |  6 +++---
 tap.c        | 37 ++++++++++++++++++-----------------
 tcp.c        | 10 +++++-----
 tcp_splice.c |  4 ++--
 udp.c        |  3 +--
 util.c       | 10 +++++-----
 util.h       |  8 ++++----
 11 files changed, 85 insertions(+), 68 deletions(-)

-- 
2.45.2

[PATCH 1/9] tcp: Make pointer const in tcp_revert_seq

[David Gibson]

The th pointer could be const, which causes a cppcheck warning on at least
some cppcheck versions (e.g. Cppcheck 2.13.0 in Fedora 40).

Fixes: e84a01e94 "tcp: move seq_to_tap update to when frame is queued"

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

diff --git a/tcp.c b/tcp.c
index 89a5b19a..ff1198dd 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1261,8 +1261,8 @@ static void tcp_revert_seq(struct tcp_tap_conn **conns, struct iovec (*frames)[T
 	int i;
 
 	for (i = 0; i < num_frames; i++) {
+		const struct tcphdr *th = frames[i][TCP_IOV_PAYLOAD].iov_base;
 		struct tcp_tap_conn *conn = conns[i];
-		struct tcphdr *th = frames[i][TCP_IOV_PAYLOAD].iov_base;
 		uint32_t seq = ntohl(th->seq);
 
 		if (SEQ_LE(conn->seq_to_tap, seq))
-- 
@@ -1261,8 +1261,8 @@ static void tcp_revert_seq(struct tcp_tap_conn **conns, struct iovec (*frames)[T
 	int i;
 
 	for (i = 0; i < num_frames; i++) {
+		const struct tcphdr *th = frames[i][TCP_IOV_PAYLOAD].iov_base;
 		struct tcp_tap_conn *conn = conns[i];
-		struct tcphdr *th = frames[i][TCP_IOV_PAYLOAD].iov_base;
 		uint32_t seq = ntohl(th->seq);
 
 		if (SEQ_LE(conn->seq_to_tap, seq))
-- 
2.45.2

[PATCH 2/9] udp: Make rport calculation more local

[David Gibson]

cppcheck 2.14.1 complains about the rport variable not being in as small
as scope as it could be.  It's also only used once, so we might as well
just open code the calculation for it.

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

diff --git a/udp.c b/udp.c
index 3abafc99..61e106a9 100644
--- a/udp.c
+++ b/udp.c
@@ -277,10 +277,9 @@ static void udp_invert_portmap(struct udp_fwd_ports *fwd)
 		      "Forward and reverse delta arrays must have same size");
 	for (i = 0; i < ARRAY_SIZE(fwd->f.delta); i++) {
 		in_port_t delta = fwd->f.delta[i];
-		in_port_t rport = i + delta;
 
 		if (delta)
-			fwd->rdelta[rport] = NUM_PORTS - delta;
+			fwd->rdelta[i + delta] = NUM_PORTS - delta;
 	}
 }
 
-- 
@@ -277,10 +277,9 @@ static void udp_invert_portmap(struct udp_fwd_ports *fwd)
 		      "Forward and reverse delta arrays must have same size");
 	for (i = 0; i < ARRAY_SIZE(fwd->f.delta); i++) {
 		in_port_t delta = fwd->f.delta[i];
-		in_port_t rport = i + delta;
 
 		if (delta)
-			fwd->rdelta[rport] = NUM_PORTS - delta;
+			fwd->rdelta[i + delta] = NUM_PORTS - delta;
 	}
 }
 
-- 
2.45.2

[PATCH 3/9] cppcheck: Suppress constParameterCallback errors

[David Gibson]

We have several functions which are used as callbacks for NS_CALL() which
only read their void * parameter, they don't write it.  The new
constParameterCallback warning in cppcheck 2.14.1 complains that this
parameter could be const void *, also pointing out that that would require
casting the function pointer when used as a callback.

Casting the function pointers seems substantially uglier than using a
non-const void * as the parameter, especially since in each case we cast
the void * to a const pointer of specific type immediately.  So, suppress
that error from cppcheck.

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

diff --git a/Makefile b/Makefile
index 8ea17576..22f05813 100644
--- a/Makefile
+++ b/Makefile
@@ -314,5 +314,6 @@ cppcheck: $(SRCS) $(HEADERS)
 	$(SYSTEM_INCLUDES:%=--suppress=unmatchedSuppression:%/*)	\
 	--inline-suppr							\
 	--suppress=unusedStructMember					\
+	--suppress=constParameterCallback				\
 	$(filter -D%,$(FLAGS) $(CFLAGS) $(CPPFLAGS))			\
 	$(SRCS) $(HEADERS)
-- 
@@ -314,5 +314,6 @@ cppcheck: $(SRCS) $(HEADERS)
 	$(SYSTEM_INCLUDES:%=--suppress=unmatchedSuppression:%/*)	\
 	--inline-suppr							\
 	--suppress=unusedStructMember					\
+	--suppress=constParameterCallback				\
 	$(filter -D%,$(FLAGS) $(CFLAGS) $(CPPFLAGS))			\
 	$(SRCS) $(HEADERS)
-- 
2.45.2

Re: [PATCH 3/9] cppcheck: Suppress constParameterCallback errors

[Stefano Brivio]

On Thu,  6 Jun 2024 20:09:43 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> We have several functions which are used as callbacks for NS_CALL() which
> only read their void * parameter, they don't write it.  The new
> constParameterCallback warning in cppcheck 2.14.1 complains that this
> parameter could be const void *, also pointing out that that would require
> casting the function pointer when used as a callback.
> 
> Casting the function pointers seems substantially uglier than using a
> non-const void * as the parameter, especially since in each case we cast
> the void * to a const pointer of specific type immediately.  So, suppress
> that error from cppcheck.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  Makefile | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Makefile b/Makefile
> index 8ea17576..22f05813 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -314,5 +314,6 @@ cppcheck: $(SRCS) $(HEADERS)
>  	$(SYSTEM_INCLUDES:%=--suppress=unmatchedSuppression:%/*)	\
>  	--inline-suppr							\
>  	--suppress=unusedStructMember					\
> +	--suppress=constParameterCallback				\

On versions before 2.14, this now raises an unmatchedSuppression... I'm
not sure how to deal with this. Should we give up and just add a
--suppress=unmatchedSuppression for all the source files? I can't think
of anything better at the moment.

I applied the rest of the series, just not this patch.

-- 
Stefano

Re: [PATCH 3/9] cppcheck: Suppress constParameterCallback errors

[David Gibson]

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

On Fri, Jun 07, 2024 at 08:49:40PM +0200, Stefano Brivio wrote:
> On Thu,  6 Jun 2024 20:09:43 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > We have several functions which are used as callbacks for NS_CALL() which
> > only read their void * parameter, they don't write it.  The new
> > constParameterCallback warning in cppcheck 2.14.1 complains that this
> > parameter could be const void *, also pointing out that that would require
> > casting the function pointer when used as a callback.
> > 
> > Casting the function pointers seems substantially uglier than using a
> > non-const void * as the parameter, especially since in each case we cast
> > the void * to a const pointer of specific type immediately.  So, suppress
> > that error from cppcheck.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  Makefile | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/Makefile b/Makefile
> > index 8ea17576..22f05813 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -314,5 +314,6 @@ cppcheck: $(SRCS) $(HEADERS)
> >  	$(SYSTEM_INCLUDES:%=--suppress=unmatchedSuppression:%/*)	\
> >  	--inline-suppr							\
> >  	--suppress=unusedStructMember					\
> > +	--suppress=constParameterCallback				\
> 
> On versions before 2.14, this now raises an unmatchedSuppression... I'm
> not sure how to deal with this. Should we give up and just add a
> --suppress=unmatchedSuppression for all the source files? I can't think
> of anything better at the moment.

No, I don't think we want to do that, that's likely to leave stale
suppressions about.

Although it logically makes sense to suppress this globally, there are
only three spots it actually occurs, so I'll rewrite to suppress in
just those places, along with a similarly local unmatchedSuppression
suppression.

> I applied the rest of the series, just not this patch.

Nice.. although this was the only one actually blocking other work.

-- 
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 3/9] cppcheck: Suppress constParameterCallback errors

[Stefano Brivio]

On Sat, 8 Jun 2024 16:32:22 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jun 07, 2024 at 08:49:40PM +0200, Stefano Brivio wrote:
> > On Thu,  6 Jun 2024 20:09:43 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > We have several functions which are used as callbacks for NS_CALL() which
> > > only read their void * parameter, they don't write it.  The new
> > > constParameterCallback warning in cppcheck 2.14.1 complains that this
> > > parameter could be const void *, also pointing out that that would require
> > > casting the function pointer when used as a callback.
> > > 
> > > Casting the function pointers seems substantially uglier than using a
> > > non-const void * as the parameter, especially since in each case we cast
> > > the void * to a const pointer of specific type immediately.  So, suppress
> > > that error from cppcheck.
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  Makefile | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/Makefile b/Makefile
> > > index 8ea17576..22f05813 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -314,5 +314,6 @@ cppcheck: $(SRCS) $(HEADERS)
> > >  	$(SYSTEM_INCLUDES:%=--suppress=unmatchedSuppression:%/*)	\
> > >  	--inline-suppr							\
> > >  	--suppress=unusedStructMember					\
> > > +	--suppress=constParameterCallback				\  
> > 
> > On versions before 2.14, this now raises an unmatchedSuppression... I'm
> > not sure how to deal with this. Should we give up and just add a
> > --suppress=unmatchedSuppression for all the source files? I can't think
> > of anything better at the moment.  
> 
> No, I don't think we want to do that, that's likely to leave stale
> suppressions about.
> 
> Although it logically makes sense to suppress this globally, there are
> only three spots it actually occurs, so I'll rewrite to suppress in
> just those places, along with a similarly local unmatchedSuppression
> suppression.

Oh, you're right, I instinctively thought it would be a much bigger
mess, I didn't even try.

> > I applied the rest of the series, just not this patch.  
> 
> Nice.. although this was the only one actually blocking other work.

...of course. :) I just applied the new patch.

-- 
Stefano

[PATCH 4/9] Remove pointless macro parameters in CALL_PROTO_HANDLER

[David Gibson]

The 'c' parameter is always passed exactly 'c'.  The 'now' parameter is
always passed exactly 'now'.

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

diff --git a/passt.c b/passt.c
index a8c4cd3f..dabd86ed 100644
--- a/passt.c
+++ b/passt.c
@@ -84,7 +84,7 @@ static_assert(ARRAY_SIZE(epoll_type_str) == EPOLL_NUM_TYPES,
  */
 static void post_handler(struct ctx *c, const struct timespec *now)
 {
-#define CALL_PROTO_HANDLER(c, now, lc, uc)				\
+#define CALL_PROTO_HANDLER(lc, uc)					\
 	do {								\
 		extern void						\
 		lc ## _defer_handler (struct ctx *c)			\
@@ -103,9 +103,9 @@ static void post_handler(struct ctx *c, const struct timespec *now)
 	} while (0)
 
 	/* NOLINTNEXTLINE(bugprone-branch-clone): intervals can be the same */
-	CALL_PROTO_HANDLER(c, now, tcp, TCP);
+	CALL_PROTO_HANDLER(tcp, TCP);
 	/* NOLINTNEXTLINE(bugprone-branch-clone): intervals can be the same */
-	CALL_PROTO_HANDLER(c, now, udp, UDP);
+	CALL_PROTO_HANDLER(udp, UDP);
 
 	flow_defer_handler(c, now);
 #undef CALL_PROTO_HANDLER
-- 
@@ -84,7 +84,7 @@ static_assert(ARRAY_SIZE(epoll_type_str) == EPOLL_NUM_TYPES,
  */
 static void post_handler(struct ctx *c, const struct timespec *now)
 {
-#define CALL_PROTO_HANDLER(c, now, lc, uc)				\
+#define CALL_PROTO_HANDLER(lc, uc)					\
 	do {								\
 		extern void						\
 		lc ## _defer_handler (struct ctx *c)			\
@@ -103,9 +103,9 @@ static void post_handler(struct ctx *c, const struct timespec *now)
 	} while (0)
 
 	/* NOLINTNEXTLINE(bugprone-branch-clone): intervals can be the same */
-	CALL_PROTO_HANDLER(c, now, tcp, TCP);
+	CALL_PROTO_HANDLER(tcp, TCP);
 	/* NOLINTNEXTLINE(bugprone-branch-clone): intervals can be the same */
-	CALL_PROTO_HANDLER(c, now, udp, UDP);
+	CALL_PROTO_HANDLER(udp, UDP);
 
 	flow_defer_handler(c, now);
 #undef CALL_PROTO_HANDLER
-- 
2.45.2

[PATCH 5/9] clang-tidy: Enable the bugprone-macro-parentheses check

[David Gibson]

We globally disabled this, with a justification lumped together with
several checks about braces.  They don't really go together, the others
are essentially a stylistic choice which doesn't match our style.  Omitting
brackets on macro parameters can lead to real and hard to track down bugs
if an expression is ever passed to the macro instead of a plain identifier.

We've only gotten away with the macros which trigger the warning, because
of other conventions its been unlikely to invoke them with anything other
than a simple identifier.  Fix the macros, and enable the warning for the
future.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 Makefile     |  2 --
 tap.c        | 37 +++++++++++++++++++------------------
 tcp.c        |  8 ++++----
 tcp_splice.c |  4 ++--
 4 files changed, 25 insertions(+), 26 deletions(-)

diff --git a/Makefile b/Makefile
index 22f05813..4d00f48c 100644
--- a/Makefile
+++ b/Makefile
@@ -192,7 +192,6 @@ docs: README.md
 # - llvmlibc-restrict-system-libc-headers
 #	TODO: this is Linux-only for the moment, nice to fix eventually
 #
-# - bugprone-macro-parentheses
 # - google-readability-braces-around-statements
 # - hicpp-braces-around-statements
 # - readability-braces-around-statements
@@ -269,7 +268,6 @@ clang-tidy: $(SRCS) $(HEADERS)
 	-clang-analyzer-valist.Uninitialized,\
 	-cppcoreguidelines-init-variables,\
 	-bugprone-assignment-in-if-condition,\
-	-bugprone-macro-parentheses,\
 	-google-readability-braces-around-statements,\
 	-hicpp-braces-around-statements,\
 	-readability-braces-around-statements,\
diff --git a/tap.c b/tap.c
index 2ea08491..26084b48 100644
--- a/tap.c
+++ b/tap.c
@@ -674,18 +674,18 @@ resume:
 			continue;
 		}
 
-#define L4_MATCH(iph, uh, seq)						\
-	(seq->protocol == iph->protocol &&				\
-	 seq->source   == uh->source    && seq->dest  == uh->dest &&	\
-	 seq->saddr.s_addr == iph->saddr && seq->daddr.s_addr == iph->daddr)
+#define L4_MATCH(iph, uh, seq)							\
+	((seq)->protocol == (iph)->protocol &&					\
+	 (seq)->source   == (uh)->source    && (seq)->dest  == (uh)->dest &&	\
+	 (seq)->saddr.s_addr == (iph)->saddr && (seq)->daddr.s_addr == (iph)->daddr)
 
 #define L4_SET(iph, uh, seq)						\
 	do {								\
-		seq->protocol		= iph->protocol;		\
-		seq->source		= uh->source;			\
-		seq->dest		= uh->dest;			\
-		seq->saddr.s_addr	= iph->saddr;			\
-		seq->daddr.s_addr	= iph->daddr;			\
+		(seq)->protocol		= (iph)->protocol;		\
+		(seq)->source		= (uh)->source;			\
+		(seq)->dest		= (uh)->dest;			\
+		(seq)->saddr.s_addr	= (iph)->saddr;			\
+		(seq)->daddr.s_addr	= (iph)->daddr;			\
 	} while (0)
 
 		if (seq && L4_MATCH(iph, uh, seq) && seq->p.count < UIO_MAXIOV)
@@ -848,18 +848,19 @@ resume:
 		}
 
 #define L4_MATCH(ip6h, proto, uh, seq)					\
-	(seq->protocol == proto         &&				\
-	 seq->source   == uh->source    && seq->dest  == uh->dest &&	\
-	 IN6_ARE_ADDR_EQUAL(&seq->saddr, saddr)			  &&	\
-	 IN6_ARE_ADDR_EQUAL(&seq->daddr, daddr))
+		((seq)->protocol == (proto)                &&		\
+		 (seq)->source   == (uh)->source           &&		\
+		 (seq)->dest == (uh)->dest                 &&		\
+		 IN6_ARE_ADDR_EQUAL(&(seq)->saddr, saddr)  &&		\
+		 IN6_ARE_ADDR_EQUAL(&(seq)->daddr, daddr))
 
 #define L4_SET(ip6h, proto, uh, seq)					\
 	do {								\
-		seq->protocol	= proto;				\
-		seq->source	= uh->source;				\
-		seq->dest	= uh->dest;				\
-		seq->saddr	= *saddr;				\
-		seq->daddr	= *daddr;				\
+		(seq)->protocol	= (proto);				\
+		(seq)->source	= (uh)->source;				\
+		(seq)->dest	= (uh)->dest;				\
+		(seq)->saddr	= *saddr;				\
+		(seq)->daddr	= *daddr;				\
 	} while (0)
 
 		if (seq && L4_MATCH(ip6h, proto, uh, seq) &&
diff --git a/tcp.c b/tcp.c
index ff1198dd..0dccde9c 100644
--- a/tcp.c
+++ b/tcp.c
@@ -326,7 +326,7 @@
 
 #define WINDOW_DEFAULT			14600		/* RFC 6928 */
 #ifdef HAS_SND_WND
-# define KERNEL_REPORTS_SND_WND(c)	(c->tcp.kernel_snd_wnd)
+# define KERNEL_REPORTS_SND_WND(c)	((c)->tcp.kernel_snd_wnd)
 #else
 # define KERNEL_REPORTS_SND_WND(c)	(0 && (c))
 #endif
@@ -373,9 +373,9 @@
 #define CONN_V4(conn)		(!!inany_v4(&(conn)->faddr))
 #define CONN_V6(conn)		(!CONN_V4(conn))
 #define CONN_IS_CLOSING(conn)						\
-	((conn->events & ESTABLISHED) &&				\
-	 (conn->events & (SOCK_FIN_RCVD | TAP_FIN_RCVD)))
-#define CONN_HAS(conn, set)	((conn->events & (set)) == (set))
+	(((conn)->events & ESTABLISHED) &&				\
+	 ((conn)->events & (SOCK_FIN_RCVD | TAP_FIN_RCVD)))
+#define CONN_HAS(conn, set)	(((conn)->events & (set)) == (set))
 
 static const char *tcp_event_str[] __attribute((__unused__)) = {
 	"SOCK_ACCEPTED", "TAP_SYN_RCVD", "ESTABLISHED", "TAP_SYN_ACK_SENT",
diff --git a/tcp_splice.c b/tcp_splice.c
index b8f64a95..3f9d395a 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -73,9 +73,9 @@ static int ns_sock_pool6	[TCP_SOCK_POOL_SIZE];
 /* Pool of pre-opened pipes */
 static int splice_pipe_pool		[TCP_SPLICE_PIPE_POOL_SIZE][2];
 
-#define CONN_V6(x)			(x->flags & SPLICE_V6)
+#define CONN_V6(x)			((x)->flags & SPLICE_V6)
 #define CONN_V4(x)			(!CONN_V6(x))
-#define CONN_HAS(conn, set)		((conn->events & (set)) == (set))
+#define CONN_HAS(conn, set)		(((conn)->events & (set)) == (set))
 #define CONN(idx)			(&FLOW(idx)->tcp_splice)
 
 /* Display strings for connection events */
-- 
@@ -73,9 +73,9 @@ static int ns_sock_pool6	[TCP_SOCK_POOL_SIZE];
 /* Pool of pre-opened pipes */
 static int splice_pipe_pool		[TCP_SPLICE_PIPE_POOL_SIZE][2];
 
-#define CONN_V6(x)			(x->flags & SPLICE_V6)
+#define CONN_V6(x)			((x)->flags & SPLICE_V6)
 #define CONN_V4(x)			(!CONN_V6(x))
-#define CONN_HAS(conn, set)		((conn->events & (set)) == (set))
+#define CONN_HAS(conn, set)		(((conn)->events & (set)) == (set))
 #define CONN(idx)			(&FLOW(idx)->tcp_splice)
 
 /* Display strings for connection events */
-- 
2.45.2

[PATCH 6/9] util: Use unsigned indices for bits in bitmaps

[David Gibson]

A negative bit index in a bitmap doesn't make sense.  Avoid this by
construction by using unsigned indices.  While we're there adjust
bitmap_isset() to return a bool instead of an int.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 util.c | 8 ++++----
 util.h | 6 +++---
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/util.c b/util.c
index cc1c73ba..5e854a26 100644
--- a/util.c
+++ b/util.c
@@ -232,7 +232,7 @@ int timespec_diff_ms(const struct timespec *a, const struct timespec *b)
  * @map:	Pointer to bitmap
  * @bit:	Bit number to set
  */
-void bitmap_set(uint8_t *map, int bit)
+void bitmap_set(uint8_t *map, unsigned bit)
 {
 	unsigned long *word = (unsigned long *)map + BITMAP_WORD(bit);
 
@@ -244,7 +244,7 @@ void bitmap_set(uint8_t *map, int bit)
  * @map:	Pointer to bitmap
  * @bit:	Bit number to clear
  */
-void bitmap_clear(uint8_t *map, int bit)
+void bitmap_clear(uint8_t *map, unsigned bit)
 {
 	unsigned long *word = (unsigned long *)map + BITMAP_WORD(bit);
 
@@ -256,9 +256,9 @@ void bitmap_clear(uint8_t *map, int bit)
  * @map:	Pointer to bitmap
  * @bit:	Bit number to check
  *
- * Return: one if given bit is set, zero if it's not
+ * Return: true if given bit is set, false if it's not
  */
-int bitmap_isset(const uint8_t *map, int bit)
+bool bitmap_isset(const uint8_t *map, unsigned bit)
 {
 	const unsigned long *word
 		= (const unsigned long *)map + BITMAP_WORD(bit);
diff --git a/util.h b/util.h
index aa6e4b48..cf9c4b66 100644
--- a/util.h
+++ b/util.h
@@ -148,9 +148,9 @@ int sock_l4(const struct ctx *c, sa_family_t af, uint8_t proto,
 	    uint32_t data);
 void sock_probe_mem(struct ctx *c);
 int timespec_diff_ms(const struct timespec *a, const struct timespec *b);
-void bitmap_set(uint8_t *map, int bit);
-void bitmap_clear(uint8_t *map, int bit);
-int bitmap_isset(const uint8_t *map, int bit);
+void bitmap_set(uint8_t *map, unsigned bit);
+void bitmap_clear(uint8_t *map, unsigned bit);
+bool bitmap_isset(const uint8_t *map, unsigned bit);
 void bitmap_or(uint8_t *dst, size_t size, const uint8_t *a, const uint8_t *b);
 char *line_read(char *buf, size_t len, int fd);
 void ns_enter(const struct ctx *c);
-- 
@@ -148,9 +148,9 @@ int sock_l4(const struct ctx *c, sa_family_t af, uint8_t proto,
 	    uint32_t data);
 void sock_probe_mem(struct ctx *c);
 int timespec_diff_ms(const struct timespec *a, const struct timespec *b);
-void bitmap_set(uint8_t *map, int bit);
-void bitmap_clear(uint8_t *map, int bit);
-int bitmap_isset(const uint8_t *map, int bit);
+void bitmap_set(uint8_t *map, unsigned bit);
+void bitmap_clear(uint8_t *map, unsigned bit);
+bool bitmap_isset(const uint8_t *map, unsigned bit);
 void bitmap_or(uint8_t *dst, size_t size, const uint8_t *a, const uint8_t *b);
 char *line_read(char *buf, size_t len, int fd);
 void ns_enter(const struct ctx *c);
-- 
2.45.2

[PATCH 7/9] conf: Safer parsing of MAC addresses

[David Gibson]

In conf() we parse a MAC address in two places, for the --ns-mac-addr and
the -M options.  As well as duplicating code, the logic for this parsing
has several bugs:
  * The most serious is that if the given string is shorter than a MAC
    address should be, we'll access past the end of it.
  * We don't check the endptr supplied by strtol() which means we could
    ignore certain erroneous contents
  * We never check the separator characters between each octet
  * We ignore certain sorts of garbage that follow the MAC address

Correct all these bugs in a new parse_mac() helper.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c | 53 ++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 17 deletions(-)

diff --git a/conf.c b/conf.c
index 72ca1fc8..fdeb9db7 100644
--- a/conf.c
+++ b/conf.c
@@ -1124,6 +1124,39 @@ static void conf_open_files(struct ctx *c)
 	c->pidfile_fd = pidfile_open(c->pidfile);
 }
 
+/**
+ * parse_mac - Parse a MAC address from a string
+ * @mac:	Binary MAC address, initialised on success
+ * @str:	String to parse
+ *
+ * Parses @str as an Ethernet MAC address stored in @mac on success.  Exits on
+ * failure.
+ */
+static void parse_mac(unsigned char mac[ETH_ALEN], const char *str)
+{
+	size_t i;
+
+	if (strlen(str) != (ETH_ALEN * 3 - 1))
+		goto fail;
+
+	for (i = 0; i < ETH_ALEN; i++) {
+		const char *octet = str + 3 * i;
+		unsigned long b;
+		char *end;
+
+		errno = 0;
+		b = strtoul(octet, &end, 16);
+		if (b > UCHAR_MAX || errno || end != octet + 2 ||
+		    *end != ((i == ETH_ALEN - 1) ? '\0' : ':'))
+			goto fail;
+		mac[i] = b;
+	}
+	return;
+
+fail:
+	die("Invalid MAC address: %s", str);
+}
+
 /**
  * conf() - Process command-line arguments and set configuration
  * @c:		Execution context
@@ -1200,9 +1233,9 @@ void conf(struct ctx *c, int argc, char **argv)
 	unsigned int ifi4 = 0, ifi6 = 0;
 	const char *logfile = NULL;
 	const char *optstring;
-	int name, ret, b, i;
 	size_t logsize = 0;
 	char *runas = NULL;
+	int name, ret;
 	uid_t uid;
 	gid_t gid;
 
@@ -1243,14 +1276,7 @@ void conf(struct ctx *c, int argc, char **argv)
 			if (c->mode != MODE_PASTA)
 				die("--ns-mac-addr is for pasta mode only");
 
-			for (i = 0; i < ETH_ALEN; i++) {
-				errno = 0;
-				b = strtol(optarg + (intptr_t)i * 3, NULL, 16);
-				if (b < 0 || b > UCHAR_MAX || errno)
-					die("Invalid MAC address: %s", optarg);
-
-				c->mac_guest[i] = b;
-			}
+			parse_mac(c->mac_guest, optarg);
 			break;
 		case 5:
 			if (c->mode != MODE_PASTA)
@@ -1510,14 +1536,7 @@ void conf(struct ctx *c, int argc, char **argv)
 
 			break;
 		case 'M':
-			for (i = 0; i < ETH_ALEN; i++) {
-				errno = 0;
-				b = strtol(optarg + (intptr_t)i * 3, NULL, 16);
-				if (b < 0 || b > UCHAR_MAX || errno)
-					die("Invalid MAC address: %s", optarg);
-
-				c->mac[i] = b;
-			}
+			parse_mac(c->mac, optarg);
 			break;
 		case 'g':
 			if (c->mode == MODE_PASTA)
-- 
@@ -1124,6 +1124,39 @@ static void conf_open_files(struct ctx *c)
 	c->pidfile_fd = pidfile_open(c->pidfile);
 }
 
+/**
+ * parse_mac - Parse a MAC address from a string
+ * @mac:	Binary MAC address, initialised on success
+ * @str:	String to parse
+ *
+ * Parses @str as an Ethernet MAC address stored in @mac on success.  Exits on
+ * failure.
+ */
+static void parse_mac(unsigned char mac[ETH_ALEN], const char *str)
+{
+	size_t i;
+
+	if (strlen(str) != (ETH_ALEN * 3 - 1))
+		goto fail;
+
+	for (i = 0; i < ETH_ALEN; i++) {
+		const char *octet = str + 3 * i;
+		unsigned long b;
+		char *end;
+
+		errno = 0;
+		b = strtoul(octet, &end, 16);
+		if (b > UCHAR_MAX || errno || end != octet + 2 ||
+		    *end != ((i == ETH_ALEN - 1) ? '\0' : ':'))
+			goto fail;
+		mac[i] = b;
+	}
+	return;
+
+fail:
+	die("Invalid MAC address: %s", str);
+}
+
 /**
  * conf() - Process command-line arguments and set configuration
  * @c:		Execution context
@@ -1200,9 +1233,9 @@ void conf(struct ctx *c, int argc, char **argv)
 	unsigned int ifi4 = 0, ifi6 = 0;
 	const char *logfile = NULL;
 	const char *optstring;
-	int name, ret, b, i;
 	size_t logsize = 0;
 	char *runas = NULL;
+	int name, ret;
 	uid_t uid;
 	gid_t gid;
 
@@ -1243,14 +1276,7 @@ void conf(struct ctx *c, int argc, char **argv)
 			if (c->mode != MODE_PASTA)
 				die("--ns-mac-addr is for pasta mode only");
 
-			for (i = 0; i < ETH_ALEN; i++) {
-				errno = 0;
-				b = strtol(optarg + (intptr_t)i * 3, NULL, 16);
-				if (b < 0 || b > UCHAR_MAX || errno)
-					die("Invalid MAC address: %s", optarg);
-
-				c->mac_guest[i] = b;
-			}
+			parse_mac(c->mac_guest, optarg);
 			break;
 		case 5:
 			if (c->mode != MODE_PASTA)
@@ -1510,14 +1536,7 @@ void conf(struct ctx *c, int argc, char **argv)
 
 			break;
 		case 'M':
-			for (i = 0; i < ETH_ALEN; i++) {
-				errno = 0;
-				b = strtol(optarg + (intptr_t)i * 3, NULL, 16);
-				if (b < 0 || b > UCHAR_MAX || errno)
-					die("Invalid MAC address: %s", optarg);
-
-				c->mac[i] = b;
-			}
+			parse_mac(c->mac, optarg);
 			break;
 		case 'g':
 			if (c->mode == MODE_PASTA)
-- 
2.45.2

[PATCH 8/9] lineread: Use ssize_t for line lengths

[David Gibson]

Functions and structures in lineread.c use plain int to record and report
the length of lines we receive.  This means we truncate the result from
read(2) in some circumstances.  Use ssize_t to avoid that.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c     |  2 +-
 lineread.c | 10 ++++------
 lineread.h |  7 ++++---
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/conf.c b/conf.c
index fdeb9db7..3998bfaa 100644
--- a/conf.c
+++ b/conf.c
@@ -401,9 +401,9 @@ static void get_dns(struct ctx *c)
 	struct fqdn *s = c->dns_search;
 	struct lineread resolvconf;
 	unsigned int added = 0;
+	ssize_t line_len;
 	char *line, *end;
 	const char *p;
-	int line_len;
 
 	dns4_set = !c->ifi4 || !IN4_IS_ADDR_UNSPECIFIED(dns4);
 	dns6_set = !c->ifi6 || !IN6_IS_ADDR_UNSPECIFIED(dns6);
diff --git a/lineread.c b/lineread.c
index d631da44..0387f4a0 100644
--- a/lineread.c
+++ b/lineread.c
@@ -39,13 +39,11 @@ void lineread_init(struct lineread *lr, int fd)
  *
  * Return: length of line in bytes, -1 if no line was found
  */
-static int peek_line(struct lineread *lr, bool eof)
+static ssize_t peek_line(struct lineread *lr, bool eof)
 {
 	char *nl;
 
 	/* Sanity checks (which also document invariants) */
-	ASSERT(lr->count >= 0);
-	ASSERT(lr->next_line >= 0);
 	ASSERT(lr->next_line + lr->count >= lr->next_line);
 	ASSERT(lr->next_line + lr->count <= LINEREAD_BUFFER_SIZE);
 
@@ -74,13 +72,13 @@ static int peek_line(struct lineread *lr, bool eof)
  *
  * Return:	Length of line read on success, 0 on EOF, negative on error
  */
-int lineread_get(struct lineread *lr, char **line)
+ssize_t lineread_get(struct lineread *lr, char **line)
 {
 	bool eof = false;
-	int line_len;
+	ssize_t line_len;
 
 	while ((line_len = peek_line(lr, eof)) < 0) {
-		int rc;
+		ssize_t rc;
 
 		if ((lr->next_line + lr->count) == LINEREAD_BUFFER_SIZE) {
 			/* No space at end */
diff --git a/lineread.h b/lineread.h
index af864186..9203e280 100644
--- a/lineread.h
+++ b/lineread.h
@@ -18,14 +18,15 @@
  * @buf:	Buffer storing data read from file.
  */
 struct lineread {
-	int fd; int next_line;
-	int count;
+	int fd;
+	ssize_t next_line;
+	ssize_t count;
 
 	/* One extra byte for possible trailing \0 */
 	char buf[LINEREAD_BUFFER_SIZE+1];
 };
 
 void lineread_init(struct lineread *lr, int fd);
-int lineread_get(struct lineread *lr, char **line);
+ssize_t lineread_get(struct lineread *lr, char **line);
 
 #endif /* _LINEREAD_H */
-- 
@@ -18,14 +18,15 @@
  * @buf:	Buffer storing data read from file.
  */
 struct lineread {
-	int fd; int next_line;
-	int count;
+	int fd;
+	ssize_t next_line;
+	ssize_t count;
 
 	/* One extra byte for possible trailing \0 */
 	char buf[LINEREAD_BUFFER_SIZE+1];
 };
 
 void lineread_init(struct lineread *lr, int fd);
-int lineread_get(struct lineread *lr, char **line);
+ssize_t lineread_get(struct lineread *lr, char **line);
 
 #endif /* _LINEREAD_H */
-- 
2.45.2

[PATCH 9/9] util: Use 'long' to represent millisecond durations

[David Gibson]

timespec_diff_ms() returns an int representing a duration in milliseconds.
This will overflow in about 25 days when an int is 32 bits.  The way we
use this function, we're probably not going to get a result that long, but
it's not outrageously implausible.  Use a long for safety.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 util.c | 2 +-
 util.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/util.c b/util.c
index 5e854a26..44461801 100644
--- a/util.c
+++ b/util.c
@@ -216,7 +216,7 @@ void sock_probe_mem(struct ctx *c)
  *
  * Return: difference in milliseconds
  */
-int timespec_diff_ms(const struct timespec *a, const struct timespec *b)
+long timespec_diff_ms(const struct timespec *a, const struct timespec *b)
 {
 	if (a->tv_nsec < b->tv_nsec) {
 		return (b->tv_nsec - a->tv_nsec) / 1000000 +
diff --git a/util.h b/util.h
index cf9c4b66..eebb027b 100644
--- a/util.h
+++ b/util.h
@@ -147,7 +147,7 @@ int sock_l4(const struct ctx *c, sa_family_t af, uint8_t proto,
 	    const void *bind_addr, const char *ifname, uint16_t port,
 	    uint32_t data);
 void sock_probe_mem(struct ctx *c);
-int timespec_diff_ms(const struct timespec *a, const struct timespec *b);
+long timespec_diff_ms(const struct timespec *a, const struct timespec *b);
 void bitmap_set(uint8_t *map, unsigned bit);
 void bitmap_clear(uint8_t *map, unsigned bit);
 bool bitmap_isset(const uint8_t *map, unsigned bit);
-- 
@@ -147,7 +147,7 @@ int sock_l4(const struct ctx *c, sa_family_t af, uint8_t proto,
 	    const void *bind_addr, const char *ifname, uint16_t port,
 	    uint32_t data);
 void sock_probe_mem(struct ctx *c);
-int timespec_diff_ms(const struct timespec *a, const struct timespec *b);
+long timespec_diff_ms(const struct timespec *a, const struct timespec *b);
 void bitmap_set(uint8_t *map, unsigned bit);
 void bitmap_clear(uint8_t *map, unsigned bit);
 bool bitmap_isset(const uint8_t *map, unsigned bit);
-- 
2.45.2