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. David Gibson (2): packet: Avoid shadowing index(3) util: Consolidate and improve workarounds for clang-tidy issue 58992 Makefile | 2 +- icmp.c | 5 ----- packet.c | 28 ++++++++++++++-------------- packet.h | 10 +++++----- tcp.c | 8 +------- util.h | 41 +++++++++++++++++++++++++++++++++++++++++ 6 files changed, 62 insertions(+), 32 deletions(-) -- 2.41.0
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 few
places in packet.[ch] which hit this trap so rename the variables to avoid
it.
Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
packet.c | 28 ++++++++++++++--------------
packet.h | 10 +++++-----
2 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/packet.c b/packet.c
index ce807e2..8e3a87c 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++;
}
@@ -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 { \
--
2.41.0
On Fri, 15 Sep 2023 16:43:36 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote: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 few places in packet.[ch] which hit this trap so rename the variables to avoid it. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- packet.c | 28 ++++++++++++++-------------- packet.h | 10 +++++----- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/packet.c b/packet.c index ce807e2..8e3a87c 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++; } @@ -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,Really minor, but... the comment about @index should be updated as well. -- Stefano
On Mon, Sep 18, 2023 at 10:16:00AM +0200, Stefano Brivio wrote:On Fri, 15 Sep 2023 16:43:36 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:Oops, yes. Fixed. -- 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/~dgibsonA 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 few places in packet.[ch] which hit this trap so rename the variables to avoid it. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- packet.c | 28 ++++++++++++++-------------- packet.h | 10 +++++----- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/packet.c b/packet.c index ce807e2..8e3a87c 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++; } @@ -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,Really minor, but... the comment about @index should be updated as well.
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(a)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 dd3142d..a1fc94f 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 195023f..f750bf0 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 */
--
2.41.0
On Fri, 15 Sep 2023 16:43:37 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote: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.I'm probably missing something, but wouldn't it be more obvious to conditionally define the wrapper itself? That is, #ifdef CLANG_TIDY_58992 # define recvfrom(s, buf, len, flags, src, addrlen) \ wrap_recvfrom((s), (buf), (len), (flags), (src), (addrlen)) #endif instead of doing that in sa_init()? -- Stefano
On Mon, Sep 18, 2023 at 10:16:08AM +0200, Stefano Brivio wrote:On Fri, 15 Sep 2023 16:43:37 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:Eh.. maybe? I was going for minimal differences in the preprocessed code between the two cases, to reduce the chances of missing some unrelated real problem due to the fact we're kind of lying to our static checker. I don't feel that strongly about it though, so whichever you'd prefer is fine. -- 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/~dgibsonWe 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.I'm probably missing something, but wouldn't it be more obvious to conditionally define the wrapper itself? That is, #ifdef CLANG_TIDY_58992 # define recvfrom(s, buf, len, flags, src, addrlen) \ wrap_recvfrom((s), (buf), (len), (flags), (src), (addrlen)) #endif instead of doing that in sa_init()?
On Tue, 19 Sep 2023 11:08:51 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:On Mon, Sep 18, 2023 at 10:16:08AM +0200, Stefano Brivio wrote:Ah, okay, I see your point -- in both cases we'd call a function (even though one is going to be inlined, the other one not necessarily)... sure, it makes sense. -- StefanoOn Fri, 15 Sep 2023 16:43:37 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:Eh.. maybe? I was going for minimal differences in the preprocessed code between the two cases, to reduce the chances of missing some unrelated real problem due to the fact we're kind of lying to our static checker.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.I'm probably missing something, but wouldn't it be more obvious to conditionally define the wrapper itself? That is, #ifdef CLANG_TIDY_58992 # define recvfrom(s, buf, len, flags, src, addrlen) \ wrap_recvfrom((s), (buf), (len), (flags), (src), (addrlen)) #endif instead of doing that in sa_init()?