From 4b95bda8c155db338409bd0b466ff6f0b8ac0971 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Mon, 2 Nov 2026 16:08:23 +0100 Subject: [PATCH] clang-tidy: flag direct open(), openat() and creat() calls The tree opens files via the xopenat() family, which sets O_CLOEXEC on its own and refuses to create files through dangling symlinks. Nothing stops new code from calling open() directly though. The repart manifest code added such a call after the migration. Let's add open(), openat() and creat() to the custom functions of bugprone-unsafe-functions so that clang-tidy reports direct calls. The functions that implement xopenat() and fd_reopen() call the syscalls directly, and so do the seccomp tests and the O_EMPTYPATH probe in test-fd-util.c. We suppress the check in those places. openat2() isn't on the list. override/fcntl.h defines openat2 as a macro for openat2_shim(), and the check ignores calls that come from a macro expansion. An entry for openat2() wouldn't report anything. --- .clang-tidy | 3 +++ docs/CODING_STYLE.md | 4 ++++ src/basic/fd-util.c | 3 +++ src/basic/fs-util.c | 4 ++++ src/test/test-fd-util.c | 5 +++-- src/test/test-seccomp.c | 10 +++++++++- 6 files changed, 26 insertions(+), 3 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index c784c9532b1..ccd2d12069d 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -63,6 +63,9 @@ CheckOptions: ^ntohs$,be16toh(),is confusing; ^strerror$,STRERROR() or printf %m,is not thread-safe; ^accept$,accept4(),is not O_CLOEXEC-safe; + ^open$,xopenat(),does not set O_CLOEXEC by default; + ^openat$,xopenat(),does not set O_CLOEXEC by default; + ^creat$,xopenat(),is an obsolete relic that does not set O_CLOEXEC; ^dirname$,path_extract_directory(),is icky; ^basename$,path_extract_filename(),is icky; ^setmntent$,libmount_parse_fstab(),libmount parser should be used instead; diff --git a/docs/CODING_STYLE.md b/docs/CODING_STYLE.md index df3730e493b..378de8248e2 100644 --- a/docs/CODING_STYLE.md +++ b/docs/CODING_STYLE.md @@ -911,6 +911,10 @@ SPDX-License-Identifier: LGPL-2.1-or-later `execve()` the `O_CLOEXEC` bit must be cleared explicitly via `fd_cloexec(fd, false)` right before the `execve()`. +- Open files with `xopenat()` or one of the other helpers above instead of + calling `open()`, `openat()` or `creat()` directly. This also applies to + tests. clang-tidy reports direct calls. + - It's a good idea to use `O_NONBLOCK` when opening 'foreign' regular files, i.e. file system objects that are supposed to be regular files whose paths were specified by the user and hence might actually refer to other types of diff --git a/src/basic/fd-util.c b/src/basic/fd-util.c index b791d895da6..93a1bd5137f 100644 --- a/src/basic/fd-util.c +++ b/src/basic/fd-util.c @@ -783,6 +783,8 @@ finish: return r; } +/* fd_reopen() calls open() and openat() directly. clang-tidy reports direct calls in all other code. */ +/* NOLINTBEGIN(bugprone-unsafe-functions) */ int fd_reopen(int fd, int flags) { assert(wildcard_fd_is_valid(fd)); assert(!FLAGS_SET(flags, O_CREAT)); @@ -854,6 +856,7 @@ int fd_reopen(int fd, int flags) { return new_fd; } +/* NOLINTEND(bugprone-unsafe-functions) */ static int fd_reopen_propagate_append_and_position(int fd, int flags) { /* Invokes fd_reopen(fd, flags), but propagates O_APPEND if set on original fd, and also tries to diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index 125079ae8fa..94b340fa9a3 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -1081,6 +1081,9 @@ int open_mkdir_at_full_label(int dirfd, const char *path, int flags, XOpenFlags return TAKE_FD(fd); } +/* openat_report_new() and xopenat_full_label() call open() and openat() directly. clang-tidy reports + * direct calls in all other code. */ +/* NOLINTBEGIN(bugprone-unsafe-functions) */ int openat_report_new(int dirfd, const char *pathname, int flags, mode_t mode, bool *ret_newly_created) { int fd, r; @@ -1531,6 +1534,7 @@ error: return r; } +/* NOLINTEND(bugprone-unsafe-functions) */ int xopenat_lock_full_label( int dir_fd, diff --git a/src/test/test-fd-util.c b/src/test/test-fd-util.c index d68f06c30eb..13625f6e732 100644 --- a/src/test/test-fd-util.c +++ b/src/test/test-fd-util.c @@ -997,8 +997,9 @@ TEST(fd_reopen_emptypath) { ASSERT_OK(tfd = mkdtemp_open(/* template= */ NULL, /* flags= */ 0, &t)); - /* Older kernels ignore the flag and miss the empty path */ - fd = openat(tfd, "", O_PATH|O_CLOEXEC|O_EMPTYPATH); + /* Older kernels ignore the flag and miss the empty path. xopenat() returns -ENOENT for an empty path + * without calling openat(), so the probe calls openat() directly. */ + fd = openat(tfd, "", O_PATH|O_CLOEXEC|O_EMPTYPATH); /* NOLINT(bugprone-unsafe-functions) */ if (fd < 0 && errno == ENOENT) return (void) log_tests_skipped("kernel lacks O_EMPTYPATH"); ASSERT_OK_ERRNO(fd); diff --git a/src/test/test-seccomp.c b/src/test/test-seccomp.c index 39c6752814b..063aa905751 100644 --- a/src/test/test-seccomp.c +++ b/src/test/test-seccomp.c @@ -1013,7 +1013,7 @@ static int real_open(const char *path, int flags, mode_t mode) { #if defined __NR_open && __NR_open >= 0 return (int) syscall(__NR_open, path, flags, mode); #else - return open(path, flags, mode); + return open(path, flags, mode); /* NOLINT(bugprone-unsafe-functions) */ #endif } @@ -1035,6 +1035,9 @@ static int try_fchmodat2(int dirfd, const char *path, mode_t mode, int flags) { return RET_NERRNO(fchmodat(dirfd, path, mode, flags)); } +/* restrict_suid_sgid tests the seccomp filter on the creat() and openat() syscalls. The test has to call + * creat() and openat() directly. */ +/* NOLINTBEGIN(bugprone-unsafe-functions) */ TEST(restrict_suid_sgid) { int r; @@ -1228,7 +1231,11 @@ TEST(restrict_suid_sgid) { _exit(EXIT_SUCCESS); } } +/* NOLINTEND(bugprone-unsafe-functions) */ +/* After seccomp_suppress_sync(), the open() syscall fails with EINVAL when O_SYNC is set. The test calls + * open() directly to check that. */ +/* NOLINTBEGIN(bugprone-unsafe-functions) */ static void test_seccomp_suppress_sync_child(void) { _cleanup_(unlink_and_freep) char *path = NULL; _cleanup_close_ int fd = -EBADF; @@ -1260,6 +1267,7 @@ static void test_seccomp_suppress_sync_child(void) { ASSERT_ERROR_ERRNO(fsync(-1), EBADF); ASSERT_ERROR_ERRNO(syncfs(-1), EBADF); } +/* NOLINTEND(bugprone-unsafe-functions) */ TEST(seccomp_suppress_sync) { int r;