Close original pipe end after dup2 in child process

This avoids "leaking" duplicated file descriptors in the child process.

This also allows the child process to signal the end of its processing
by closing its stdout and stderr. This can now be reliably detected by
the parent process, if needed.

Bug: chromium:1009857
Test: Unit tests pass
Change-Id: Ie1cd4ff9e95f18e423df007f88bfff34456346f3

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.