Pipe fds to move to the "high range"

This concerns how fish prevents its own fds from interfering with
user-defined fd redirections, like `echo hi >&5`. fish has historically
done this by tracking all user defined redirections when running a job,
and ensuring that pipes are not assigned the same fds. However this is
annoying to pass around - it means that we have to thread user-defined
redirections into pipe creation.

Take a page from zsh and just ensure that all pipes we create have fds in
the "high range," which here means at least 10. The primary way to do this
is via the F_DUPFD_CLOEXEC syscall, which also sets CLOEXEC, so we aren't
invoking additional syscalls in the common case. This will free us from
having to track which fds are in user-defined redirections.

src/exec.cpp

@@ -968,7 +968,7 @@ bool exec_job(parser_t &parser, const shared_ptr<job_t> &j, const io_chain_t &bl
         autoclose_pipes_t proc_pipes;
         proc_pipes.read = std::move(pipe_next_read);
         if (!p->is_last_in_job) {
-            auto pipes = make_autoclose_pipes(conflicts);
+            auto pipes = make_autoclose_pipes();
             if (!pipes) {
                 FLOGF(warning, PIPE_ERROR);
                 wperror(L"pipe");

src/fd_monitor.cpp

@@ -12,7 +12,7 @@ static constexpr uint64_t kUsecPerMsec = 1000;
 static constexpr uint64_t kUsecPerSec = 1000 * kUsecPerMsec;
 
 fd_monitor_t::fd_monitor_t() {
-    auto self_pipe = make_autoclose_pipes({});
+    auto self_pipe = make_autoclose_pipes();
     if (!self_pipe) {
         DIE("Unable to create pipes");
     }

src/fds.cpp

@@ -12,60 +12,85 @@
 #include "flog.h"
 #include "wutil.h"
 
+#include <fcntl.h>
+
 #if defined(__linux__)
 #include <sys/statfs.h>
 #endif
 
+// The first fd in the "high range." fds below this are allowed to be used directly by users in
+// redirections, e.g. >&3
+const int k_first_high_fd = 10;
+
 void autoclose_fd_t::close() {
     if (fd_ < 0) return;
     exec_close(fd_);
     fd_ = -1;
 }
 
-autoclose_fd_t move_fd_to_unused(autoclose_fd_t fd, const fd_set_t &fdset) {
-    if (!fd.valid() || !fdset.contains(fd.fd())) {
+/// If the given fd is in the "user range", move it to a new fd in the "high range".
+/// zsh calls this movefd().
+/// \p input_has_cloexec describes whether the input has CLOEXEC already set, so we can avoid
+/// setting it again.
+/// \return the fd, which always has CLOEXEC set; or an invalid fd on failure, in
+/// which case an error will have been printed, and the input fd closed.
+static autoclose_fd_t heightenize_fd(autoclose_fd_t fd, bool input_has_cloexec) {
+    // Check if the fd is invalid or already in our high range.
+    if (!fd.valid()) {
         return fd;
     }
-
-    // We have fd >= 0, and it's a conflict. dup it and recurse. Note that we recurse before
+    if (fd.fd() >= k_first_high_fd) {
+        if (!input_has_cloexec) set_cloexec(fd.fd());
+        return fd;
+    }
+#if defined(F_DUPFD_CLOEXEC)
+    // Here we are asking the kernel to give us a
+    int newfd = fcntl(fd.fd(), F_DUPFD_CLOEXEC, k_first_high_fd);
+    if (newfd < 0) {
+        wperror(L"fcntl");
+        return autoclose_fd_t{};
+    }
+    return autoclose_fd_t(newfd);
+#elif defined(F_DUPFD)
+    int newfd = fcntl(fd.fd(), F_DUPFD, k_first_high_fd);
+    if (newfd < 0) {
+        wperror(L"fcntl");
+        return autoclose_fd_t{};
+    }
+    set_cloexec(newfd);
+    return autoclose_fd_t(newfd);
+#else
+    // We have fd >= 0, and it's in the user range. 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 tmp_fd;
     do {
         tmp_fd = dup(fd.fd());
     } while (tmp_fd < 0 && errno == EINTR);
-
-    assert(tmp_fd != fd.fd());
-    if (tmp_fd < 0) {
-        // Likely fd exhaustion.
-        return autoclose_fd_t{};
-    }
     // Ok, we have a new candidate fd. Recurse.
-    set_cloexec(tmp_fd);
-    return move_fd_to_unused(autoclose_fd_t{tmp_fd}, fdset);
+    return heightenize_fd(autoclose_fd_t{tmp_fd}, false);
+#endif
 }
 
-maybe_t<autoclose_pipes_t> make_autoclose_pipes(const fd_set_t &fdset) {
+maybe_t<autoclose_pipes_t> make_autoclose_pipes() {
     int pipes[2] = {-1, -1};
 
+    // TODO: use pipe2 here if available.
     if (pipe(pipes) < 0) {
         FLOGF(warning, PIPE_ERROR);
         wperror(L"pipe");
         return none();
     }
-    set_cloexec(pipes[0]);
-    set_cloexec(pipes[1]);
 
     autoclose_fd_t read_end{pipes[0]};
     autoclose_fd_t write_end{pipes[1]};
 
-    // Ensure we have no conflicts.
-    if (!fdset.empty()) {
-        read_end = move_fd_to_unused(std::move(read_end), fdset);
-        if (!read_end.valid()) return none();
+    // Ensure our fds are out of the user range.
+    read_end = heightenize_fd(std::move(read_end), false);
+    if (!read_end.valid()) return none();
+
+    write_end = heightenize_fd(std::move(write_end), false);
+    if (!write_end.valid()) return none();
 
-        write_end = move_fd_to_unused(std::move(write_end), fdset);
-        if (!write_end.valid()) return none();
-    }
     return autoclose_pipes_t(std::move(read_end), std::move(write_end));
 }
 

src/fds.h

@@ -14,6 +14,9 @@ using wcstring = std::wstring;
 /// Pipe redirection error message.
 #define PIPE_ERROR _(L"An error occurred while setting up pipe")
 
+/// The first "high fd", which is considered outside the range of valid user-specified redirections (like >&5).
+extern const int k_first_high_fd;
+
 /// A helper class for managing and automatically closing a file descriptor.
 class autoclose_fd_t {
     int fd_;
@@ -88,15 +91,10 @@ struct fd_set_t {
     bool empty() const { return fds.empty(); }
 };
 
-/// Call pipe(), populating autoclose fds, avoiding conflicts.
-/// The pipes are marked CLO_EXEC.
+/// Call pipe(), populating autoclose fds.
+/// The pipes are marked CLO_EXEC and are placed in the high fd range.
 /// \return pipes on success, none() on error.
-maybe_t<autoclose_pipes_t> make_autoclose_pipes(const fd_set_t &fdset);
-
-/// 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. Marks the fd as
-/// cloexec. \returns invalid fd 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);
+maybe_t<autoclose_pipes_t> make_autoclose_pipes();
 
 /// Sets CLO_EXEC on a given fd according to the value of \p should_set.
 int set_cloexec(int fd, bool should_set = true);

src/fish_tests.cpp

@@ -795,7 +795,7 @@ static void test_fd_monitor() {
         autoclose_fd_t writer;
 
         explicit item_maker_t(uint64_t timeout_usec) {
-            auto pipes = make_autoclose_pipes({}).acquire();
+            auto pipes = make_autoclose_pipes().acquire();
             writer = std::move(pipes.write);
             auto callback = [this](autoclose_fd_t &fd, item_wake_reason_t reason) {
                 bool was_closed = false;
@@ -6110,6 +6110,26 @@ static void test_topic_monitor_torture() {
     for (auto &t : threads) t.join();
 }
 
+static void test_pipes() {
+    say(L"Testing pipes");
+    // Here we just test that each pipe has CLOEXEC set and is in the high range.
+    // Note pipe creation may fail due to fd exhaustion; don't fail in that case.
+    std::vector<autoclose_pipes_t> pipes;
+    for (int i=0; i < 10; i++) {
+        if (auto pipe = make_autoclose_pipes()) {
+            pipes.push_back(pipe.acquire());
+        }
+    }
+    for (const auto &pipe : pipes) {
+        for (int fd : {pipe.read.fd(), pipe.write.fd()}) {
+            do_test(fd >= k_first_high_fd);
+            int flags = fcntl(fd, F_GETFD, 0);
+            do_test(flags >= 0);
+            do_test(bool(flags & FD_CLOEXEC));
+        }
+    }
+}
+
 static void test_timer_format() {
     say(L"Testing timer format");
     // This test uses numeric output, so we need to set the locale.
@@ -6346,6 +6366,7 @@ int main(int argc, char **argv) {
     if (should_test_function("normalize")) test_normalize_path();
     if (should_test_function("topics")) test_topic_monitor();
     if (should_test_function("topics")) test_topic_monitor_torture();
+    if (should_test_function("pipes")) test_pipes();
     if (should_test_function("timer_format")) test_timer_format();
     // history_tests_t::test_history_speed();
 

src/io.cpp

@@ -158,10 +158,11 @@ separated_buffer_t io_buffer_t::complete_background_fillthread_and_take_buffer()
 
 shared_ptr<io_bufferfill_t> io_bufferfill_t::create(const fd_set_t &conflicts, size_t buffer_limit,
                                                     int target) {
+    (void)conflicts;
     assert(target >= 0 && "Invalid target fd");
 
     // Construct our pipes.
-    auto pipes = make_autoclose_pipes(conflicts);
+    auto pipes = make_autoclose_pipes();
     if (!pipes) {
         return nullptr;
     }

src/iothread.cpp

@@ -154,7 +154,7 @@ struct notify_pipes_t {
 /// \return the (immortal) set of pipes used for notifying of completions.
 static const notify_pipes_t &get_notify_pipes() {
     static const notify_pipes_t s_notify_pipes = [] {
-        auto pipes = make_autoclose_pipes({});
+        auto pipes = make_autoclose_pipes();
         if (!pipes) {
             DIE_WITH_ERRNO("Unable to create iothread notify pipes");
         }

src/topic_monitor.cpp

@@ -45,7 +45,7 @@ binary_semaphore_t::binary_semaphore_t() : sem_ok_(false) {
     sem_ok_ = (0 == sem_init(&sem_, 0, 0));
 #endif
     if (!sem_ok_) {
-        auto pipes = make_autoclose_pipes({});
+        auto pipes = make_autoclose_pipes();
         assert(pipes.has_value() && "Failed to make pubsub pipes");
         pipes_ = pipes.acquire();