diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index bf36a928b1..73fa7c23da 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -3338,17 +3338,15 @@ int bus_exec_context_set_transient_property( if (r < 0) return r; - if (!isempty(s)) { - if (!path_is_absolute(s)) - return sd_bus_error_setf(reterr_error, SD_BUS_ERROR_INVALID_ARGS, "Path %s is not absolute", s); - if (!path_is_normalized(s)) - return sd_bus_error_setf(reterr_error, SD_BUS_ERROR_INVALID_ARGS, "Path %s is not normalized", s); - } + if (!path_is_absolute(s)) + return sd_bus_error_setf(reterr_error, SD_BUS_ERROR_INVALID_ARGS, "Path %s is not absolute", s); + if (!path_is_normalized(s)) + return sd_bus_error_setf(reterr_error, SD_BUS_ERROR_INVALID_ARGS, "Path %s is not normalized", s); if (!UNIT_WRITE_FLAGS_NOOP(flags)) { if (streq(name, "StandardInputFile")) { - r = free_and_strdup(&c->stdio_file[STDIN_FILENO], empty_to_null(s)); + r = free_and_strdup(&c->stdio_file[STDIN_FILENO], s); if (r < 0) return r; @@ -3356,7 +3354,7 @@ int bus_exec_context_set_transient_property( unit_write_settingf(u, flags|UNIT_ESCAPE_SPECIFIERS, name, "StandardInput=file:%s", s); } else if (STR_IN_SET(name, "StandardOutputFile", "StandardOutputFileToAppend", "StandardOutputFileToTruncate")) { - r = free_and_strdup(&c->stdio_file[STDOUT_FILENO], empty_to_null(s)); + r = free_and_strdup(&c->stdio_file[STDOUT_FILENO], s); if (r < 0) return r; @@ -3374,7 +3372,7 @@ int bus_exec_context_set_transient_property( } else { assert(STR_IN_SET(name, "StandardErrorFile", "StandardErrorFileToAppend", "StandardErrorFileToTruncate")); - r = free_and_strdup(&c->stdio_file[STDERR_FILENO], empty_to_null(s)); + r = free_and_strdup(&c->stdio_file[STDERR_FILENO], s); if (r < 0) return r; diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index fe836de6c0..24e553e122 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -308,7 +308,6 @@ static int acquire_path(const char *path, int flags, mode_t mode) { static int fixup_input( const ExecContext *context, - int socket_fd, bool apply_tty_stdin) { ExecInput std_input; @@ -320,23 +319,12 @@ static int fixup_input( if (exec_input_is_terminal(std_input) && !apply_tty_stdin) return EXEC_INPUT_NULL; - if (std_input == EXEC_INPUT_SOCKET && socket_fd < 0) - return EXEC_INPUT_NULL; - if (std_input == EXEC_INPUT_DATA && context->stdin_data_size == 0) return EXEC_INPUT_NULL; return std_input; } -static int fixup_output(ExecOutput output, int socket_fd) { - - if (output == EXEC_OUTPUT_SOCKET && socket_fd < 0) - return EXEC_OUTPUT_INHERIT; - - return output; -} - static int setup_input( const ExecContext *context, const ExecParameters *params, @@ -361,7 +349,7 @@ static int setup_input( return STDIN_FILENO; } - i = fixup_input(context, socket_fd, params->flags & EXEC_APPLY_TTY_STDIN); + i = fixup_input(context, params->flags & EXEC_APPLY_TTY_STDIN); switch (i) { @@ -430,8 +418,8 @@ static int setup_input( assert(context->stdio_file[STDIN_FILENO]); - rw = (context->std_output == EXEC_OUTPUT_FILE && streq_ptr(context->stdio_file[STDIN_FILENO], context->stdio_file[STDOUT_FILENO])) || - (context->std_error == EXEC_OUTPUT_FILE && streq_ptr(context->stdio_file[STDIN_FILENO], context->stdio_file[STDERR_FILENO])); + rw = (context->std_output == EXEC_OUTPUT_FILE && path_equal(context->stdio_file[STDIN_FILENO], context->stdio_file[STDOUT_FILENO])) || + (context->std_error == EXEC_OUTPUT_FILE && path_equal(context->stdio_file[STDIN_FILENO], context->stdio_file[STDERR_FILENO])); fd = acquire_path(context->stdio_file[STDIN_FILENO], rw ? O_RDWR : O_RDONLY, 0666 & ~context->umask); if (fd < 0) @@ -445,30 +433,68 @@ static int setup_input( } } -static bool can_inherit_stderr_from_stdout( - const ExecContext *context, - ExecOutput o, - ExecOutput e) { +static bool can_inherit_stderr_from_stdout(const ExecContext *context) { + ExecOutput o, e; assert(context); /* Returns true, if given the specified STDERR and STDOUT output we can directly dup() the stdout fd to the * stderr fd */ + o = context->std_output; + e = context->std_error; + if (e == EXEC_OUTPUT_INHERIT) return true; if (e != o) return false; + /* Let's not shortcut named fds here, even though we in theory can by comparing fd names, since + * we have the named_iofds array readily available, and the inherit practice would simply be duplicative. */ if (e == EXEC_OUTPUT_NAMED_FD) - return streq_ptr(context->stdio_fdname[STDOUT_FILENO], context->stdio_fdname[STDERR_FILENO]); + return false; if (IN_SET(e, EXEC_OUTPUT_FILE, EXEC_OUTPUT_FILE_APPEND, EXEC_OUTPUT_FILE_TRUNCATE)) - return streq_ptr(context->stdio_file[STDOUT_FILENO], context->stdio_file[STDERR_FILENO]); + return path_equal(context->stdio_file[STDOUT_FILENO], context->stdio_file[STDERR_FILENO]); return true; } +static int maybe_inherit_stdout_from_stdin(const ExecContext *context, ExecInput i) { + int r; + + assert(context); + + if (context->std_output != EXEC_OUTPUT_INHERIT) + return 0; + + /* If input got downgraded, inherit the original value */ + if (i == EXEC_INPUT_NULL && exec_input_is_terminal(context->std_input)) + return open_terminal_as(exec_context_tty_path(context), O_WRONLY, STDOUT_FILENO); + + if (!exec_input_is_inheritable(i)) + goto fallback; + + 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 stdout, using fallback: %m"); + else + log_warning("Inherited stdin is not writable for stdout, using fallback: %m"); + goto fallback; + } + + return RET_NERRNO(dup2(STDIN_FILENO, STDOUT_FILENO)); + +fallback: + /* If we are not started from PID 1 we just inherit STDOUT from our parent process. */ + if (getppid() != 1) + return STDOUT_FILENO; + + /* We need to open /dev/null here anew, to get the right access mode. */ + return open_null_as(O_WRONLY, STDOUT_FILENO); +} + static int setup_output( const ExecContext *context, const ExecParameters *params, @@ -506,65 +532,33 @@ static int setup_output( return STDERR_FILENO; } - i = fixup_input(context, socket_fd, params->flags & EXEC_APPLY_TTY_STDIN); - o = fixup_output(context->std_output, socket_fd); + i = fixup_input(context, params->flags & EXEC_APPLY_TTY_STDIN); if (fileno == STDERR_FILENO) { - ExecOutput e; - e = fixup_output(context->std_error, socket_fd); - /* This expects the input and output are already set up */ /* Don't change the stderr file descriptor if we inherit all * the way and are not on a tty */ - if (e == EXEC_OUTPUT_INHERIT && - o == EXEC_OUTPUT_INHERIT && - i == EXEC_INPUT_NULL && - !exec_input_is_terminal(context->std_input) && + if (context->std_error == EXEC_OUTPUT_INHERIT && + context->std_output == EXEC_OUTPUT_INHERIT && + i == EXEC_INPUT_NULL && !exec_input_is_terminal(context->std_input) && getppid() != 1) return fileno; /* Duplicate from stdout if possible */ - 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); - } + if (can_inherit_stderr_from_stdout(context)) return RET_NERRNO(dup2(STDOUT_FILENO, fileno)); - } - o = e; + o = context->std_error; - } else if (o == EXEC_OUTPUT_INHERIT) { - /* If input got downgraded, inherit the original value */ - if (i == EXEC_INPUT_NULL && exec_input_is_terminal(context->std_input)) - return open_terminal_as(exec_context_tty_path(context), O_WRONLY, fileno); + } else { + assert(fileno == STDOUT_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)) { - 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)); - } + r = maybe_inherit_stdout_from_stdin(context, i); + if (r != 0) + return r; - /* If we are not started from PID 1 we just inherit STDOUT from our parent process. */ - if (getppid() != 1) - return fileno; - - /* We need to open /dev/null here anew, to get the right access mode. */ - return open_null_as(O_WRONLY, fileno); + o = context->std_output; } switch (o) { @@ -630,15 +624,14 @@ static int setup_output( case EXEC_OUTPUT_FILE: case EXEC_OUTPUT_FILE_APPEND: case EXEC_OUTPUT_FILE_TRUNCATE: { - bool rw; int fd, flags; assert(context->stdio_file[fileno]); - rw = context->std_input == EXEC_INPUT_FILE && - streq_ptr(context->stdio_file[fileno], context->stdio_file[STDIN_FILENO]); - - if (rw) + /* stdin points to the same file hence setup_input() opened it as rw already? + * Then just duplicate it. */ + if (context->std_input == EXEC_INPUT_FILE && + path_equal(context->stdio_file[fileno], context->stdio_file[STDIN_FILENO])) return RET_NERRNO(dup2(STDIN_FILENO, fileno)); flags = O_WRONLY; @@ -4794,28 +4787,34 @@ static int exec_context_named_iofds( /* Note that socket fds are always placed at the beginning of the fds array, no need for extra * manipulation. */ - for (size_t i = 0; i < p->n_socket_fds && targets > 0; i++) + for (size_t i = 0; i < p->n_socket_fds && targets > 0; i++) { if (named_iofds[STDIN_FILENO] < 0 && c->std_input == EXEC_INPUT_NAMED_FD && streq(p->fd_names[i], stdio_fdname[STDIN_FILENO])) { named_iofds[STDIN_FILENO] = p->fds[i]; targets--; + continue; + } - } else if (named_iofds[STDOUT_FILENO] < 0 && - c->std_output == EXEC_OUTPUT_NAMED_FD && - streq(p->fd_names[i], stdio_fdname[STDOUT_FILENO])) { + /* Allow stdout and stderr to use the same named fd */ + + if (named_iofds[STDOUT_FILENO] < 0 && + c->std_output == EXEC_OUTPUT_NAMED_FD && + streq(p->fd_names[i], stdio_fdname[STDOUT_FILENO])) { named_iofds[STDOUT_FILENO] = p->fds[i]; targets--; + } - } else if (named_iofds[STDERR_FILENO] < 0 && - c->std_error == EXEC_OUTPUT_NAMED_FD && - streq(p->fd_names[i], stdio_fdname[STDERR_FILENO])) { + if (named_iofds[STDERR_FILENO] < 0 && + c->std_error == EXEC_OUTPUT_NAMED_FD && + streq(p->fd_names[i], stdio_fdname[STDERR_FILENO])) { named_iofds[STDERR_FILENO] = p->fds[i]; targets--; } + } return targets == 0 ? 0 : -ENOENT; } diff --git a/src/core/execute-serialize.c b/src/core/execute-serialize.c index a634b0308e..a652b9a467 100644 --- a/src/core/execute-serialize.c +++ b/src/core/execute-serialize.c @@ -1539,48 +1539,6 @@ static int exec_parameters_deserialize(ExecParameters *p, FILE *f, FDSet *fds) { return 0; } -static int serialize_std_out_err(const ExecContext *c, FILE *f, int fileno) { - char *key, *value; - const char *type; - - assert(c); - assert(f); - assert(IN_SET(fileno, STDOUT_FILENO, STDERR_FILENO)); - - type = fileno == STDOUT_FILENO ? "output" : "error"; - - switch (fileno == STDOUT_FILENO ? c->std_output : c->std_error) { - case EXEC_OUTPUT_NAMED_FD: - key = strjoina("exec-context-std-", type, "-fd-name"); - value = c->stdio_fdname[fileno]; - - break; - - case EXEC_OUTPUT_FILE: - key = strjoina("exec-context-std-", type, "-file"); - value = c->stdio_file[fileno]; - - break; - - case EXEC_OUTPUT_FILE_APPEND: - key = strjoina("exec-context-std-", type, "-file-append"); - value = c->stdio_file[fileno]; - - break; - - case EXEC_OUTPUT_FILE_TRUNCATE: - key = strjoina("exec-context-std-", type, "-file-truncate"); - value = c->stdio_file[fileno]; - - break; - - default: - return 0; - } - - return serialize_item(f, key, value); -} - static int exec_context_serialize(const ExecContext *c, FILE *f) { int r; @@ -2004,6 +1962,10 @@ static int exec_context_serialize(const ExecContext *c, FILE *f) { return r; } + r = serialize_bool_elide(f, "exec-context-root-directory-as-fd", c->root_directory_as_fd); + if (r < 0) + return r; + r = serialize_item(f, "exec-context-std-input", exec_input_to_string(c->std_input)); if (r < 0) return r; @@ -2020,36 +1982,60 @@ static int exec_context_serialize(const ExecContext *c, FILE *f) { if (r < 0) return r; - r = serialize_bool_elide(f, "exec-context-root-directory-as-fd", c->root_directory_as_fd); - if (r < 0) - return r; - switch (c->std_input) { + case EXEC_INPUT_NAMED_FD: r = serialize_item(f, "exec-context-std-input-fd-name", c->stdio_fdname[STDIN_FILENO]); - if (r < 0) - return r; break; case EXEC_INPUT_FILE: - r = serialize_item(f, "exec-context-std-input-file", c->stdio_file[STDIN_FILENO]); - if (r < 0) - return r; + r = serialize_item_escaped(f, "exec-context-std-input-file", c->stdio_file[STDIN_FILENO]); + break; + + case EXEC_INPUT_DATA: + r = serialize_item_base64mem(f, "exec-context-std-input-data", c->stdin_data, c->stdin_data_size); break; default: - ; + r = 0; } - - r = serialize_std_out_err(c, f, STDOUT_FILENO); if (r < 0) return r; - r = serialize_std_out_err(c, f, STDERR_FILENO); + switch (c->std_output) { + + case EXEC_OUTPUT_NAMED_FD: + r = serialize_item(f, "exec-context-std-output-fd-name", c->stdio_fdname[STDOUT_FILENO]); + break; + + case EXEC_OUTPUT_FILE: + case EXEC_OUTPUT_FILE_APPEND: + case EXEC_OUTPUT_FILE_TRUNCATE: + r = serialize_item_escaped(f, "exec-context-std-output-file", c->stdio_file[STDOUT_FILENO]); + break; + + default: + r = 0; + } if (r < 0) return r; - r = serialize_item_base64mem(f, "exec-context-stdin-data", c->stdin_data, c->stdin_data_size); + + switch (c->std_error) { + + case EXEC_OUTPUT_NAMED_FD: + r = serialize_item(f, "exec-context-std-error-fd-name", c->stdio_fdname[STDERR_FILENO]); + break; + + case EXEC_OUTPUT_FILE: + case EXEC_OUTPUT_FILE_APPEND: + case EXEC_OUTPUT_FILE_TRUNCATE: + r = serialize_item_escaped(f, "exec-context-std-error-file", c->stdio_file[STDERR_FILENO]); + break; + + default: + r = 0; + } if (r < 0) return r; @@ -3017,6 +3003,11 @@ static int exec_context_deserialize(ExecContext *c, FILE *f) { r = deserialize_usec(val, (usec_t *)&c->timer_slack_nsec); if (r < 0) return r; + } else if ((val = startswith(l, "exec-context-root-directory-as-fd="))) { + r = parse_boolean(val); + if (r < 0) + return r; + c->root_directory_as_fd = r; } else if ((val = startswith(l, "exec-context-std-input="))) { c->std_input = exec_input_from_string(val); if (c->std_input < 0) @@ -3034,11 +3025,13 @@ static int exec_context_deserialize(ExecContext *c, FILE *f) { if (r < 0) return r; c->stdio_as_fds = r; - } else if ((val = startswith(l, "exec-context-root-directory-as-fd="))) { - r = parse_boolean(val); + } else if ((val = startswith(l, "exec-context-std-input-data="))) { + if (c->stdin_data) + return -EINVAL; /* duplicated */ + + r = unbase64mem(val, &c->stdin_data, &c->stdin_data_size); if (r < 0) return r; - c->root_directory_as_fd = r; } else if ((val = startswith(l, "exec-context-std-input-fd-name="))) { r = free_and_strdup(&c->stdio_fdname[STDIN_FILENO], val); if (r < 0) @@ -3052,40 +3045,35 @@ static int exec_context_deserialize(ExecContext *c, FILE *f) { if (r < 0) return r; } else if ((val = startswith(l, "exec-context-std-input-file="))) { - r = free_and_strdup(&c->stdio_file[STDIN_FILENO], val); - if (r < 0) - return r; + ssize_t k; + char *p; + + k = cunescape(val, 0, &p); + if (k < 0) + return k; + + free_and_replace(c->stdio_file[STDIN_FILENO], p); + } else if ((val = startswith(l, "exec-context-std-output-file="))) { - r = free_and_strdup(&c->stdio_file[STDOUT_FILENO], val); - if (r < 0) - return r; - } else if ((val = startswith(l, "exec-context-std-output-file-append="))) { - r = free_and_strdup(&c->stdio_file[STDOUT_FILENO], val); - if (r < 0) - return r; - } else if ((val = startswith(l, "exec-context-std-output-file-truncate="))) { - r = free_and_strdup(&c->stdio_file[STDOUT_FILENO], val); - if (r < 0) - return r; + ssize_t k; + char *p; + + k = cunescape(val, 0, &p); + if (k < 0) + return k; + + free_and_replace(c->stdio_file[STDOUT_FILENO], p); + } else if ((val = startswith(l, "exec-context-std-error-file="))) { - r = free_and_strdup(&c->stdio_file[STDERR_FILENO], val); - if (r < 0) - return r; - } else if ((val = startswith(l, "exec-context-std-error-file-append="))) { - r = free_and_strdup(&c->stdio_file[STDERR_FILENO], val); - if (r < 0) - return r; - } else if ((val = startswith(l, "exec-context-std-error-file-truncate="))) { - r = free_and_strdup(&c->stdio_file[STDERR_FILENO], val); - if (r < 0) - return r; - } else if ((val = startswith(l, "exec-context-stdin-data="))) { - if (c->stdin_data) - return -EINVAL; /* duplicated */ + ssize_t k; + char *p; + + k = cunescape(val, 0, &p); + if (k < 0) + return k; + + free_and_replace(c->stdio_file[STDERR_FILENO], p); - r = unbase64mem(val, &c->stdin_data, &c->stdin_data_size); - if (r < 0) - return r; } else if ((val = startswith(l, "exec-context-tty-path="))) { r = free_and_strdup(&c->tty_path, val); if (r < 0) diff --git a/src/core/execute.h b/src/core/execute.h index 42127347cf..92a2c730fd 100644 --- a/src/core/execute.h +++ b/src/core/execute.h @@ -480,6 +480,16 @@ static inline bool exec_context_has_tty(const ExecContext *context) { context->std_error == EXEC_OUTPUT_TTY; } +static inline bool exec_input_is_inheritable(ExecInput i) { + /* We assume these listed inputs refer to bidirectional streams, and hence duplicating them from + * stdin to stdout/stderr makes sense and hence allowing EXEC_OUTPUT_INHERIT makes sense, too. + * Outputs such as regular files or sealed data memfds otoh don't really make sense to be + * duplicated for both input and output at the same time (since they then would cause a feedback + * loop). */ + + return exec_input_is_terminal(i) || IN_SET(i, EXEC_INPUT_SOCKET, EXEC_INPUT_NAMED_FD); +} + int exec_spawn( Unit *unit, ExecCommand *command, diff --git a/src/core/service.c b/src/core/service.c index 46caa1d3e3..ec3fd2ed8c 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -808,19 +808,11 @@ static void service_fix_stdio(Service *s) { s->exec_context.stdin_data_size > 0) s->exec_context.std_input = EXEC_INPUT_DATA; - if (IN_SET(s->exec_context.std_input, - EXEC_INPUT_TTY, - EXEC_INPUT_TTY_FORCE, - EXEC_INPUT_TTY_FAIL, - EXEC_INPUT_SOCKET, - EXEC_INPUT_NAMED_FD)) + if (exec_input_is_inheritable(s->exec_context.std_input)) return; - /* We assume these listed inputs refer to bidirectional streams, and hence duplicating them from - * stdin to stdout/stderr makes sense and hence leaving EXEC_OUTPUT_INHERIT in place makes sense, - * too. Outputs such as regular files or sealed data memfds otoh don't really make sense to be - * duplicated for both input and output at the same time (since they then would cause a feedback - * loop), hence override EXEC_OUTPUT_INHERIT with the default stderr/stdout setting. */ + /* Override EXEC_OUTPUT_INHERIT with the default stderr/stdout setting if not applicable for + * given stdin mode. */ if (s->exec_context.std_error == EXEC_OUTPUT_INHERIT && s->exec_context.std_output == EXEC_OUTPUT_INHERIT) diff --git a/test/fuzz/fuzz-execute-serialize/initial b/test/fuzz/fuzz-execute-serialize/initial index 4d9103c3ab..786ad89678 100644 --- a/test/fuzz/fuzz-execute-serialize/initial +++ b/test/fuzz/fuzz-execute-serialize/initial @@ -63,14 +63,10 @@ exec-context-stdio-as-fds= exec-context-std-input-fd-name= exec-context-std-output-fd-name= exec-context-std-error-fd-name= +exec-context-std-input-data= exec-context-std-input-file= exec-context-std-output-file= -exec-context-std-output-file-append= -exec-context-std-output-file-truncate= exec-context-std-error-file= -exec-context-std-error-file-append= -exec-context-std-error-file-truncate= -exec-context-stdin-data= exec-context-tty-path= exec-context-tty-reset= exec-context-tty-vhangup=