diff --git a/src/exec.cpp b/src/exec.cpp index c4a235f43..bf87e694f 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -968,7 +968,7 @@ bool exec_job(parser_t &parser, const shared_ptr &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"); diff --git a/src/fd_monitor.cpp b/src/fd_monitor.cpp index d85b86014..5939537fa 100644 --- a/src/fd_monitor.cpp +++ b/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"); } diff --git a/src/fds.cpp b/src/fds.cpp index 610cfd84d..f71c282ed 100644 --- a/src/fds.cpp +++ b/src/fds.cpp @@ -12,60 +12,85 @@ #include "flog.h" #include "wutil.h" +#include + #if defined(__linux__) #include #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 make_autoclose_pipes(const fd_set_t &fdset) { +maybe_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)); } diff --git a/src/fds.h b/src/fds.h index 808a8cb75..21dcd9c99 100644 --- a/src/fds.h +++ b/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 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 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); diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 89fbb97aa..789aeafd4 100644 --- a/src/fish_tests.cpp +++ b/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 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(); diff --git a/src/io.cpp b/src/io.cpp index 9573c06df..cc5642c23 100644 --- a/src/io.cpp +++ b/src/io.cpp @@ -158,10 +158,11 @@ separated_buffer_t io_buffer_t::complete_background_fillthread_and_take_buffer() shared_ptr 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; } diff --git a/src/iothread.cpp b/src/iothread.cpp index 90f4a7512..19ed562c2 100644 --- a/src/iothread.cpp +++ b/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"); } diff --git a/src/topic_monitor.cpp b/src/topic_monitor.cpp index 1384fafed..83fb1ab25 100644 --- a/src/topic_monitor.cpp +++ b/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();