diff --git a/src/exec.cpp b/src/exec.cpp index 041eb654b..c973c6d28 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -518,44 +518,26 @@ static bool exec_external_command(parser_t &parser, const std::shared_ptr bool use_posix_spawn = g_use_posix_spawn && can_use_posix_spawn_for_job(j, dup2s); if (use_posix_spawn) { s_fork_count++; // spawn counts as a fork+exec - // Create posix spawn attributes and actions. - pid_t pid = 0; - posix_spawnattr_t attr = posix_spawnattr_t(); - posix_spawn_file_actions_t actions = posix_spawn_file_actions_t(); - bool made_it = fork_actions_make_spawn_properties(&attr, &actions, j.get(), dup2s); - if (made_it) { - // We successfully made the attributes and actions; actually call - // posix_spawn. - int spawn_ret = - posix_spawn(&pid, actual_cmd, &actions, &attr, const_cast(argv), - const_cast(envv)); - // This usleep can be used to test for various race conditions - // (https://github.com/fish-shell/fish-shell/issues/360). - // usleep(10000); - - if (spawn_ret != 0) { - safe_report_exec_error(spawn_ret, actual_cmd, argv, envv); - // Make sure our pid isn't set. - pid = 0; - } - - // Clean up our actions. - posix_spawn_file_actions_destroy(&actions); - posix_spawnattr_destroy(&attr); - } - - // A 0 pid means we failed to posix_spawn. Since we have no pid, we'll never get - // told when it's exited, so we have to mark the process as failed. - FLOGF(exec_fork, L"Fork #%d, pid %d: spawn external command '%s' from '%ls'", - int(s_fork_count), pid, actual_cmd, file ? file : L""); - if (pid == 0) { + posix_spawner_t spawner(j.get(), dup2s); + maybe_t pid = spawner.spawn(actual_cmd, const_cast(argv), + const_cast(envv)); + if (int err = spawner.get_error()) { + safe_report_exec_error(err, actual_cmd, argv, envv); job_mark_process_as_failed(j, p); return false; } + assert(pid.has_value() && *pid > 0 && "Should have either a valid pid, or an error"); + + // This usleep can be used to test for various race conditions + // (https://github.com/fish-shell/fish-shell/issues/360). + // usleep(10000); + + FLOGF(exec_fork, L"Fork #%d, pid %d: spawn external command '%s' from '%ls'", + int(s_fork_count), *pid, actual_cmd, file ? file : L""); // these are all things do_fork() takes care of normally (for forked processes): - p->pid = pid; + p->pid = *pid; pid_t pgid = maybe_assign_pgid_from_child(j, p->pid); // posix_spawn should in principle set the pgid before returning. diff --git a/src/postfork.cpp b/src/postfork.cpp index 634524a57..9c4733f65 100644 --- a/src/postfork.cpp +++ b/src/postfork.cpp @@ -189,17 +189,35 @@ pid_t execute_fork() { } #if FISH_USE_POSIX_SPAWN -bool fork_actions_make_spawn_properties(posix_spawnattr_t *attr, - posix_spawn_file_actions_t *actions, const job_t *j, - const dup2_list_t &dup2s) { - // Initialize the output. - if (posix_spawnattr_init(attr) != 0) { - return false; + +// Given an error code, if it is the first error, record it. +// \return whether we have any error. +bool posix_spawner_t::check_fail(int err) { + if (error_ == 0) error_ = err; + return error_ != 0; +} + +posix_spawner_t::~posix_spawner_t() { + if (attr_) { + posix_spawnattr_destroy(this->attr()); + } + if (actions_) { + posix_spawn_file_actions_destroy(this->actions()); + } +} + +posix_spawner_t::posix_spawner_t(const job_t *j, const dup2_list_t &dup2s) { + // Initialize our fields. This may fail. + { + posix_spawnattr_t attr; + if (check_fail(posix_spawnattr_init(&attr))) return; + this->attr_ = attr; } - if (posix_spawn_file_actions_init(actions) != 0) { - posix_spawnattr_destroy(attr); - return false; + { + posix_spawn_file_actions_t actions; + if (check_fail(posix_spawn_file_actions_init(&actions))) return; + this->actions_ = actions; } // desired_pgid tracks the pgroup for the process. If it is none, the pgroup is left unchanged. @@ -226,46 +244,45 @@ bool fork_actions_make_spawn_properties(posix_spawnattr_t *attr, if (reset_sigmask) flags |= POSIX_SPAWN_SETSIGMASK; if (desired_pgid.has_value()) flags |= POSIX_SPAWN_SETPGROUP; - int err = 0; - if (!err) err = posix_spawnattr_setflags(attr, flags); + if (check_fail(posix_spawnattr_setflags(attr(), flags))) return; - if (!err && desired_pgid.has_value()) { - err = posix_spawnattr_setpgroup(attr, *desired_pgid); + if (desired_pgid.has_value()) { + if (check_fail(posix_spawnattr_setpgroup(attr(), *desired_pgid))) return; } // Everybody gets default handlers. - if (!err && reset_signal_handlers) { + if (reset_signal_handlers) { sigset_t sigdefault; get_signals_with_handlers(&sigdefault); - err = posix_spawnattr_setsigdefault(attr, &sigdefault); + if (check_fail(posix_spawnattr_setsigdefault(attr(), &sigdefault))) return; } // No signals blocked. - sigset_t sigmask; - sigemptyset(&sigmask); - if (!err && reset_sigmask) { + if (reset_sigmask) { + sigset_t sigmask; + sigemptyset(&sigmask); blocked_signals_for_job(*j, &sigmask); - err = posix_spawnattr_setsigmask(attr, &sigmask); + if (check_fail(posix_spawnattr_setsigmask(attr(), &sigmask))) return; } // Apply our dup2s. for (const auto &act : dup2s.get_actions()) { - if (err) break; if (act.target < 0) { - err = posix_spawn_file_actions_addclose(actions, act.src); + if (check_fail(posix_spawn_file_actions_addclose(actions(), act.src))) return; } else { - err = posix_spawn_file_actions_adddup2(actions, act.src, act.target); + if (check_fail(posix_spawn_file_actions_adddup2(actions(), act.src, act.target))) + return; } } - - // Clean up on error. - if (err) { - posix_spawnattr_destroy(attr); - posix_spawn_file_actions_destroy(actions); - } - - return !err; } + +maybe_t posix_spawner_t::spawn(const char *cmd, char *const argv[], char *const envp[]) { + if (get_error()) return none(); + pid_t pid = -1; + if (check_fail(posix_spawn(&pid, cmd, &*actions_, &*attr_, argv, envp))) return none(); + return pid; +} + #endif // FISH_USE_POSIX_SPAWN void safe_report_exec_error(int err, const char *actual_cmd, const char *const *argv, @@ -385,4 +402,4 @@ static char *get_interpreter(const char *command, char *buffer, size_t buff_size return buffer + 2; } return nullptr; -} +}; diff --git a/src/postfork.h b/src/postfork.h index 78e7d7b69..b876ed6de 100644 --- a/src/postfork.h +++ b/src/postfork.h @@ -7,6 +7,8 @@ #include #include + +#include "maybe.h" #if HAVE_SPAWN_H #include #endif @@ -51,11 +53,39 @@ void safe_report_exec_error(int err, const char *actual_cmd, const char *const * const char *const *envv); #ifdef FISH_USE_POSIX_SPAWN -/// Initializes and fills in a posix_spawnattr_t; on success, the caller should destroy it via -/// posix_spawnattr_destroy. -bool fork_actions_make_spawn_properties(posix_spawnattr_t *attr, - posix_spawn_file_actions_t *actions, const job_t *j, - const dup2_list_t &dup2s); +/// A RAII type which wraps up posix_spawn's data structures. +class posix_spawner_t { + public: + /// Attempt to construct from a job and dup2 list. + /// The caller must check the error function, as this may fail. + posix_spawner_t(const job_t *j, const dup2_list_t &dup2s); + + /// \return the last error code, or 0 if there is no error. + int get_error() const { return error_; } + + /// If this spawner does not have an error, invoke posix_spawn. Parameters are the same as + /// posix_spawn. + /// \return the pid, or none() on failure, in which case our error will be set. + maybe_t spawn(const char *cmd, char *const argv[], char *const envp[]); + + ~posix_spawner_t(); + + posix_spawner_t(const posix_spawner_t &) = delete; + void operator=(const posix_spawner_t &) = delete; + void operator=(posix_spawner_t &&) = delete; + posix_spawner_t(posix_spawner_t &&) = delete; + + private: + bool check_fail(int err); + posix_spawnattr_t *attr() { return &*attr_; } + posix_spawn_file_actions_t *actions() { return &*actions_; } + + posix_spawner_t(); + int error_{0}; + maybe_t attr_{}; + maybe_t actions_{}; +}; + #endif #endif