From b1236ce38bb2f918c7150df36b32fcb84b1a23fb Mon Sep 17 00:00:00 2001 From: Stefan Hansson Date: Sun, 13 Apr 2025 20:35:49 +0200 Subject: [PATCH] missing_fcntl: Introduce O_ACCMODE_STRICT On musl, O_ACCMODE is defined as (03|O_SEARCH), unlike glibc which defines it as (O_RDONLY|O_WRONLY|O_RDWR). Additionally, O_SEARCH is simply defined as O_PATH. This causes problems for systemd on musl, as it changes the behaviour of open_mkdir_at_full() to return -EINVAL if O_PATH is included in flags due to the fact that O_ACCMODE includes O_SEARCH (i.e. O_PATH). Consequently, this makes the test-fs-util test fail. Upstream musl seems content with this behaviour and doesn't seem interested in matching glibc's behaviour due to that defining it this way allows for O_SEARCH to match POSIX better by allowing it to open directories where read permission is missing. Apparently musl does some emulation in other places to make this work more consistently as well. Initially I took the approach of working around this by redefining O_SEARCH as O_RDONLY if O_SEARCH == O_PATH. This fixes the test and is the approach taken by both XZ[1] and Gzip[2][3], but was not taken as redefining system headers potentially could be problematic. Instead, introduce O_ACCMODE_STRICT which just is a copy of glibc's O_ACCMODE and use it everywhere. This way we don't have to deal with unusual definitions of O_ACCMODE from C standard libraries other than glibc. [1]: https://git.tukaani.org/?p=xz.git;a=blob;f=src/xz/file_io.c;h=8c83269b13fa31284f7ea5f3627a1dfbce7d6e14;hb=HEAD#l72 [2]: https://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/fcntl.in.h (lines 380 and 396, commit d7f551b30f3f2a0fa57c1b10c12f4eea41a9b89e) [3]: https://lists.gnu.org/archive/html/bug-gzip/2025-01/msg00000.html --- src/basic/fd-util.c | 6 +++--- src/basic/fs-util.c | 2 +- src/basic/missing_fcntl.h | 6 ++++++ src/core/exec-invoke.c | 6 +++--- src/libsystemd/sd-journal/journal-file.c | 6 +++--- src/libsystemd/sd-journal/journal-file.h | 4 ++-- src/login/logind-session-dbus.c | 2 +- src/mountfsd/mountwork.c | 2 +- src/shared/data-fd-util.c | 4 ++-- src/shared/journal-file-util.c | 4 ++-- src/shared/loop-util.c | 2 +- src/test/test-fileio.c | 6 +++--- 12 files changed, 28 insertions(+), 22 deletions(-) diff --git a/src/basic/fd-util.c b/src/basic/fd-util.c index a6a63454fc..5f17ed1792 100644 --- a/src/basic/fd-util.c +++ b/src/basic/fd-util.c @@ -1001,13 +1001,13 @@ int fd_verify_safe_flags_full(int fd, int extra_flags) { if (flags < 0) return -errno; - unexpected_flags = flags & ~(O_ACCMODE|O_NOFOLLOW|RAW_O_LARGEFILE|extra_flags); + unexpected_flags = flags & ~(O_ACCMODE_STRICT|O_NOFOLLOW|RAW_O_LARGEFILE|extra_flags); if (unexpected_flags != 0) return log_debug_errno(SYNTHETIC_ERRNO(EREMOTEIO), "Unexpected flags set for extrinsic fd: 0%o", (unsigned) unexpected_flags); - return flags & (O_ACCMODE | extra_flags); /* return the flags variable, but remove the noise */ + return flags & (O_ACCMODE_STRICT | extra_flags); /* return the flags variable, but remove the noise */ } int read_nr_open(void) { @@ -1132,7 +1132,7 @@ int fds_are_same_mount(int fd1, int fd2) { } const char* accmode_to_string(int flags) { - switch (flags & O_ACCMODE) { + switch (flags & O_ACCMODE_STRICT) { case O_RDONLY: return "ro"; case O_WRONLY: diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index 655f59a548..2f16b6a5a7 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -1036,7 +1036,7 @@ int open_mkdir_at_full(int dirfd, const char *path, int flags, XOpenFlags xopen_ if (flags & ~(O_RDONLY|O_CLOEXEC|O_DIRECTORY|O_EXCL|O_NOATIME|O_NOFOLLOW|O_PATH)) return -EINVAL; - if ((flags & O_ACCMODE) != O_RDONLY) + if ((flags & O_ACCMODE_STRICT) != O_RDONLY) return -EINVAL; /* Note that O_DIRECTORY|O_NOFOLLOW is implied, but we allow specifying it anyway. The following diff --git a/src/basic/missing_fcntl.h b/src/basic/missing_fcntl.h index 3c3f3a445f..b767186a4a 100644 --- a/src/basic/missing_fcntl.h +++ b/src/basic/missing_fcntl.h @@ -43,3 +43,9 @@ #ifndef AT_HANDLE_FID #define AT_HANDLE_FID AT_REMOVEDIR #endif + +/* On musl, O_ACCMODE is defined as (03|O_SEARCH), unlike glibc which defines it as + * (O_RDONLY|O_WRONLY|O_RDWR). Additionally, O_SEARCH is simply defined as O_PATH. This changes the behaviour + * of O_ACCMODE in certain situations, which we don't want. This definition is copied from glibc and works + * around the problems with musl's definition. */ +#define O_ACCMODE_STRICT (O_RDONLY|O_WRONLY|O_RDWR) diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index ef129eb770..0bf6c83601 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -267,7 +267,7 @@ static int acquire_path(const char *path, int flags, mode_t mode) { assert(path); - if (IN_SET(flags & O_ACCMODE, O_WRONLY, O_RDWR)) + if (IN_SET(flags & O_ACCMODE_STRICT, O_WRONLY, O_RDWR)) flags |= O_CREAT; fd = open(path, flags|O_NOCTTY, mode); @@ -291,9 +291,9 @@ static int acquire_path(const char *path, int flags, mode_t mode) { if (r < 0) return r; - if ((flags & O_ACCMODE) == O_RDONLY) + if ((flags & O_ACCMODE_STRICT) == O_RDONLY) r = shutdown(fd, SHUT_WR); - else if ((flags & O_ACCMODE) == O_WRONLY) + else if ((flags & O_ACCMODE_STRICT) == O_WRONLY) r = shutdown(fd, SHUT_RD); else r = 0; diff --git a/src/libsystemd/sd-journal/journal-file.c b/src/libsystemd/sd-journal/journal-file.c index 30a5c716a8..a0e618cf1d 100644 --- a/src/libsystemd/sd-journal/journal-file.c +++ b/src/libsystemd/sd-journal/journal-file.c @@ -98,7 +98,7 @@ DEFINE_HASH_OPS_WITH_VALUE_DESTRUCTOR( JournalFile, journal_file_close); static int mmap_prot_from_open_flags(int flags) { - switch (flags & O_ACCMODE) { + switch (flags & O_ACCMODE_STRICT) { case O_RDONLY: return PROT_READ; case O_WRONLY: @@ -4075,10 +4075,10 @@ int journal_file_open( assert(mmap_cache); assert(ret); - if (!IN_SET((open_flags & O_ACCMODE), O_RDONLY, O_RDWR)) + if (!IN_SET((open_flags & O_ACCMODE_STRICT), O_RDONLY, O_RDWR)) return -EINVAL; - if ((open_flags & O_ACCMODE) == O_RDONLY && FLAGS_SET(open_flags, O_CREAT)) + if ((open_flags & O_ACCMODE_STRICT) == O_RDONLY && FLAGS_SET(open_flags, O_CREAT)) return -EINVAL; if (fname && (open_flags & O_CREAT) && !endswith(fname, ".journal")) diff --git a/src/libsystemd/sd-journal/journal-file.h b/src/libsystemd/sd-journal/journal-file.h index 669c066156..35f8f6ce6b 100644 --- a/src/libsystemd/sd-journal/journal-file.h +++ b/src/libsystemd/sd-journal/journal-file.h @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #pragma once -#include #include #include @@ -15,6 +14,7 @@ #include "compress.h" #include "hashmap.h" #include "journal-def.h" +#include "missing_fcntl.h" #include "mmap-cache.h" #include "sparse-endian.h" #include "time-util.h" @@ -391,5 +391,5 @@ static inline uint32_t COMPRESSION_TO_HEADER_INCOMPATIBLE_FLAG(Compression c) { static inline bool journal_file_writable(JournalFile *f) { assert(f); - return (f->open_flags & O_ACCMODE) != O_RDONLY; + return (f->open_flags & O_ACCMODE_STRICT) != O_RDONLY; } diff --git a/src/login/logind-session-dbus.c b/src/login/logind-session-dbus.c index 126020acf1..5e374ae91e 100644 --- a/src/login/logind-session-dbus.c +++ b/src/login/logind-session-dbus.c @@ -536,7 +536,7 @@ static int method_set_tty(sd_bus_message *message, void *userdata, sd_bus_error flags = fcntl(fd, F_GETFL, 0); if (flags < 0) return -errno; - if ((flags & O_ACCMODE) != O_RDWR) + if ((flags & O_ACCMODE_STRICT) != O_RDWR) return -EACCES; if (FLAGS_SET(flags, O_PATH)) return -ENOTTY; diff --git a/src/mountfsd/mountwork.c b/src/mountfsd/mountwork.c index a693419eec..9567146fdc 100644 --- a/src/mountfsd/mountwork.c +++ b/src/mountfsd/mountwork.c @@ -99,7 +99,7 @@ static int validate_image_fd(int fd, MountImageParameters *p) { if (fl < 0) return log_debug_errno(fl, "Image file descriptor has unsafe flags set: %m"); - switch (fl & O_ACCMODE) { + switch (fl & O_ACCMODE_STRICT) { case O_RDONLY: p->read_only = true; diff --git a/src/shared/data-fd-util.c b/src/shared/data-fd-util.c index 6f4825d8c1..20d54f325d 100644 --- a/src/shared/data-fd-util.c +++ b/src/shared/data-fd-util.c @@ -146,13 +146,13 @@ int memfd_clone_fd(int fd, const char *name, int mode) { assert(fd >= 0); assert(name); - assert(IN_SET(mode & O_ACCMODE, O_RDONLY, O_RDWR)); + assert(IN_SET(mode & O_ACCMODE_STRICT, O_RDONLY, O_RDWR)); assert((mode & ~(O_RDONLY|O_RDWR|O_CLOEXEC)) == 0); if (fstat(fd, &st) < 0) return -errno; - ro = (mode & O_ACCMODE) == O_RDONLY; + ro = (mode & O_ACCMODE_STRICT) == O_RDONLY; exec = st.st_mode & 0111; mfd = memfd_create_wrapper(name, diff --git a/src/shared/journal-file-util.c b/src/shared/journal-file-util.c index b352ede275..edd5056ad9 100644 --- a/src/shared/journal-file-util.c +++ b/src/shared/journal-file-util.c @@ -504,7 +504,7 @@ int journal_file_open_reliably( -EIDRM)) /* File has been deleted */ return r; - if ((open_flags & O_ACCMODE) == O_RDONLY) + if ((open_flags & O_ACCMODE_STRICT) == O_RDONLY) return r; if (!(open_flags & O_CREAT)) @@ -519,7 +519,7 @@ int journal_file_open_reliably( /* The file is corrupted. Try opening it read-only as the template before rotating to inherit its * sequence number and ID. */ r = journal_file_open(-EBADF, fname, - (open_flags & ~(O_ACCMODE|O_CREAT|O_EXCL)) | O_RDONLY, + (open_flags & ~(O_ACCMODE_STRICT|O_CREAT|O_EXCL)) | O_RDONLY, file_flags, 0, compress_threshold_bytes, NULL, mmap_cache, /* template = */ NULL, &old_file); if (r < 0) diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c index 4a6e0046df..5f2fe03e2e 100644 --- a/src/shared/loop-util.c +++ b/src/shared/loop-util.c @@ -500,7 +500,7 @@ static int loop_device_make_internal( .block_size = sector_size, .info = { /* Use the specified flags, but configure the read-only flag from the open flags, and force autoclear */ - .lo_flags = (loop_flags & ~LO_FLAGS_READ_ONLY) | ((open_flags & O_ACCMODE) == O_RDONLY ? LO_FLAGS_READ_ONLY : 0) | LO_FLAGS_AUTOCLEAR, + .lo_flags = (loop_flags & ~LO_FLAGS_READ_ONLY) | ((open_flags & O_ACCMODE_STRICT) == O_RDONLY ? LO_FLAGS_READ_ONLY : 0) | LO_FLAGS_AUTOCLEAR, .lo_offset = offset, .lo_sizelimit = size == UINT64_MAX ? 0 : size, }, diff --git a/src/test/test-fileio.c b/src/test/test-fileio.c index dbd175ad70..67943b65ba 100644 --- a/src/test/test-fileio.c +++ b/src/test/test-fileio.c @@ -1104,7 +1104,7 @@ TEST(fdopen_independent) { zero(buf); assert_se(fread(buf, 1, sizeof(buf), f) == strlen(TEST_TEXT)); ASSERT_STREQ(buf, TEST_TEXT); - assert_se((fcntl(fileno(f), F_GETFL) & O_ACCMODE) == O_RDONLY); + assert_se((fcntl(fileno(f), F_GETFL) & O_ACCMODE_STRICT) == O_RDONLY); assert_se(FLAGS_SET(fcntl(fileno(f), F_GETFD), FD_CLOEXEC)); f = safe_fclose(f); @@ -1112,7 +1112,7 @@ TEST(fdopen_independent) { zero(buf); assert_se(fread(buf, 1, sizeof(buf), f) == strlen(TEST_TEXT)); ASSERT_STREQ(buf, TEST_TEXT); - assert_se((fcntl(fileno(f), F_GETFL) & O_ACCMODE) == O_RDONLY); + assert_se((fcntl(fileno(f), F_GETFL) & O_ACCMODE_STRICT) == O_RDONLY); assert_se(!FLAGS_SET(fcntl(fileno(f), F_GETFD), FD_CLOEXEC)); f = safe_fclose(f); @@ -1120,7 +1120,7 @@ TEST(fdopen_independent) { zero(buf); assert_se(fread(buf, 1, sizeof(buf), f) == strlen(TEST_TEXT)); ASSERT_STREQ(buf, TEST_TEXT); - assert_se((fcntl(fileno(f), F_GETFL) & O_ACCMODE) == O_RDWR); + assert_se((fcntl(fileno(f), F_GETFL) & O_ACCMODE_STRICT) == O_RDWR); assert_se(FLAGS_SET(fcntl(fileno(f), F_GETFD), FD_CLOEXEC)); f = safe_fclose(f); }