From af473d4d0c62d781abf9dd33723939178e0abf05 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Thu, 12 Dec 2019 16:44:24 -0800 Subject: [PATCH] Introduce redirection_spec_t Prior to this change, a process after it has been constructed by parse_execution, but before it is executed, was given a list of io_data_t redirections. The problem is that redirections have a sensitive ownership policy because they hold onto fds. This made it rather hard to reason about fd lifetime. Change these to redirection_spec_t. This is a textual description of a redirection after expansion. It does not represent an open file and so its lifetime is no longer important. This enables files to be held only on the stack, and are no longer owned by a process of indeterminate lifetime. --- src/exec.cpp | 27 ++++++++++---- src/io.cpp | 24 ++++++++++++ src/io.h | 5 +++ src/parse_execution.cpp | 82 +++++++++++++++-------------------------- src/parse_execution.h | 7 ++-- src/proc.h | 11 +++--- src/redirection.cpp | 23 ++++++++++++ src/redirection.h | 43 ++++++++++++++++++++- src/tokenizer.h | 9 +---- 9 files changed, 152 insertions(+), 79 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index 84c4ff416..5f4d2ec10 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -292,7 +292,9 @@ void internal_exec(env_stack_t &vars, job_t *j, const io_chain_t &block_io) { // Do a regular launch - but without forking first... process_t *p = j->processes.front().get(); io_chain_t all_ios = block_io; - all_ios.append(p->io_chain()); + if (!all_ios.append_from_specs(p->redirection_specs())) { + return; + } // child_setup_process makes sure signals are properly set up. auto redirs = dup2_list_t::resolve_chain(all_ios); @@ -532,15 +534,19 @@ static bool exec_internal_builtin_proc(parser_t &parser, const std::shared_ptris_first_in_job) { // We must have a pipe stdin_is_directly_redirected = true; } else { // We are not a pipe. Check if there is a redirection local to the process // that's not io_mode_t::close. - const shared_ptr stdin_io = p->io_chain().io_for_fd(STDIN_FILENO); - stdin_is_directly_redirected = stdin_io && stdin_io->io_mode != io_mode_t::close; + for (const auto &redir : p->redirection_specs()) { + if (redir.fd == STDIN_FILENO && !redir.is_close()) { + stdin_is_directly_redirected = true; + break; + } + } } streams.stdin_fd = local_builtin_stdin; @@ -975,13 +981,18 @@ static bool exec_process_in_job(parser_t &parser, process_t *p, std::shared_ptr< // The IO chain for this process. io_chain_t process_net_io_chain = block_io; + if (pipes.write.valid()) { process_net_io_chain.push_back(std::make_shared( p->pipe_write_fd, false /* not input */, std::move(pipes.write))); } - // The explicit IO redirections associated with the process. - process_net_io_chain.append(p->io_chain()); + // Append IOs from the process's redirection specs. + // This may fail. + if (!process_net_io_chain.append_from_specs(p->redirection_specs())) { + // Error. + return false; + } // Read pipe goes last. shared_ptr pipe_read{}; @@ -1149,8 +1160,8 @@ bool exec_job(parser_t &parser, shared_ptr j, const job_lineage_t &lineag // Get the list of all FDs so we can ensure our pipes do not conflict. fd_set_t conflicts = lineage.block_io.fd_set(); for (const auto &p : j->processes) { - for (const auto &io : p->io_chain()) { - conflicts.add(io->fd); + for (const auto &spec : p->redirection_specs()) { + conflicts.add(spec.fd); } } diff --git a/src/io.cpp b/src/io.cpp index 3fcdd9070..5b59105a9 100644 --- a/src/io.cpp +++ b/src/io.cpp @@ -222,6 +222,30 @@ void io_chain_t::append(const io_chain_t &chain) { this->insert(this->end(), chain.begin(), chain.end()); } +bool io_chain_t::append_from_specs(const redirection_spec_list_t &specs) { + for (const auto &spec : specs) { + switch (spec.mode) { + case redirection_mode_t::fd: { + if (spec.is_close()) { + this->push_back(make_unique(spec.fd)); + } else { + auto target_fd = spec.get_target_as_fd(); + assert(target_fd.has_value() && + "fd redirection should have been validated already"); + this->push_back( + make_unique(spec.fd, *target_fd, true /* user supplied */)); + } + break; + } + default: { + this->push_back(make_unique(spec.fd, spec.target, spec.oflags())); + break; + } + } + } + return true; +} + void io_chain_t::print() const { if (this->empty()) { std::fwprintf(stderr, L"Empty chain %p\n", this); diff --git a/src/io.h b/src/io.h index 406bd8e84..d16cf2625 100644 --- a/src/io.h +++ b/src/io.h @@ -16,6 +16,7 @@ #include "env.h" #include "global_safety.h" #include "maybe.h" +#include "redirection.h" using std::shared_ptr; @@ -359,6 +360,10 @@ class io_chain_t : public std::vector { /// if none. io_data_ref_t io_for_fd(int fd) const; + /// Attempt to resolve a list of redirection specs to IOs, appending to 'this'. + /// \return true on success, false on error, in which case an error will have been printed. + bool append_from_specs(const redirection_spec_list_t &specs); + /// Output debugging information to stderr. void print() const; diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index f972fa422..d707c4164 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -82,8 +82,9 @@ static wcstring profiling_cmd_name_for_redirectable_block(const parse_node_t &no } /// Get a redirection from stderr to stdout (i.e. 2>&1). -static std::shared_ptr get_stderr_merge() { - return std::make_shared(STDERR_FILENO, STDOUT_FILENO, true /* user_supplied */); +static redirection_spec_t get_stderr_merge() { + const wchar_t *stdout_fileno_str = L"1"; + return redirection_spec_t{STDERR_FILENO, redirection_mode_t::fd, stdout_fileno_str}; } parse_execution_context_t::parse_execution_context_t(parsed_source_ref_t pstree, parser_t *p, @@ -832,7 +833,7 @@ parse_execution_result_t parse_execution_context_t::populate_plain_process( // Produce the full argument list and the set of IO redirections. wcstring_list_t cmd_args; - io_chain_t process_io_chain; + redirection_spec_list_t redirections; if (use_implicit_cd) { // Implicit cd is simple. cmd_args = {L"cd", cmd}; @@ -858,7 +859,7 @@ parse_execution_result_t parse_execution_context_t::populate_plain_process( } // The set of IO redirections that we construct for the process. - if (!this->determine_io_chain(statement.child<1>(), &process_io_chain)) { + if (!this->determine_redirections(statement.child<1>(), &redirections)) { return parse_execution_errored; } @@ -869,7 +870,7 @@ parse_execution_result_t parse_execution_context_t::populate_plain_process( // Populate the process. proc->type = process_type; proc->set_argv(cmd_args); - proc->set_io_chain(process_io_chain); + proc->set_redirection_specs(std::move(redirections)); proc->actual_cmd = std::move(path_to_external_command); return parse_execution_success; } @@ -928,18 +929,15 @@ parse_execution_result_t parse_execution_context_t::expand_arguments_from_nodes( return parse_execution_success; } -bool parse_execution_context_t::determine_io_chain(tnode_t node, - io_chain_t *out_chain) { - io_chain_t result; - bool errored = false; - +bool parse_execution_context_t::determine_redirections( + tnode_t node, redirection_spec_list_t *out_redirections) { // Get all redirection nodes underneath the statement. while (auto redirect_node = node.next_in_list()) { wcstring target; // file path or target fd auto redirect = redirection_for_node(redirect_node, pstree->src, &target); if (!redirect || !redirect->is_valid()) { - // TODO: improve this error message. + // TODO: figure out if this can ever happen. If so, improve this error message. report_error(redirect_node, _(L"Invalid redirection: %ls"), redirect_node.get_source(pstree->src).c_str()); return false; @@ -955,50 +953,27 @@ bool parse_execution_context_t::determine_io_chain(tnode_t new_io; + // Make a redirection spec from the redirect token. assert(redirect && redirect->is_valid() && "expected to have a valid redirection"); - switch (redirect->mode) { - case redirection_mode_t::fd: { - if (target == L"-") { - new_io.reset(new io_close_t(redirect->fd)); - } else { - int old_fd = fish_wcstoi(target.c_str()); - if (errno || old_fd < 0) { - const wchar_t *fmt = - _(L"Requested redirection to '%ls', " - L"which is not a valid file descriptor"); - errored = report_error(redirect_node, fmt, target.c_str()); - } else { - new_io.reset(new io_fd_t(redirect->fd, old_fd, true)); - } - } - break; - } - default: { - int oflags = redirect->oflags(); - io_file_t *new_io_file = new io_file_t(redirect->fd, target, oflags); - new_io.reset(new_io_file); - break; - } - } - // Append the new_io if we got one. - if (new_io.get() != nullptr) { - result.push_back(new_io); + redirection_spec_t spec{redirect->fd, redirect->mode, std::move(target)}; + + // Validate this spec. + if (spec.mode == redirection_mode_t::fd && !spec.is_close() && !spec.get_target_as_fd()) { + const wchar_t *fmt = + _(L"Requested redirection to '%ls', which is not a valid file descriptor"); + report_error(redirect_node, fmt, spec.target.c_str()); + return false; } + out_redirections->push_back(std::move(spec)); if (redirect->stderr_merge) { // This was a redirect like &> which also modifies stderr. // Also redirect stderr to stdout. - result.push_back(get_stderr_merge()); + out_redirections->push_back(get_stderr_merge()); } } - - if (out_chain && !errored) { - *out_chain = std::move(result); - } - return !errored; + return true; } parse_execution_result_t parse_execution_context_t::populate_not_process( @@ -1026,14 +1001,15 @@ parse_execution_result_t parse_execution_context_t::populate_block_process( // The set of IO redirections that we construct for the process. // TODO: fix this ugly find_child. auto arguments = specific_statement.template find_child(); - io_chain_t process_io_chain; - bool errored = !this->determine_io_chain(arguments, &process_io_chain); - if (errored) return parse_execution_errored; + redirection_spec_list_t redirections; + if (!this->determine_redirections(arguments, &redirections)) { + return parse_execution_errored; + } proc->type = process_type_t::block_node; proc->block_node_source = pstree; proc->internal_block_node = statement; - proc->set_io_chain(process_io_chain); + proc->set_redirection_specs(std::move(redirections)); return parse_execution_success; } @@ -1172,9 +1148,9 @@ parse_execution_result_t parse_execution_context_t::populate_job_from_job_node( if (parsed_pipe->stderr_merge) { // This was a pipe like &| which redirects both stdout and stderr. // Also redirect stderr to stdout. - auto ios = processes.back()->io_chain(); - ios.push_back(get_stderr_merge()); - processes.back()->set_io_chain(std::move(ios)); + auto specs = processes.back()->redirection_specs(); + specs.push_back(get_stderr_merge()); + processes.back()->set_redirection_specs(std::move(specs)); } // Store the new process (and maybe with an error). diff --git a/src/parse_execution.h b/src/parse_execution.h index 5c8d2f7dd..4edf7dc76 100644 --- a/src/parse_execution.h +++ b/src/parse_execution.h @@ -125,9 +125,10 @@ class parse_execution_context_t { wcstring_list_t *out_arguments, globspec_t glob_behavior); - // Determines the IO chain. Returns true on success, false on error. - bool determine_io_chain(tnode_t node, - io_chain_t *out_chain); + // Determines the list of redirections for a node. Returns none() on failure, for example, an + // invalid fd. + bool determine_redirections(tnode_t node, + redirection_spec_list_t *out_redirs); parse_execution_result_t run_1_job(tnode_t job, const block_t *associated_block); parse_execution_result_t run_job_conjunction(tnode_t job_expr, diff --git a/src/proc.h b/src/proc.h index 4cd507745..0118d560a 100644 --- a/src/proc.h +++ b/src/proc.h @@ -172,7 +172,7 @@ class process_t { private: null_terminated_array_t argv_array; - io_chain_t process_io_chain; + redirection_spec_list_t proc_redirection_specs; // No copying. process_t(const process_t &rhs) = delete; @@ -221,10 +221,11 @@ class process_t { return argv ? argv[0] : nullptr; } - /// IO chain getter and setter. - const io_chain_t &io_chain() const { return process_io_chain; } - - void set_io_chain(io_chain_t chain) { this->process_io_chain = std::move(chain); } + /// Redirection list getter and setter. + const redirection_spec_list_t &redirection_specs() const { return proc_redirection_specs; } + void set_redirection_specs(redirection_spec_list_t specs) { + this->proc_redirection_specs = std::move(specs); + } /// Store the current topic generations. That is, right before the process is launched, record /// the generations of all topics; then we can tell which generation values have changed after diff --git a/src/redirection.cpp b/src/redirection.cpp index e6395b40a..8c65ea4c1 100644 --- a/src/redirection.cpp +++ b/src/redirection.cpp @@ -4,6 +4,7 @@ #include +#include "io.h" #include "wutil.h" #define NOCLOB_ERROR _(L"The file '%ls' already exists") @@ -15,6 +16,28 @@ dup2_list_t::~dup2_list_t() = default; +maybe_t redirection_spec_t::get_target_as_fd() const { + errno = 0; + int result = fish_wcstoi(target.c_str()); + if (errno || result < 0) return none(); + return result; +} + +int redirection_spec_t::oflags() const { + switch (mode) { + case redirection_mode_t::append: + return O_CREAT | O_APPEND | O_WRONLY; + case redirection_mode_t::overwrite: + return O_CREAT | O_WRONLY | O_TRUNC; + case redirection_mode_t::noclob: + return O_CREAT | O_EXCL | O_WRONLY; + case redirection_mode_t::input: + return O_RDONLY; + case redirection_mode_t::fd: + DIE("Not a file redirection"); + } +} + maybe_t dup2_list_t::resolve_chain(const io_chain_t &io_chain) { ASSERT_IS_NOT_FORKED_CHILD(); dup2_list_t result; diff --git a/src/redirection.h b/src/redirection.h index 9d5861f1d..0d43e0882 100644 --- a/src/redirection.h +++ b/src/redirection.h @@ -4,10 +4,49 @@ #include #include "common.h" -#include "io.h" #include "maybe.h" -/// This file supports "applying" redirections. +/// This file supports specifying and applying redirections. + +enum class redirection_mode_t { + overwrite, // normal redirection: > file.txt + append, // appending redirection: >> file.txt + input, // input redirection: < file.txt + fd, // fd redirection: 2>&1 + noclob // noclobber redirection: >? file.txt +}; + +class io_chain_t; + +/// A struct which represents a redirection specification from the user. +/// Here the file descriptors don't represent open files - it's purely textual. +struct redirection_spec_t { + /// The redirected fd, or -1 on overflow. + /// In the common case of a pipe, this is 1 (STDOUT_FILENO). + /// For example, in the case of "3>&1" this will be 3. + int fd{-1}; + + /// The redirection mode. + redirection_mode_t mode{redirection_mode_t::overwrite}; + + /// The target of the redirection. + /// For example in "3>&1", this will be "1". + /// In "< file.txt" this will be "file.txt". + wcstring target{}; + + /// \return if this is a close-type redirection. + bool is_close() const { return mode == redirection_mode_t::fd && target == L"-"; } + + /// Attempt to parse target as an fd. Return the fd, or none() if none. + maybe_t get_target_as_fd() const; + + /// \return the open flags for this redirection. + int oflags() const; + + redirection_spec_t(int fd, redirection_mode_t mode, wcstring target) + : fd(fd), mode(mode), target(std::move(target)) {} +}; +using redirection_spec_list_t = std::vector; /// A class representing a sequence of basic redirections. class dup2_list_t { diff --git a/src/tokenizer.h b/src/tokenizer.h index 5e6e3a12b..90563b701 100644 --- a/src/tokenizer.h +++ b/src/tokenizer.h @@ -8,6 +8,7 @@ #include "common.h" #include "maybe.h" #include "parse_constants.h" +#include "redirection.h" /// Token types. enum class token_type_t { @@ -22,14 +23,6 @@ enum class token_type_t { comment, /// comment token }; -enum class redirection_mode_t { - overwrite, // normal redirection: > file.txt - append, // appending redirection: >> file.txt - input, // input redirection: < file.txt - fd, // fd redirection: 2>&1 - noclob // noclobber redirection: >? file.txt -}; - /// Flag telling the tokenizer to accept incomplete parameters, i.e. parameters with mismatching /// parenthesis, etc. This is useful for tab-completion. #define TOK_ACCEPT_UNFINISHED 1