Clean up how pipe fd avoidance works

fish has to ensure that the pipes it creates do not conflict with any
explicit fds named in redirections. Switch this code to using
autoclose_fd_t to make the ownership logic more explicit, and also
introduce fd_set_t to reduce the dependence on io_chain_t.

src/exec.cpp

@@ -288,15 +288,13 @@ static bool can_use_posix_spawn_for_job(const std::shared_ptr<job_t> &job) {
     return true;
 }
 
-void internal_exec(env_stack_t &vars, job_t *j, const io_chain_t &all_ios) {
+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());
 
     // child_setup_process makes sure signals are properly set up.
-
-    // PCA This is for handling exec. Passing all_ios here matches what fish 2.0.0 and 1.x did.
-    // 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.
     auto redirs = dup2_list_t::resolve_chain(all_ios);
     if (redirs && !child_setup_process(INVALID_PID, false, *redirs)) {
         // Decrement SHLVL as we're removing ourselves from the shell "stack".
@@ -311,7 +309,7 @@ void internal_exec(env_stack_t &vars, job_t *j, const io_chain_t &all_ios) {
         vars.set_one(L"SHLVL", ENV_GLOBAL | ENV_EXPORT, std::move(shlvl_str));
 
         // launch_process _never_ returns.
-        launch_process_nofork(vars, j->processes.front().get());
+        launch_process_nofork(vars, p);
     } else {
         j->mark_constructed();
         j->processes.front()->completed = true;
@@ -875,18 +873,17 @@ static proc_performer_t get_performer_for_process(process_t *p, const job_t *job
 }
 
 /// Execute a block node or function "process".
-/// \p user_ios contains the list of user-specified ios, used so we can avoid stomping on them with
-/// our pipes.
+/// \p conflicts contains the list of fds which pipes should avoid.
 /// \p allow_buffering if true, permit buffering the output.
 /// \return true on success, false on error.
 static bool exec_block_or_func_process(parser_t &parser, std::shared_ptr<job_t> j, process_t *p,
-                                       const io_chain_t &user_ios, io_chain_t io_chain,
+                                       const fd_set_t &conflicts, io_chain_t io_chain,
                                        bool allow_buffering) {
     // Create an output buffer if we're piping to another process.
     shared_ptr<io_bufferfill_t> block_output_bufferfill{};
     if (!p->is_last_in_job && allow_buffering) {
         // Be careful to handle failure, e.g. too many open fds.
-        block_output_bufferfill = io_bufferfill_t::create(user_ios);
+        block_output_bufferfill = io_bufferfill_t::create(conflicts);
         if (!block_output_bufferfill) {
             job_mark_process_as_failed(j, p);
             return false;
@@ -940,7 +937,7 @@ static bool exec_block_or_func_process(parser_t &parser, std::shared_ptr<job_t>
 /// certain buffering works. \returns true on success, false on exec error.
 static bool exec_process_in_job(parser_t &parser, process_t *p, std::shared_ptr<job_t> j,
                                 const io_chain_t &block_io, autoclose_pipes_t pipes,
-                                const io_chain_t &all_ios, const autoclose_pipes_t &deferred_pipes,
+                                const fd_set_t &conflicts, const autoclose_pipes_t &deferred_pipes,
                                 size_t stdout_read_limit, bool is_deferred_run = false) {
     // The write pipe (destined for stdout) needs to occur before redirections. For example,
     // with a redirection like this:
@@ -1025,7 +1022,7 @@ static bool exec_process_in_job(parser_t &parser, process_t *p, std::shared_ptr<
             // Allow buffering unless this is a deferred run. If deferred, then processes after us
             // were already launched, so they are ready to receive (or reject) our output.
             bool allow_buffering = !is_deferred_run;
-            if (!exec_block_or_func_process(parser, j, p, all_ios, process_net_io_chain,
+            if (!exec_block_or_func_process(parser, j, p, conflicts, process_net_io_chain,
                                             allow_buffering)) {
                 return false;
             }
@@ -1149,15 +1146,17 @@ bool exec_job(parser_t &parser, shared_ptr<job_t> j, const job_lineage_t &lineag
 
     const size_t stdout_read_limit = parser.libdata().read_limit;
 
-    // Get the list of all IOs so we can ensure our pipes do not conflict.
-    io_chain_t all_ios = lineage.block_io;
+    // 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) {
-        all_ios.append(p->io_chain());
+        for (const auto &io : p->io_chain()) {
+            conflicts.add(io->fd);
+        }
     }
 
     // Handle an exec call.
     if (j->processes.front()->type == process_type_t::exec) {
-        internal_exec(parser.vars(), j.get(), all_ios);
+        internal_exec(parser.vars(), j.get(), lineage.block_io);
         // internal_exec only returns if it failed to set up redirections.
         // In case of an successful exec, this code is not reached.
         bool status = !j->flags().negate;
@@ -1188,7 +1187,7 @@ bool exec_job(parser_t &parser, shared_ptr<job_t> j, const job_lineage_t &lineag
         autoclose_pipes_t proc_pipes;
         proc_pipes.read = std::move(pipe_next_read);
         if (!p->is_last_in_job) {
-            auto pipes = make_autoclose_pipes(all_ios);
+            auto pipes = make_autoclose_pipes(conflicts);
             if (!pipes) {
                 debug(1, PIPE_ERROR);
                 wperror(L"pipe");
@@ -1204,7 +1203,7 @@ bool exec_job(parser_t &parser, shared_ptr<job_t> j, const job_lineage_t &lineag
             deferred_pipes = std::move(proc_pipes);
         } else {
             if (!exec_process_in_job(parser, p.get(), j, lineage.block_io, std::move(proc_pipes),
-                                     all_ios, deferred_pipes, stdout_read_limit)) {
+                                     conflicts, deferred_pipes, stdout_read_limit)) {
                 exec_error = true;
                 break;
             }
@@ -1216,7 +1215,8 @@ bool exec_job(parser_t &parser, shared_ptr<job_t> j, const job_lineage_t &lineag
     if (!exec_error && deferred_process) {
         assert(deferred_pipes.write.valid() && "Deferred process should always have a write pipe");
         if (!exec_process_in_job(parser, deferred_process, j, lineage.block_io,
-                                 std::move(deferred_pipes), all_ios, {}, stdout_read_limit, true)) {
+                                 std::move(deferred_pipes), conflicts, {}, stdout_read_limit,
+                                 true)) {
             exec_error = true;
         }
     }
@@ -1259,7 +1259,7 @@ static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, wcstrin
     // IO buffer creation may fail (e.g. if we have too many open files to make a pipe), so this may
     // be null.
     std::shared_ptr<io_buffer_t> buffer;
-    if (auto bufferfill = io_bufferfill_t::create(io_chain_t{}, ld.read_limit)) {
+    if (auto bufferfill = io_bufferfill_t::create(fd_set_t{}, ld.read_limit)) {
         if (parser.eval(cmd, io_chain_t{bufferfill}, SUBST) == 0) {
             subcommand_statuses = parser.get_last_statuses();
         }

src/fish_tests.cpp

@@ -1031,7 +1031,7 @@ static void test_parser() {
 }
 
 static void test_1_cancellation(const wchar_t *src) {
-    auto filler = io_bufferfill_t::create(io_chain_t{});
+    auto filler = io_bufferfill_t::create(fd_set_t{});
     pthread_t thread = pthread_self();
     double delay = 0.25 /* seconds */;
     iothread_perform([=]() {
@@ -3188,6 +3188,21 @@ static void test_input() {
     }
 }
 
+static void test_fd_set() {
+    say(L"Testing fd_set");
+    fd_set_t fds;
+    do_test(!fds.contains(0));
+    do_test(!fds.contains(100));
+    fds.add(1);
+    do_test(!fds.contains(0));
+    do_test(!fds.contains(100));
+    do_test(fds.contains(1));
+    fds.add(1);
+    do_test(!fds.contains(0));
+    do_test(!fds.contains(100));
+    do_test(fds.contains(1));
+}
+
 static void test_line_iterator() {
     say(L"Testing line iterator");
 
@@ -5518,6 +5533,7 @@ int main(int argc, char **argv) {
     if (should_test_function("complete")) test_complete();
     if (should_test_function("autoload")) test_autoload();
     if (should_test_function("input")) test_input();
+    if (should_test_function("io")) test_fd_set();
     if (should_test_function("line_iterator")) test_line_iterator();
     if (should_test_function("universal")) test_universal();
     if (should_test_function("universal")) test_universal_output();

src/io.cpp

@@ -160,7 +160,7 @@ void io_buffer_t::complete_background_fillthread() {
     fillthread_waiter_ = {};
 }
 
-shared_ptr<io_bufferfill_t> io_bufferfill_t::create(const io_chain_t &conflicts,
+shared_ptr<io_bufferfill_t> io_bufferfill_t::create(const fd_set_t &conflicts,
                                                     size_t buffer_limit) {
     // Construct our pipes.
     auto pipes = make_autoclose_pipes(conflicts);
@@ -240,64 +240,37 @@ void io_chain_t::print() const {
     }
 }
 
-int move_fd_to_unused(int fd, const io_chain_t &io_chain, bool cloexec) {
-    if (fd < 0 || io_chain.io_for_fd(fd).get() == nullptr) {
+fd_set_t io_chain_t::fd_set() const {
+    fd_set_t result;
+    for (const auto &io : *this) {
+        result.add(io->fd);
+    }
+    return result;
+}
+
+autoclose_fd_t move_fd_to_unused(autoclose_fd_t fd, const fd_set_t &fdset, bool cloexec) {
+    if (!fd.valid() || !fdset.contains(fd.fd())) {
         return fd;
     }
 
     // We have fd >= 0, and it's a conflict. dup it and recurse. Note that we recurse before
     // anything is closed; this forces the kernel to give us a new one (or report fd exhaustion).
-    int new_fd = fd;
     int tmp_fd;
     do {
-        tmp_fd = dup(fd);
+        tmp_fd = dup(fd.fd());
     } while (tmp_fd < 0 && errno == EINTR);
 
-    assert(tmp_fd != fd);
+    assert(tmp_fd != fd.fd());
     if (tmp_fd < 0) {
         // Likely fd exhaustion.
-        new_fd = -1;
-    } else {
-        // Ok, we have a new candidate fd. Recurse. If we get a valid fd, either it's the same as
-        // what we gave it, or it's a new fd and what we gave it has been closed. If we get a
-        // negative value, the fd also has been closed.
-        if (cloexec) set_cloexec(tmp_fd);
-        new_fd = move_fd_to_unused(tmp_fd, io_chain);
+        return autoclose_fd_t{};
     }
-
-    // We're either returning a new fd or an error. In both cases, we promise to close the old one.
-    assert(new_fd != fd);
-    int saved_errno = errno;
-    exec_close(fd);
-    errno = saved_errno;
-    return new_fd;
+    // Ok, we have a new candidate fd. Recurse.
+    if (cloexec) set_cloexec(tmp_fd);
+    return move_fd_to_unused(autoclose_fd_t{tmp_fd}, fdset, cloexec);
 }
 
-static bool pipe_avoid_conflicts_with_io_chain(int fds[2], const io_chain_t &ios) {
-    bool success = true;
-    for (int i = 0; i < 2; i++) {
-        fds[i] = move_fd_to_unused(fds[i], ios);
-        if (fds[i] < 0) {
-            success = false;
-            break;
-        }
-    }
-
-    // If any fd failed, close all valid fds.
-    if (!success) {
-        int saved_errno = errno;
-        for (int i = 0; i < 2; i++) {
-            if (fds[i] >= 0) {
-                exec_close(fds[i]);
-                fds[i] = -1;
-            }
-        }
-        errno = saved_errno;
-    }
-    return success;
-}
-
-maybe_t<autoclose_pipes_t> make_autoclose_pipes(const io_chain_t &ios) {
+maybe_t<autoclose_pipes_t> make_autoclose_pipes(const fd_set_t &fdset) {
     int pipes[2] = {-1, -1};
 
     if (pipe(pipes) < 0) {
@@ -308,14 +281,13 @@ maybe_t<autoclose_pipes_t> make_autoclose_pipes(const io_chain_t &ios) {
     set_cloexec(pipes[0]);
     set_cloexec(pipes[1]);
 
-    if (!pipe_avoid_conflicts_with_io_chain(pipes, ios)) {
-        // The pipes are closed on failure here.
-        return none();
-    }
-    autoclose_pipes_t result;
-    result.read = autoclose_fd_t(pipes[0]);
-    result.write = autoclose_fd_t(pipes[1]);
-    return {std::move(result)};
+    auto read = move_fd_to_unused(autoclose_fd_t{pipes[0]}, fdset, true);
+    if (!read.valid()) return none();
+
+    auto write = move_fd_to_unused(autoclose_fd_t{pipes[1]}, fdset, true);
+    if (!write.valid()) return none();
+
+    return autoclose_pipes_t(std::move(read), std::move(write));
 }
 
 shared_ptr<const io_data_t> io_chain_t::io_for_fd(int fd) const {

src/io.h

@@ -19,6 +19,24 @@
 
 using std::shared_ptr;
 
+/// A simple set of FDs.
+struct fd_set_t {
+    std::vector<bool> fds;
+
+    void add(int fd) {
+        assert(fd >= 0 && "Invalid fd");
+        if ((size_t)fd >= fds.size()) {
+            fds.resize(fd + 1);
+        }
+        fds[fd] = true;
+    }
+
+    bool contains(int fd) const {
+        assert(fd >= 0 && "Invalid fd");
+        return (size_t)fd < fds.size() && fds[fd];
+    }
+};
+
 /// separated_buffer_t is composed of a sequence of elements, some of which may be explicitly
 /// separated (e.g. through string spit0) and some of which the separation is inferred. This enum
 /// tracks the type.
@@ -257,9 +275,9 @@ class io_bufferfill_t : public io_data_t {
     /// Create an io_bufferfill_t which, when written from, fills a buffer with the contents.
     /// \returns nullptr on failure, e.g. too many open fds.
     ///
-    /// \param conflicts A set of IO redirections. The function ensures that any pipe it makes does
+    /// \param conflicts A set of fds. The function ensures that any pipe it makes does
     /// not conflict with an fd redirection in this list.
-    static shared_ptr<io_bufferfill_t> create(const io_chain_t &conflicts, size_t buffer_limit = 0);
+    static shared_ptr<io_bufferfill_t> create(const fd_set_t &conflicts, size_t buffer_limit = 0);
 
     /// Reset the receiver (possibly closing the write end of the pipe), and complete the fillthread
     /// of the buffer. \return the buffer.
@@ -343,6 +361,9 @@ class io_chain_t : public std::vector<io_data_ref_t> {
 
     /// Output debugging information to stderr.
     void print() const;
+
+    /// \return the set of redirected FDs.
+    fd_set_t fd_set() const;
 };
 
 /// Helper type returned from making autoclose pipes.
@@ -360,13 +381,13 @@ struct autoclose_pipes_t {
 /// Call pipe(), populating autoclose fds, avoiding conflicts.
 /// The pipes are marked CLO_EXEC.
 /// \return pipes on success, none() on error.
-maybe_t<autoclose_pipes_t> make_autoclose_pipes(const io_chain_t &ios);
+maybe_t<autoclose_pipes_t> make_autoclose_pipes(const fd_set_t &fdset);
 
-/// If the given fd is used by the io chain, duplicates it repeatedly until an fd not used in the io
-/// chain is found, or we run out. If we return a new fd or an error, closes the old one.
-/// If \p cloexec is set, any fd created is marked close-on-exec.
-/// \returns -1 on failure (in which case the given fd is still closed).
-int move_fd_to_unused(int fd, const io_chain_t &io_chain, bool cloexec = true);
+/// If the given fd is present in \p fdset, duplicates it repeatedly until an fd not used in the set
+/// is found or we run out. If we return a new fd or an error, closes the old one. If \p cloexec is
+/// set, any fd created is marked close-on-exec. \returns -1 on failure (in which case the given fd
+/// is still closed).
+autoclose_fd_t move_fd_to_unused(autoclose_fd_t fd, const fd_set_t &fdset, bool cloexec = true);
 
 /// Class representing the output that a builtin can generate.
 class output_stream_t {

src/redirection.cpp

@@ -24,8 +24,8 @@ maybe_t<dup2_list_t> dup2_list_t::resolve_chain(const io_chain_t &io_chain) {
                 // 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());
-                int file_fd = wopen(io_file->filename, io_file->flags, OPEN_MASK);
-                if (file_fd < 0) {
+                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 {
@@ -38,22 +38,23 @@ maybe_t<dup2_list_t> dup2_list_t::resolve_chain(const io_chain_t &io_chain) {
                 // 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 != io_file->fd) {
-                    file_fd = move_fd_to_unused(file_fd, io_chain, false /* cloexec */);
-                    if (file_fd < 0) {
+                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();
                     }
                 }
-
-                // Record that we opened this file, so we will auto-close it.
-                assert(file_fd >= 0 && "Should have a valid file_fd");
-                result.opened_fds_.emplace_back(file_fd);
+                assert(file_fd.valid() && "Should have a valid file_fd");
 
                 // Mark our dup2 and our close actions.
-                result.add_dup2(file_fd, io_file->fd);
-                result.add_close(file_fd);
+                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));
                 break;
             }