Correctly handle "self fd redirections"

This adds a test for the obscure case where an fd is redirected to
itself. This is tricky because the dup2 will not clear the CLO_EXEC bit.
So do it manually; also posix_spawn can't be used in this case.

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_t> &job) {
+static bool can_use_posix_spawn_for_job(const std::shared_ptr<job_t> &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<job_t>
 
 #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.

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");

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:
+        // Note: record these even if src and target is the same.
-        //   /bin/stuff 6< file.txt
+        // This is a note that we must clear the CLO_EXEC bit.
-        // and file.txt happens to get fd 6, then the file will be closed.
+        actions_.push_back(action_t{src, target});
-        if (src != target) {
-            actions_.push_back(action_t{src, target});
-        }
     }
 
     /// Append a close action.

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) {
+    int new_flags = flags;
-        return true;
+    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) {

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.
+/// Sets CLO_EXEC on a given fd according to the value of \p should_set.
-bool set_cloexec(int fd);
+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);

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 </dev/null
 # CHECK: 0 1 2
 
-$helper print_fds <(status current-filename)
+$helper print_fds </dev/null
 # CHECK: 0 1 2
 
-$helper print_fds 3<(status current-filename)
+$helper print_fds 3</dev/null
 # CHECK: 0 1 2 3
+
+# This attempts to trip a case where the file opened in fish
+# has the same fd as the redirection. In this case, the dup2
+# does not clear the CLO_EXEC bit.
+
+$helper print_fds 4</dev/null
+# CHECK: 0 1 2 4
+
+$helper print_fds 5</dev/null
+# CHECK: 0 1 2 5
+
+$helper print_fds 6</dev/null
+# CHECK: 0 1 2 6
+
+$helper print_fds 7</dev/null
+# CHECK: 0 1 2 7
+
+$helper print_fds 8</dev/null
+# CHECK: 0 1 2 8
+
+$helper print_fds 9</dev/null
+# CHECK: 0 1 2 9
+
+$helper print_fds 10</dev/null
+# CHECK: 0 1 2 10
+
+$helper print_fds 11</dev/null
+# CHECK: 0 1 2 11
+
+$helper print_fds 12</dev/null
+# CHECK: 0 1 2 12
+
+$helper print_fds 13</dev/null
+# CHECK: 0 1 2 13
+
+$helper print_fds 14</dev/null
+# CHECK: 0 1 2 14
+
+$helper print_fds 15</dev/null
+# CHECK: 0 1 2 15
+
+$helper print_fds 16</dev/null
+# CHECK: 0 1 2 16
+
+$helper print_fds 17</dev/null
+# CHECK: 0 1 2 17
+
+$helper print_fds 18</dev/null
+# CHECK: 0 1 2 18
+
+$helper print_fds 19</dev/null
+# CHECK: 0 1 2 19
+
+$helper print_fds 20</dev/null
+# CHECK: 0 1 2 20