[git commit] hush: fix/explain corner cases of redirection colliding with script fd

[Denys Vlasenko]

commit: https://git.busybox.net/busybox/commit/?id=945e9b05c910d3b946a5d5a396c5a7e075513e6e
branch: https://git.busybox.net/busybox/commit/?id=refs/heads/master

function                                             old     new   delta
save_fd_on_redirect                                  200     208      +8
run_pipe                                            1870    1873      +3
setup_redirects                                      321     322      +1
------------------------------------------------------------------------------
(add/remove: 0/0 grow/shrink: 3/0 up/down: 12/0)               Total: 12 bytes

Signed-off-by: Denys Vlasenko <vda.linux at googlemail.com>
---
 shell/hush.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 12 deletions(-)

diff --git a/shell/hush.c b/shell/hush.c
index 7a1513c24..e3c6e2de9 100644
--- a/shell/hush.c
+++ b/shell/hush.c
@@ -7470,16 +7470,54 @@ static int save_fd_on_redirect(int fd, int avoid_fd, struct squirrel **sqp)
 		return 1; /* "we closed fd" */
 	}
 #endif
+	/* Are we called from setup_redirects(squirrel==NULL)
+	 * in redirect in a [v]forked child?
+	 */
+	if (sqp == NULL) {
+		/* No need to move script fds.
+		 * For NOMMU case, it's actively wrong: we'd change ->fd
+		 * fields in memory for the parent, but parent's fds
+		 * aren't be moved, it would use wrong fd!
+		 * Reproducer: "cmd 3>FILE" in script.
+		 * If we would call move_HFILEs_on_redirect(), child would:
+		 *  fcntl64(3, F_DUPFD_CLOEXEC, 10)   = 10
+		 *  close(3)                          = 0
+		 * and change ->fd to 10 if fd#3 is a script fd. WRONG.
+		 */
+		//bb_error_msg("sqp == NULL: [v]forked child");
+		return 0;
+	}
+
 	/* If this one of script's fds? */
 	if (move_HFILEs_on_redirect(fd, avoid_fd))
 		return 1; /* yes. "we closed fd" (actually moved it) */
 
-	/* Are we called from setup_redirects(squirrel==NULL)? Two cases:
-	 * (1) Redirect in a forked child.
-	 * (2) "exec 3>FILE".
+	/* Are we called for "exec 3>FILE"? Came through
+	 * redirect_and_varexp_helper(squirrel=ERR_PTR) -> setup_redirects(ERR_PTR)
+	 * This case used to fail for this script:
+	 *  exec 3>FILE
+	 *  echo Ok
+	 *  ...100000 more lines...
+	 *  echo Ok
+	 * as follows:
+	 *  read(3, "exec 3>FILE\necho Ok\necho Ok"..., 1024) = 1024
+	 *  open("FILE", O_WRONLY|O_CREAT|O_TRUNC|O_LARGEFILE, 0666) = 4
+	 *  dup2(4, 3)                        = 3
+	 *  ^^^^^^^^ oops, we lost fd#3 opened to our script!
+	 *  close(4)                          = 0
+	 *  write(1, "Ok\n", 3)               = 3
+	 *  ...                               = 3
+	 *  write(1, "Ok\n", 3)               = 3
+	 *  read(3, 0x94fbc08, 1024)          = -1 EBADF (Bad file descriptor)
+	 *  ^^^^^^^^ oops, wrong fd!!!
+	 * With this case separate from sqp == NULL and *after* move_HFILEs,
+	 * it now works:
 	 */
-	if (!sqp)
+	if (sqp == ERR_PTR) {
+		/* Don't preserve redirected fds: exec is _meant_ to change these */
+		//bb_error_msg("sqp == ERR_PTR: exec >FILE");
 		return 0;
+	}
 
 	/* Check whether it collides with any open fds (e.g. stdio), save fds as needed */
 	*sqp = add_squirrel(*sqp, fd, avoid_fd);
@@ -7618,7 +7656,7 @@ static int setup_redirects(struct command *prog, struct squirrel **sqp)
 			 */
 		} else {
 			/* if newfd is a script fd or saved fd, simulate EBADF */
-			if (internally_opened_fd(newfd, sqp ? *sqp : NULL)) {
+			if (internally_opened_fd(newfd, sqp && sqp != ERR_PTR ? *sqp : NULL)) {
 				//errno = EBADF;
 				//bb_perror_msg_and_die("can't duplicate file descriptor");
 				newfd = -1; /* same effect as code above */
@@ -8633,8 +8671,8 @@ static int checkjobs_and_fg_shell(struct pipe *fg_pipe)
  * subshell:     ( list ) [&]
  */
 #if !ENABLE_HUSH_MODE_X
-#define redirect_and_varexp_helper(command, squirrel, argv_expanded) \
-	redirect_and_varexp_helper(command, squirrel)
+#define redirect_and_varexp_helper(command, sqp, argv_expanded) \
+	redirect_and_varexp_helper(command, sqp)
 #endif
 static int redirect_and_varexp_helper(
 		struct command *command,
@@ -8651,10 +8689,6 @@ static int redirect_and_varexp_helper(
 	/* this takes ownership of new_env[i] elements, and frees new_env: */
 	set_vars_and_save_old(new_env);
 
-	/* setup_redirects acts on file descriptors, not FILEs.
-	 * This is perfect for work that comes after exec().
-	 * Is it really safe for inline use?  Experimentally,
-	 * things seem to work. */
 	return setup_redirects(command, sqp);
 }
 static NOINLINE int run_pipe(struct pipe *pi)
@@ -8855,7 +8889,10 @@ static NOINLINE int run_pipe(struct pipe *pi)
 				 */
 				enter_var_nest_level();
 				G.shadowed_vars_pp = &old_vars;
-				rcode = redirect_and_varexp_helper(command, /*squirrel:*/ NULL, argv_expanded);
+				rcode = redirect_and_varexp_helper(command,
+					/*squirrel:*/ ERR_PTR,
+					argv_expanded
+				);
 				G.shadowed_vars_pp = sv_shadowed;
 				/* rcode=1 can be if redir file can't be opened */