hush: fix "redirects can close script fd" bug

Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>

3 files changed, 170 insertions, 61 deletions

diff --git a/shell/hush.c b/shell/hush.c
index 6b0d85039..a8ec54ec0 100644
--- a/shell/hush.c
+++ b/shell/hush.c
@@ -103,6 +103,9 @@
 #else
 # define CLEAR_RANDOM_T(rnd) ((void)0)
 #endif
+#ifndef F_DUPFD_CLOEXEC
+# define F_DUPFD_CLOEXEC F_DUPFD
+#endif
 #ifndef PIPE_BUF
 # define PIPE_BUF 4096  /* amount of buffering in a pipe */
 #endif
@@ -711,10 +714,10 @@ enum {
 };
 
 
-/* Can be extended to handle a FIXME in setup_redirects about not saving script fds */
 struct FILE_list {
         struct FILE_list *next;
         FILE *fp;
+        int fd;
 };
 
 
@@ -809,9 +812,7 @@ struct globals {
         unsigned handled_SIGCHLD;
         smallint we_have_children;
 #endif
-#if ENABLE_FEATURE_SH_STANDALONE
         struct FILE_list *FILE_list;
-#endif
         /* Which signals have non-DFL handler (even with no traps set)?
          * Set at the start to:
          * (SIGQUIT + maybe SPECIAL_INTERACTIVE_SIGS + maybe SPECIAL_JOBSTOP_SIGS)
@@ -1262,35 +1263,34 @@ static void free_strings(char **strings)
 }
 
 
+static int xdup_and_close(int fd, int F_DUPFD_maybe_CLOEXEC)
+{
+        /* We avoid taking stdio fds. Mimicking ash: use fds above 9 */
+        int newfd = fcntl(fd, F_DUPFD_maybe_CLOEXEC, 10);
+        if (newfd < 0) {
+                /* fd was not open? */
+                if (errno == EBADF)
+                        return fd;
+                xfunc_die();
+        }
+        close(fd);
+        return newfd;
+}
+
+
 /* Manipulating the list of open FILEs */
 static FILE *remember_FILE(FILE *fp)
 {
         if (fp) {
-#if ENABLE_FEATURE_SH_STANDALONE
                 struct FILE_list *n = xmalloc(sizeof(*n));
-                n->fp = fp;
                 n->next = G.FILE_list;
                 G.FILE_list = n;
-#endif
-                close_on_exec_on(fileno(fp));
+                n->fp = fp;
+                n->fd = fileno(fp);
+                close_on_exec_on(n->fd);
         }
         return fp;
 }
-#if ENABLE_FEATURE_SH_STANDALONE
-static void close_all_FILE_list(void)
-{
-        struct FILE_list *fl = G.FILE_list;
-        while (fl) {
-                /* fclose would also free FILE object.
-                 * It is disastrous if we share memory with a vforked parent.
-                 * I'm not sure we never come here after vfork.
-                 * Therefore just close fd, nothing more.
-                 */
-                /*fclose(fl->fp); - unsafe */
-                close(fileno(fl->fp));
-                fl = fl->next;
-        }
-}
 static void fclose_and_forget(FILE *fp)
 {
         struct FILE_list **pp = &G.FILE_list;
@@ -1305,8 +1305,46 @@ static void fclose_and_forget(FILE *fp)
         }
         fclose(fp);
 }
-#else
-# define fclose_and_forget(fp) fclose(fp);
+static int save_FILEs_on_redirect(int fd)
+{
+        struct FILE_list *fl = G.FILE_list;
+        while (fl) {
+                if (fd == fl->fd) {
+                        /* We use it only on script files, they are all CLOEXEC */
+                        fl->fd = xdup_and_close(fd, F_DUPFD_CLOEXEC);
+                        return 1;
+                }
+                fl = fl->next;
+        }
+        return 0;
+}
+static void restore_redirected_FILEs(void)
+{
+        struct FILE_list *fl = G.FILE_list;
+        while (fl) {
+                int should_be = fileno(fl->fp);
+                if (fl->fd != should_be) {
+                        xmove_fd(fl->fd, should_be);
+                        fl->fd = should_be;
+                }
+                fl = fl->next;
+        }
+}
+#if ENABLE_FEATURE_SH_STANDALONE
+static void close_all_FILE_list(void)
+{
+        struct FILE_list *fl = G.FILE_list;
+        while (fl) {
+                /* fclose would also free FILE object.
+                 * It is disastrous if we share memory with a vforked parent.
+                 * I'm not sure we never come here after vfork.
+                 * Therefore just close fd, nothing more.
+                 */
+                /*fclose(fl->fp); - unsafe */
+                close(fl->fd);
+                fl = fl->next;
+        }
+}
 #endif
 
 
@@ -6147,6 +6185,74 @@ static void setup_heredoc(struct redir_struct *redir)
         wait(NULL); /* wait till child has died */
 }
 
+/* fd: redirect wants this fd to be used (e.g. 3>file).
+ * Move all conflicting internally used fds,
+ * and remember them so that we can restore them later.
+ */
+static int save_fds_on_redirect(int fd, int squirrel[3])
+{
+        if (squirrel) {
+                /* Handle redirects of fds 0,1,2 */
+
+                /* If we collide with an already moved stdio fd... */
+                if (fd == squirrel[0]) {
+                        squirrel[0] = xdup_and_close(squirrel[0], F_DUPFD);
+                        return 1;
+                }
+                if (fd == squirrel[1]) {
+                        squirrel[1] = xdup_and_close(squirrel[1], F_DUPFD);
+                        return 1;
+                }
+                if (fd == squirrel[2]) {
+                        squirrel[2] = xdup_and_close(squirrel[2], F_DUPFD);
+                        return 1;
+                }
+                /* If we are about to redirect stdio fd, and did not yet move it... */
+                if (fd <= 2 && squirrel[fd] < 0) {
+                        /* We avoid taking stdio fds */
+                        squirrel[fd] = fcntl(fd, F_DUPFD, 10);
+                        if (squirrel[fd] < 0 && errno != EBADF)
+                                xfunc_die();
+                        return 0; /* "we did not close fd" */
+                }
+        }
+
+#if ENABLE_HUSH_INTERACTIVE
+        if (fd != 0 && fd == G.interactive_fd) {
+                G.interactive_fd = xdup_and_close(G.interactive_fd, F_DUPFD_CLOEXEC);
+                return 1;
+        }
+#endif
+
+        /* Are we called from setup_redirects(squirrel==NULL)? Two cases:
+         * (1) Redirect in a forked child. No need to save FILEs' fds,
+         * we aren't going to use them anymore, ok to trash.
+         * (2) "exec 3>FILE". Bummer. We can save FILEs' fds,
+         * but how are we doing to use them?
+         * "fileno(fd) = new_fd" can't be done.
+         */
+        if (!squirrel)
+                return 0;
+
+        return save_FILEs_on_redirect(fd);
+}
+
+static void restore_redirects(int squirrel[3])
+{
+        int i, fd;
+        for (i = 0; i <= 2; i++) {
+                fd = squirrel[i];
+                if (fd != -1) {
+                        /* We simply die on error */
+                        xmove_fd(fd, i);
+                }
+        }
+
+        /* Moved G.interactive_fd stays on new fd, not doing anything for it */
+
+        restore_redirected_FILEs();
+}
+
 /* squirrel != NULL means we squirrel away copies of stdin, stdout,
  * and stderr if they are redirected. */
 static int setup_redirects(struct command *prog, int squirrel[])
@@ -6157,13 +6263,7 @@ static int setup_redirects(struct command *prog, int squirrel[])
         for (redir = prog->redirects; redir; redir = redir->next) {
                 if (redir->rd_type == REDIRECT_HEREDOC2) {
                         /* "rd_fd<<HERE" case */
-                        if (redir->rd_fd <= 2
-                         && squirrel
-                         && squirrel[redir->rd_fd] < 0
-                        ) {
-                                /* Save old fds 0..2 if redirect uses them */
-                                squirrel[redir->rd_fd] = dup(redir->rd_fd);
-                        }
+                        save_fds_on_redirect(redir->rd_fd, squirrel);
                         /* for REDIRECT_HEREDOC2, rd_filename holds _contents_
                          * of the heredoc */
                         debug_printf_parse("set heredoc '%s'\n",
@@ -6199,47 +6299,26 @@ static int setup_redirects(struct command *prog, int squirrel[])
                 }
 
                 if (openfd != redir->rd_fd) {
-                        if (redir->rd_fd <= 2
-                         && squirrel
-                         && squirrel[redir->rd_fd] < 0
-                        ) {
-                                /* Save old fds 0..2 if redirect uses them */
-//FIXME: script fd's also need this! "sh SCRIPT" and SCRIPT has "echo FOO 3>&-":
-// open("SCRIPT", O_RDONLY)          = 3
-// fcntl(3, F_SETFD, FD_CLOEXEC)     = 0
-// read(3, "echo FOO 3>&-\n....", 4096) = N
-// close(3)                          = 0
-// write(1, "FOO\n", 4)              = 4
-// ...
-// read(3, 0x205f8e0, 4096)          = -1 EBADF <== BUG
-//
-                                squirrel[redir->rd_fd] = dup(redir->rd_fd);
-                        }
+                        int closed = save_fds_on_redirect(redir->rd_fd, squirrel);
                         if (openfd == REDIRFD_CLOSE) {
-                                /* "n>-" means "close me" */
-                                close(redir->rd_fd);
+                                /* "rd_fd >&-" means "close me" */
+                                if (!closed) {
+                                        /* ^^^ optimization: saving may already
+                                         * have closed it. If not... */
+                                        close(redir->rd_fd);
+                                }
                         } else {
                                 xdup2(openfd, redir->rd_fd);
                                 if (redir->rd_dup == REDIRFD_TO_FILE)
+                                        /* "rd_fd > FILE" */
                                         close(openfd);
+                                /* else: "rd_fd > rd_dup" */
                         }
                 }
         }
         return 0;
 }
 
-static void restore_redirects(int squirrel[])
-{
-        int i, fd;
-        for (i = 0; i <= 2; i++) {
-                fd = squirrel[i];
-                if (fd != -1) {
-                        /* We simply die on error */
-                        xmove_fd(fd, i);
-                }
-        }
-}
-
 static char *find_in_path(const char *arg)
 {
         char *ret = NULL;
diff --git a/shell/hush_test/hush-misc/redir_script.right b/shell/hush_test/hush-misc/redir_script.right
new file mode 100644
index 000000000..6694ed334
--- /dev/null
+++ b/shell/hush_test/hush-misc/redir_script.right
@@ -0,0 +1 @@
+Ok: script fd is not closed
diff --git a/shell/hush_test/hush-misc/redir_script.tests b/shell/hush_test/hush-misc/redir_script.tests
new file mode 100755
index 000000000..ccc497d7b
--- /dev/null
+++ b/shell/hush_test/hush-misc/redir_script.tests
@@ -0,0 +1,29 @@
+# Builds a " 3>&- 4>&-" string.
+# Note: one of these fds is a directory opened to /proc/self/fd
+# for globbing. It is unwanted, but I don't know how to filter it out.
+find_fds() {
+        fds=""
+        for f in /proc/self/fd/*; do
+                test "$f" = "/proc/self/fd/0" && continue
+                test "$f" = "/proc/self/fd/1" && continue
+                test "$f" = "/proc/self/fd/2" && continue
+                fds="$fds ${f##*/}>&-"
+        done
+}
+
+find_fds
+fds1="$fds"
+
+# One of the fds is open to the script body
+# Close it while executing something.
+eval "find_fds $fds"
+
+# Shell should not lose that fd. Did it?
+find_fds
+test x"$fds1" = x"$fds" && { echo "Ok: script fd is not closed"; exit 0; }
+
+echo "Bug: script fd is closed"
+echo "fds1:$fds1"
+echo "fds2:$fds"
+exit 1
+