From d62576ce22b6ba53d9632011325cdd605933bc9d Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Mon, 28 Jan 2019 14:35:56 -0800 Subject: [PATCH] Adopt dup2_list_t in fork execution path This switches IO redirections after fork() to use the dup2_list_t, instead of io_chain_t. This results in simpler code with much simpler error handling. --- src/exec.cpp | 31 ++++++++-- src/postfork.cpp | 147 ++++------------------------------------------ src/postfork.h | 5 +- src/redirection.h | 2 + 4 files changed, 42 insertions(+), 143 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index 75af77921..3d51914b2 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -39,6 +39,7 @@ #include "postfork.h" #include "proc.h" #include "reader.h" +#include "redirection.h" #include "signal.h" #include "wutil.h" // IWYU pragma: keep @@ -367,7 +368,8 @@ void internal_exec(env_stack_t &vars, job_t *j, const io_chain_t &all_ios) { // It's known to be wrong - for example, it means that redirections bound for subsequent // commands in the pipeline will apply to exec. However, using exec in a pipeline doesn't // really make sense, so I'm not trying to fix it here. - if (!setup_child_process(0, all_ios)) { + auto redirs = dup2_list_t::resolve_chain(all_ios); + if (redirs && !setup_child_process(0, *redirs)) { // Decrement SHLVL as we're removing ourselves from the shell "stack". auto shlvl_var = vars.get(L"SHLVL", ENV_GLOBAL | ENV_EXPORT); wcstring shlvl_str = L"0"; @@ -404,7 +406,7 @@ static void on_process_created(const std::shared_ptr &j, pid_t child_pid) /// Call fork() as part of executing a process \p p in a job \j. Execute \p child_action in the /// context of the child. Returns true if fork succeeded, false if fork failed. static bool fork_child_for_process(const std::shared_ptr &job, process_t *p, - const io_chain_t &io_chain, bool drain_threads, + const dup2_list_t &dup2s, bool drain_threads, const char *fork_type, const std::function &child_action) { pid_t pid = execute_fork(drain_threads); @@ -413,7 +415,7 @@ static bool fork_child_for_process(const std::shared_ptr &job, process_t // stdout and stderr, and then exit. p->pid = getpid(); child_set_group(job.get(), p); - setup_child_process(p, io_chain); + setup_child_process(p, dup2s); child_action(); DIE("Child process returned control to fork_child lambda!"); } @@ -634,9 +636,15 @@ static bool handle_builtin_output(const std::shared_ptr &j, process_t *p, const char *errbuff = errbuff_str.data(); size_t errbuff_len = errbuff_str.size(); + // Resolve our IO chain to a sequence of dup2s. + auto dup2s = dup2_list_t::resolve_chain(*io_chain); + if (!dup2s) { + return false; + } + fflush(stdout); fflush(stderr); - if (!fork_child_for_process(j, p, *io_chain, false, "internal builtin", [&] { + if (!fork_child_for_process(j, p, *dup2s, false, "internal builtin", [&] { do_builtin_io(outbuff, outbuff_len, errbuff, errbuff_len); exit_without_destructors(p->status); })) { @@ -655,6 +663,11 @@ static bool exec_external_command(env_stack_t &vars, const std::shared_ptr argv_array; convert_wide_array_to_narrow(p->get_argv_array(), &argv_array); + // Convert our IO chain to a dup2 sequence. + auto dup2s = dup2_list_t::resolve_chain(proc_io_chain); + if (! dup2s) + return false; + // Ensure that stdin is blocking before we hand it off (see issue #176). It's a // little strange that we only do this with stdin and not with stdout or stderr. // However in practice, setting or clearing O_NONBLOCK on stdin also sets it for the @@ -741,7 +754,7 @@ static bool exec_external_command(env_stack_t &vars, const std::shared_ptr io_chain.remove(block_output_io_buffer); block_output_io_buffer->read(); + // Resolve our IO chain to a sequence of dup2s. + auto dup2s = dup2_list_t::resolve_chain(io_chain); + if (!dup2s) { + return false; + } + const std::string buffer_contents = block_output_io_buffer->buffer().newline_serialized(); const char *buffer = buffer_contents.data(); size_t count = buffer_contents.size(); @@ -827,7 +846,7 @@ static bool exec_block_or_func_process(parser_t &parser, std::shared_ptr // We don't have to drain threads here because our child process is simple. const char *fork_reason = p->type == INTERNAL_BLOCK_NODE ? "internal block io" : "internal function io"; - if (!fork_child_for_process(j, p, io_chain, false, fork_reason, [&] { + if (!fork_child_for_process(j, p, *dup2s, false, fork_reason, [&] { exec_write_and_exit(block_output_io_buffer->fd, buffer, count, status); })) { return false; diff --git a/src/postfork.cpp b/src/postfork.cpp index 7a6f655c0..4326d077a 100644 --- a/src/postfork.cpp +++ b/src/postfork.cpp @@ -19,6 +19,7 @@ #include "iothread.h" #include "postfork.h" #include "proc.h" +#include "redirection.h" #include "signal.h" #include "wutil.h" // IWYU pragma: keep @@ -38,27 +39,6 @@ /// Fork error message. #define FORK_ERROR "Could not create child process - exiting" -/// File redirection clobbering error message. -#define NOCLOB_ERROR "The file '%s' already exists" - -/// File redirection error message. -#define FILE_ERROR "An error occurred while redirecting file '%s'" - -/// File descriptor redirection error message. -#define FD_ERROR "An error occurred while redirecting file descriptor %s" - -/// Pipe error message. -#define LOCAL_PIPE_ERROR "An error occurred while setting up pipe" - -static bool log_redirections = false; - -/// Cover for debug_safe that can take an int. The format string should expect a %s. -static void debug_safe_int(int level, const char *format, int val) { - char buff[128]; - format_long_safe(buff, val); - debug_safe(level, format, buff); -} - /// Called only by the child to set its own process group (possibly creating a new group in the /// process if it is the first in a JOB_CONTROL job. /// Returns true on sucess, false on failiure. @@ -175,126 +155,23 @@ bool maybe_assign_terminal(const job_t *j) { return true; } -/// Set up a childs io redirections. Should only be called by setup_child_process(). Does the -/// following: First it closes any open file descriptors not related to the child by calling -/// close_unused_internal_pipes() and closing the universal variable server file descriptor. It then -/// goes on to perform all the redirections described by \c io. -/// -/// \param io_chain the list of IO redirections for the child -/// -/// \return 0 on sucess, -1 on failure -static int handle_child_io(const io_chain_t &io_chain) { - for (size_t idx = 0; idx < io_chain.size(); idx++) { - const io_data_t *io = io_chain.at(idx).get(); - - if (io->io_mode == io_mode_t::fd && io->fd == static_cast(io)->old_fd) { - continue; - } - - switch (io->io_mode) { - case io_mode_t::close: { - if (log_redirections) fwprintf(stderr, L"%d: close %d\n", getpid(), io->fd); - if (close(io->fd)) { - debug_safe_int(0, "Failed to close file descriptor %s", io->fd); - safe_perror("close"); - } - break; - } - - case io_mode_t::file: { - // Here we definitely do not want to set CLO_EXEC because our child needs access. - const io_file_t *io_file = static_cast(io); - int tmp = open(io_file->filename_cstr, io_file->flags, OPEN_MASK); - if (tmp < 0) { - if ((io_file->flags & O_EXCL) && (errno == EEXIST)) { - debug_safe(1, NOCLOB_ERROR, io_file->filename_cstr); - } else { - debug_safe(1, FILE_ERROR, io_file->filename_cstr); - safe_perror("open"); - } - - return -1; - } else if (tmp != io->fd) { - // This call will sometimes fail, but that is ok, this is just a precausion. - close(io->fd); - - if (dup2(tmp, io->fd) == -1) { - debug_safe_int(1, FD_ERROR, io->fd); - safe_perror("dup2"); - exec_close(tmp); - return -1; - } - exec_close(tmp); - } - break; - } - - case io_mode_t::fd: { - int old_fd = static_cast(io)->old_fd; - if (log_redirections) - fwprintf(stderr, L"%d: fd dup %d to %d\n", getpid(), old_fd, io->fd); - - // This call will sometimes fail, but that is ok, this is just a precausion. - close(io->fd); - - if (dup2(old_fd, io->fd) == -1) { - debug_safe_int(1, FD_ERROR, io->fd); - safe_perror("dup2"); - return -1; - } - break; - } - - case io_mode_t::buffer: - case io_mode_t::pipe: { - const io_pipe_t *io_pipe = static_cast(io); - // If write_pipe_idx is 0, it means we're connecting to the read end (first pipe - // fd). If it's 1, we're connecting to the write end (second pipe fd). - unsigned int write_pipe_idx = (io_pipe->is_input ? 0 : 1); -#if 0 - debug(0, L"%ls %ls on fd %d (%d %d)", write_pipe?L"write":L"read", - (io->io_mode == io_mode_t::buffer)?L"buffer":L"pipe", io->fd, io->pipe_fd[0], - io->pipe_fd[1]); -#endif - if (log_redirections) - fwprintf(stderr, L"%d: %s dup %d to %d\n", getpid(), - io->io_mode == io_mode_t::buffer ? "buffer" : "pipe", - io_pipe->pipe_fd[write_pipe_idx], io->fd); - if (dup2(io_pipe->pipe_fd[write_pipe_idx], io->fd) != io->fd) { - debug_safe(1, LOCAL_PIPE_ERROR); - safe_perror("dup2"); - return -1; - } - - if (io_pipe->pipe_fd[0] >= 0) exec_close(io_pipe->pipe_fd[0]); - if (io_pipe->pipe_fd[1] >= 0) exec_close(io_pipe->pipe_fd[1]); - break; +int setup_child_process(process_t *p, const dup2_list_t &dup2s) { + for (const auto &act : dup2s.get_actions()) { + int err = act.target < 0 ? close(act.src) : dup2(act.src, act.target); + if (err < 0) { + // We have a null p if this is for the exec (non-fork) path. + if (p != nullptr) { + debug_safe(4, "redirect_in_child_after_fork failed in setup_child_process"); + exit_without_destructors(1); } + return err; } } - + // Set the handling for job control signals back to the default. + signal_reset_handlers(); return 0; } -int setup_child_process(process_t *p, const io_chain_t &io_chain) { - bool ok = true; - - if (ok) { - // In the case of io_mode_t::file, this can hang until data is available to read/write! - ok = (0 == handle_child_io(io_chain)); - if (p != 0 && !ok) { - debug_safe(4, "handle_child_io failed in setup_child_process"); - exit_without_destructors(1); - } - } - - if (ok) { - // Set the handling for job control signals back to the default. - signal_reset_handlers(); - } - - return ok ? 0 : -1; -} int g_fork_count = 0; diff --git a/src/postfork.h b/src/postfork.h index f7cfe080d..8ac8e82a3 100644 --- a/src/postfork.h +++ b/src/postfork.h @@ -14,6 +14,7 @@ #define FISH_USE_POSIX_SPAWN HAVE_SPAWN_H #endif +class dup2_list_t; class io_chain_t; class job_t; class process_t; @@ -29,11 +30,11 @@ bool maybe_assign_terminal(const job_t *j); /// descriptor actions are performed. /// /// \param p the child process to set up -/// \param io_chain the IO chain to use +/// \param dup2 the dup2 list to apply /// /// \return 0 on sucess, -1 on failiure. When this function returns, signals are always unblocked. /// On failiure, signal handlers, io redirections and process group of the process is undefined. -int setup_child_process(process_t *p, const io_chain_t &io_chain); +int setup_child_process(process_t *p, const dup2_list_t &dup2s); /// Call fork(), optionally waiting until we are no longer multithreaded. If the forked child /// doesn't do anything that could allocate memory, take a lock, etc. (like call exec), then it's diff --git a/src/redirection.h b/src/redirection.h index e82d4376d..95cc8fac0 100644 --- a/src/redirection.h +++ b/src/redirection.h @@ -11,6 +11,7 @@ /// A class representing a sequence of basic redirections. class dup2_list_t { + public: /// A type that represents the action dup2(src, target). /// If target is negative, this represents close(src). /// Note none of the fds here are considered 'owned'. @@ -19,6 +20,7 @@ class dup2_list_t { int target; }; + private: /// The list of actions. std::vector actions_;