[PATCH v2 0/2] Some static checker fixes

[PATCH v2 0/2] Some static checker fixes

[David Gibson]

We already had a couple of places we were working around clang-tidy
issue 58992, and the flow table series adds more.  I got sick of ugly
inlines every time we used a syscall which returns a socket address,
so wrote a patch to consolidate the workarounds in one place.

However, that patch added an include of <string.h> to util.h which
exposed a classic C library gotcha in packet.c, so I fixed that too.

Changes since v1:
 * Updated missed comment to match code changes in 1/2
 * Fixed more places which shadowed index(3)

David Gibson (2):
  Avoid shadowing index(3)
  util: Consolidate and improve workarounds for clang-tidy issue 58992

 Makefile     |  2 +-
 icmp.c       |  5 -----
 packet.c     | 30 +++++++++++++++---------------
 packet.h     | 10 +++++-----
 tcp.c        | 22 ++++++++--------------
 tcp_splice.c |  2 +-
 util.c       | 12 ++++++------
 util.h       | 43 ++++++++++++++++++++++++++++++++++++++++++-
 8 files changed, 78 insertions(+), 48 deletions(-)

-- 
2.41.0

[PATCH v2 1/2] Avoid shadowing index(3)

[David Gibson]

A classic gotcha of the standard C library is that its unwise to call any
variable 'index' because it will shadow the standard string library
function index(3).  This can cause warnings from cppcheck amongst others,
and it also means that if the variable is removed you tend to get confusing
type errors (or sometimes nothing at all) instead of a nice simple "name is
not defined" error.

Strictly speaking this only occurs if <string.h> is included, but that
is so common that as a rule it's best to just avoid it always.  We
have a number of places which hit this trap, so rename variables and
parameters to avoid it.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 packet.c     | 30 +++++++++++++++---------------
 packet.h     | 10 +++++-----
 tcp.c        | 14 +++++++-------
 tcp_splice.c |  2 +-
 util.c       | 12 ++++++------
 util.h       |  2 +-
 6 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/packet.c b/packet.c
index ce807e2..693e034 100644
--- a/packet.c
+++ b/packet.c
@@ -33,11 +33,11 @@
 void packet_add_do(struct pool *p, size_t len, const char *start,
 		   const char *func, int line)
 {
-	size_t index = p->count;
+	size_t idx = p->count;
 
-	if (index >= p->size) {
+	if (idx >= p->size) {
 		trace("add packet index %lu to pool with size %lu, %s:%i",
-		      index, p->size, func, line);
+		      idx, p->size, func, line);
 		return;
 	}
 
@@ -66,8 +66,8 @@ void packet_add_do(struct pool *p, size_t len, const char *start,
 	}
 #endif
 
-	p->pkt[index].offset = start - p->buf;
-	p->pkt[index].len = len;
+	p->pkt[idx].offset = start - p->buf;
+	p->pkt[idx].len = len;
 
 	p->count++;
 }
@@ -75,7 +75,7 @@ void packet_add_do(struct pool *p, size_t len, const char *start,
 /**
  * packet_get_do() - Get data range from packet descriptor from given pool
  * @p:		Packet pool
- * @index:	Index of packet descriptor in pool
+ * @idx:	Index of packet descriptor in pool
  * @offset:	Offset of data range in packet descriptor
  * @len:	Length of desired data range
  * @left:	Length of available data after range, set on return, can be NULL
@@ -84,13 +84,13 @@ void packet_add_do(struct pool *p, size_t len, const char *start,
  *
  * Return: pointer to start of data range, NULL on invalid range or descriptor
  */
-void *packet_get_do(const struct pool *p, size_t index, size_t offset,
+void *packet_get_do(const struct pool *p, size_t idx, size_t offset,
 		    size_t len, size_t *left, const char *func, int line)
 {
-	if (index >= p->size || index >= p->count) {
+	if (idx >= p->size || idx >= p->count) {
 		if (func) {
 			trace("packet %lu from pool size: %lu, count: %lu, "
-			      "%s:%i", index, p->size, p->count, func, line);
+			      "%s:%i", idx, p->size, p->count, func, line);
 		}
 		return NULL;
 	}
@@ -103,28 +103,28 @@ void *packet_get_do(const struct pool *p, size_t index, size_t offset,
 		return NULL;
 	}
 
-	if (p->pkt[index].offset + len + offset > p->buf_size) {
+	if (p->pkt[idx].offset + len + offset > p->buf_size) {
 		if (func) {
 			trace("packet offset plus length %lu from size %lu, "
-			      "%s:%i", p->pkt[index].offset + len + offset,
+			      "%s:%i", p->pkt[idx].offset + len + offset,
 			      p->buf_size, func, line);
 		}
 		return NULL;
 	}
 
-	if (len + offset > p->pkt[index].len) {
+	if (len + offset > p->pkt[idx].len) {
 		if (func) {
 			trace("data length %lu, offset %lu from length %u, "
-			      "%s:%i", len, offset, p->pkt[index].len,
+			      "%s:%i", len, offset, p->pkt[idx].len,
 			      func, line);
 		}
 		return NULL;
 	}
 
 	if (left)
-		*left = p->pkt[index].len - offset - len;
+		*left = p->pkt[idx].len - offset - len;
 
-	return p->buf + p->pkt[index].offset + offset;
+	return p->buf + p->pkt[idx].offset + offset;
 }
 
 /**
diff --git a/packet.h b/packet.h
index 9e5fc74..a784b07 100644
--- a/packet.h
+++ b/packet.h
@@ -34,7 +34,7 @@ struct pool {
 
 void packet_add_do(struct pool *p, size_t len, const char *start,
 		   const char *func, int line);
-void *packet_get_do(const struct pool *p, const size_t index,
+void *packet_get_do(const struct pool *p, const size_t idx,
 		    size_t offset, size_t len, size_t *left,
 		    const char *func, int line);
 void pool_flush(struct pool *p);
@@ -42,11 +42,11 @@ void pool_flush(struct pool *p);
 #define packet_add(p, len, start)					\
 	packet_add_do(p, len, start, __func__, __LINE__)
 
-#define packet_get(p, index, offset, len, left)				\
-	packet_get_do(p, index, offset, len, left, __func__, __LINE__)
+#define packet_get(p, idx, offset, len, left)				\
+	packet_get_do(p, idx, offset, len, left, __func__, __LINE__)
 
-#define packet_get_try(p, index, offset, len, left)			\
-	packet_get_do(p, index, offset, len, left, NULL, 0)
+#define packet_get_try(p, idx, offset, len, left)			\
+	packet_get_do(p, idx, offset, len, left, NULL, 0)
 
 #define PACKET_POOL_DECL(_name, _size, _buf)				\
 struct _name ## _t {							\
diff --git a/tcp.c b/tcp.c
index dd3142d..e58625d 100644
--- a/tcp.c
+++ b/tcp.c
@@ -563,20 +563,20 @@ static unsigned int tcp6_l2_flags_buf_used;
 /* TCP connections */
 union tcp_conn tc[TCP_MAX_CONNS];
 
-#define CONN(index)		(&tc[(index)].tap)
+#define CONN(idx)		(&tc[(idx)].tap)
 #define CONN_IDX(conn)		((union tcp_conn *)(conn) - tc)
 
 /** conn_at_idx() - Find a connection by index, if present
- * @index:	Index of connection to lookup
+ * @idx:	Index of connection to lookup
  *
- * Return: pointer to connection, or NULL if @index is out of bounds
+ * Return: pointer to connection, or NULL if @idx is out of bounds
  */
-static inline struct tcp_tap_conn *conn_at_idx(int index)
+static inline struct tcp_tap_conn *conn_at_idx(int idx)
 {
-	if ((index < 0) || (index >= TCP_MAX_CONNS))
+	if ((idx < 0) || (idx >= TCP_MAX_CONNS))
 		return NULL;
-	ASSERT(!(CONN(index)->c.spliced));
-	return CONN(index);
+	ASSERT(!(CONN(idx)->c.spliced));
+	return CONN(idx);
 }
 
 /* Table for lookup from remote address, local port, remote port */
diff --git a/tcp_splice.c b/tcp_splice.c
index 5b36975..be01908 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -73,7 +73,7 @@ static int splice_pipe_pool		[TCP_SPLICE_PIPE_POOL_SIZE][2][2];
 #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(index)			(&tc[(index)].splice)
+#define CONN(idx)			(&tc[(idx)].splice)
 #define CONN_IDX(conn)			((union tcp_conn *)(conn) - tc)
 
 /* Display strings for connection events */
diff --git a/util.c b/util.c
index d965f48..34c32b9 100644
--- a/util.c
+++ b/util.c
@@ -38,15 +38,15 @@
 
 /**
  * ipv6_l4hdr() - Find pointer to L4 header in IPv6 packet and extract protocol
- * @p:		Packet pool, packet number @index has IPv6 header at @offset
- * @index:	Index of packet in pool
+ * @p:		Packet pool, packet number @idx has IPv6 header at @offset
+ * @idx:	Index of packet in pool
  * @offset:	Pre-calculated IPv6 header offset
  * @proto:	Filled with L4 protocol number
  * @dlen:	Data length (payload excluding header extensions), set on return
  *
  * Return: pointer to L4 header, NULL if not found
  */
-char *ipv6_l4hdr(const struct pool *p, int index, size_t offset, uint8_t *proto,
+char *ipv6_l4hdr(const struct pool *p, int idx, size_t offset, uint8_t *proto,
 		 size_t *dlen)
 {
 	struct ipv6_opt_hdr *o;
@@ -55,8 +55,8 @@ char *ipv6_l4hdr(const struct pool *p, int index, size_t offset, uint8_t *proto,
 	int hdrlen;
 	uint8_t nh;
 
-	base = packet_get(p, index, 0, 0, NULL);
-	ip6h = packet_get(p, index, offset, sizeof(*ip6h), dlen);
+	base = packet_get(p, idx, 0, 0, NULL);
+	ip6h = packet_get(p, idx, offset, sizeof(*ip6h), dlen);
 	if (!ip6h)
 		return NULL;
 
@@ -66,7 +66,7 @@ char *ipv6_l4hdr(const struct pool *p, int index, size_t offset, uint8_t *proto,
 	if (!IPV6_NH_OPT(nh))
 		goto found;
 
-	while ((o = packet_get_try(p, index, offset, sizeof(*o), dlen))) {
+	while ((o = packet_get_try(p, idx, offset, sizeof(*o), dlen))) {
 		nh = o->nexthdr;
 		hdrlen = (o->hdrlen + 1) * 8;
 
diff --git a/util.h b/util.h
index 195023f..41cf123 100644
--- a/util.h
+++ b/util.h
@@ -205,7 +205,7 @@ struct ipv6_opt_hdr {
 
 /* cppcheck-suppress funcArgNamesDifferent */
 __attribute__ ((weak)) int ffsl(long int i) { return __builtin_ffsl(i); }
-char *ipv6_l4hdr(const struct pool *p, int index, size_t offset, uint8_t *proto,
+char *ipv6_l4hdr(const struct pool *p, int idx, size_t offset, uint8_t *proto,
 		 size_t *dlen);
 int sock_l4(const struct ctx *c, int af, uint8_t proto,
 	    const void *bind_addr, const char *ifname, uint16_t port,
-- 
@@ -205,7 +205,7 @@ struct ipv6_opt_hdr {
 
 /* cppcheck-suppress funcArgNamesDifferent */
 __attribute__ ((weak)) int ffsl(long int i) { return __builtin_ffsl(i); }
-char *ipv6_l4hdr(const struct pool *p, int index, size_t offset, uint8_t *proto,
+char *ipv6_l4hdr(const struct pool *p, int idx, size_t offset, uint8_t *proto,
 		 size_t *dlen);
 int sock_l4(const struct ctx *c, int af, uint8_t proto,
 	    const void *bind_addr, const char *ifname, uint16_t port,
-- 
2.41.0

[PATCH v2 2/2] util: Consolidate and improve workarounds for clang-tidy issue 58992

[David Gibson]

We have several workarounds for a clang-tidy bug where the checker doesn't
recognize that a number of system calls write to - and therefore initialise
- a socket address.  We can't neatly use a suppression, because the bogus
warning shows up some time after the actual system call, when we access
a field of the socket address which clang-tidy erroneously thinks is
uninitialised.

Consolidate these workarounds into one place by using macros to implement
wrappers around affected system calls which add a memset() of the sockaddr
to silence clang-tidy.  This removes the need for the individual memset()
workarounds at the callers - and the somewhat longwinded explanatory
comments.

We can then use a #define to not include the hack in "real" builds, but
only consider it for clang-tidy.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 Makefile |  2 +-
 icmp.c   |  5 -----
 tcp.c    |  8 +-------
 util.h   | 41 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 43 insertions(+), 13 deletions(-)

diff --git a/Makefile b/Makefile
index 4435bd6..c28556f 100644
--- a/Makefile
+++ b/Makefile
@@ -276,7 +276,7 @@ clang-tidy: $(SRCS) $(HEADERS)
 	-concurrency-mt-unsafe,\
 	-readability-identifier-length \
 	-config='{CheckOptions: [{key: bugprone-suspicious-string-compare.WarnOnImplicitComparison, value: "false"}]}' \
-	--warnings-as-errors=* $(SRCS) -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS))
+	--warnings-as-errors=* $(SRCS) -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) -DCLANG_TIDY_58992
 
 SYSTEM_INCLUDES := /usr/include $(wildcard /usr/include/$(TARGET))
 ifeq ($(shell $(CC) -v 2>&1 | grep -c "gcc version"),1)
diff --git a/icmp.c b/icmp.c
index f2cc4d6..41b9f8b 100644
--- a/icmp.c
+++ b/icmp.c
@@ -76,11 +76,6 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref)
 	if (c->no_icmp)
 		return;
 
-	/* FIXME: Workaround clang-tidy not realizing that recvfrom()
-	 * writes the socket address.  See
-	 * https://github.com/llvm/llvm-project/issues/58992
-	 */
-	memset(&sr, 0, sizeof(sr));
 	n = recvfrom(ref.fd, buf, sizeof(buf), 0, (struct sockaddr *)&sr, &sl);
 	if (n < 0)
 		return;
diff --git a/tcp.c b/tcp.c
index e58625d..b471783 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2752,19 +2752,13 @@ void tcp_listen_handler(struct ctx *c, union epoll_ref ref,
 			const struct timespec *now)
 {
 	struct sockaddr_storage sa;
+	socklen_t sl = sizeof(sa);
 	union tcp_conn *conn;
-	socklen_t sl;
 	int s;
 
 	if (c->no_tcp || c->tcp.conn_count >= TCP_MAX_CONNS)
 		return;
 
-	sl = sizeof(sa);
-	/* FIXME: Workaround clang-tidy not realizing that accept4()
-	 * writes the socket address.  See
-	 * https://github.com/llvm/llvm-project/issues/58992
-	 */
-	memset(&sa, 0, sizeof(struct sockaddr_in6));
 	s = accept4(ref.fd, (struct sockaddr *)&sa, &sl, SOCK_NONBLOCK);
 	if (s < 0)
 		return;
diff --git a/util.h b/util.h
index 41cf123..57a05fb 100644
--- a/util.h
+++ b/util.h
@@ -9,6 +9,7 @@
 #include <stdlib.h>
 #include <stdarg.h>
 #include <stdbool.h>
+#include <string.h>
 
 #include "log.h"
 
@@ -225,4 +226,44 @@ int __daemon(int pidfile_fd, int devnull_fd);
 int fls(unsigned long x);
 int write_file(const char *path, const char *buf);
 
+/*
+ * Workarounds for https://github.com/llvm/llvm-project/issues/58992
+ *
+ * For a number (maybe all) system calls that _write_ a socket address,
+ * clang-tidy doesn't register that the memory of the socket address will be
+ * initialised after the call.  This can't easily be worked around with
+ * clang-tidy suppressions, because the warning doesn't show on the syscall
+ * itself but later when we access the supposedly uninitialised field.
+ */
+static inline void sa_init(struct sockaddr *sa, socklen_t *sl)
+{
+#ifdef CLANG_TIDY_58992
+	if (sa)
+		memset(sa, 0, *sl);
+#else
+	(void)sa;
+	(void)sl;
+#endif /* CLANG_TIDY_58992 */
+}
+
+static inline ssize_t wrap_recvfrom(int sockfd, void *buf, size_t len,
+				    int flags,
+				    struct sockaddr *src_addr,
+				    socklen_t *addrlen)
+{
+	sa_init(src_addr, addrlen);
+	return recvfrom(sockfd, buf, len, flags, src_addr, addrlen);
+}
+#define recvfrom(s, buf, len, flags, src, addrlen)		\
+	wrap_recvfrom((s), (buf), (len), (flags), (src), (addrlen))
+
+static inline int wrap_accept4(int sockfd, struct sockaddr *addr,
+			       socklen_t *addrlen, int flags)
+{
+	sa_init(addr, addrlen);
+	return accept4(sockfd, addr, addrlen, flags);
+}
+#define accept4(s, addr, addrlen, flags) \
+	wrap_accept4((s), (addr), (addrlen), (flags))
+
 #endif /* UTIL_H */
-- 
@@ -9,6 +9,7 @@
 #include <stdlib.h>
 #include <stdarg.h>
 #include <stdbool.h>
+#include <string.h>
 
 #include "log.h"
 
@@ -225,4 +226,44 @@ int __daemon(int pidfile_fd, int devnull_fd);
 int fls(unsigned long x);
 int write_file(const char *path, const char *buf);
 
+/*
+ * Workarounds for https://github.com/llvm/llvm-project/issues/58992
+ *
+ * For a number (maybe all) system calls that _write_ a socket address,
+ * clang-tidy doesn't register that the memory of the socket address will be
+ * initialised after the call.  This can't easily be worked around with
+ * clang-tidy suppressions, because the warning doesn't show on the syscall
+ * itself but later when we access the supposedly uninitialised field.
+ */
+static inline void sa_init(struct sockaddr *sa, socklen_t *sl)
+{
+#ifdef CLANG_TIDY_58992
+	if (sa)
+		memset(sa, 0, *sl);
+#else
+	(void)sa;
+	(void)sl;
+#endif /* CLANG_TIDY_58992 */
+}
+
+static inline ssize_t wrap_recvfrom(int sockfd, void *buf, size_t len,
+				    int flags,
+				    struct sockaddr *src_addr,
+				    socklen_t *addrlen)
+{
+	sa_init(src_addr, addrlen);
+	return recvfrom(sockfd, buf, len, flags, src_addr, addrlen);
+}
+#define recvfrom(s, buf, len, flags, src, addrlen)		\
+	wrap_recvfrom((s), (buf), (len), (flags), (src), (addrlen))
+
+static inline int wrap_accept4(int sockfd, struct sockaddr *addr,
+			       socklen_t *addrlen, int flags)
+{
+	sa_init(addr, addrlen);
+	return accept4(sockfd, addr, addrlen, flags);
+}
+#define accept4(s, addr, addrlen, flags) \
+	wrap_accept4((s), (addr), (addrlen), (flags))
+
 #endif /* UTIL_H */
-- 
2.41.0

Re: [PATCH v2 0/2] Some static checker fixes

[Stefano Brivio]

On Thu, 21 Sep 2023 14:49:37 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> We already had a couple of places we were working around clang-tidy
> issue 58992, and the flow table series adds more.  I got sick of ugly
> inlines every time we used a syscall which returns a socket address,
> so wrote a patch to consolidate the workarounds in one place.
> 
> However, that patch added an include of <string.h> to util.h which
> exposed a classic C library gotcha in packet.c, so I fixed that too.

Applied, sorry for the delay.

-- 
Stefano