[PATCH 0/2] Some static checker fixes - dev - passt

newer
[PATCH] dhcp: Actually note down...

[PATCH 0/2] Some static checker fixes

older
[PATCH] passt: introduce...

David Gibson

15 Sep 2023 15 Sep '23

8:43 a.m.

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

Reply

Sign in to reply online Use email software

Show replies by date

David Gibson

15 Sep 15 Sep

8:43 a.m.

New subject: [PATCH 1/2] packet: Avoid shadowing index(3)

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

Reply

Sign in to reply online Use email software

Stefano Brivio

18 Sep 18 Sep

10:16 a.m.

New subject: [PATCH 1/2] packet: Avoid shadowing index(3)

On Fri, 15 Sep 2023 16:43:36 +1000 David Gibson 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 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 --- 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

Reply

Sign in to reply online Use email software

David Gibson

19 Sep 19 Sep

3:05 a.m.

New subject: [PATCH 1/2] packet: Avoid shadowing index(3)

On Mon, Sep 18, 2023 at 10:16:00AM +0200, Stefano Brivio wrote:

On Fri, 15 Sep 2023 16:43:36 +1000 David Gibson 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 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 --- 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.

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/~dgibson

Reply

Sign in to reply online Use email software

David Gibson

15 Sep 15 Sep

8:43 a.m.

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

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 --- 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 #include #include +#include #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

Reply

Sign in to reply online Use email software

Stefano Brivio

18 Sep 18 Sep

10:16 a.m.

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

On Fri, 15 Sep 2023 16:43:37 +1000 David Gibson 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

Reply

Sign in to reply online Use email software

David Gibson

19 Sep 19 Sep

3:08 a.m.

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

On Mon, Sep 18, 2023 at 10:16:08AM +0200, Stefano Brivio wrote:

On Fri, 15 Sep 2023 16:43:37 +1000 David Gibson 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()?

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/~dgibson

Reply

Sign in to reply online Use email software

Stefano Brivio

20 Sep 20 Sep

12:17 a.m.

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

On Tue, 19 Sep 2023 11:08:51 +1000 David Gibson wrote:

On Mon, Sep 18, 2023 at 10:16:08AM +0200, Stefano Brivio wrote:

...
On Fri, 15 Sep 2023 16:43:37 +1000 David Gibson 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()?

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.

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

Reply

Sign in to reply online Use email software

627

Age (days ago)

631

Last active (days ago)

Download

7 comments

2 participants

tags

participants (2)

David Gibson
Stefano Brivio