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

@@ -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<job_t> &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_t> &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<void()> &child_action) {
     pid_t pid = execute_fork(drain_threads);
@@ -413,7 +415,7 @@ static bool fork_child_for_process(const std::shared_ptr<job_t> &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<job_t> &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<job_t
     null_terminated_array_t<char> 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<job_t
     } else
 #endif
     {
-        if (!fork_child_for_process(j, p, proc_io_chain, false, "external command",
+        if (!fork_child_for_process(j, p, *dup2s, false, "external command",
                                     [&] { safe_launch_process(p, actual_cmd, argv, envv); })) {
             return false;
         }
@@ -820,6 +833,12 @@ static bool exec_block_or_func_process(parser_t &parser, std::shared_ptr<job_t>
     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<job_t>
         // 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;

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
+int setup_child_process(process_t *p, const dup2_list_t &dup2s) {
-/// following: First it closes any open file descriptors not related to the child by calling
+    for (const auto &act : dup2s.get_actions()) {
-/// close_unused_internal_pipes() and closing the universal variable server file descriptor. It then
+        int err = act.target < 0 ? close(act.src) : dup2(act.src, act.target);
-/// goes on to perform all the redirections described by \c io.
+        if (err < 0) {
-///
+            // We have a null p if this is for the exec (non-fork) path.
-/// \param io_chain the list of IO redirections for the child
+            if (p != nullptr) {
-///
+                debug_safe(4, "redirect_in_child_after_fork failed in setup_child_process");
-/// \return 0 on sucess, -1 on failure
+                exit_without_destructors(1);
-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<const io_fd_t *>(io)->old_fd) {
-            continue;
             }
-
+            return err;
-        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<const io_file_t *>(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<const io_fd_t *>(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<const io_pipe_t *>(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;
         }
     }
-    }
+    // 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;
 

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

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<action_t> actions_;