From b8e25bff386548f679902c6b797a5a8ff1542c8b Mon Sep 17 00:00:00 2001 From: Lars Ellenberg Date: Wed, 7 Feb 2024 13:12:50 +0100 Subject: [PATCH] fs-utils: new wrapper fd_reopen_propagate_append_and_position() We may want to propagate O_APPEND, or (try to) keep the current file position, even if we use fd_reopen() to re-initialize (and "unshare") other file description status. For now, used only with --pty to keep/propagate O_APPEND (and/or) position if set on stdin/stdout. If we re-open stdout and "drop" the O_APPEND, we get rather "unexpected" behavior, for example with repeated "systemd-run --pty >> some-log". If someone carefully pre-positioned the passed in original file descriptors, we avoid surprises if we do not reset file postition to zero. fcntl F_GETFL first, and propagate O_APPEND if present in the existing flags. Then use lseek to propagate the file position. --- src/basic/fd-util.c | 43 +++++++++++++++++++++++++++++++++++++++++++ src/basic/fd-util.h | 1 + src/shared/ptyfwd.c | 11 ++++++++--- 3 files changed, 52 insertions(+), 3 deletions(-) diff --git a/src/basic/fd-util.c b/src/basic/fd-util.c index 8372c54918d07..5d0053029698b 100644 --- a/src/basic/fd-util.c +++ b/src/basic/fd-util.c @@ -869,6 +869,49 @@ int fd_reopen(int fd, int flags) { return new_fd; } +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 + * keep current file position. + * + * You should use this if the original fd potentially is O_APPEND, otherwise we get rather + * "unexpected" behavior. Unless you intentionally want to overwrite pre-existing data, and have + * your output overwritten by the next user. + * + * Use case: "systemd-run --pty >> some-log". + * + * The "keep position" part is obviously nonsense for the O_APPEND case, but should reduce surprises + * if someone carefully pre-positioned the passed in original input or non-append output FDs. */ + + assert(fd >= 0); + assert(!(flags & (O_APPEND|O_DIRECTORY))); + + int existing_flags = fcntl(fd, F_GETFL); + if (existing_flags < 0) + return -errno; + + int new_fd = fd_reopen(fd, flags | (existing_flags & O_APPEND)); + if (new_fd < 0) + return new_fd; + + /* Try to adjust the offset, but ignore errors for now. */ + off_t p = lseek(fd, 0, SEEK_CUR); + if (p <= 0) + return new_fd; + + off_t new_p = lseek(new_fd, p, SEEK_SET); + if (new_p == (off_t) -1) + log_debug_errno(errno, + "Failed to propagate file position for re-opened fd %d, ignoring: %m", + fd); + else if (new_p != p) + log_debug("Failed to propagate file position for re-opened fd %d (%lld != %lld), ignoring: %m", + fd, + (long long) new_p, + (long long) p); + + return new_fd; +} + int fd_reopen_condition( int fd, int flags, diff --git a/src/basic/fd-util.h b/src/basic/fd-util.h index 1e591085ef88c..af17481dd868a 100644 --- a/src/basic/fd-util.h +++ b/src/basic/fd-util.h @@ -111,6 +111,7 @@ static inline int make_null_stdio(void) { }) int fd_reopen(int fd, int flags); +int fd_reopen_propagate_append_and_position(int fd, int flags); int fd_reopen_condition(int fd, int flags, int mask, int *ret_new_fd); int fd_is_opath(int fd); diff --git a/src/shared/ptyfwd.c b/src/shared/ptyfwd.c index d572a02ca486e..11997130540a0 100644 --- a/src/shared/ptyfwd.c +++ b/src/shared/ptyfwd.c @@ -830,9 +830,13 @@ int pty_forward_new( * them. This has two advantages: when we are killed abruptly the stdin/stdout fds won't be * left in O_NONBLOCK state for the next process using them. In addition, if some process * running in the background wants to continue writing to our stdout it can do so without - * being confused by O_NONBLOCK. */ + * being confused by O_NONBLOCK. + * We keep O_APPEND (if present) on the output FD and (try to) keep current file position on + * both input and output FD (principle of least surprise). + */ - f->input_fd = fd_reopen(STDIN_FILENO, O_RDONLY|O_CLOEXEC|O_NOCTTY|O_NONBLOCK); + f->input_fd = fd_reopen_propagate_append_and_position( + STDIN_FILENO, O_RDONLY|O_CLOEXEC|O_NOCTTY|O_NONBLOCK); if (f->input_fd < 0) { /* Handle failures gracefully, after all certain fd types cannot be reopened * (sockets, …) */ @@ -846,7 +850,8 @@ int pty_forward_new( } else f->close_input_fd = true; - f->output_fd = fd_reopen(STDOUT_FILENO, O_WRONLY|O_CLOEXEC|O_NOCTTY|O_NONBLOCK); + f->output_fd = fd_reopen_propagate_append_and_position( + STDOUT_FILENO, O_WRONLY|O_CLOEXEC|O_NOCTTY|O_NONBLOCK); if (f->output_fd < 0) { log_debug_errno(f->output_fd, "Failed to reopen stdout, using original fd: %m");