11 files changed, 225 insertions, 143 deletions

diff --git a/shell/ash.c b/shell/ash.c
index 1e720aec4..ac676c2dc 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -5192,7 +5192,7 @@ stoppedjobs(void)
 #undef EMPTY
 #undef CLOSED
 #define EMPTY -2                /* marks an unused slot in redirtab */
-#define CLOSED -3               /* marks a slot of previously-closed fd */
+#define CLOSED -1               /* marks a slot of previously-closed fd */
 
 /*
  * Handle here documents.  Normally we fork off a process to write the
@@ -5349,74 +5349,158 @@ dup2_or_raise(int from, int to)
         }
         return newfd;
 }
+static int
+fcntl_F_DUPFD(int fd, int avoid_fd)
+{
+        int newfd;
+ repeat:
+        newfd = fcntl(fd, F_DUPFD, avoid_fd + 1);
+        if (newfd < 0) {
+                if (errno == EBUSY)
+                        goto repeat;
+                if (errno == EINTR)
+                        goto repeat;
+        }
+        return newfd;
+}
+static int
+xdup_CLOEXEC_and_close(int fd, int avoid_fd)
+{
+        int newfd;
+ repeat:
+        newfd = fcntl(fd, F_DUPFD, avoid_fd + 1);
+        if (newfd < 0) {
+                if (errno == EBUSY)
+                        goto repeat;
+                if (errno == EINTR)
+                        goto repeat;
+                /* fd was not open? */
+                if (errno == EBADF)
+                        return fd;
+                ash_msg_and_raise_perror("%d", newfd);
+        }
+        fcntl(newfd, F_SETFD, FD_CLOEXEC);
+        close(fd);
+        return newfd;
+}
 
 /* Struct def and variable are moved down to the first usage site */
-struct two_fd_t {
-        int orig, copy;
+struct squirrel {
+        int orig_fd;
+        int moved_to;
 };
 struct redirtab {
         struct redirtab *next;
         int pair_count;
-        struct two_fd_t two_fd[];
+        struct squirrel two_fd[];
 };
 #define redirlist (G_var.redirlist)
-enum {
-        COPYFD_RESTORE = (int)~(INT_MAX),
-};
 
-static int
-need_to_remember(struct redirtab *rp, int fd)
+static void
+add_squirrel_closed(struct redirtab *sq, int fd)
 {
         int i;
 
-        if (!rp) /* remembering was not requested */
-                return 0;
+        if (!sq)
+                return;
 
-        for (i = 0; i < rp->pair_count; i++) {
-                if (rp->two_fd[i].orig == fd) {
-                        /* already remembered */
-                        return 0;
+        for (i = 0; sq->two_fd[i].orig_fd != EMPTY; i++) {
+                /* If we collide with an already moved fd... */
+                if (fd == sq->two_fd[i].orig_fd) {
+                        /* Examples:
+                         * "echo 3>FILE 3>&- 3>FILE"
+                         * "echo 3>&- 3>FILE"
+                         * No need for last redirect to insert
+                         * another "need to close 3" indicator.
+                         */
+                        TRACE(("redirect_fd %d: already moved or closed\n", fd));
+                        return;
                 }
         }
-        return 1;
+        TRACE(("redirect_fd %d: previous fd was closed\n", fd));
+        sq->two_fd[i].orig_fd = fd;
+        sq->two_fd[i].moved_to = CLOSED;
 }
 
-/* "hidden" fd is a fd used to read scripts, or a copy of such */
 static int
-is_hidden_fd(struct redirtab *rp, int fd)
+save_fd_on_redirect(int fd, int avoid_fd, struct redirtab *sq)
 {
-        int i;
-        struct parsefile *pf;
+        int i, new_fd;
+
+        if (avoid_fd < 9) /* the important case here is that it can be -1 */
+                avoid_fd = 9;
 
-        if (fd == -1)
+#if JOBS
+        if (fd == ttyfd) {
+                /* Testcase: "ls -l /proc/$$/fd 10>&-" should work */
+                ttyfd = xdup_CLOEXEC_and_close(ttyfd, avoid_fd);
+                TRACE(("redirect_fd %d: matches ttyfd, moving it to %d\n", fd, ttyfd));
+                return 1; /* "we closed fd" */
+        }
+#endif
+        /* Are we called from redirect(0)? E.g. redirect
+         * in a forked child. No need to save fds,
+         * we aren't going to use them anymore, ok to trash.
+         */
+        if (!sq)
                 return 0;
-        /* Check open scripts' fds */
-        pf = g_parsefile;
-        while (pf) {
-                /* We skip pf_fd == 0 case because of the following case:
-                 * $ ash  # running ash interactively
-                 * $ . ./script.sh
-                 * and in script.sh: "exec 9>&0".
-                 * Even though top-level pf_fd _is_ 0,
-                 * it's still ok to use it: "read" builtin uses it,
-                 * why should we cripple "exec" builtin?
-                 */
-                if (pf->pf_fd > 0 && fd == pf->pf_fd) {
-                        return 1;
+
+        /* If this one of script's fds? */
+        if (fd != 0) {
+                struct parsefile *pf = g_parsefile;
+                while (pf) {
+                        /* We skip fd == 0 case because of the following:
+                         * $ ash  # running ash interactively
+                         * $ . ./script.sh
+                         * and in script.sh: "exec 9>&0".
+                         * Even though top-level pf_fd _is_ 0,
+                         * it's still ok to use it: "read" builtin uses it,
+                         * why should we cripple "exec" builtin?
+                         */
+                        if (fd == pf->pf_fd) {
+                                pf->pf_fd = xdup_CLOEXEC_and_close(fd, avoid_fd);
+                                return 1; /* "we closed fd" */
+                        }
+                        pf = pf->prev;
                 }
-                pf = pf->prev;
         }
 
-        if (!rp)
-                return 0;
-        /* Check saved fds of redirects */
-        fd |= COPYFD_RESTORE;
-        for (i = 0; i < rp->pair_count; i++) {
-                if (rp->two_fd[i].copy == fd) {
-                        return 1;
+        /* Check whether it collides with any open fds (e.g. stdio), save fds as needed */
+
+        /* First: do we collide with some already moved fds? */
+        for (i = 0; sq->two_fd[i].orig_fd != EMPTY; i++) {
+                /* If we collide with an already moved fd... */
+                if (fd == sq->two_fd[i].moved_to) {
+                        new_fd = fcntl_F_DUPFD(fd, avoid_fd);
+                        sq->two_fd[i].moved_to = new_fd;
+                        TRACE(("redirect_fd %d: already busy, moving to %d\n", fd, new_fd));
+                        if (new_fd < 0) /* what? */
+                                xfunc_die();
+                        return 0; /* "we did not close fd" */
+                }
+                if (fd == sq->two_fd[i].orig_fd) {
+                        /* Example: echo Hello >/dev/null 1>&2 */
+                        TRACE(("redirect_fd %d: already moved\n", fd));
+                        return 0; /* "we did not close fd" */
                 }
         }
-        return 0;
+
+        /* If this fd is open, we move and remember it; if it's closed, new_fd = CLOSED (-1) */
+        new_fd = fcntl_F_DUPFD(fd, avoid_fd);
+        TRACE(("redirect_fd %d: previous fd is moved to %d (-1 if it was closed)\n", fd, new_fd));
+        if (new_fd < 0) {
+                if (errno != EBADF)
+                        xfunc_die();
+                /* new_fd = CLOSED; - already is -1 */
+        }
+        sq->two_fd[i].moved_to = new_fd;
+        sq->two_fd[i].orig_fd = fd;
+
+        /* if we move stderr, let "set -x" code know */
+        if (fd == preverrout_fd)
+                preverrout_fd = new_fd;
+
+        return 0; /* "we did not close fd" */
 }
 
 /*
@@ -5431,109 +5515,80 @@ redirect(union node *redir, int flags)
 {
         struct redirtab *sv;
         int sv_pos;
-        int fd;
-        int newfd;
 
         if (!redir)
                 return;
+
+        sv_pos = 0;
         sv = NULL;
         INT_OFF;
         if (flags & REDIR_PUSH)
                 sv = redirlist;
-        sv_pos = 0;
         do {
-                int right_fd = -1;
+                int fd;
+                int newfd;
+                int close_fd;
+                int closed;
+
                 fd = redir->nfile.fd;
                 if (redir->nfile.type == NTOFD || redir->nfile.type == NFROMFD) {
-                        right_fd = redir->ndup.dupfd;
-                        //bb_error_msg("doing %d > %d", fd, right_fd);
-                        /* redirect from/to same file descriptor? */
-                        if (right_fd == fd)
-                                continue;
-                        /* "echo >&10" and 10 is a fd opened to a sh script? */
-                        if (is_hidden_fd(sv, right_fd)) {
-                                errno = EBADF; /* as if it is closed */
-                                ash_msg_and_raise_perror("%d", right_fd);
-                        }
-                        newfd = -1;
+                        //bb_error_msg("doing %d > %d", fd, newfd);
+                        newfd = redir->ndup.dupfd;
+                        close_fd = -1;
                 } else {
                         newfd = openredirect(redir); /* always >= 0 */
                         if (fd == newfd) {
-                                /* Descriptor wasn't open before redirect.
-                                 * Mark it for close in the future */
-                                if (need_to_remember(sv, fd)) {
-                                        goto remember_to_close;
-                                }
+                                /* open() gave us precisely the fd we wanted.
+                                 * This means that this fd was not busy
+                                 * (not opened to anywhere).
+                                 * Remember to close it on restore:
+                                 */
+                                add_squirrel_closed(sv, fd);
                                 continue;
                         }
+                        close_fd = newfd;
                 }
-#if BASH_REDIR_OUTPUT
- redirect_more:
-#endif
-                if (need_to_remember(sv, fd)) {
-                        /* Copy old descriptor */
-                        /* Careful to not accidentally "save"
-                         * to the same fd as right side fd in N>&M */
-                        int minfd = right_fd < 10 ? 10 : right_fd + 1;
-#if defined(F_DUPFD_CLOEXEC)
-                        int copy = fcntl(fd, F_DUPFD_CLOEXEC, minfd);
-#else
-                        int copy = fcntl(fd, F_DUPFD, minfd);
-#endif
-                        if (copy == -1) {
-                                int e = errno;
-                                if (e != EBADF) {
-                                        /* Strange error (e.g. "too many files" EMFILE?) */
-                                        if (newfd >= 0)
-                                                close(newfd);
-                                        errno = e;
-                                        ash_msg_and_raise_perror("%d", fd);
-                                        /* NOTREACHED */
-                                }
-                                /* EBADF: it is not open - good, remember to close it */
- remember_to_close:
-                                copy = CLOSED;
-                        } else { /* fd is open, save its copy */
-#if !defined(F_DUPFD_CLOEXEC)
-                                fcntl(copy, F_SETFD, FD_CLOEXEC);
-#endif
-                                /* "exec fd>&-" should not close fds
-                                 * which point to script file(s).
-                                 * Force them to be restored afterwards */
-                                if (is_hidden_fd(sv, fd))
-                                        copy |= COPYFD_RESTORE;
-                        }
-                        /* if we move stderr, let "set -x" code know */
-                        if (fd == preverrout_fd)
-                                preverrout_fd = copy;
-                        sv->two_fd[sv_pos].orig = fd;
-                        sv->two_fd[sv_pos].copy = copy;
-                        sv_pos++;
-                }
-                if (newfd < 0) {
-                        /* NTOFD/NFROMFD: copy redir->ndup.dupfd to fd */
-                        if (redir->ndup.dupfd < 0) { /* "fd>&-" */
-                                /* Don't want to trigger debugging */
-                                if (fd != -1)
-                                        close(fd);
-                        } else {
-                                dup2_or_raise(redir->ndup.dupfd, fd);
+
+                if (fd == newfd)
+                        continue;
+
+                /* if "N>FILE": move newfd to fd */
+                /* if "N>&M": dup newfd to fd */
+                /* if "N>&-": close fd (newfd is -1) */
+
+ IF_BASH_REDIR_OUTPUT(redirect_more:)
+
+                closed = save_fd_on_redirect(fd, /*avoid:*/ newfd, sv);
+                if (newfd == -1) {
+                        /* "N>&-" means "close me" */
+                        if (!closed) {
+                                /* ^^^ optimization: saving may already
+                                 * have closed it. If not... */
+                                close(fd);
                         }
-                } else if (fd != newfd) { /* move newfd to fd */
+                } else {
+///TODO: if _newfd_ is a script fd or saved fd, then simulate EBADF!
+//if (newfd == ttyfd) {
+//      errno = EBADF;
+//      ash_msg_and_raise_perror("A %d", newfd);
+//}
+//if (newfd == g_parsefile->pf_fd) {
+//      errno = EBADF;
+//      ash_msg_and_raise_perror("B %d", newfd);
+//}
                         dup2_or_raise(newfd, fd);
+                        if (close_fd >= 0) /* "N>FILE" or ">&FILE" or heredoc? */
+                                close(close_fd);
 #if BASH_REDIR_OUTPUT
-                        if (!(redir->nfile.type == NTO2 && fd == 2))
+                        if (redir->nfile.type == NTO2 && fd == 1) {
+                                /* ">&FILE". we already redirected to 1, now copy 1 to 2 */
+                                fd = 2;
+                                newfd = 1;
+                                close_fd = -1;
+                                goto redirect_more;
+                        }
 #endif
-                                close(newfd);
                 }
-#if BASH_REDIR_OUTPUT
-                if (redir->nfile.type == NTO2 && fd == 1) {
-                        /* We already redirected it to fd 1, now copy it to 2 */
-                        newfd = 1;
-                        fd = 2;
-                        goto redirect_more;
-                }
-#endif
         } while ((redir = redir->nfile.next) != NULL);
         INT_ON;
 
@@ -5542,7 +5597,7 @@ redirect(union node *redir, int flags)
         // dash has a bug: since REDIR_SAVEFD2=3 and REDIR_PUSH=1, this test
         // triggers for pure REDIR_PUSH too. Thus, this is done almost always,
         // not only for calls with flags containing REDIR_SAVEFD2.
-        // We do this unconditionally (see code above).
+        // We do this unconditionally (see save_fd_on_redirect()).
         //if ((flags & REDIR_SAVEFD2) && copied_fd2 >= 0)
         //      preverrout_fd = copied_fd2;
 }
@@ -5557,7 +5612,7 @@ redirectsafe(union node *redir, int flags)
 
         SAVE_INT(saveint);
         /* "echo 9>/dev/null; echo >&9; echo result: $?" - result should be 1, not 2! */
-        err = setjmp(jmploc.loc); // huh?? was = setjmp(jmploc.loc) * 2;
+        err = setjmp(jmploc.loc); /* was = setjmp(jmploc.loc) * 2; */
         if (!err) {
                 exception_handler = &jmploc;
                 redirect(redir, flags);
@@ -5591,7 +5646,7 @@ pushredir(union node *redir)
         sv = ckzalloc(sizeof(*sv) + i * sizeof(sv->two_fd[0]));
         sv->pair_count = i;
         while (--i >= 0)
-                sv->two_fd[i].orig = sv->two_fd[i].copy = EMPTY;
+                sv->two_fd[i].orig_fd = sv->two_fd[i].moved_to = EMPTY;
         sv->next = redirlist;
         redirlist = sv;
         return sv->next;
@@ -5601,7 +5656,7 @@ pushredir(union node *redir)
  * Undo the effects of the last redirection.
  */
 static void
-popredir(int drop, int restore)
+popredir(int drop)
 {
         struct redirtab *rp;
         int i;
@@ -5611,20 +5666,19 @@ popredir(int drop, int restore)
         INT_OFF;
         rp = redirlist;
         for (i = 0; i < rp->pair_count; i++) {
-                int fd = rp->two_fd[i].orig;
-                int copy = rp->two_fd[i].copy;
+                int fd = rp->two_fd[i].orig_fd;
+                int copy = rp->two_fd[i].moved_to;
                 if (copy == CLOSED) {
                         if (!drop)
                                 close(fd);
                         continue;
                 }
                 if (copy != EMPTY) {
-                        if (!drop || (restore && (copy & COPYFD_RESTORE))) {
-                                copy &= ~COPYFD_RESTORE;
+                        if (!drop) {
                                 /*close(fd);*/
                                 dup2_or_raise(copy, fd);
                         }
-                        close(copy & ~COPYFD_RESTORE);
+                        close(copy);
                 }
         }
         redirlist = rp->next;
@@ -5636,7 +5690,7 @@ static void
 unwindredir(struct redirtab *stop)
 {
         while (redirlist != stop)
-                popredir(/*drop:*/ 0, /*restore:*/ 0);
+                popredir(/*drop:*/ 0);
 }
 
 
@@ -7715,7 +7769,7 @@ tryexec(IF_FEATURE_SH_STANDALONE(int applet_no,) const char *cmd, char **argv, c
                         clearenv();
                         while (*envp)
                                 putenv(*envp++);
-                        popredir(/*drop:*/ 1, /*restore:*/ 0);
+                        popredir(/*drop:*/ 1);
                         run_applet_no_and_exit(applet_no, cmd, argv);
                 }
                 /* re-exec ourselves with the new arguments */
@@ -8754,7 +8808,7 @@ evaltree(union node *n, int flags)
                         status = evaltree(n->nredir.n, flags & EV_TESTED);
                 }
                 if (n->nredir.redirect)
-                        popredir(/*drop:*/ 0, /*restore:*/ 0 /* not sure */);
+                        popredir(/*drop:*/ 0);
                 goto setstatus;
         case NCMD:
                 evalfn = evalcommand;
@@ -9902,7 +9956,7 @@ evalcommand(union node *cmd, int flags)
 
  out:
         if (cmd->ncmd.redirect)
-                popredir(/*drop:*/ cmd_is_exec, /*restore:*/ cmd_is_exec);
+                popredir(/*drop:*/ cmd_is_exec);
         unwindredir(redir_stop);
         unwindlocalvars(localvar_stop);
         if (lastarg) {
@@ -13727,7 +13781,7 @@ int ash_main(int argc UNUSED_PARAM, char **argv)
                  * Testcase: ash -c 'exec 1>&0' must not complain. */
                 // if (!sflag) g_parsefile->pf_fd = -1;
                 // ^^ not necessary since now we special-case fd 0
-                // in is_hidden_fd() to not be considered "hidden fd"
+                // in save_fd_on_redirect()
                 evalstring(minusc, sflag ? 0 : EV_EXIT);
         }
 
diff --git a/shell/ash_test/ash-redir/redir_script.tests b/shell/ash_test/ash-redir/redir_script.tests
index ccc497d7b..740daa461 100755
--- a/shell/ash_test/ash-redir/redir_script.tests
+++ b/shell/ash_test/ash-redir/redir_script.tests
@@ -20,10 +20,15 @@ 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; }
+test x"$fds1" = x"$fds" \
+&& { echo "Ok: script fd is not closed"; exit 0; }
+
+# One legit way to handle it is to move script fd. For example, if we see that fd 10 moved to fd 11:
+test x"$fds1" = x" 10>&- 3>&-" && \
+test x"$fds"  = x" 11>&- 3>&-" \
+&& { echo "Ok: script fd is not closed"; exit 0; }
 
 echo "Bug: script fd is closed"
 echo "fds1:$fds1"
 echo "fds2:$fds"
 exit 1
-
diff --git a/shell/ash_test/ash-redir/redir_to_bad_fd255.right b/shell/ash_test/ash-redir/redir_to_bad_fd255.right
new file mode 100644
index 000000000..9c5e35b36
--- /dev/null
+++ b/shell/ash_test/ash-redir/redir_to_bad_fd255.right
@@ -0,0 +1,2 @@
+./redir_to_bad_fd255.tests: line 2: 255: Bad file descriptor
+OK
diff --git a/shell/ash_test/ash-redir/redir_to_bad_fd255.tests b/shell/ash_test/ash-redir/redir_to_bad_fd255.tests
new file mode 100755
index 000000000..2266af6da
--- /dev/null
+++ b/shell/ash_test/ash-redir/redir_to_bad_fd255.tests
@@ -0,0 +1,3 @@
+# ash uses fd 10 (usually) for reading the script
+echo LOST >&255
+echo OK
diff --git a/shell/ash_test/ash-redir/redir_to_bad_fd3.right b/shell/ash_test/ash-redir/redir_to_bad_fd3.right
new file mode 100644
index 000000000..895a4a0a6
--- /dev/null
+++ b/shell/ash_test/ash-redir/redir_to_bad_fd3.right
@@ -0,0 +1,2 @@
+./redir_to_bad_fd3.tests: line 2: 3: Bad file descriptor
+OK
diff --git a/shell/ash_test/ash-redir/redir_to_bad_fd3.tests b/shell/ash_test/ash-redir/redir_to_bad_fd3.tests
new file mode 100755
index 000000000..98c54cfc6
--- /dev/null
+++ b/shell/ash_test/ash-redir/redir_to_bad_fd3.tests
@@ -0,0 +1,3 @@
+# ash uses fd 10 (usually) for reading the script
+echo LOST >&3
+echo OK
diff --git a/shell/hush_test/hush-redir/redir_script.tests b/shell/hush_test/hush-redir/redir_script.tests
index ccc497d7b..740daa461 100755
--- a/shell/hush_test/hush-redir/redir_script.tests
+++ b/shell/hush_test/hush-redir/redir_script.tests
@@ -20,10 +20,15 @@ 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; }
+test x"$fds1" = x"$fds" \
+&& { echo "Ok: script fd is not closed"; exit 0; }
+
+# One legit way to handle it is to move script fd. For example, if we see that fd 10 moved to fd 11:
+test x"$fds1" = x" 10>&- 3>&-" && \
+test x"$fds"  = x" 11>&- 3>&-" \
+&& { echo "Ok: script fd is not closed"; exit 0; }
 
 echo "Bug: script fd is closed"
 echo "fds1:$fds1"
 echo "fds2:$fds"
 exit 1
-
diff --git a/shell/hush_test/hush-redir/redir_to_bad_fd255.right b/shell/hush_test/hush-redir/redir_to_bad_fd255.right
new file mode 100644
index 000000000..936911ce5
--- /dev/null
+++ b/shell/hush_test/hush-redir/redir_to_bad_fd255.right
@@ -0,0 +1 @@
+hush: can't duplicate file descriptor: Bad file descriptor
diff --git a/shell/hush_test/hush-redir/redir_to_bad_fd255.tests b/shell/hush_test/hush-redir/redir_to_bad_fd255.tests
new file mode 100755
index 000000000..2266af6da
--- /dev/null
+++ b/shell/hush_test/hush-redir/redir_to_bad_fd255.tests
@@ -0,0 +1,3 @@
+# ash uses fd 10 (usually) for reading the script
+echo LOST >&255
+echo OK
diff --git a/shell/hush_test/hush-redir/redir_to_bad_fd3.right b/shell/hush_test/hush-redir/redir_to_bad_fd3.right
new file mode 100644
index 000000000..936911ce5
--- /dev/null
+++ b/shell/hush_test/hush-redir/redir_to_bad_fd3.right
@@ -0,0 +1 @@
+hush: can't duplicate file descriptor: Bad file descriptor
diff --git a/shell/hush_test/hush-redir/redir_to_bad_fd3.tests b/shell/hush_test/hush-redir/redir_to_bad_fd3.tests
new file mode 100755
index 000000000..98c54cfc6
--- /dev/null
+++ b/shell/hush_test/hush-redir/redir_to_bad_fd3.tests
@@ -0,0 +1,3 @@
+# ash uses fd 10 (usually) for reading the script
+echo LOST >&3
+echo OK