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
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 <david(a)gibson.dropbear.id.au>
---
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
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 <david(a)gibson.dropbear.id.au>
---
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 <fcntl.h>\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 <linux/falloc.h>
+
+#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
On Wed, 6 Nov 2024 17:54:15 +1100 David Gibson <david(a)gibson.dropbear.id.au> 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 <david(a)gibson.dropbear.id.au> --- 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 <fcntl.h>\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 <linux/falloc.h> + +#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_RANGEThis 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
On Wed, Nov 06, 2024 at 08:10:41PM +0100, Stefano Brivio wrote:On Wed, 6 Nov 2024 17:54:15 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote: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/~dgibsonlog.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 <david(a)gibson.dropbear.id.au> --- 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 <fcntl.h>\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 <linux/falloc.h> + +#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_RANGEThis 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.
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 <david(a)gibson.dropbear.id.au>
---
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 <linux/close_range.h>
+
+#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 <arpa/inet.h>
#include <unistd.h>
#include <sys/syscall.h>
-#include <linux/close_range.h>
#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
On Wed, 6 Nov 2024 17:54:16 +1100 David Gibson <david(a)gibson.dropbear.id.au> 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 <david(a)gibson.dropbear.id.au> --- 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 <linux/close_range.h> + +#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 <arpa/inet.h> #include <unistd.h> #include <sys/syscall.h> -#include <linux/close_range.h> #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
On Wed, Nov 06, 2024 at 08:10:53PM +0100, Stefano Brivio wrote:On Wed, 6 Nov 2024 17:54:16 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote: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/~dgibsonutil.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 <david(a)gibson.dropbear.id.au> --- 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 <linux/close_range.h> + +#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 <arpa/inet.h> #include <unistd.h> #include <sys/syscall.h> -#include <linux/close_range.h> #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.
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 <david(a)gibson.dropbear.id.au>
---
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 <linux/close_range.h>
-#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
On Wed, 6 Nov 2024 17:54:17 +1100 David Gibson <david(a)gibson.dropbear.id.au> 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 ifdefsFor 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 caseIt'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 <david(a)gibson.dropbear.id.au> --- 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 <linux/close_range.h> -#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
On Wed, Nov 06, 2024 at 08:12:23PM +0100, Stefano Brivio wrote:On Wed, 6 Nov 2024 17:54:17 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote: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.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 ifdefsFor 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.Uh... what's the connection to running as root?* 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 caseIt'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.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.-- 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/~dgibsonAs bonus this fixes a cppcheck error I see with some different options I'm looking to apply in future. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- 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 <linux/close_range.h> -#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 */
On Thu, 7 Nov 2024 08:01:11 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:On Wed, Nov 06, 2024 at 08:12:23PM +0100, Stefano Brivio wrote:Ah, right, it's a different case, indeed.On Wed, 6 Nov 2024 17:54:17 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote: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.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 ifdefsFor 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.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.Uh... what's the connection to running as root?* 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 caseIt's arguably even worse to force users to run containers or guests as root. That's the reason for the no-op implementation.Yes, but that would be a trivial fix.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.Right... I think we should do that. -- StefanoIf 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.
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 <david(a)gibson.dropbear.id.au>
---
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
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 <david(a)gibson.dropbear.id.au>
---
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
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 <david(a)gibson.dropbear.id.au> --- 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
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 <david(a)gibson.dropbear.id.au>
---
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
On Wed, 6 Nov 2024 17:54:13 +1100 David Gibson <david(a)gibson.dropbear.id.au> 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 headersApplied, 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
On Thu, Nov 07, 2024 at 03:55:16PM +0100, Stefano Brivio wrote:On Wed, 6 Nov 2024 17:54:13 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote: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/~dgibsonIt 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 headersApplied, 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.