Switch io_file_t to store an fd, not a path

Prior to this fix, a file redirection was turned into an io_file_t. This is
annoying because every place where we want to apply the redirection, we
might fail due to open() failing. Switch to opening the file at the point
we resolve the redirection spec. This will simplify a lot of code.

src/exec.cpp

@@ -80,8 +80,7 @@ static bool redirection_is_to_real_file(const shared_ptr<const io_data_t> &io) {
     bool result = false;
     if (io && io->io_mode == io_mode_t::file) {
         // It's a file redirection. Compare the path to /dev/null.
-        const wcstring &path = static_cast<const io_file_t *>(io.get())->filename;
-        if (path != L"/dev/null") {
+        if (!static_cast<const io_file_t *>(io.get())->is_dev_null()) {
             // It's not /dev/null.
             result = true;
         }
@@ -213,26 +212,11 @@ static bool resolve_file_redirections_to_fds(const io_chain_t &in_chain, const w
             case io_mode_t::pipe:
             case io_mode_t::bufferfill:
             case io_mode_t::fd:
+            case io_mode_t::file:
             case io_mode_t::close: {
                 result_chain.push_back(in);
                 break;
             }
-            case io_mode_t::file: {
-                // We have a path-based redireciton. Resolve it to a file.
-                const io_file_t *in_file = static_cast<const io_file_t *>(in.get());
-                int fd = wopen(path_apply_working_directory(in_file->filename, pwd), in_file->flags,
-                               OPEN_MASK);
-                if (fd < 0) {
-                    debug(1, FILE_ERROR, in_file->filename.c_str());
-                    wperror(L"open");
-                    success = false;
-                    break;
-                }
-
-                opened_fds.push_back(autoclose_fd_t(fd));
-                result_chain.push_back(std::make_shared<io_fd_t>(in->fd, fd, false));
-                break;
-            }
         }
         if (!success) {
             break;
@@ -292,7 +276,7 @@ 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;
-    if (!all_ios.append_from_specs(p->redirection_specs())) {
+    if (!all_ios.append_from_specs(p->redirection_specs(), vars.get_pwd_slash())) {
         return;
     }
 
@@ -507,13 +491,7 @@ static bool exec_internal_builtin_proc(parser_t &parser, const std::shared_ptr<j
             }
             case io_mode_t::file: {
                 const io_file_t *in_file = static_cast<const io_file_t *>(in.get());
-                locally_opened_stdin =
-                    autoclose_fd_t{wopen(in_file->filename, in_file->flags, OPEN_MASK)};
-                if (!locally_opened_stdin.valid()) {
-                    debug(1, FILE_ERROR, in_file->filename.c_str());
-                    wperror(L"open");
-                }
-                local_builtin_stdin = locally_opened_stdin.fd();
+                local_builtin_stdin = in_file->file_fd();
                 break;
             }
             case io_mode_t::close: {
@@ -989,7 +967,8 @@ static bool exec_process_in_job(parser_t &parser, process_t *p, std::shared_ptr<
 
     // Append IOs from the process's redirection specs.
     // This may fail.
-    if (!process_net_io_chain.append_from_specs(p->redirection_specs())) {
+    if (!process_net_io_chain.append_from_specs(p->redirection_specs(),
+                                                parser.vars().get_pwd_slash())) {
         // Error.
         return false;
     }

src/fish_tests.cpp

@@ -2479,15 +2479,6 @@ static void test_dup2s() {
     auto act2 = list->get_actions().at(1);
     do_test(act2.src == 19);
     do_test(act2.target == 3);
-
-    // Invalid files should fail to open.
-    // Suppress the debug() message.
-    int saved_debug_level = debug_level;
-    debug_level = -1;
-    chain.push_back(make_shared<io_file_t>(2, L"/definitely/not/a/valid/path/for/this/test", 0666));
-    list = dup2_list_t::resolve_chain(chain);
-    do_test(!list.has_value());
-    debug_level = saved_debug_level;
 }
 
 static void test_dup2s_fd_for_target_fd() {

src/io.cpp

@@ -4,6 +4,7 @@
 #include "io.h"
 
 #include <errno.h>
+#include <fcntl.h>
 #include <stddef.h>
 #include <stdio.h>
 #include <unistd.h>
@@ -15,16 +16,29 @@
 #include "exec.h"
 #include "fallback.h"  // IWYU pragma: keep
 #include "iothread.h"
+#include "path.h"
 #include "redirection.h"
 #include "wutil.h"  // IWYU pragma: keep
 
+/// File redirection error message.
+#define FILE_ERROR _(L"An error occurred while redirecting file '%ls'")
+#define NOCLOB_ERROR _(L"The file '%ls' already exists")
+
+/// Base open mode to pass to calls to open.
+#define OPEN_MASK 0666
+
 io_data_t::~io_data_t() = default;
 
+io_file_t::io_file_t(int f, autoclose_fd_t file, const wcstring &path)
+    : io_data_t(io_mode_t::file, f), file_fd_(std::move(file)), is_dev_null_(path == L"/dev/null") {
+    assert(file_fd_.valid() && "File is not valid");
+}
+
 void io_close_t::print() const { std::fwprintf(stderr, L"close %d\n", fd); }
 
 void io_fd_t::print() const { std::fwprintf(stderr, L"FD map %d -> %d\n", old_fd, fd); }
 
-void io_file_t::print() const { std::fwprintf(stderr, L"file (%ls)\n", filename.c_str()); }
+void io_file_t::print() const { std::fwprintf(stderr, L"file (%d)\n", file_fd_.fd()); }
 
 void io_pipe_t::print() const {
     std::fwprintf(stderr, L"pipe {%d} (input: %s)\n", pipe_fd(), is_input_ ? "yes" : "no");
@@ -195,6 +209,8 @@ std::shared_ptr<io_buffer_t> io_bufferfill_t::finish(std::shared_ptr<io_bufferfi
 
 io_pipe_t::~io_pipe_t() = default;
 
+io_file_t::~io_file_t() = default;
+
 io_bufferfill_t::~io_bufferfill_t() = default;
 
 io_buffer_t::~io_buffer_t() {
@@ -222,7 +238,7 @@ 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) {
+bool io_chain_t::append_from_specs(const redirection_spec_list_t &specs, const wcstring &pwd) {
     for (const auto &spec : specs) {
         switch (spec.mode) {
             case redirection_mode_t::fd: {
@@ -238,7 +254,20 @@ bool io_chain_t::append_from_specs(const redirection_spec_list_t &specs) {
                 break;
             }
             default: {
-                this->push_back(make_unique<io_file_t>(spec.fd, spec.target, spec.oflags()));
+                // We have a path-based redireciton. Resolve it to a file.
+                wcstring path = path_apply_working_directory(spec.target, pwd);
+                int oflags = spec.oflags();
+                autoclose_fd_t file{wopen(path, oflags, OPEN_MASK)};
+                if (!file.valid()) {
+                    if ((oflags & O_EXCL) && (errno == EEXIST)) {
+                        debug(1, NOCLOB_ERROR, spec.target.c_str());
+                    } else {
+                        debug(1, FILE_ERROR, spec.target.c_str());
+                        if (should_debug(1)) wperror(L"open");
+                    }
+                    return false;
+                }
+                this->push_back(std::make_shared<io_file_t>(spec.fd, std::move(file), path));
                 break;
             }
         }

src/io.h

@@ -210,19 +210,25 @@ class io_fd_t : public io_data_t {
         : io_data_t(io_mode_t::fd, f), old_fd(old), user_supplied(us) {}
 };
 
+/// Represents a redirection to or from an opened file.
 class io_file_t : public io_data_t {
    public:
-    /// The filename.
-    wcstring filename;
-    /// file creation flags to send to open.
-    const int flags;
-
     void print() const override;
 
-    io_file_t(int f, wcstring fname, int fl = 0)
-        : io_data_t(io_mode_t::file, f), filename(std::move(fname)), flags(fl) {}
+    io_file_t(int f, autoclose_fd_t file, const wcstring &path);
 
-    ~io_file_t() override = default;
+    ~io_file_t() override;
+
+    int file_fd() const { return file_fd_.fd(); }
+
+    bool is_dev_null() const { return is_dev_null_; }
+
+   private:
+    // The fd for the file which we are writing to or reading from.
+    autoclose_fd_t file_fd_;
+
+    // Hackish - whether this is /dev/null.
+    const bool is_dev_null_;
 };
 
 /// Represents (one end) of a pipe.
@@ -362,7 +368,7 @@ class io_chain_t : public std::vector<io_data_ref_t> {
 
     /// 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);
+    bool append_from_specs(const redirection_spec_list_t &specs, const wcstring &pwd);
 
     /// Output debugging information to stderr.
     void print() const;

src/redirection.cpp

@@ -44,40 +44,8 @@ maybe_t<dup2_list_t> dup2_list_t::resolve_chain(const io_chain_t &io_chain) {
     for (const auto &io_ref : io_chain) {
         switch (io_ref->io_mode) {
             case io_mode_t::file: {
-                // Here we definitely do not want to set CLO_EXEC because our child needs access.
-                // Open the file.
-                const io_file_t *io_file = static_cast<const io_file_t *>(io_ref.get());
-                autoclose_fd_t file_fd{wopen(io_file->filename, io_file->flags, OPEN_MASK)};
-                if (!file_fd.valid()) {
-                    if ((io_file->flags & O_EXCL) && (errno == EEXIST)) {
-                        debug(1, NOCLOB_ERROR, io_file->filename.c_str());
-                    } else {
-                        debug(1, FILE_ERROR, io_file->filename.c_str());
-                        if (should_debug(1)) wperror(L"open");
-                    }
-                    return none();
-                }
-
-                // If by chance we got the file we want, we're done. Otherwise move the fd to an
-                // unused place and dup2 it.
-                // Note move_fd_to_unused() will close the incoming file_fd.
-                if (file_fd.fd() != io_file->fd) {
-                    file_fd = move_fd_to_unused(std::move(file_fd), io_chain.fd_set(),
-                                                false /* cloexec */);
-                    if (!file_fd.valid()) {
-                        debug(1, FILE_ERROR, io_file->filename.c_str());
-                        if (should_debug(1)) wperror(L"dup");
-                        return none();
-                    }
-                }
-                assert(file_fd.valid() && "Should have a valid file_fd");
-
-                // Mark our dup2 and our close actions.
-                result.add_dup2(file_fd.fd(), io_file->fd);
-                result.add_close(file_fd.fd());
-
-                // Store our file.
-                result.opened_fds_.push_back(std::move(file_fd));
+                const io_file_t *io = static_cast<const io_file_t *>(io_ref.get());
+                result.add_dup2(io->file_fd(), io->fd);
                 break;
             }