diff --git a/src/exec.cpp b/src/exec.cpp index c5a47e460..a8a419dbf 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -182,7 +182,17 @@ static void launch_process_nofork(env_stack_t &vars, process_t *p) { // To avoid the race between the caller calling tcsetpgrp() and the client checking the // foreground process group, we don't use posix_spawn if we're going to foreground the process. (If // we use fork(), we can call tcsetpgrp after the fork, before the exec, and avoid the race). -static bool can_use_posix_spawn_for_job(const std::shared_ptr &job) { +static bool can_use_posix_spawn_for_job(const std::shared_ptr &job, + const dup2_list_t &dup2s) { + // Hack - do not use posix_spawn if there are self-fd redirections. + // For example if you were to write: + // cmd 6< /dev/null + // it is possible that the open() of /dev/null would result in fd 6. Here even if we attempted + // to add a dup2 action, it would be ignored and the CLO_EXEC bit would remain. So don't use + // posix_spawn in this case; instead we'll call fork() and clear the CLO_EXEC bit manually. + for (const auto &action : dup2s.get_actions()) { + if (action.src == action.target) return false; + } if (job->wants_job_control()) { //!OCLINT(collapsible if statements) // We are going to use job control; therefore when we launch this job it will get its own // process group ID. But will it be foregrounded? @@ -594,7 +604,7 @@ static bool exec_external_command(parser_t &parser, const std::shared_ptr #if FISH_USE_POSIX_SPAWN // Prefer to use posix_spawn, since it's faster on some systems like OS X. - bool use_posix_spawn = g_use_posix_spawn && can_use_posix_spawn_for_job(j); + 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. diff --git a/src/postfork.cpp b/src/postfork.cpp index e1080e47d..9d939a83d 100644 --- a/src/postfork.cpp +++ b/src/postfork.cpp @@ -138,7 +138,18 @@ bool set_child_group(job_t *j, pid_t child_pid) { int child_setup_process(pid_t new_termowner, bool is_forked, const dup2_list_t &dup2s) { // Note we are called in a forked child. for (const auto &act : dup2s.get_actions()) { - int err = act.target < 0 ? close(act.src) : dup2(act.src, act.target); + int err; + if (act.target < 0) { + err = close(act.src); + } else if (act.target != act.src) { + // Normal redirection. + err = dup2(act.src, act.target); + } else { + // This is a weird case like /bin/cmd 6< file.txt + // The opened file (which is CLO_EXEC) wants to be dup2'd to its own fd. + // We need to unset the CLO_EXEC flag. + err = set_cloexec(act.src, false); + } if (err < 0) { if (is_forked) { debug_safe(4, "redirect_in_child_after_fork failed in child_setup_process"); diff --git a/src/redirection.h b/src/redirection.h index 297afc366..7bde368c4 100644 --- a/src/redirection.h +++ b/src/redirection.h @@ -66,12 +66,9 @@ class dup2_list_t { /// Append a dup2 action. void add_dup2(int src, int target) { assert(src >= 0 && target >= 0 && "Invalid fd in add_dup2"); - // TODO: this is sloppy about CLO_EXEC. For example if the user does this: - // /bin/stuff 6< file.txt - // and file.txt happens to get fd 6, then the file will be closed. - if (src != target) { - actions_.push_back(action_t{src, target}); - } + // Note: record these even if src and target is the same. + // This is a note that we must clear the CLO_EXEC bit. + actions_.push_back(action_t{src, target}); } /// Append a close action. diff --git a/src/wutil.cpp b/src/wutil.cpp index 14365ecf7..9f21f3131 100644 --- a/src/wutil.cpp +++ b/src/wutil.cpp @@ -177,17 +177,24 @@ FILE *wfopen(const wcstring &path, const char *mode) { return result; } -bool set_cloexec(int fd) { +int set_cloexec(int fd, bool should_set) { // Note we don't want to overwrite existing flags like O_NONBLOCK which may be set. So fetch the - // existing flags and OR in our new one. + // existing flags and modify them. int flags = fcntl(fd, F_GETFD, 0); if (flags < 0) { - return false; + return -1; } - if (flags & FD_CLOEXEC) { - return true; + int new_flags = flags; + if (should_set) { + new_flags |= FD_CLOEXEC; + } else { + new_flags &= ~FD_CLOEXEC; + } + if (flags == new_flags) { + return 0; + } else { + return fcntl(fd, F_SETFD, new_flags); } - return fcntl(fd, F_SETFD, flags | FD_CLOEXEC) >= 0; } int open_cloexec(const std::string &cstring, int flags, mode_t mode, bool cloexec) { diff --git a/src/wutil.h b/src/wutil.h index 0363e4f13..8a3b74cd4 100644 --- a/src/wutil.h +++ b/src/wutil.h @@ -22,8 +22,8 @@ /// Wide character version of fopen(). This sets CLO_EXEC. FILE *wfopen(const wcstring &path, const char *mode); -/// Sets CLO_EXEC on a given fd. -bool set_cloexec(int fd); +/// Sets CLO_EXEC on a given fd according to the value of \p should_set. +int set_cloexec(int fd, bool should_set = true); /// Wide character version of open(). int wopen(const wcstring &pathname, int flags, mode_t mode = 0); diff --git a/tests/checks/fds.fish b/tests/checks/fds.fish index 82c20161d..c3b606a2c 100644 --- a/tests/checks/fds.fish +++ b/tests/checks/fds.fish @@ -14,11 +14,66 @@ $helper print_fds 0>&- 2>&- false | $helper print_fds 0>&- # CHECK: 0 1 2 -$helper print_fds <(status current-filename) +$helper print_fds