Close original pipe end after dup2 in child process

am: a8be2c42e3

Change-Id: Ie9a2ec36bf09d25bed1a3bd36eb253cf79d79f2c

5 files changed, 84 insertions, 14 deletions

diff --git a/libminijail.c b/libminijail.c
index 7cc862d9..6d6b2dc4 100644
--- a/libminijail.c
+++ b/libminijail.c
@@ -2989,8 +2989,8 @@ static int minijail_run_internal(struct minijail *j,
 	 * set up the read end of the pipe.
 	 */
 	if (status_out->pstdin_fd) {
-		if (setup_and_dupe_pipe_end(stdin_fds, 0 /* read end */,
-					    STDIN_FILENO) < 0)
+		if (dupe_and_close_fd(stdin_fds, 0 /* read end */,
+                          STDIN_FILENO) < 0)
 			die("failed to set up stdin pipe");
 	}
 
@@ -2999,8 +2999,8 @@ static int minijail_run_internal(struct minijail *j,
 	 * set up the write end of the pipe.
 	 */
 	if (status_out->pstdout_fd) {
-		if (setup_and_dupe_pipe_end(stdout_fds, 1 /* write end */,
-					    STDOUT_FILENO) < 0)
+		if (dupe_and_close_fd(stdout_fds, 1 /* write end */,
+                          STDOUT_FILENO) < 0)
 			die("failed to set up stdout pipe");
 	}
 
@@ -3009,8 +3009,8 @@ static int minijail_run_internal(struct minijail *j,
 	 * set up the write end of the pipe.
 	 */
 	if (status_out->pstderr_fd) {
-		if (setup_and_dupe_pipe_end(stderr_fds, 1 /* write end */,
-					    STDERR_FILENO) < 0)
+		if (dupe_and_close_fd(stderr_fds, 1 /* write end */,
+                          STDERR_FILENO) < 0)
 			die("failed to set up stderr pipe");
 	}
 
diff --git a/libminijail_unittest.cc b/libminijail_unittest.cc
index afcf527a..e403ef70 100644
--- a/libminijail_unittest.cc
+++ b/libminijail_unittest.cc
@@ -314,6 +314,73 @@ TEST(WaitTest, can_wait_only_once) {
   EXPECT_EQ(minijail_wait(j.get()), -ECHILD);
 }
 
+TEST(Test, close_original_pipes_after_dup2) {
+  // Pipe used by child process to signal that it continued after reading from
+  // stdin.
+  int to_wait[2];
+  ASSERT_EQ(pipe(to_wait), 0);
+
+  const ScopedMinijail j(minijail_new());
+  char* program;
+  ASSERT_GT(asprintf(&program, R"(
+      echo Hi >&1;
+      echo There >&2;
+      exec 1>&-;
+      exec 2>&-;
+      read line1;
+      read line2;
+      echo "$line1$line2 and Goodbye" >&%d;
+      exit 42;
+    )", to_wait[1]), 0);
+  char *const argv[] = {"sh", "-c", program, nullptr};
+
+  int in = -1;
+  int out = -1;
+  int err = -1;
+  EXPECT_EQ(minijail_run_pid_pipes(j.get(), kShellPath, argv, nullptr, &in,
+				   &out, &err),
+	    0);
+  free(program);
+
+  EXPECT_GT(in, 0);
+  EXPECT_GT(out, 0);
+  EXPECT_GT(err, 0);
+
+  char buf[PIPE_BUF];
+  ssize_t n;
+
+  // Check that stdout and stderr pipes work.
+  n = read(out, buf, PIPE_BUF);
+  ASSERT_GT(n, 0);
+  EXPECT_EQ(std::string(buf, n), "Hi\n");
+
+  n = read(err, buf, PIPE_BUF);
+  ASSERT_GT(n, 0);
+  EXPECT_EQ(std::string(buf, n), "There\n");
+
+  // Check that the write ends of stdout and stderr pipes got closed by the
+  // child process. If the child process kept other file descriptors connected
+  // to stdout and stderr, then the parent process wouldn't be able to detect
+  // that all write ends of these pipes are closed and it would block here.
+  EXPECT_EQ(read(out, buf, PIPE_BUF), 0);
+  EXPECT_EQ(read(err, buf, PIPE_BUF), 0);
+
+  // Check that stdin pipe works.
+  const std::string s = "Greetings\n";
+  EXPECT_EQ(write(in, s.data(), s.size()), s.size());
+
+  // Close write end of pipe connected to child's stdin. If there was another
+  // file descriptor connected to this write end, then the child wouldn't be
+  // able to detect that this write end is closed and it would block.
+  ASSERT_EQ(close(in), 0);
+
+  // Check that child process continued and ended.
+  n = read(to_wait[0], buf, PIPE_BUF);
+  ASSERT_GT(n, 0);
+  EXPECT_EQ(std::string(buf, n), "Greetings and Goodbye\n");
+  EXPECT_EQ(minijail_wait(j.get()), 42);
+}
+
 TEST(Test, minijail_run_pid_pipes) {
   // TODO(crbug.com/895875): The preload library interferes with ASan since they
   // both need to use LD_PRELOAD.
diff --git a/system.c b/system.c
index 175e0885..118daf43 100644
--- a/system.c
+++ b/system.c
@@ -222,14 +222,17 @@ int setup_pipe_end(int fds[2], size_t index)
 	return fds[index];
 }
 
-int setup_and_dupe_pipe_end(int fds[2], size_t index, int fd)
+int dupe_and_close_fd(int fds[2], size_t index, int fd)
 {
 	if (index > 1)
 		return -1;
 
-	close(fds[1 - index]);
 	/* dup2(2) the corresponding end of the pipe into |fd|. */
-	return dup2(fds[index], fd);
+	fd = dup2(fds[index], fd);
+
+	close(fds[0]);
+	close(fds[1]);
+	return fd;
 }
 
 int write_pid_to_path(pid_t pid, const char *path)
diff --git a/system.h b/system.h
index 7369846d..c9e04d5a 100644
--- a/system.h
+++ b/system.h
@@ -47,7 +47,7 @@ int cap_ambient_supported(void);
 int config_net_loopback(void);
 
 int setup_pipe_end(int fds[2], size_t index);
-int setup_and_dupe_pipe_end(int fds[2], size_t index, int fd);
+int dupe_and_close_fd(int fds[2], size_t index, int fd);
 
 int write_pid_to_path(pid_t pid, const char *path);
 int write_proc_file(pid_t pid, const char *content, const char *basename);
diff --git a/system_unittest.cc b/system_unittest.cc
index 4156e6e6..e13630ab 100644
--- a/system_unittest.cc
+++ b/system_unittest.cc
@@ -187,10 +187,10 @@ TEST(setup_pipe_end, index1) {
 }
 
 // Invalid indexes should return errors, not crash.
-TEST(setup_and_dupe_pipe_end, bad_index) {
-  EXPECT_LT(setup_and_dupe_pipe_end(nullptr, 2, -1), 0);
-  EXPECT_LT(setup_and_dupe_pipe_end(nullptr, 3, -1), 0);
-  EXPECT_LT(setup_and_dupe_pipe_end(nullptr, 4, -1), 0);
+TEST(dupe_and_close_fd, bad_index) {
+  EXPECT_LT(dupe_and_close_fd(nullptr, 2, -1), 0);
+  EXPECT_LT(dupe_and_close_fd(nullptr, 3, -1), 0);
+  EXPECT_LT(dupe_and_close_fd(nullptr, 4, -1), 0);
 }
 
 // An invalid path should return an error.