Report programs that can't be started On POSIX, ExecProcess() forks and calls execvp() in the child, which exits with 127 when that fails. So a program that couldn't be started looked like one that ran and failed: a missing ninja was reported as "Ninja has quit with exit code 127", without naming it, and a missing exec_script() interpreter as a script that returned a non-zero exit code. Have the child send the error of execvp(), or of its setup before it, through a pipe that's closed when the program starts. Also, return why the program couldn't be started in std_err, and show it in the errors. A program killed by a signal also made ExecProcess() return false, which callers reported as a program that couldn't be executed. Report it as exiting with 128 plus the signal instead, like shells do. When exec_script() couldn't run its command, the error always said it was trying to execute the interpreter. Show the program that was actually run, and say whether it was the interpreter or the script. Change-Id: I878e9afa1e03b340c0623d416c814ae7a30035f5 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/28641 Reviewed-by: Andrew Grieve <agrieve@google.com> Reviewed-by: David Turner <digit@google.com> Commit-Queue: David Turner <digit@google.com>
diff --git a/src/gn/exec_process.cc b/src/gn/exec_process.cc index 26c24e9..0b9da04 100644 --- a/src/gn/exec_process.cc +++ b/src/gn/exec_process.cc
@@ -7,15 +7,18 @@ #include <stddef.h> #include <memory> +#include <tuple> #include "base/command_line.h" #include "base/files/file_util.h" #include "base/logging.h" +#include "gn/filesystem_utils.h" #include "util/build_config.h" #if defined(OS_WIN) #include <windows.h> +#include "base/strings/string_util.h" #include "base/win/scoped_handle.h" #include "base/win/scoped_process_information.h" #include "base/win/win_util.h" @@ -34,6 +37,12 @@ namespace internal { +std::string DescribeStartFailure(const base::CommandLine& cmdline, + const std::string& std_err) { + return "When trying to execute \"" + FilePathToUTF8(cmdline.GetProgram()) + + "\":\n " + std_err; +} + #if defined(OS_WIN) bool ExecProcess(const base::CommandLine& cmdline, const base::FilePath& startup_dir, @@ -105,6 +114,13 @@ TRUE, // Handles are inherited. NORMAL_PRIORITY_CLASS, nullptr, base::ToWCharT(&startup_dir.value()), &start_info, &temp_process_info)) { + *std_err = + logging::SystemErrorCodeToString(logging::GetLastSystemErrorCode()); + // Some messages have a "%1" placeholder for the program. + base::ReplaceSubstringsAfterOffset( + std_err, 0, "%1", + FilePathToUTF8( + base::CommandLine::FromString(cmdline_str).GetProgram())); return false; } base::win::ScopedProcessInformation proc_info(temp_process_info); @@ -155,22 +171,38 @@ return true; } -bool WaitForExit(int pid, int* exit_code) { +namespace { + +// Sets `std_err` to the description of `error`, and returns false. +bool FailWith(int error, std::string* std_err) { + *std_err = logging::SystemErrorCodeToString(error); + return false; +} + +// Called in the child of ExecProcess() when the program can't be started: +// sends `errno` to the parent through `fd`, and exits. +[[noreturn]] void ExitWithErrno(int fd) { + int error = errno; + std::ignore = HANDLE_EINTR(write(fd, &error, sizeof(error))); + _exit(127); +} + +} // namespace + +bool WaitForExit(int pid, int* exit_code, std::string* std_err) { int status; - if (waitpid(pid, &status, 0) < 0) { - PLOG(ERROR) << "waitpid"; - return false; - } + if (waitpid(pid, &status, 0) < 0) + return FailWith(errno, std_err); if (WIFEXITED(status)) { *exit_code = WEXITSTATUS(status); return true; - } else if (WIFSIGNALED(status)) { - if (WTERMSIG(status) == SIGINT || WTERMSIG(status) == SIGTERM || - WTERMSIG(status) == SIGHUP) - return false; } - return false; + + // Otherwise, it was killed by a signal. Like shells do, report it like any + // other failure. + *exit_code = 128 + WTERMSIG(status); + return true; } bool ExecProcess(const base::CommandLine& cmdline, @@ -191,19 +223,32 @@ fd_shuffle2.reserve(3); if (pipe(out_fd) < 0) - return false; + return FailWith(errno, std_err); base::ScopedFD out_read(out_fd[0]), out_write(out_fd[1]); if (pipe(err_fd) < 0) - return false; + return FailWith(errno, std_err); base::ScopedFD err_read(err_fd[0]), err_write(err_fd[1]); + // select() can't wait for them. if (out_read.get() >= FD_SETSIZE || err_read.get() >= FD_SETSIZE) - return false; + return FailWith(EMFILE, std_err); + + // If the program can't be started, the child writes `errno` to this pipe. + // Both ends are closed on exec, so the parent reads end of file when the + // program starts. + int exec_fd[2]; + if (pipe(exec_fd) < 0) + return FailWith(errno, std_err); + base::ScopedFD exec_read(exec_fd[0]), exec_write(exec_fd[1]); + if (!base::SetCloseOnExec(exec_read.get()) || + !base::SetCloseOnExec(exec_write.get())) { + return FailWith(errno, std_err); + } switch (pid = fork()) { case -1: // error - return false; + return FailWith(errno, std_err); case 0: // child { #if defined(OS_MAC) @@ -226,7 +271,7 @@ // in the child. int dev_null = open("/dev/null", O_WRONLY); if (dev_null < 0) - _exit(127); + ExitWithErrno(exec_write.get()); fd_shuffle1.push_back( base::InjectionArc(out_write.get(), STDOUT_FILENO, true)); @@ -241,9 +286,10 @@ fd_shuffle2.push_back(fd_shuffle1[i]); if (!ShuffleFileDescriptors(&fd_shuffle1)) - _exit(127); + ExitWithErrno(exec_write.get()); - base::SetCurrentDirectory(startup_dir); + if (!base::SetCurrentDirectory(startup_dir)) + ExitWithErrno(exec_write.get()); // TODO(brettw) the base version GetAppOutput does a // CloseSuperfluousFds call here. Do we need this? @@ -253,18 +299,27 @@ argv_cstr[i] = const_cast<char*>(argv[i].c_str()); argv_cstr[argv.size()] = nullptr; execvp(argv_cstr[0], argv_cstr.get()); - _exit(127); + ExitWithErrno(exec_write.get()); // Avoid `fall through` warning on some gcc versions. break; } default: // parent { - // Close our writing end of pipe now. Otherwise later read would not - // be able to detect end of child's output (in theory we could still - // write to the pipe). + // Close our writing ends of the pipes now. Otherwise later reads would + // not be able to detect their end (in theory we could still write to + // them). + exec_write.reset(); out_write.reset(); err_write.reset(); + // Wait for the child to start the program, or to send why it couldn't. + int error; + if (HANDLE_EINTR(read(exec_read.get(), &error, sizeof(error))) == + sizeof(error)) { + HANDLE_EINTR(waitpid(pid, nullptr, 0)); + return FailWith(error, std_err); + } + bool out_open = true, err_open = true; while (out_open || err_open) { fd_set read_fds; @@ -282,7 +337,7 @@ err_open = ReadFromPipe(err_read.get(), std_err); } - return WaitForExit(pid, exit_code); + return WaitForExit(pid, exit_code, std_err); } }
diff --git a/src/gn/exec_process.h b/src/gn/exec_process.h index 91ee876..28d62ab 100644 --- a/src/gn/exec_process.h +++ b/src/gn/exec_process.h
@@ -16,6 +16,9 @@ namespace internal { +// Runs `cmdline` in `startup_dir`, returning what it prints in `std_out` and +// `std_err`, and its exit code in `exit_code`. If the program can't be +// started, returns false with the reason in `std_err`. bool ExecProcess(const base::CommandLine& cmdline, const base::FilePath& startup_dir, std::string* std_out, @@ -30,6 +33,11 @@ int* exit_code); #endif // OS_WIN +// Describes why the program of `cmdline` couldn't be run, from the `std_err` +// that ExecProcess() returned along with false. +std::string DescribeStartFailure(const base::CommandLine& cmdline, + const std::string& std_err); + } // namespace internal #endif // TOOLS_GN_EXEC_PROCESS_H_
diff --git a/src/gn/exec_process_unittest.cc b/src/gn/exec_process_unittest.cc index 599892d..317f760 100644 --- a/src/gn/exec_process_unittest.cc +++ b/src/gn/exec_process_unittest.cc
@@ -5,13 +5,21 @@ #include "gn/exec_process.h" #include "base/command_line.h" +#include "base/files/file_util.h" #include "base/files/scoped_temp_dir.h" +#include "base/logging.h" #include "base/strings/string_util.h" +#include "gn/filesystem_utils.h" #include "util/build_config.h" #include "util/test/test.h" #if defined(OS_WIN) +#include <windows.h> + #include "base/strings/utf_string_conversions.h" +#else +#include <errno.h> +#include <signal.h> #endif namespace internal { @@ -120,4 +128,68 @@ EXPECT_EQ(10001u, std_err.size()); } #endif + +namespace { + +// Expects `program` not to start because of `error`, which `std_err` reports. +void ExpectStartFailure(const base::FilePath& program, + logging::SystemErrorCode error) { + base::CommandLine cmdline(program); + std::string std_out, std_err; + int exit_code; + EXPECT_FALSE( + ExecProcess(cmdline, program.DirName(), &std_out, &std_err, &exit_code)); + std::string reason = logging::SystemErrorCodeToString(error); +#if defined(OS_WIN) + base::ReplaceSubstringsAfterOffset(&reason, 0, "%1", FilePathToUTF8(program)); +#endif + EXPECT_EQ(reason, std_err); +} + +} // namespace + +// A program that can't be started is a failure, rather than a process that +// exited with an error. +TEST(ExecProcessTest, MissingProgram) { + base::ScopedTempDir temp_dir; + ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); + base::FilePath program = + temp_dir.GetPath().Append(FILE_PATH_LITERAL("missing")); +#if defined(OS_WIN) + ExpectStartFailure(program, ERROR_FILE_NOT_FOUND); +#else + ExpectStartFailure(program, ENOENT); +#endif +} + +// A file that exists but can't be run fails for another reason. +TEST(ExecProcessTest, ProgramNotExecutable) { + base::ScopedTempDir temp_dir; + ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); + // It's not a valid program on Windows, and not executable on POSIX. Without + // the extension, CreateProcess() would look for "not_executable.exe". + base::FilePath program = + temp_dir.GetPath().Append(FILE_PATH_LITERAL("not_executable.exe")); + ASSERT_EQ(0, base::WriteFile(program, "", 0)); +#if defined(OS_WIN) + ExpectStartFailure(program, ERROR_BAD_EXE_FORMAT); +#else + ExpectStartFailure(program, EACCES); +#endif +} + +#if !defined(OS_WIN) +// A program killed by a signal exits with 128 plus the signal, like in shells. +TEST(ExecProcessTest, ProgramKilledBySignal) { + base::CommandLine cmdline(base::FilePath("/bin/sh")); + cmdline.AppendArg("-c"); + cmdline.AppendArg("kill -KILL $$"); + std::string std_out, std_err; + int exit_code; + EXPECT_TRUE(ExecProcess(cmdline, base::FilePath("/"), &std_out, &std_err, + &exit_code)); + EXPECT_EQ(128 + SIGKILL, exit_code); +} +#endif + } // namespace internal
diff --git a/src/gn/function_exec_script.cc b/src/gn/function_exec_script.cc index 966dbfc..b7afdc8 100644 --- a/src/gn/function_exec_script.cc +++ b/src/gn/function_exec_script.cc
@@ -233,9 +233,11 @@ { if (!internal::ExecProcess(cmdline, startup_dir, &output, &stderr_output, &exit_code)) { - *err = Err(function->function(), "Could not execute interpreter.", - "I was trying to execute \"" + - FilePathToUTF8(interpreter_path) + "\"."); + // Without an interpreter, the script itself was run. + *err = Err(function->function(), + interpreter_path.empty() ? "Could not execute script." + : "Could not execute interpreter.", + internal::DescribeStartFailure(cmdline, stderr_output)); return Value(); } }
diff --git a/src/gn/invoke_python.cc b/src/gn/invoke_python.cc index 52db756..1e79e1b 100644 --- a/src/gn/invoke_python.cc +++ b/src/gn/invoke_python.cc
@@ -10,7 +10,6 @@ #include "gn/build_settings.h" #include "gn/err.h" #include "gn/exec_process.h" -#include "gn/filesystem_utils.h" namespace internal { @@ -40,9 +39,8 @@ int exit_code = 0; if (!internal::ExecProcess(cmdline, startup_dir, &output, &stderr_output, &exit_code)) { - *err = - Err(Location(), "Could not execute python.", - "I was trying to execute \"" + FilePathToUTF8(python_path) + "\"."); + *err = Err(Location(), "Could not execute python.", + internal::DescribeStartFailure(cmdline, stderr_output)); return false; }
diff --git a/src/gn/ninja_tools.cc b/src/gn/ninja_tools.cc index dce4639..8b999f1 100644 --- a/src/gn/ninja_tools.cc +++ b/src/gn/ninja_tools.cc
@@ -11,7 +11,6 @@ #include "base/strings/string_number_conversions.h" #include "gn/err.h" #include "gn/exec_process.h" -#include "gn/filesystem_utils.h" namespace { @@ -35,8 +34,7 @@ if (!internal::ExecProcess(cmdline, startup_dir, output, &stderr_output, &exit_code)) { *err = Err(Location(), "Could not execute Ninja.", - "I was trying to execute \"" + - FilePathToUTF8(cmdline.GetProgram()) + "\"."); + internal::DescribeStartFailure(cmdline, stderr_output)); return false; }