Add and alternative GetAppOutput() to process_util that takes a timeout.

Contributed by tessamac@chromium.org

TEST=none
BUG=47356

Review URL: http://codereview.chromium.org/2810014
Patch from Tessa MacDuff <tessamac@chromium.org>.

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@52608 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 141 insertions, 19 deletions

diff --git a/base/process_util.h b/base/process_util.h
index 48cef2c..db68b84 100644
--- a/base/process_util.h
+++ b/base/process_util.h
@@ -85,6 +85,9 @@ struct IoCounters {
 // A minimalistic but hopefully cross-platform set of exit codes.
 // Do not change the enumeration values or you will break third-party
 // installers.
+// Warning: This enum is duplicated and extended in chrome/common/result_codes.h
+// The two must be kept in sync and users of that enum would prefer the
+// numbering didn't change.
 enum {
   PROCESS_END_NORMAL_TERMINATION = 0,
   PROCESS_END_KILLED_BY_USER     = 1,
@@ -239,6 +242,15 @@ bool LaunchApp(const CommandLine& cl,
 bool GetAppOutput(const CommandLine& cl, std::string* output);
 
 #if defined(OS_POSIX)
+// Similar to |GetAppOutput()|. Returns true if process launched and
+// exited cleanly, with exit code indicating success. Waits no more than
+// |timeout_milliseconds| for the launched process to exit. After timeout
+// is triggered, the process is killed and true is stored in |timed_out|.
+// Will wait for process to die so this may overrun |timeout_milliseconds|.
+// TODO(tessamac): implement this for windows.
+bool GetAppOutputWithTimeout(const CommandLine& cl, std::string* output,
+                             bool* timed_out, int timeout_milliseconds);
+
 // A restricted version of |GetAppOutput()| which (a) clears the environment,
 // and (b) stores at most |max_output| bytes; also, it doesn't search the path
 // for the command.
diff --git a/base/process_util_posix.cc b/base/process_util_posix.cc
index cae70a5..433a1e1 100644
--- a/base/process_util_posix.cc
+++ b/base/process_util_posix.cc
@@ -5,6 +5,7 @@
 #include <dirent.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <poll.h>
 #include <signal.h>
 #include <stdlib.h>
 #include <sys/resource.h>
@@ -737,8 +738,10 @@ int64 TimeValToMicroseconds(const struct timeval& tv) {
   return ret;
 }
 
-// Executes the application specified by |cl| and wait for it to exit. Stores
-// the output (stdout) in |output|. If |do_search_path| is set, it searches the
+// Executes the application specified by |cl| and waits for it to exit, but
+// at most |timeout_milliseconds| (use kNoTimeout for no timeout). Stores
+// the output (stdout) in |output|, and whether or not the timeout
+// triggered in |timed_out|.  If |do_search_path| is set, it searches the
 // path for the application; in that case, |envp| must be null, and it will use
 // the current environment. If |do_search_path| is false, |cl| should fully
 // specify the path of the application, and |envp| will be used as the
@@ -746,7 +749,8 @@ int64 TimeValToMicroseconds(const struct timeval& tv) {
 // (application launched and exited cleanly, with exit code indicating success).
 static bool GetAppOutputInternal(const CommandLine& cl, char* const envp[],
                                  std::string* output, size_t max_output,
-                                 bool do_search_path) {
+                                 bool do_search_path, bool* timed_out,
+                                 int timeout_milliseconds) {
   int pipe_fd[2];
   pid_t pid;
   InjectiveMultimap fd_shuffle1, fd_shuffle2;
@@ -788,7 +792,7 @@ static bool GetAppOutputInternal(const CommandLine& cl, char* const envp[],
         fd_shuffle1.push_back(InjectionArc(pipe_fd[1], STDOUT_FILENO, true));
         fd_shuffle1.push_back(InjectionArc(dev_null, STDERR_FILENO, true));
         fd_shuffle1.push_back(InjectionArc(dev_null, STDIN_FILENO, true));
-        // Adding another element here? Remeber to increase the argument to
+        // Adding another element here? Remember to increase the argument to
         // reserve(), above.
 
         std::copy(fd_shuffle1.begin(), fd_shuffle1.end(),
@@ -821,19 +825,50 @@ static bool GetAppOutputInternal(const CommandLine& cl, char* const envp[],
         ssize_t bytes_read = 1;  // A lie to properly handle |max_output == 0|
                                  // case in the logic below.
 
+        *timed_out = false;
+        const Time timeout_time = Time::Now() +
+            TimeDelta::FromMilliseconds(timeout_milliseconds);
+        int remaining_ms = timeout_milliseconds;
         while (output_buf_left > 0) {
+          struct pollfd poll_struct = { pipe_fd[0], POLLIN, 0 };
+          const int poll_ret_code = poll(&poll_struct, 1, remaining_ms);
+
+          // poll() should only fail due to interrupt.
+          DCHECK(poll_ret_code != -1 || errno == EINTR);
+          if (poll_ret_code == 0) {
+            *timed_out = true;
+            break;
+          }
+
           bytes_read = HANDLE_EINTR(read(pipe_fd[0], buffer,
                                     std::min(output_buf_left, sizeof(buffer))));
           if (bytes_read <= 0)
             break;
           output->append(buffer, bytes_read);
           output_buf_left -= static_cast<size_t>(bytes_read);
+
+          // Update remaining_ms if we're not using an infinite timeout.
+          if (timeout_milliseconds >= 0) {
+            remaining_ms = (timeout_time - Time::Now()).InMilliseconds();
+            if (remaining_ms < 0) {
+              *timed_out = true;
+              break;
+            }
+          }
         }
         close(pipe_fd[0]);
 
-        // Always wait for exit code (even if we know we'll declare success).
-        int exit_code = EXIT_FAILURE;
-        bool success = WaitForExitCode(pid, &exit_code);
+        bool success = false;
+        int exit_code = PROCESS_END_PROCESS_WAS_HUNG;
+        if (*timed_out) {
+          KillProcess(pid, exit_code, true);
+          // This waits up to 60 seconds for process to actually be dead
+          // which means this may overrun the timeout.
+          LOG(ERROR) << "Process "<< pid << " killed by timeout (waited "
+                     << timeout_milliseconds << " ms).";
+        } else {
+          success = WaitForExitCode(pid, &exit_code);
+        }
 
         // If we stopped because we read as much as we wanted, we always declare
         // success (because the child may exit due to |SIGPIPE|).
@@ -849,17 +884,26 @@ static bool GetAppOutputInternal(const CommandLine& cl, char* const envp[],
 
 bool GetAppOutput(const CommandLine& cl, std::string* output) {
   // Run |execve()| with the current environment and store "unlimited" data.
+  bool timed_out;
+  return GetAppOutputInternal(cl, NULL, output,
+                              std::numeric_limits<std::size_t>::max(), true,
+                              &timed_out, base::kNoTimeout);
+}
+
+bool GetAppOutputWithTimeout(const CommandLine& cl, std::string* output,
+                             bool* timed_out, int timeout_milliseconds) {
   return GetAppOutputInternal(cl, NULL, output,
-                              std::numeric_limits<std::size_t>::max(), true);
+                              std::numeric_limits<std::size_t>::max(), true,
+                              timed_out, timeout_milliseconds);
 }
 
-// TODO(viettrungluu): Conceivably, we should have a timeout as well, so we
-// don't hang if what we're calling hangs.
 bool GetAppOutputRestricted(const CommandLine& cl,
                             std::string* output, size_t max_output) {
   // Run |execve()| with the empty environment.
   char* const empty_environ = NULL;
-  return GetAppOutputInternal(cl, &empty_environ, output, max_output, false);
+  bool timed_out;
+  return GetAppOutputInternal(cl, &empty_environ, output, max_output, false,
+                              &timed_out, base::kNoTimeout);
 }
 
 bool WaitForProcessesToExit(const std::wstring& executable_name,
diff --git a/base/process_util_unittest.cc b/base/process_util_unittest.cc
index 481c759..675f38f 100644
--- a/base/process_util_unittest.cc
+++ b/base/process_util_unittest.cc
@@ -436,19 +436,85 @@ TEST_F(ProcessUtilTest, AlterEnvironment) {
   delete[] e;
 }
 
-TEST_F(ProcessUtilTest, GetAppOutput) {
+TEST_F(ProcessUtilTest, GetAppOutput_Success) {
+  std::string output;
+  EXPECT_TRUE(base::GetAppOutput(CommandLine(FilePath(
+      FILE_PATH_LITERAL("true"))), &output));
+  EXPECT_STREQ("", output.c_str());
+
+  CommandLine command_line(FilePath(FILE_PATH_LITERAL("echo")));
+  command_line.AppendLooseValue(L"foobar42");
+  EXPECT_TRUE(base::GetAppOutput(command_line, &output));
+  EXPECT_STREQ("foobar42\n", output.c_str());
+}
+
+TEST_F(ProcessUtilTest, GetAppOutput_Failure) {
   std::string output;
-  EXPECT_TRUE(base::GetAppOutput(CommandLine(FilePath("true")), &output));
+  EXPECT_FALSE(base::GetAppOutput(CommandLine(FilePath(
+      FILE_PATH_LITERAL("false"))), &output));
   EXPECT_STREQ("", output.c_str());
 
-  EXPECT_FALSE(base::GetAppOutput(CommandLine(FilePath("false")), &output));
+  // Make sure we capture output in case of failure.
+  std::vector<std::string> argv;
+  argv.push_back("/bin/sh");
+  argv.push_back("-c");
+  argv.push_back("echo hello && false");
+  EXPECT_FALSE(base::GetAppOutput(CommandLine(argv), &output));
+  EXPECT_STREQ("hello\n", output.c_str());
+}
+
+TEST_F(ProcessUtilTest, GetAppOutputWithTimeout_SuccessWithinTimeout) {
+  CommandLine command_line(FilePath(FILE_PATH_LITERAL("echo")));
+  command_line.AppendLooseValue(L"hello");
+  std::string output;
+  bool timed_out;
+
+  EXPECT_TRUE(base::GetAppOutputWithTimeout(
+      command_line, &output, &timed_out,
+      std::numeric_limits<int>::max()));
+  EXPECT_EQ("hello\n", output);
+  EXPECT_FALSE(timed_out);
+}
 
+TEST_F(ProcessUtilTest, GetAppOutputWithTimeout_FailureWithinTimeout) {
   std::vector<std::string> argv;
-  argv.push_back("/bin/echo");
-  argv.push_back("-n");
-  argv.push_back("foobar42");
-  EXPECT_TRUE(base::GetAppOutput(CommandLine(argv), &output));
-  EXPECT_STREQ("foobar42", output.c_str());
+  argv.push_back("/bin/sh");
+  argv.push_back("-c");
+  argv.push_back("echo hello && false");
+  std::string output;
+  bool timed_out;
+
+  EXPECT_FALSE(base::GetAppOutputWithTimeout(
+      CommandLine(argv), &output, &timed_out,
+      std::numeric_limits<int>::max()));
+  EXPECT_EQ("hello\n", output);
+  EXPECT_FALSE(timed_out);
+}
+
+TEST_F(ProcessUtilTest, WaitForExitCodeWithTimeout) {
+  CommandLine command_line(FilePath(FILE_PATH_LITERAL("sleep")));
+  command_line.AppendLooseValue(L"10000");
+
+  base::ProcessHandle process_handle;
+  EXPECT_TRUE(base::LaunchApp(command_line, false, false,
+                              &process_handle));
+  int exit_code = 42;
+  EXPECT_FALSE(base::WaitForExitCodeWithTimeout(process_handle, &exit_code, 1));
+  EXPECT_EQ(42, exit_code);  // exit_code is unchanged if timeout triggers.
+}
+
+TEST_F(ProcessUtilTest, GetAppOutputWithTimeout_TimedOutWhileOutputing) {
+  std::vector<std::string> argv;
+  argv.push_back("/bin/sh");
+  argv.push_back("-c");
+  argv.push_back("echo asleep && sleep 10 && echo awake");
+  std::string output;
+  bool timed_out;
+
+  EXPECT_FALSE(base::GetAppOutputWithTimeout(CommandLine(argv), &output,
+                                             &timed_out, 1000));
+  EXPECT_EQ("asleep\n", output);  // Timed out before printing "awake".
+  EXPECT_TRUE(timed_out);
 }
 
 TEST_F(ProcessUtilTest, GetAppOutputRestricted) {