From: Stefan Hansson Date: Sun, 13 Apr 2025 18:35:49 +0000 (+0200) Subject: missing_fcntl: Introduce O_ACCMODE_STRICT X-Git-Tag: v258-rc1~793 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=b1236ce38bb2f918c7150df36b32fcb84b1a23fb;p=thirdparty%2Fsystemd.git 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 --- diff --git a/src/basic/fd-util.c b/src/basic/fd-util.c index a6a63454fca..5f17ed1792b 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 655f59a5480..2f16b6a5a78 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 3c3f3a445f5..b767186a4a6 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 ef129eb770d..0bf6c836013 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 30a5c716a83..a0e618cf1dc 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 669c0661562..35f8f6ce6b4 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 126020acf16..5e374ae91e8 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 a693419eec7..9567146fdc1 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 6f4825d8c13..20d54f325d9 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 b352ede275a..edd5056ad94 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 4a6e0046dfc..5f2fe03e2e6 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 dbd175ad704..67943b65ba7 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); }