mirror of
https://github.com/morgan9e/systemd
synced 2026-04-14 00:14:32 +09:00
core: Verify inherited FDs are writable for stdout/stderr (#39674)
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.
This commit is contained in:
@@ -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;
|
||||
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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);
|
||||
|
||||
|
||||
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user