[PATCH 0/8] Avoid running cppcheck on system headers

newer
[PATCH 0/6] Fix some more static...

older
[PATCH 00/12] Minor fixups for or...

David Gibson

6 Nov 2024 6 Nov '24

7:54 a.m.

It turns out cppcheck has inbuilt knowledge of the C library, and isn't typically given the system headers. Avoiding doing so reduces the runtime to less than half of what it currently is. For non-obvious reasons, this change also exposes some new warnings. Some are real, one is a cppcheck bug. Fix and/or workaround these then make the change to the cppcheck options. This is based on my earlier series with clangd configuration and fixes. David Gibson (8): linux_dep: Generalise tcp_info.h to handling Linux extension compatibility log: Only check for FALLOC_FL_COLLAPSE_RANGE availability at runtime linux_dep: Move close_range() conditional handling to linux_dep.h linux_dep: Fix CLOSE_RANGE_UNSHARE availability handling ndp: Use const pointer for ndp_ns packet udp: Don't dereference uflow before NULL check in udp_reply_sock_handler() util: Work around cppcheck bug 6936 cppcheck: Don't check the system headers Makefile | 17 ++--------------- tcp_info.h => linux_dep.h | 32 ++++++++++++++++++++++++++++---- log.c | 9 +-------- ndp.c | 4 ++-- tcp.c | 2 +- udp.c | 5 +++-- util.h | 29 ++++++++++------------------- 7 files changed, 47 insertions(+), 51 deletions(-) rename tcp_info.h => linux_dep.h (85%) -- 2.47.0

Show replies by date

David Gibson

6 Nov 6 Nov

7:54 a.m.

New subject: [PATCH 1/8] linux_dep: Generalise tcp_info.h to handling Linux extension compatibility

tcp_info.h exists just to contain a modern enough version of struct tcp_info for our needs, removing compile time dependency on the version of kernel headers. There are several other cases where we can remove similar compile time dependencies on kernel version. Prepare for that by renaming tcp_info.h to linux_dep.h. Signed-off-by: David Gibson --- tcp_info.h => linux_dep.h | 10 ++++++---- tcp.c | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) rename tcp_info.h => linux_dep.h (97%) diff --git a/tcp_info.h b/linux_dep.h similarity index 97% rename from tcp_info.h rename to linux_dep.h index 06ccb16..8921623 100644 --- a/tcp_info.h +++ b/linux_dep.h @@ -1,13 +1,15 @@ /* SPDX-License-Identifier: GPL-2.0-or-later * Copyright Red Hat * - * Largely derived from include/linux/tcp.h in the Linux kernel + * Declarations for Linux specific dependencies */ -#ifndef TCP_INFO_H -#define TCP_INFO_H +#ifndef LINUX_DEP_H +#define LINUX_DEP_H /* struct tcp_info_linux - Information from Linux TCP_INFO getsockopt() + * + * Largely derived from include/linux/tcp.h in the Linux kernel * * Some fields returned by TCP_INFO have been there for ages and are shared with * BSD. struct tcp_info from netinet/tcp.h has only those fields. There are @@ -117,4 +119,4 @@ struct tcp_info_linux { */ }; -#endif /* TCP_INFO_H */ +#endif /* LINUX_DEP_H */ diff --git a/tcp.c b/tcp.c index 56ceba6..1bb122b 100644 --- a/tcp.c +++ b/tcp.c @@ -299,10 +299,10 @@ #include "log.h" #include "inany.h" #include "flow.h" +#include "linux_dep.h" #include "flow_table.h" #include "tcp_internal.h" -#include "tcp_info.h" #include "tcp_buf.h" /* MSS rounding: see SET_MSS() */ -- 2.47.0

David Gibson

7:54 a.m.

New subject: [PATCH 2/8] log: Only check for FALLOC_FL_COLLAPSE_RANGE availability at runtime

log.c has several #ifdefs on FALLOC_FL_COLLAPSE_RANGE that won't attempt to use it if not defined. But even if the value is defined at compile time, it might not be available in the runtime kernel, so we need to check for errors from a fallocate() call and fall back to other methods. Simplify this to only need the runtime check by using linux_dep.h to define FALLOC_FL_COLLAPSE_RANGE if it's not in the kernel headers. Signed-off-by: David Gibson --- Makefile | 5 ----- linux_dep.h | 6 ++++++ log.c | 9 +-------- 3 files changed, 7 insertions(+), 13 deletions(-) diff --git a/Makefile b/Makefile index 56bf2e8..cb91535 100644 --- a/Makefile +++ b/Makefile @@ -59,11 +59,6 @@ ifeq ($(shell :|$(CC) -fstack-protector-strong -S -xc - -o - >/dev/null 2>&1; ec FLAGS += -fstack-protector-strong endif -C := \#define _GNU_SOURCE\n\#include \nint x = FALLOC_FL_COLLAPSE_RANGE; -ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0) - EXTRA_SYSCALLS += fallocate -endif - prefix ?= /usr/local exec_prefix ?= $(prefix) bindir ?= $(exec_prefix)/bin diff --git a/linux_dep.h b/linux_dep.h index 8921623..eae9c3c 100644 --- a/linux_dep.h +++ b/linux_dep.h @@ -119,4 +119,10 @@ struct tcp_info_linux { */ }; +#include + +#ifndef FALLOC_FL_COLLAPSE_RANGE +#define FALLOC_FL_COLLAPSE_RANGE 0x08 +#endif + #endif /* LINUX_DEP_H */ diff --git a/log.c b/log.c index 19f1d98..3c1b39c 100644 --- a/log.c +++ b/log.c @@ -92,7 +92,6 @@ const char *logfile_prefix[] = { " ", /* LOG_DEBUG */ }; -#ifdef FALLOC_FL_COLLAPSE_RANGE /** * logfile_rotate_fallocate() - Write header, set log_written after fallocate() * @fd: Log file descriptor @@ -126,7 +125,6 @@ static void logfile_rotate_fallocate(int fd, const struct timespec *now) log_written -= log_cut_size; } -#endif /* FALLOC_FL_COLLAPSE_RANGE */ /** * logfile_rotate_move() - Fallback: move recent entries toward start, then cut @@ -198,21 +196,17 @@ out: * * Return: 0 on success, negative error code on failure * - * #syscalls fcntl - * - * fallocate() passed as EXTRA_SYSCALL only if FALLOC_FL_COLLAPSE_RANGE is there + * #syscalls fcntl fallocate */ static int logfile_rotate(int fd, const struct timespec *now) { if (fcntl(fd, F_SETFL, O_RDWR /* Drop O_APPEND: explicit lseek() */)) return -errno; -#ifdef FALLOC_FL_COLLAPSE_RANGE /* Only for Linux >= 3.15, extent-based ext4 or XFS, glibc >= 2.18 */ if (!fallocate(fd, FALLOC_FL_COLLAPSE_RANGE, 0, log_cut_size)) logfile_rotate_fallocate(fd, now); else -#endif logfile_rotate_move(fd, now); if (fcntl(fd, F_SETFL, O_RDWR | O_APPEND)) @@ -432,4 +426,3 @@ void logfile_init(const char *name, const char *path, size_t size) /* For FALLOC_FL_COLLAPSE_RANGE: VFS block size can be up to one page */ log_cut_size = ROUND_UP(log_size * LOGFILE_CUT_RATIO / 100, PAGE_SIZE); } - -- 2.47.0

Stefano Brivio

8:10 p.m.

New subject: [PATCH 2/8] log: Only check for FALLOC_FL_COLLAPSE_RANGE availability at runtime

On Wed, 6 Nov 2024 17:54:15 +1100 David Gibson wrote:

...

log.c has several #ifdefs on FALLOC_FL_COLLAPSE_RANGE that won't attempt to use it if not defined. But even if the value is defined at compile time, it might not be available in the runtime kernel, so we need to check for errors from a fallocate() call and fall back to other methods.

Simplify this to only need the runtime check by using linux_dep.h to define FALLOC_FL_COLLAPSE_RANGE if it's not in the kernel headers.

Signed-off-by: David Gibson --- Makefile | 5 ----- linux_dep.h | 6 ++++++ log.c | 9 +-------- 3 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/Makefile b/Makefile index 56bf2e8..cb91535 100644 --- a/Makefile +++ b/Makefile @@ -59,11 +59,6 @@ ifeq ($(shell :|$(CC) -fstack-protector-strong -S -xc - -o - >/dev/null 2>&1; ec FLAGS += -fstack-protector-strong endif

-C := \#define _GNU_SOURCE\n\#include \nint x = FALLOC_FL_COLLAPSE_RANGE; -ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0) - EXTRA_SYSCALLS += fallocate -endif - prefix ?= /usr/local exec_prefix ?= $(prefix) bindir ?= $(exec_prefix)/bin diff --git a/linux_dep.h b/linux_dep.h index 8921623..eae9c3c 100644 --- a/linux_dep.h +++ b/linux_dep.h @@ -119,4 +119,10 @@ struct tcp_info_linux { */ };

+#include + +#ifndef FALLOC_FL_COLLAPSE_RANGE +#define FALLOC_FL_COLLAPSE_RANGE 0x08 +#endif + #endif /* LINUX_DEP_H */ diff --git a/log.c b/log.c index 19f1d98..3c1b39c 100644 --- a/log.c +++ b/log.c @@ -92,7 +92,6 @@ const char *logfile_prefix[] = { " ", /* LOG_DEBUG */ };

-#ifdef FALLOC_FL_COLLAPSE_RANGE

This breaks the build on Alpine (and I suppose on Void Linux too, that is, whenever we build against musl): log.c: In function 'logfile_rotate': log.c:207:28: error: 'FALLOC_FL_COLLAPSE_RANGE' undeclared (first use in this function) 207 | if (!fallocate(fd, FALLOC_FL_COLLAPSE_RANGE, 0, log_cut_size)) | ^~~~~~~~~~~~~~~~~~~~~~~~ log.c:207:28: note: each undeclared identifier is reported only once for each function it appears in and it's fixed by including linux_dep.h from log.c. -- Stefano

David Gibson

9:54 p.m.

New subject: [PATCH 2/8] log: Only check for FALLOC_FL_COLLAPSE_RANGE availability at runtime

On Wed, Nov 06, 2024 at 08:10:41PM +0100, Stefano Brivio wrote:

...

On Wed, 6 Nov 2024 17:54:15 +1100 David Gibson wrote:

...
log.c has several #ifdefs on FALLOC_FL_COLLAPSE_RANGE that won't attempt to use it if not defined. But even if the value is defined at compile time, it might not be available in the runtime kernel, so we need to check for errors from a fallocate() call and fall back to other methods.

Simplify this to only need the runtime check by using linux_dep.h to define FALLOC_FL_COLLAPSE_RANGE if it's not in the kernel headers.

Signed-off-by: David Gibson --- Makefile | 5 ----- linux_dep.h | 6 ++++++ log.c | 9 +-------- 3 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/Makefile b/Makefile index 56bf2e8..cb91535 100644 --- a/Makefile +++ b/Makefile @@ -59,11 +59,6 @@ ifeq ($(shell :|$(CC) -fstack-protector-strong -S -xc - -o - >/dev/null 2>&1; ec FLAGS += -fstack-protector-strong endif

-C := \#define _GNU_SOURCE\n\#include \nint x = FALLOC_FL_COLLAPSE_RANGE; -ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0) - EXTRA_SYSCALLS += fallocate -endif - prefix ?= /usr/local exec_prefix ?= $(prefix) bindir ?= $(exec_prefix)/bin diff --git a/linux_dep.h b/linux_dep.h index 8921623..eae9c3c 100644 --- a/linux_dep.h +++ b/linux_dep.h @@ -119,4 +119,10 @@ struct tcp_info_linux { */ };

+#include + +#ifndef FALLOC_FL_COLLAPSE_RANGE +#define FALLOC_FL_COLLAPSE_RANGE 0x08 +#endif + #endif /* LINUX_DEP_H */ diff --git a/log.c b/log.c index 19f1d98..3c1b39c 100644 --- a/log.c +++ b/log.c @@ -92,7 +92,6 @@ const char *logfile_prefix[] = { " ", /* LOG_DEBUG */ };

-#ifdef FALLOC_FL_COLLAPSE_RANGE

This breaks the build on Alpine (and I suppose on Void Linux too, that is, whenever we build against musl):

log.c: In function 'logfile_rotate': log.c:207:28: error: 'FALLOC_FL_COLLAPSE_RANGE' undeclared (first use in this function) 207 | if (!fallocate(fd, FALLOC_FL_COLLAPSE_RANGE, 0, log_cut_size)) | ^~~~~~~~~~~~~~~~~~~~~~~~ log.c:207:28: note: each undeclared identifier is reported only once for each function it appears in

and it's fixed by including linux_dep.h from log.c.

Oops, that was careless. Fixed for the next spin. -- David Gibson (he or they) | 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

David Gibson

7:54 a.m.

New subject: [PATCH 3/8] linux_dep: Move close_range() conditional handling to linux_dep.h

util.h has some #ifdefs and weak definitions to handle compatibility with various kernel versions. Move this to linux_dep.h which handles several other similar cases. Signed-off-by: David Gibson --- linux_dep.h | 20 ++++++++++++++++++++ util.h | 19 ------------------- 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/linux_dep.h b/linux_dep.h index eae9c3c..3a41e42 100644 --- a/linux_dep.h +++ b/linux_dep.h @@ -125,4 +125,24 @@ struct tcp_info_linux { #define FALLOC_FL_COLLAPSE_RANGE 0x08 #endif +#include + +#ifdef CLOSE_RANGE_UNSHARE /* Linux kernel >= 5.9 */ +/* glibc < 2.34 and musl as of 1.2.5 need these */ +#ifndef SYS_close_range +#define SYS_close_range 436 +#endif +__attribute__ ((weak)) +/* cppcheck-suppress funcArgNamesDifferent */ +int close_range(unsigned int first, unsigned int last, int flags) { + return syscall(SYS_close_range, first, last, flags); +} +#else +/* No reasonable fallback option */ +/* cppcheck-suppress funcArgNamesDifferent */ +int close_range(unsigned int first, unsigned int last, int flags) { + return 0; +} +#endif + #endif /* LINUX_DEP_H */ diff --git a/util.h b/util.h index 2858b10..fdc3af8 100644 --- a/util.h +++ b/util.h @@ -17,7 +17,6 @@ #include #include #include -#include #include "log.h" @@ -158,24 +157,6 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags, struct ctx; -#ifdef CLOSE_RANGE_UNSHARE /* Linux kernel >= 5.9 */ -/* glibc < 2.34 and musl as of 1.2.5 need these */ -#ifndef SYS_close_range -#define SYS_close_range 436 -#endif -__attribute__ ((weak)) -/* cppcheck-suppress funcArgNamesDifferent */ -int close_range(unsigned int first, unsigned int last, int flags) { - return syscall(SYS_close_range, first, last, flags); -} -#else -/* No reasonable fallback option */ -/* cppcheck-suppress funcArgNamesDifferent */ -int close_range(unsigned int first, unsigned int last, int flags) { - return 0; -} -#endif - int sock_l4_sa(const struct ctx *c, enum epoll_type type, const void *sa, socklen_t sl, const char *ifname, bool v6only, uint32_t data); -- 2.47.0

Stefano Brivio

8:10 p.m.

New subject: [PATCH 3/8] linux_dep: Move close_range() conditional handling to linux_dep.h

On Wed, 6 Nov 2024 17:54:16 +1100 David Gibson wrote:

...

util.h has some #ifdefs and weak definitions to handle compatibility with various kernel versions. Move this to linux_dep.h which handles several other similar cases.

Signed-off-by: David Gibson --- linux_dep.h | 20 ++++++++++++++++++++ util.h | 19 ------------------- 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/linux_dep.h b/linux_dep.h index eae9c3c..3a41e42 100644 --- a/linux_dep.h +++ b/linux_dep.h @@ -125,4 +125,24 @@ struct tcp_info_linux { #define FALLOC_FL_COLLAPSE_RANGE 0x08 #endif

+#include + +#ifdef CLOSE_RANGE_UNSHARE /* Linux kernel >= 5.9 */ +/* glibc < 2.34 and musl as of 1.2.5 need these */ +#ifndef SYS_close_range +#define SYS_close_range 436 +#endif +__attribute__ ((weak)) +/* cppcheck-suppress funcArgNamesDifferent */ +int close_range(unsigned int first, unsigned int last, int flags) { + return syscall(SYS_close_range, first, last, flags); +} +#else +/* No reasonable fallback option */ +/* cppcheck-suppress funcArgNamesDifferent */ +int close_range(unsigned int first, unsigned int last, int flags) { + return 0; +} +#endif + #endif /* LINUX_DEP_H */ diff --git a/util.h b/util.h index 2858b10..fdc3af8 100644 --- a/util.h +++ b/util.h @@ -17,7 +17,6 @@ #include #include #include -#include

#include "log.h"

@@ -158,24 +157,6 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,

struct ctx;

-#ifdef CLOSE_RANGE_UNSHARE /* Linux kernel >= 5.9 */ -/* glibc < 2.34 and musl as of 1.2.5 need these */ -#ifndef SYS_close_range -#define SYS_close_range 436 -#endif -__attribute__ ((weak)) -/* cppcheck-suppress funcArgNamesDifferent */ -int close_range(unsigned int first, unsigned int last, int flags) { - return syscall(SYS_close_range, first, last, flags); -} -#else -/* No reasonable fallback option */ -/* cppcheck-suppress funcArgNamesDifferent */ -int close_range(unsigned int first, unsigned int last, int flags) { - return 0; -} -#endif -

This breaks the build on Alpine as well: util.c: In function 'close_open_files': util.c:729:22: error: implicit declaration of function 'close_range'; did you mean 'SYS_close_range'? [-Wimplicit-function-declaration] 729 | rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE); | ^~~~~~~~~~~ | SYS_close_range util.c:729:58: error: 'CLOSE_RANGE_UNSHARE' undeclared (first use in this function) 729 | rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE); | ^~~~~~~~~~~~~~~~~~~ and is fixed by including "linux_dep.h" from util.c. -- Stefano

David Gibson

9:56 p.m.

New subject: [PATCH 3/8] linux_dep: Move close_range() conditional handling to linux_dep.h

On Wed, Nov 06, 2024 at 08:10:53PM +0100, Stefano Brivio wrote:

...

On Wed, 6 Nov 2024 17:54:16 +1100 David Gibson wrote:

...
util.h has some #ifdefs and weak definitions to handle compatibility with various kernel versions. Move this to linux_dep.h which handles several other similar cases.

Signed-off-by: David Gibson --- linux_dep.h | 20 ++++++++++++++++++++ util.h | 19 ------------------- 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/linux_dep.h b/linux_dep.h index eae9c3c..3a41e42 100644 --- a/linux_dep.h +++ b/linux_dep.h @@ -125,4 +125,24 @@ struct tcp_info_linux { #define FALLOC_FL_COLLAPSE_RANGE 0x08 #endif

+#include + +#ifdef CLOSE_RANGE_UNSHARE /* Linux kernel >= 5.9 */ +/* glibc < 2.34 and musl as of 1.2.5 need these */ +#ifndef SYS_close_range +#define SYS_close_range 436 +#endif +__attribute__ ((weak)) +/* cppcheck-suppress funcArgNamesDifferent */ +int close_range(unsigned int first, unsigned int last, int flags) { + return syscall(SYS_close_range, first, last, flags); +} +#else +/* No reasonable fallback option */ +/* cppcheck-suppress funcArgNamesDifferent */ +int close_range(unsigned int first, unsigned int last, int flags) { + return 0; +} +#endif + #endif /* LINUX_DEP_H */ diff --git a/util.h b/util.h index 2858b10..fdc3af8 100644 --- a/util.h +++ b/util.h @@ -17,7 +17,6 @@ #include #include #include -#include

#include "log.h"

@@ -158,24 +157,6 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,

struct ctx;

-#ifdef CLOSE_RANGE_UNSHARE /* Linux kernel >= 5.9 */ -/* glibc < 2.34 and musl as of 1.2.5 need these */ -#ifndef SYS_close_range -#define SYS_close_range 436 -#endif -__attribute__ ((weak)) -/* cppcheck-suppress funcArgNamesDifferent */ -int close_range(unsigned int first, unsigned int last, int flags) { - return syscall(SYS_close_range, first, last, flags); -} -#else -/* No reasonable fallback option */ -/* cppcheck-suppress funcArgNamesDifferent */ -int close_range(unsigned int first, unsigned int last, int flags) { - return 0; -} -#endif -

This breaks the build on Alpine as well:

util.c: In function 'close_open_files': util.c:729:22: error: implicit declaration of function 'close_range'; did you mean 'SYS_close_range'? [-Wimplicit-function-declaration] 729 | rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE); | ^~~~~~~~~~~ | SYS_close_range util.c:729:58: error: 'CLOSE_RANGE_UNSHARE' undeclared (first use in this function) 729 | rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE); | ^~~~~~~~~~~~~~~~~~~

and is fixed by including "linux_dep.h" from util.c.

Oops again, adjusted. -- David Gibson (he or they) | 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

David Gibson

7:54 a.m.

New subject: [PATCH 4/8] linux_dep: Fix CLOSE_RANGE_UNSHARE availability handling

If CLOSE_RANGE_UNSHARE isn't defined, we define a fallback version of close_range() which is a (successful) no-op. This is broken in several ways: * It doesn't actually fix compile if using old kernel headers, because the caller of close_range() still directly uses CLOSE_RANGE_UNSHARE unprotected by ifdefs * Even if it did fix the compile, it means inconsistent behaviour between a compile time failure to find the value (we silently don't close files) and a runtime failure (we die with an error from close_range()) * Silently not closing the files we intend to close for security reasons is probably not a good idea in any case As bonus this fixes a cppcheck error I see with some different options I'm looking to apply in future. Signed-off-by: David Gibson --- linux_dep.h | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/linux_dep.h b/linux_dep.h index 3a41e42..240f50a 100644 --- a/linux_dep.h +++ b/linux_dep.h @@ -127,22 +127,18 @@ struct tcp_info_linux { #include -#ifdef CLOSE_RANGE_UNSHARE /* Linux kernel >= 5.9 */ /* glibc < 2.34 and musl as of 1.2.5 need these */ #ifndef SYS_close_range #define SYS_close_range 436 #endif +#ifndef CLOSE_RANGE_UNSHARE /* Linux kernel < 5.9 */ +#define CLOSE_RANGE_UNSHARE (1U << 1) +#endif + __attribute__ ((weak)) /* cppcheck-suppress funcArgNamesDifferent */ int close_range(unsigned int first, unsigned int last, int flags) { return syscall(SYS_close_range, first, last, flags); } -#else -/* No reasonable fallback option */ -/* cppcheck-suppress funcArgNamesDifferent */ -int close_range(unsigned int first, unsigned int last, int flags) { - return 0; -} -#endif #endif /* LINUX_DEP_H */ -- 2.47.0

Stefano Brivio

8:12 p.m.

New subject: [PATCH 4/8] linux_dep: Fix CLOSE_RANGE_UNSHARE availability handling

On Wed, 6 Nov 2024 17:54:17 +1100 David Gibson wrote:

...

If CLOSE_RANGE_UNSHARE isn't defined, we define a fallback version of close_range() which is a (successful) no-op. This is broken in several ways: * It doesn't actually fix compile if using old kernel headers, because the caller of close_range() still directly uses CLOSE_RANGE_UNSHARE unprotected by ifdefs

For users using distribution packages, that is, pretty much everybody not developing passt, this is generally not a concern, because build environments typically ship kernel headers matching the C library and the distribution version at hand.

...

* Even if it did fix the compile, it means inconsistent behaviour between a compile time failure to find the value (we silently don't close files) and a runtime failure (we die with an error from close_range())

Given that this is mostly relevant for stuff built against musl (and running on a musl-based distribution), that's not really a problem in practice. See 6e9ecf57410b ("util: Provide own version of close_range(), and no-op fallback"). But sure, I'm fine with these changes in general, as they're strictly speaking more correct than the current behaviour, minus the next point.

...

* Silently not closing the files we intend to close for security reasons is probably not a good idea in any case

It's arguably even worse to force users to run containers or guests as root. That's the reason for the no-op implementation. I don't think that introducing a dependency on a >= 5.9 kernel is a good idea. If this bothers you or cppcheck, I'd rather call close_range() (possibly the weakly aliased implementation), and log a warning on ENOSYS instead of failing.

...

As bonus this fixes a cppcheck error I see with some different options I'm looking to apply in future.

Signed-off-by: David Gibson --- linux_dep.h | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/linux_dep.h b/linux_dep.h index 3a41e42..240f50a 100644 --- a/linux_dep.h +++ b/linux_dep.h @@ -127,22 +127,18 @@ struct tcp_info_linux {

#include

-#ifdef CLOSE_RANGE_UNSHARE /* Linux kernel >= 5.9 */ /* glibc < 2.34 and musl as of 1.2.5 need these */ #ifndef SYS_close_range #define SYS_close_range 436 #endif +#ifndef CLOSE_RANGE_UNSHARE /* Linux kernel < 5.9 */ +#define CLOSE_RANGE_UNSHARE (1U << 1) +#endif + __attribute__ ((weak)) /* cppcheck-suppress funcArgNamesDifferent */ int close_range(unsigned int first, unsigned int last, int flags) { return syscall(SYS_close_range, first, last, flags); } -#else -/* No reasonable fallback option */ -/* cppcheck-suppress funcArgNamesDifferent */ -int close_range(unsigned int first, unsigned int last, int flags) { - return 0; -} -#endif

#endif /* LINUX_DEP_H */

-- Stefano

David Gibson

10:01 p.m.

New subject: [PATCH 4/8] linux_dep: Fix CLOSE_RANGE_UNSHARE availability handling

On Wed, Nov 06, 2024 at 08:12:23PM +0100, Stefano Brivio wrote:

...

On Wed, 6 Nov 2024 17:54:17 +1100 David Gibson wrote:

...
If CLOSE_RANGE_UNSHARE isn't defined, we define a fallback version of close_range() which is a (successful) no-op. This is broken in several ways: * It doesn't actually fix compile if using old kernel headers, because the caller of close_range() still directly uses CLOSE_RANGE_UNSHARE unprotected by ifdefs

For users using distribution packages, that is, pretty much everybody not developing passt, this is generally not a concern, because build environments typically ship kernel headers matching the C library and the distribution version at hand.

Unlike some of the other things fixed recently, this one is not related to compile time versus runtime checks. This one is simply broken compiling against an older kernel, regardless of the runtime version. Without this patch, close_open_files() directly uses CLOSE_RANGE_UNSHARE unprotected by an #ifdef.

...

...
* Even if it did fix the compile, it means inconsistent behaviour between a compile time failure to find the value (we silently don't close files) and a runtime failure (we die with an error from close_range())

Given that this is mostly relevant for stuff built against musl (and running on a musl-based distribution), that's not really a problem in practice. See 6e9ecf57410b ("util: Provide own version of close_range(), and no-op fallback").

But sure, I'm fine with these changes in general, as they're strictly speaking more correct than the current behaviour, minus the next point.

...
* Silently not closing the files we intend to close for security reasons is probably not a good idea in any case

It's arguably even worse to force users to run containers or guests as root. That's the reason for the no-op implementation.

Uh... what's the connection to running as root?

...

I don't think that introducing a dependency on a >= 5.9 kernel is a good idea.

We already have a dependency on compiling against a >= 5.9 kernel, see above.

...

If this bothers you or cppcheck, I'd rather call close_range() (possibly the weakly aliased implementation), and log a warning on ENOSYS instead of failing.

We could do that. We'd need to consider EINVAL as well as ENOSYS, for the case that the syscall exists, but the CLOSE_RANGE_UNSHARE flag isn't recognized.

...

...
As bonus this fixes a cppcheck error I see with some different options I'm looking to apply in future.

Signed-off-by: David Gibson --- linux_dep.h | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/linux_dep.h b/linux_dep.h index 3a41e42..240f50a 100644 --- a/linux_dep.h +++ b/linux_dep.h @@ -127,22 +127,18 @@ struct tcp_info_linux {

#include

-#ifdef CLOSE_RANGE_UNSHARE /* Linux kernel >= 5.9 */ /* glibc < 2.34 and musl as of 1.2.5 need these */ #ifndef SYS_close_range #define SYS_close_range 436 #endif +#ifndef CLOSE_RANGE_UNSHARE /* Linux kernel < 5.9 */ +#define CLOSE_RANGE_UNSHARE (1U << 1) +#endif + __attribute__ ((weak)) /* cppcheck-suppress funcArgNamesDifferent */ int close_range(unsigned int first, unsigned int last, int flags) { return syscall(SYS_close_range, first, last, flags); } -#else -/* No reasonable fallback option */ -/* cppcheck-suppress funcArgNamesDifferent */ -int close_range(unsigned int first, unsigned int last, int flags) { - return 0; -} -#endif

#endif /* LINUX_DEP_H */

-- David Gibson (he or they) | 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

Stefano Brivio

7 Nov 7 Nov

8:03 a.m.

New subject: [PATCH 4/8] linux_dep: Fix CLOSE_RANGE_UNSHARE availability handling

On Thu, 7 Nov 2024 08:01:11 +1100 David Gibson wrote:

...

On Wed, Nov 06, 2024 at 08:12:23PM +0100, Stefano Brivio wrote:

...
On Wed, 6 Nov 2024 17:54:17 +1100 David Gibson wrote:

...
If CLOSE_RANGE_UNSHARE isn't defined, we define a fallback version of close_range() which is a (successful) no-op. This is broken in several ways: * It doesn't actually fix compile if using old kernel headers, because the caller of close_range() still directly uses CLOSE_RANGE_UNSHARE unprotected by ifdefs

For users using distribution packages, that is, pretty much everybody not developing passt, this is generally not a concern, because build environments typically ship kernel headers matching the C library and the distribution version at hand.

Unlike some of the other things fixed recently, this one is not related to compile time versus runtime checks. This one is simply broken compiling against an older kernel, regardless of the runtime version. Without this patch, close_open_files() directly uses CLOSE_RANGE_UNSHARE unprotected by an #ifdef.

Ah, right, it's a different case, indeed.

...

...
...
* Even if it did fix the compile, it means inconsistent behaviour between a compile time failure to find the value (we silently don't close files) and a runtime failure (we die with an error from close_range())

Given that this is mostly relevant for stuff built against musl (and running on a musl-based distribution), that's not really a problem in practice. See 6e9ecf57410b ("util: Provide own version of close_range(), and no-op fallback").

But sure, I'm fine with these changes in general, as they're strictly speaking more correct than the current behaviour, minus the next point.

...
* Silently not closing the files we intend to close for security reasons is probably not a good idea in any case

It's arguably even worse to force users to run containers or guests as root. That's the reason for the no-op implementation.

Uh... what's the connection to running as root?

That you can't run pasta or passt altogether, and if you need some features they provide, not covered by libslirp, you might as well need to switch to run things as root.

...

...
I don't think that introducing a dependency on a >= 5.9 kernel is a good idea.

We already have a dependency on compiling against a >= 5.9 kernel, see above.

Yes, but that would be a trivial fix.

...

...
If this bothers you or cppcheck, I'd rather call close_range() (possibly the weakly aliased implementation), and log a warning on ENOSYS instead of failing.

We could do that. We'd need to consider EINVAL as well as ENOSYS, for the case that the syscall exists, but the CLOSE_RANGE_UNSHARE flag isn't recognized.

Right... I think we should do that. -- Stefano

David Gibson

6 Nov 6 Nov

7:54 a.m.

New subject: [PATCH 5/8] ndp: Use const pointer for ndp_ns packet

We don't modify this structure at all. For some reason cppcheck doesn't catch this with our current options, but did when I was experimenting with some different options. Signed-off-by: David Gibson --- ndp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ndp.c b/ndp.c index a1ee834..faae408 100644 --- a/ndp.c +++ b/ndp.c @@ -234,8 +234,8 @@ int ndp(struct ctx *c, const struct icmp6hdr *ih, const struct in6_addr *saddr, return 1; if (ih->icmp6_type == NS) { - struct ndp_ns *ns = packet_get(p, 0, 0, sizeof(struct ndp_ns), - NULL); + const struct ndp_ns *ns = + packet_get(p, 0, 0, sizeof(struct ndp_ns), NULL); if (!ns) return -1; -- 2.47.0

David Gibson

7:54 a.m.

New subject: [PATCH 6/8] udp: Don't dereference uflow before NULL check in udp_reply_sock_handler()

We have an ASSERT() verifying that we're able to look up the flow in udp_reply_sock_handler(). However, we dereference uflow before that in an initializer, rather defeating the point. Rearrange to avoid that. Signed-off-by: David Gibson --- udp.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/udp.c b/udp.c index 0c01067..4be165f 100644 --- a/udp.c +++ b/udp.c @@ -644,12 +644,13 @@ void udp_reply_sock_handler(const struct ctx *c, union epoll_ref ref, flow_sidx_t tosidx = flow_sidx_opposite(ref.flowside); const struct flowside *toside = flowside_at_sidx(tosidx); struct udp_flow *uflow = udp_at_sidx(ref.flowside); - int from_s = uflow->s[ref.flowside.sidei]; uint8_t topif = pif_at_sidx(tosidx); - int n, i; + int n, i, from_s; ASSERT(!c->no_udp && uflow); + from_s = uflow->s[ref.flowside.sidei]; + if (udp_sock_errs(c, from_s, events) < 0) { flow_err(uflow, "Unrecoverable error on reply socket"); flow_err_details(uflow); -- 2.47.0

David Gibson

7:54 a.m.

New subject: [PATCH 7/8] util: Work around cppcheck bug 6936

While experimenting with cppcheck options, I hit several false positives caused by this bug: https://trac.cppcheck.net/ticket/13227 Signed-off-by: David Gibson --- Makefile | 2 +- util.h | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index cb91535..0ba85b4 100644 --- a/Makefile +++ b/Makefile @@ -183,5 +183,5 @@ cppcheck: $(PASST_SRCS) $(HEADERS) $(SYSTEM_INCLUDES:%=--suppress=unmatchedSuppression:%/*) \ --inline-suppr \ --suppress=unusedStructMember \ - $(filter -D%,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) \ + $(filter -D%,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) -D CPPCHECK_6936 \ $(PASST_SRCS) $(HEADERS) diff --git a/util.h b/util.h index fdc3af8..1a4dfd4 100644 --- a/util.h +++ b/util.h @@ -67,6 +67,15 @@ #define STRINGIFY(x) #x #define STR(x) STRINGIFY(x) +#ifdef CPPCHECK_6936 +/* Some cppcheck versions get confused by aborts inside a loop, causing + * it to give false positive uninitialised variable warnings later in + * the function, because it doesn't realise the non-initialising path + * already exited. See https://trac.cppcheck.net/ticket/13227 + */ +#define ASSERT(expr) \ + ((expr) ? (void)0 : abort()) +#else #define ASSERT(expr) \ do { \ if (!(expr)) { \ @@ -78,6 +87,7 @@ abort(); \ } \ } while (0) +#endif #ifdef P_tmpdir #define TMPDIR P_tmpdir -- 2.47.0

David Gibson

7:54 a.m.

New subject: [PATCH 8/8] cppcheck: Don't check the system headers

We pass -I options to cppcheck so that it will find the system headers. Then we need to pass a bunch more options to suppress the zillions of cppcheck errors found in those headers. It turns out, however, that it's not recommended to give the system headers to cppcheck anyway. Instead it has built-in knowledge of the ANSI libc and uses that as the basis of its checks. We do need to suppress missingIncludeSystem warnings instead though. Not bothering with the system headers makes the cppcheck runtime go from ~37s to ~14s on my machine, which is a pretty nice win. Signed-off-by: David Gibson --- Makefile | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/Makefile b/Makefile index 0ba85b4..258d298 100644 --- a/Makefile +++ b/Makefile @@ -163,11 +163,6 @@ clang-tidy: $(PASST_SRCS) $(HEADERS) clang-tidy $(PASST_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) -VER := $(shell $(CC) -dumpversion) -SYSTEM_INCLUDES += /usr/lib/gcc/$(TARGET)/$(VER)/include -endif cppcheck: $(PASST_SRCS) $(HEADERS) if cppcheck --check-level=exhaustive /dev/null > /dev/null 2>&1; then \ CPPCHECK_EXHAUSTIVE="--check-level=exhaustive"; \ @@ -177,11 +172,8 @@ cppcheck: $(PASST_SRCS) $(HEADERS) cppcheck --std=c11 --error-exitcode=1 --enable=all --force \ --inconclusive --library=posix --quiet \ $${CPPCHECK_EXHAUSTIVE} \ - $(SYSTEM_INCLUDES:%=-I%) \ - $(SYSTEM_INCLUDES:%=--config-exclude=%) \ - $(SYSTEM_INCLUDES:%=--suppress=*:%/*) \ - $(SYSTEM_INCLUDES:%=--suppress=unmatchedSuppression:%/*) \ --inline-suppr \ + --suppress=missingIncludeSystem \ --suppress=unusedStructMember \ $(filter -D%,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) -D CPPCHECK_6936 \ $(PASST_SRCS) $(HEADERS) -- 2.47.0

Stefano Brivio

7 Nov 7 Nov

3:55 p.m.

On Wed, 6 Nov 2024 17:54:13 +1100 David Gibson wrote:

...

It turns out cppcheck has inbuilt knowledge of the C library, and isn't typically given the system headers. Avoiding doing so reduces the runtime to less than half of what it currently is.

For non-obvious reasons, this change also exposes some new warnings. Some are real, one is a cppcheck bug. Fix and/or workaround these then make the change to the cppcheck options.

This is based on my earlier series with clangd configuration and fixes.

David Gibson (8): linux_dep: Generalise tcp_info.h to handling Linux extension compatibility log: Only check for FALLOC_FL_COLLAPSE_RANGE availability at runtime linux_dep: Move close_range() conditional handling to linux_dep.h linux_dep: Fix CLOSE_RANGE_UNSHARE availability handling ndp: Use const pointer for ndp_ns packet udp: Don't dereference uflow before NULL check in udp_reply_sock_handler() util: Work around cppcheck bug 6936 cppcheck: Don't check the system headers

Applied, except for 2/8, 3/8, 4/8, and 8/8. I had to skip 8/8 as well for the moment because, contrary to what I reported earlier by mistake, it's actually the one leading to the new cppcheck warning: dhcpv6.c:334:14: style: The comparison 'ia_type == 3' is always true. [knownConditionTrueFalse] if (ia_type == OPT_IA_NA) { ...also on x86. The difference is not the architecture, rather the cppcheck version: it happens with 2.16.x but not with 2.14.x. I'm posting a patch for that soon. -- Stefano

David Gibson

8 Nov 8 Nov

12:58 a.m.

On Thu, Nov 07, 2024 at 03:55:16PM +0100, Stefano Brivio wrote:

...

On Wed, 6 Nov 2024 17:54:13 +1100 David Gibson wrote:

...
It turns out cppcheck has inbuilt knowledge of the C library, and isn't typically given the system headers. Avoiding doing so reduces the runtime to less than half of what it currently is.

For non-obvious reasons, this change also exposes some new warnings. Some are real, one is a cppcheck bug. Fix and/or workaround these then make the change to the cppcheck options.

This is based on my earlier series with clangd configuration and fixes.

David Gibson (8): linux_dep: Generalise tcp_info.h to handling Linux extension compatibility log: Only check for FALLOC_FL_COLLAPSE_RANGE availability at runtime linux_dep: Move close_range() conditional handling to linux_dep.h linux_dep: Fix CLOSE_RANGE_UNSHARE availability handling ndp: Use const pointer for ndp_ns packet udp: Don't dereference uflow before NULL check in udp_reply_sock_handler() util: Work around cppcheck bug 6936 cppcheck: Don't check the system headers

Applied, except for 2/8, 3/8, 4/8, and 8/8.

I had to skip 8/8 as well for the moment because, contrary to what I reported earlier by mistake, it's actually the one leading to the new cppcheck warning:

dhcpv6.c:334:14: style: The comparison 'ia_type == 3' is always true. [knownConditionTrueFalse] if (ia_type == OPT_IA_NA) {

...also on x86. The difference is not the architecture, rather the cppcheck version: it happens with 2.16.x but not with 2.14.x.

I'm posting a patch for that soon.

Oh, interesting. 8/8 exposed several new warnings for me too (hence most of this series), but not that one. -- David Gibson (he or they) | 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

187

Age (days ago)

188

Last active (days ago)

List overview

Download

17 comments

2 participants

participants (2)

David Gibson
Stefano Brivio

[PATCH 0/8] Avoid running cppcheck on system headers

tags

participants (2)