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

@@ -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_ptr<j
     if (local_builtin_stdin == -1) return false;
 
     // Determine if we have a "direct" redirection for stdin.
-    bool stdin_is_directly_redirected;
+    bool stdin_is_directly_redirected = false;
     if (!p->is_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<const io_data_t> 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<io_pipe_t>(
             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<io_pipe_t> pipe_read{};
@@ -1149,8 +1160,8 @@ bool exec_job(parser_t &parser, shared_ptr<job_t> 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);
         }
     }
 

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<io_close_t>(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<io_fd_t>(spec.fd, *target_fd, true /* user supplied */));
+                }
+                break;
+            }
+            default: {
+                this->push_back(make_unique<io_file_t>(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);

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<io_data_ref_t> {
     /// 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;
 

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<io_data_t> get_stderr_merge() {
-    return std::make_shared<io_fd_t>(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<g::arguments_or_redirections_list> node,
-                                                   io_chain_t *out_chain) {
-    io_chain_t result;
-    bool errored = false;
-
+bool parse_execution_context_t::determine_redirections(
+    tnode_t<g::arguments_or_redirections_list> node, redirection_spec_list_t *out_redirections) {
     // Get all redirection nodes underneath the statement.
     while (auto redirect_node = node.next_in_list<g::redirection>()) {
         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<g::arguments_or_redir
             return false;
         }
 
-        // Generate the actual IO redirection.
-        shared_ptr<io_data_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<g::arguments_or_redirections_list>();
-    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).

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<grammar::arguments_or_redirections_list> 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<grammar::arguments_or_redirections_list> node,
+                                redirection_spec_list_t *out_redirs);
 
     parse_execution_result_t run_1_job(tnode_t<grammar::job> job, const block_t *associated_block);
     parse_execution_result_t run_job_conjunction(tnode_t<grammar::job_conjunction> job_expr,

src/proc.h

@@ -172,7 +172,7 @@ class process_t {
    private:
     null_terminated_array_t<wchar_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

src/redirection.cpp

@@ -4,6 +4,7 @@
 
 #include <fcntl.h>
 
+#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<int> 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> dup2_list_t::resolve_chain(const io_chain_t &io_chain) {
     ASSERT_IS_NOT_FORKED_CHILD();
     dup2_list_t result;

src/redirection.h

@@ -4,10 +4,49 @@
 #include <vector>
 
 #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<int> 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<redirection_spec_t>;
 
 /// A class representing a sequence of basic redirections.
 class dup2_list_t {

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