From 592c57e586fa5aac78145d689608c487344605db Mon Sep 17 00:00:00 2001 From: Chris Down Date: Mon, 17 Nov 2025 11:05:09 +0800 Subject: [PATCH 1/2] fd-util: Add fd_is_writable() to check if FD is opened for writing This checks whether a file descriptor is valid and opened in a mode that allows writing (O_WRONLY or O_RDWR). This is useful when we want to verify that inherited FDs can actually be used for output operations before dup'ing them. The helper explicitly handles O_PATH file descriptors, which cannot be used for I/O operations and thus are never writable. --- src/basic/fd-util.c | 15 +++++++++++++++ src/basic/fd-util.h | 1 + src/test/test-fd-util.c | 23 +++++++++++++++++++++++ 3 files changed, 39 insertions(+) diff --git a/src/basic/fd-util.c b/src/basic/fd-util.c index e0dcbcecd1..834b162c47 100644 --- a/src/basic/fd-util.c +++ b/src/basic/fd-util.c @@ -998,6 +998,21 @@ int fd_vet_accmode(int fd, int mode) { return -EPROTOTYPE; } +int fd_is_writable(int fd) { + int r; + + assert(fd >= 0); + + r = fd_vet_accmode(fd, O_WRONLY); + if (r >= 0) + return true; + + if (IN_SET(r, -EPROTOTYPE, -EBADFD, -EISDIR)) + return false; + + return r; +} + int fd_verify_safe_flags_full(int fd, int extra_flags) { int flags, unexpected_flags; diff --git a/src/basic/fd-util.h b/src/basic/fd-util.h index baa81b6a66..8974188194 100644 --- a/src/basic/fd-util.h +++ b/src/basic/fd-util.h @@ -152,6 +152,7 @@ int fd_reopen_condition(int fd, int flags, int mask, int *ret_new_fd); int fd_is_opath(int fd); int fd_vet_accmode(int fd, int mode); +int fd_is_writable(int fd); int fd_verify_safe_flags_full(int fd, int extra_flags); static inline int fd_verify_safe_flags(int fd) { diff --git a/src/test/test-fd-util.c b/src/test/test-fd-util.c index a22c0e3119..db869b5d11 100644 --- a/src/test/test-fd-util.c +++ b/src/test/test-fd-util.c @@ -903,4 +903,27 @@ TEST(fd_vet_accmode) { ASSERT_ERROR(fd_vet_accmode(fd_opath, O_RDWR), EBADFD); } +TEST(fd_is_writable) { + _cleanup_(unlink_tempfilep) char name[] = "/tmp/test-fd-writable.XXXXXX"; + _cleanup_close_ int fd_ro = -EBADF, fd_wo = -EBADF, fd_rw = -EBADF, fd_path = -EBADF; + + ASSERT_OK(fd_rw = mkostemp_safe(name)); + ASSERT_OK_POSITIVE(fd_is_writable(fd_rw)); + + ASSERT_OK(fd_ro = open(name, O_RDONLY | O_CLOEXEC)); + ASSERT_OK_ZERO(fd_is_writable(fd_ro)); + + ASSERT_OK(fd_wo = open(name, O_WRONLY | O_CLOEXEC)); + ASSERT_OK_POSITIVE(fd_is_writable(fd_wo)); + + ASSERT_OK(fd_path = open(name, O_PATH | O_CLOEXEC)); + ASSERT_OK_ZERO(fd_is_writable(fd_path)); + + ASSERT_SIGNAL(fd_is_writable(-1), SIGABRT); + + safe_close(fd_ro); + ASSERT_ERROR(fd_is_writable(fd_ro), EBADF); + TAKE_FD(fd_ro); +} + DEFINE_TEST_MAIN(LOG_DEBUG); From 171ceb4a00294c700c0ba6906a3a3abad846699e Mon Sep 17 00:00:00 2001 From: Chris Down Date: Tue, 11 Nov 2025 04:26:10 +0800 Subject: [PATCH 2/2] core: Verify inherited FDs are writable for stdout/stderr When inheriting file descriptors for stdout/stderr (either from stdin or when making stderr inherit from stdout), we previously just assumed they would be writable and dup'd them. This could lead to broken setups if the inherited FD was actually opened read-only. Before dup'ing any inherited FDs to stdout/stderr, verify they are actually writable using the new fd_is_writable() helper. If not, fall back to /dev/null (or reopen the terminal in the TTY case) with a warning, rather than silently creating a broken setup where output operations would fail. --- src/core/exec-invoke.c | 40 ++++++++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index e9c4e3f624..7943fdf8b7 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -507,9 +507,6 @@ static int setup_output( i = fixup_input(context, socket_fd, params->flags & EXEC_APPLY_TTY_STDIN); o = fixup_output(context->std_output, socket_fd); - // FIXME: we probably should spend some time here to verify that if we inherit an fd from stdin - // (possibly indirect via inheritance from stdout) it is actually opened for write! - if (fileno == STDERR_FILENO) { ExecOutput e; e = fixup_output(context->std_error, socket_fd); @@ -526,8 +523,17 @@ static int setup_output( return fileno; /* Duplicate from stdout if possible */ - if (can_inherit_stderr_from_stdout(context, o, e)) + if (can_inherit_stderr_from_stdout(context, o, e)) { + r = fd_is_writable(STDOUT_FILENO); + if (r <= 0) { + if (r < 0) + log_warning_errno(r, "Failed to check if inherited stdout is writable for stderr, falling back to /dev/null."); + else + log_warning("Inherited stdout is not writable for stderr, falling back to /dev/null."); + return open_null_as(O_WRONLY, fileno); + } return RET_NERRNO(dup2(STDOUT_FILENO, fileno)); + } o = e; @@ -537,8 +543,19 @@ static int setup_output( return open_terminal_as(exec_context_tty_path(context), O_WRONLY, fileno); /* If the input is connected to anything that's not a /dev/null or a data fd, inherit that... */ - if (!IN_SET(i, EXEC_INPUT_NULL, EXEC_INPUT_DATA)) + if (!IN_SET(i, EXEC_INPUT_NULL, EXEC_INPUT_DATA)) { + r = fd_is_writable(STDIN_FILENO); + if (r <= 0) { + if (r < 0) + log_warning_errno(r, "Failed to check if inherited stdin is writable for %s, falling back to /dev/null.", + fileno == STDOUT_FILENO ? "stdout" : "stderr"); + else + log_warning("Inherited stdin is not writable for %s, falling back to /dev/null.", + fileno == STDOUT_FILENO ? "stdout" : "stderr"); + return open_null_as(O_WRONLY, fileno); + } return RET_NERRNO(dup2(STDIN_FILENO, fileno)); + } /* If we are not started from PID 1 we just inherit STDOUT from our parent process. */ if (getppid() != 1) @@ -554,8 +571,19 @@ static int setup_output( return open_null_as(O_WRONLY, fileno); case EXEC_OUTPUT_TTY: - if (exec_input_is_terminal(i)) + if (exec_input_is_terminal(i)) { + r = fd_is_writable(STDIN_FILENO); + if (r <= 0) { + if (r < 0) + log_warning_errno(r, "Failed to check if inherited stdin is writable for TTY's %s, falling back to opening terminal.", + fileno == STDOUT_FILENO ? "stdout" : "stderr"); + else + log_warning("Inherited stdin is not writable for TTY's %s, falling back to opening terminal.", + fileno == STDOUT_FILENO ? "stdout" : "stderr"); + return open_terminal_as(exec_context_tty_path(context), O_WRONLY, fileno); + } return RET_NERRNO(dup2(STDIN_FILENO, fileno)); + } return open_terminal_as(exec_context_tty_path(context), O_WRONLY, fileno);