From d6c71d77a968fe4d7169c9f07a935f297b373328 Mon Sep 17 00:00:00 2001
From: ridiculousfish <corydoras@ridiculousfish.com>
Date: Fri, 13 Dec 2019 16:16:19 -0800
Subject: [PATCH] Correctly cloexec file redirections

The IO cleanup left file redirections open in the child. For example,
/bin/cmd < file.txt would redirect stdin but also leave the file open.
Ensure these get closed properly.
---
 src/fish_test_helper.cpp | 14 ++++++++++++++
 src/io.cpp               |  3 ++-
 src/redirection.h        |  3 +++
 tests/checks/fds.fish    | 24 ++++++++++++++++++++++++
 4 files changed, 43 insertions(+), 1 deletion(-)
 create mode 100644 tests/checks/fds.fish

diff --git a/src/fish_test_helper.cpp b/src/fish_test_helper.cpp
index 9f6cd7b94..a1590487d 100644
--- a/src/fish_test_helper.cpp
+++ b/src/fish_test_helper.cpp
@@ -1,6 +1,7 @@
 // fish_test_helper is a little program with no fish dependencies that acts like certain other
 // programs, allowing fish to test its behavior.
 
+#include <fcntl.h>
 #include <unistd.h>
 
 #include <csignal>
@@ -54,6 +55,17 @@ static void print_pid_then_sleep() {
 
 static void print_pgrp() { fprintf(stdout, "%d\n", getpgrp()); }
 
+static void print_fds() {
+    bool needs_space = false;
+    for (int fd = 0; fd <= 100; fd++) {
+        if (fcntl(fd, F_GETFD) >= 0) {
+            fprintf(stdout, "%s%d", needs_space ? " " : "", fd);
+            needs_space = true;
+        }
+    }
+    fputc('\n', stdout);
+}
+
 int main(int argc, char *argv[]) {
     if (argc <= 1) {
         fprintf(stderr, "No commands given.\n");
@@ -72,6 +84,8 @@ int main(int argc, char *argv[]) {
             print_pid_then_sleep();
         } else if (!strcmp(argv[i], "print_pgrp")) {
             print_pgrp();
+        } else if (!strcmp(argv[i], "print_fds")) {
+            print_fds();
         } else {
             fprintf(stderr, "%s: Unknown command: %s\n", argv[0], argv[i]);
             return EXIT_FAILURE;
diff --git a/src/io.cpp b/src/io.cpp
index 03797bf91..ca8648d7c 100644
--- a/src/io.cpp
+++ b/src/io.cpp
@@ -255,9 +255,10 @@ bool io_chain_t::append_from_specs(const redirection_spec_list_t &specs, const w
             }
             default: {
                 // We have a path-based redireciton. Resolve it to a file.
+                // Mark it as CLO_EXEC because we don't want it to be open in any child.
                 wcstring path = path_apply_working_directory(spec.target, pwd);
                 int oflags = spec.oflags();
-                autoclose_fd_t file{wopen(path, oflags, OPEN_MASK)};
+                autoclose_fd_t file{wopen_cloexec(path, oflags, OPEN_MASK)};
                 if (!file.valid()) {
                     if ((oflags & O_EXCL) && (errno == EEXIST)) {
                         debug(1, NOCLOB_ERROR, spec.target.c_str());
diff --git a/src/redirection.h b/src/redirection.h
index c8a87e700..297afc366 100644
--- a/src/redirection.h
+++ b/src/redirection.h
@@ -66,6 +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});
         }
diff --git a/tests/checks/fds.fish b/tests/checks/fds.fish
new file mode 100644
index 000000000..82c20161d
--- /dev/null
+++ b/tests/checks/fds.fish
@@ -0,0 +1,24 @@
+# RUN: %fish -C "set helper %fish_test_helper" %s
+
+# Check that we don't leave stray FDs.
+
+$helper print_fds
+# CHECK: 0 1 2
+
+$helper print_fds 0>&-
+# CHECK: 1 2
+
+$helper print_fds 0>&- 2>&-
+# CHECK: 1
+
+false | $helper print_fds 0>&-
+# CHECK: 0 1 2
+
+$helper print_fds <(status current-filename)
+# CHECK: 0 1 2
+
+$helper print_fds <(status current-filename)
+# CHECK: 0 1 2
+
+$helper print_fds 3<(status current-filename)
+# CHECK: 0 1 2 3