Stop the child output reading when the child process ends.

Otherwise, the child process could terminate without fully closing the pipes, causing unwanted delays - see the added test.

PiperOrigin-RevId: 762172820

Author: xinhaoyuan

fuzztest/internal/subprocess.cc

@@ -79,6 +79,8 @@ class SubProcess {
   static constexpr int kStdErrIdx = 1;
   int parent_pipe_[2];
   int child_pipe_[2];
+  // Internal pipe fds for notifying child wait completion.
+  int child_wait_pipe_fds_[2];
 };
 
 // Creates parent/child pipes for piping stdout/stderr from child to parent.
@@ -95,6 +97,13 @@ void SubProcess::CreatePipes() {
         fcntl(parent_pipe_[channel], F_SETFL, O_NONBLOCK) != -1,
         "Cannot make pipe non-blocking: ", strerror(errno));
   }
+  FUZZTEST_INTERNAL_CHECK(pipe(child_wait_pipe_fds_) == 0,
+                          "Cannot create child wait pipe: ", strerror(errno));
+  for (int i = 0; i < 2; ++i) {
+    FUZZTEST_INTERNAL_CHECK(
+        fcntl(child_wait_pipe_fds_[i], F_SETFD, FD_CLOEXEC) != -1,
+        "Cannot make child wait pipe fds close on exit: ", strerror(errno));
+  }
 }
 
 void SubProcess::CloseChildPipes() {
@@ -193,25 +202,31 @@ void SubProcess::ReadChildOutput(std::string* stdout_output,
                                  std::string* stderr_output) {
   // Set up poll()-ing the pipes.
   constexpr int fd_count = 2;
-  struct pollfd pfd[fd_count];
+  struct pollfd pfd[fd_count + 1];
   std::string* out_str[fd_count];
   for (int channel : {kStdOutIdx, kStdErrIdx}) {
     pfd[channel].fd = parent_pipe_[channel];
     pfd[channel].events = POLLIN;
     pfd[channel].revents = 0;
     out_str[channel] = channel == kStdOutIdx ? stdout_output : stderr_output;
   }
+  pfd[fd_count].fd = child_wait_pipe_fds_[0];
+  pfd[fd_count].events = POLLIN;
+  pfd[fd_count].revents = 0;
 
   // Loop reading stdout and stderr from the child process.
   int fd_remain = fd_count;
   char buf[4096];
   while (fd_remain > 0) {
-    int ret = poll(pfd, fd_count, -1);
+    int ret = poll(pfd, fd_count + 1, -1);
     if ((ret == -1) && !ShouldRetry(errno)) {
       FUZZTEST_INTERNAL_CHECK(false, "Cannot poll(): ", strerror(errno));
     } else if (ret == 0) {
       FUZZTEST_INTERNAL_CHECK(false, "Impossible timeout: ", strerror(errno));
     } else if (ret > 0) {
+      if (pfd[fd_count].revents) {
+        break;
+      }
       for (int channel : {kStdOutIdx, kStdErrIdx}) {
         if ((pfd[channel].revents & (POLLIN | POLLHUP)) != 0) {
           ssize_t n = read(pfd[channel].fd, buf, sizeof(buf));
@@ -247,7 +262,7 @@ int Wait(pid_t pid) {
 // TODO(lszekeres): Consider optimizing this further by eliminating polling.
 // Could potentially be done using pselect() to wait for SIGCHLD with a timeout.
 // I.e., by setting all args to null, except timeout with a sigmask for SIGCHLD.
-int WaitWithTimeout(pid_t pid, absl::Duration timeout) {
+int WaitWithTimeout(pid_t pid, int fd_to_close, absl::Duration timeout) {
   int status;
   const absl::Time wait_until = absl::Now() + timeout;
   constexpr absl::Duration sleep_duration = absl::Milliseconds(100);
@@ -259,17 +274,21 @@ int WaitWithTimeout(pid_t pid, absl::Duration timeout) {
       if (absl::Now() > wait_until) {
         FUZZTEST_INTERNAL_CHECK(kill(pid, SIGTERM) == 0,
                                 "Cannot kill(): ", strerror(errno));
-        return Wait(pid);
+        status = Wait(pid);
+        break;
       } else {
         absl::SleepFor(sleep_duration);
         continue;
       }
     } else if (ret == pid && (WIFEXITED(status) || WIFSIGNALED(status))) {
-      return status;
+      break;
     } else {
       FUZZTEST_INTERNAL_CHECK(false, "wait() error: ", strerror(errno));
     }
   }
+  FUZZTEST_INTERNAL_CHECK(close(fd_to_close) != -1,
+                          "Cannot close fd_to_close: ", strerror(errno));
+  return status;
 }
 
 }  // anonymous namespace
@@ -282,10 +301,13 @@ RunResults SubProcess::Run(
   pid_t child_pid = StartChild(command_line, environment);
   CloseChildPipes();
   std::future<int> status =
-      std::async(std::launch::async, &WaitWithTimeout, child_pid, timeout);
+      std::async(std::launch::async, &WaitWithTimeout, child_pid,
+                 child_wait_pipe_fds_[1], timeout);
   std::string stdout_output, stderr_output;
   ReadChildOutput(&stdout_output, &stderr_output);
   CloseParentPipes();
+  FUZZTEST_INTERNAL_CHECK(close(child_wait_pipe_fds_[0]) != -1,
+                          "Cannot close child wait pipe: ", strerror(errno));
   return {TerminationStatus(status.get()), stdout_output, stderr_output};
 }
 

fuzztest/internal/subprocess_test.cc

@@ -21,6 +21,7 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include "absl/strings/str_cat.h"
+#include "absl/time/clock.h"
 #include "absl/time/time.h"
 
 namespace fuzztest::internal {
@@ -79,5 +80,14 @@ TEST(SubProcessTest, TimeoutIsEnforced) {
   EXPECT_EQ(std_err.size(), 0);
 }
 
+TEST(SubProcessTest, ReturnsAfterChildProcessEnds) {
+  const auto before_time = absl::Now();
+  auto [status, std_out, std_err] = RunCommand({"bash", "-c", "sleep 4&"});
+  const auto after_time = absl::Now();
+  EXPECT_TRUE(status.Exited());
+  EXPECT_EQ(status, ExitCode(0));
+  EXPECT_LT(after_time - before_time, absl::Seconds(2));
+}
+
 }  // namespace
 }  // namespace fuzztest::internal