From 33aff87c10d1f24cf87a4ffbae11f7317a426153 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Thu, 12 Dec 2019 17:27:48 -0800 Subject: [PATCH] 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 | 33 ++++++--------------------------- src/fish_tests.cpp | 9 --------- src/io.cpp | 35 ++++++++++++++++++++++++++++++++--- src/io.h | 24 +++++++++++++++--------- src/redirection.cpp | 36 ++---------------------------------- 5 files changed, 55 insertions(+), 82 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index 5f4d2ec10..6b9fa908e 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -80,8 +80,7 @@ static bool redirection_is_to_real_file(const shared_ptr &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(io.get())->filename; - if (path != L"/dev/null") { + if (!static_cast(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(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(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(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; } diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index cbf6616ea..74659a3fe 100644 --- a/src/fish_tests.cpp +++ b/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(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() { diff --git a/src/io.cpp b/src/io.cpp index 5b59105a9..2ff31bd90 100644 --- a/src/io.cpp +++ b/src/io.cpp @@ -4,6 +4,7 @@ #include "io.h" #include +#include #include #include #include @@ -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_bufferfill_t::finish(std::shared_ptrinsert(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(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(spec.fd, std::move(file), path)); break; } } diff --git a/src/io.h b/src/io.h index d16cf2625..24d5e6da9 100644 --- a/src/io.h +++ b/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 { /// 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; diff --git a/src/redirection.cpp b/src/redirection.cpp index 8c65ea4c1..4e0028b92 100644 --- a/src/redirection.cpp +++ b/src/redirection.cpp @@ -44,40 +44,8 @@ maybe_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(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(io_ref.get()); + result.add_dup2(io->file_fd(), io->fd); break; }