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; }