diff options
| -rw-r--r-- | base/process_util.h | 12 | ||||
| -rw-r--r-- | base/process_util_posix.cc | 66 | ||||
| -rw-r--r-- | base/process_util_unittest.cc | 82 |
3 files changed, 19 insertions, 141 deletions
diff --git a/base/process_util.h b/base/process_util.h index a230f5d..d7ff940 100644 --- a/base/process_util.h +++ b/base/process_util.h @@ -86,9 +86,6 @@ 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, @@ -243,15 +240,6 @@ 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 433a1e1..cae70a5 100644 --- a/base/process_util_posix.cc +++ b/base/process_util_posix.cc @@ -5,7 +5,6 @@ #include <dirent.h> #include <errno.h> #include <fcntl.h> -#include <poll.h> #include <signal.h> #include <stdlib.h> #include <sys/resource.h> @@ -738,10 +737,8 @@ int64 TimeValToMicroseconds(const struct timeval& tv) { return ret; } -// 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 +// 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 // 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 @@ -749,8 +746,7 @@ 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* timed_out, - int timeout_milliseconds) { + bool do_search_path) { int pipe_fd[2]; pid_t pid; InjectiveMultimap fd_shuffle1, fd_shuffle2; @@ -792,7 +788,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? Remember to increase the argument to + // Adding another element here? Remeber to increase the argument to // reserve(), above. std::copy(fd_shuffle1.begin(), fd_shuffle1.end(), @@ -825,50 +821,19 @@ 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]); - 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); - } + // Always wait for exit code (even if we know we'll declare success). + int exit_code = EXIT_FAILURE; + bool 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|). @@ -884,26 +849,17 @@ 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, - timed_out, timeout_milliseconds); + std::numeric_limits<std::size_t>::max(), true); } +// 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; - bool timed_out; - return GetAppOutputInternal(cl, &empty_environ, output, max_output, false, - &timed_out, base::kNoTimeout); + return GetAppOutputInternal(cl, &empty_environ, output, max_output, false); } bool WaitForProcessesToExit(const std::wstring& executable_name, diff --git a/base/process_util_unittest.cc b/base/process_util_unittest.cc index 675f38f..481c759 100644 --- a/base/process_util_unittest.cc +++ b/base/process_util_unittest.cc @@ -436,85 +436,19 @@ TEST_F(ProcessUtilTest, AlterEnvironment) { delete[] e; } -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) { +TEST_F(ProcessUtilTest, GetAppOutput) { std::string output; - EXPECT_FALSE(base::GetAppOutput(CommandLine(FilePath( - FILE_PATH_LITERAL("false"))), &output)); + EXPECT_TRUE(base::GetAppOutput(CommandLine(FilePath("true")), &output)); EXPECT_STREQ("", output.c_str()); - // 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); -} + EXPECT_FALSE(base::GetAppOutput(CommandLine(FilePath("false")), &output)); -TEST_F(ProcessUtilTest, GetAppOutputWithTimeout_FailureWithinTimeout) { std::vector<std::string> argv; - 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); + 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()); } TEST_F(ProcessUtilTest, GetAppOutputRestricted) { |