From c02bb6a98ffc10d686a0992e9cd94ba8d4a5f445 Mon Sep 17 00:00:00 2001 From: Filip Niksic Date: Mon, 2 Dec 2024 13:44:08 -0800 Subject: [PATCH] Add the ability to specify environment variables to unset in the child process. Not to clutter the interface of `Command`, I combined the constructor's parameters into an options struct. #Centipede PiperOrigin-RevId: 702073162 --- .../customized_centipede.cc | 7 +- centipede/binary_info.cc | 12 ++-- centipede/centipede.cc | 14 ++-- centipede/centipede_callbacks.cc | 41 ++++++----- centipede/command.cc | 68 ++++++++++--------- centipede/command.h | 60 +++++++++------- centipede/command_test.cc | 50 ++++++++------ centipede/control_flow.cc | 5 +- centipede/symbol_table.cc | 22 +++--- 9 files changed, 153 insertions(+), 126 deletions(-) diff --git a/centipede/batch_fuzz_example/customized_centipede.cc b/centipede/batch_fuzz_example/customized_centipede.cc index 64d7b30a..74f9451c 100644 --- a/centipede/batch_fuzz_example/customized_centipede.cc +++ b/centipede/batch_fuzz_example/customized_centipede.cc @@ -22,6 +22,7 @@ #include #include // NOLINT #include +#include #include #include "absl/log/check.h" @@ -148,7 +149,11 @@ bool CustomizedCallbacks::Execute(std::string_view binary, } // Execute. - Command cmd{env_.binary, args, env, tmp_log_filepath, tmp_log_filepath}; + Command cmd{env_.binary, + {.args = std::move(args), + .env_add = std::move(env), + .stdout_file = tmp_log_filepath, + .stderr_file = tmp_log_filepath}}; const int retval = cmd.Execute(); std::string tmp_log; diff --git a/centipede/binary_info.cc b/centipede/binary_info.cc index 396ef936..d2d5f3c1 100644 --- a/centipede/binary_info.cc +++ b/centipede/binary_info.cc @@ -57,12 +57,12 @@ void BinaryInfo::InitializeFromSanCovBinary( ScopedFile log_path(tmp_dir_path, "binary_info_log_tmp"); LOG(INFO) << __func__ << ": tmp_dir: " << tmp_dir; - Command cmd( - binary_path_with_args, {}, - {absl::StrCat("CENTIPEDE_RUNNER_FLAGS=:dump_binary_info:arg1=", - pc_table_path.path(), ":arg2=", cf_table_path.path(), - ":arg3=", dso_table_path.path(), ":")}, - log_path.path()); + Command cmd(binary_path_with_args, + {.env_add = {absl::StrCat( + "CENTIPEDE_RUNNER_FLAGS=:dump_binary_info:arg1=", + pc_table_path.path(), ":arg2=", cf_table_path.path(), + ":arg3=", dso_table_path.path(), ":")}, + .stdout_file = std::string(log_path.path())}); int exit_code = cmd.Execute(); if (exit_code != EXIT_SUCCESS) { LOG(INFO) << __func__ << ": exit_code: " << exit_code; diff --git a/centipede/centipede.cc b/centipede/centipede.cc index 181343d5..492e5b71 100644 --- a/centipede/centipede.cc +++ b/centipede/centipede.cc @@ -119,8 +119,9 @@ Centipede::Centipede(const Environment &env, CentipedeCallbacks &user_callbacks, stats_(stats), input_filter_path_(std::filesystem::path(TemporaryLocalDirPath()) .append("filter-input")), - input_filter_cmd_(env_.input_filter, {input_filter_path_}, {/*env*/}, - "/dev/null", "/dev/null"), + input_filter_cmd_(env_.input_filter, {.args = {input_filter_path_}, + .stdout_file = "/dev/null", + .stderr_file = "/dev/null"}), rusage_profiler_( /*scope=*/perf::RUsageScope::ThisProcess(), /*metrics=*/env.DumpRUsageTelemetryInThisShard() @@ -554,7 +555,7 @@ void Centipede::GenerateSourceBasedCoverageReport( merge_arguments.push_back(raw_profile); } - Command merge_command("llvm-profdata", merge_arguments); + Command merge_command("llvm-profdata", {.args = std::move(merge_arguments)}); if (merge_command.Execute() != EXIT_SUCCESS) { LOG(ERROR) << "Failed to run command " << merge_command.ToString(); return; @@ -562,9 +563,10 @@ void Centipede::GenerateSourceBasedCoverageReport( Command generate_report_command( "llvm-cov", - {"show", "-format=html", absl::StrCat("-output-dir=", report_path), - absl::StrCat("-instr-profile=", indexed_profile_path), - env_.clang_coverage_binary}); + {.args = {"show", "-format=html", + absl::StrCat("-output-dir=", report_path), + absl::StrCat("-instr-profile=", indexed_profile_path), + env_.clang_coverage_binary}}); if (generate_report_command.Execute() != EXIT_SUCCESS) { LOG(ERROR) << "Failed to run command " << generate_report_command.ToString(); diff --git a/centipede/centipede_callbacks.cc b/centipede/centipede_callbacks.cc index f31cfc30..6857a9e1 100644 --- a/centipede/centipede_callbacks.cc +++ b/centipede/centipede_callbacks.cc @@ -21,6 +21,7 @@ #include #include #include // NOLINT +#include #include #include "absl/log/check.h" @@ -137,13 +138,13 @@ Command &CentipedeCallbacks::GetOrCreateCommandForBinary( env_.timeout_per_batch == 0 ? absl::InfiniteDuration() : absl::Seconds(env_.timeout_per_batch) + absl::Seconds(5); - Command &cmd = commands_.emplace_back(Command( - /*path=*/binary, /*args=*/{}, - /*env=*/env, - /*out=*/execute_log_path_, - /*err=*/execute_log_path_, - /*timeout=*/amortized_timeout, - /*temp_file_path=*/temp_input_file_path_)); + Command &cmd = commands_.emplace_back( + Command{binary, + {.env_add = std::move(env), + .stdout_file = execute_log_path_, + .stderr_file = execute_log_path_, + .timeout = amortized_timeout, + .temp_file_path = temp_input_file_path_}}); if (env_.fork_server) cmd.StartForkServer(temp_dir_, Hash(binary)); return cmd; @@ -225,13 +226,12 @@ bool CentipedeCallbacks::GetSeedsViaExternalBinary( CHECK(!error); Command cmd{binary, - {}, - {absl::StrCat("CENTIPEDE_RUNNER_FLAGS=:dump_seed_inputs:arg1=", - output_dir.string(), ":")}, - /*out=*/execute_log_path_, - /*err=*/execute_log_path_, - /*timeout=*/absl::InfiniteDuration(), - /*temp_file_path=*/temp_input_file_path_}; + {.env_add = {absl::StrCat( + "CENTIPEDE_RUNNER_FLAGS=:dump_seed_inputs:arg1=", + output_dir.string(), ":")}, + .stdout_file = execute_log_path_, + .stderr_file = execute_log_path_, + .temp_file_path = temp_input_file_path_}}; const int retval = cmd.Execute(); std::vector seed_input_filenames; @@ -261,13 +261,12 @@ bool CentipedeCallbacks::GetSerializedTargetConfigViaExternalBinary( const auto config_file_path = std::filesystem::path{temp_dir_} / "configuration"; Command cmd{binary, - {}, - {absl::StrCat("CENTIPEDE_RUNNER_FLAGS=:dump_configuration:arg1=", - config_file_path.string(), ":")}, - /*out=*/execute_log_path_, - /*err=*/execute_log_path_, - /*timeout=*/absl::InfiniteDuration(), - /*temp_file_path=*/temp_input_file_path_}; + {.env_add = {absl::StrCat( + "CENTIPEDE_RUNNER_FLAGS=:dump_configuration:arg1=", + config_file_path.string(), ":")}, + .stdout_file = execute_log_path_, + .stderr_file = execute_log_path_, + .temp_file_path = temp_input_file_path_}}; const bool is_success = cmd.Execute() == 0; if (is_success) { diff --git a/centipede/command.cc b/centipede/command.cc index 8b570ca0..1d1084bb 100644 --- a/centipede/command.cc +++ b/centipede/command.cc @@ -144,24 +144,23 @@ struct Command::ForkServerProps { Command::Command(Command &&other) noexcept = default; Command::~Command() = default; -Command::Command(std::string_view path, std::vector args, - std::vector env, std::string_view out, - std::string_view err, absl::Duration timeout, - std::string_view temp_file_path) - : path_(path), - args_(std::move(args)), - env_(std::move(env)), - out_(out), - err_(err), - timeout_(timeout), - temp_file_path_(temp_file_path) {} +Command::Command(std::string_view path, Options options) + : path_(path), options_(std::move(options)) {} + +Command::Command(std::string_view path) : Command(path, {}) {} std::string Command::ToString() const { std::vector ss; + ss.reserve(/*env*/ 1 + options_.env_add.size() + options_.env_remove.size() + + /*path*/ 1 + /*args*/ options_.args.size() + /*out/err*/ 2); // env. - ss.reserve(env_.size()); - for (const auto &env : env_) { - ss.emplace_back(env); + ss.push_back("env"); + // Arguments that unset environment variables must appear first. + for (const auto &var : options_.env_remove) { + ss.push_back(absl::StrCat("-u ", var)); + } + for (const auto &var : options_.env_add) { + ss.push_back(var); } // path. std::string path = path_; @@ -172,23 +171,24 @@ std::string Command::ToString() const { // Replace @@ with temp_file_path_. constexpr std::string_view kTempFileWildCard = "@@"; if (absl::StrContains(path, kTempFileWildCard)) { - CHECK(!temp_file_path_.empty()); - path = absl::StrReplaceAll(path, {{kTempFileWildCard, temp_file_path_}}); + CHECK(!options_.temp_file_path.empty()); + path = absl::StrReplaceAll(path, + {{kTempFileWildCard, options_.temp_file_path}}); } - ss.emplace_back(path); + ss.push_back(std::move(path)); // args. - for (const auto &arg : args_) { - ss.emplace_back(arg); + for (const auto &arg : options_.args) { + ss.push_back(arg); } // out/err. - if (!out_.empty()) { - ss.emplace_back(absl::StrCat("> ", out_)); + if (!options_.stdout_file.empty()) { + ss.push_back(absl::StrCat("> ", options_.stdout_file)); } - if (!err_.empty()) { - if (out_ != err_) { - ss.emplace_back(absl::StrCat("2> ", err_)); + if (!options_.stderr_file.empty()) { + if (options_.stdout_file != options_.stderr_file) { + ss.push_back(absl::StrCat("2> ", options_.stderr_file)); } else { - ss.emplace_back("2>&1"); + ss.push_back("2>&1"); } } // Trim trailing space and return. @@ -314,7 +314,8 @@ int Command::Execute() { int exit_code = EXIT_SUCCESS; if (fork_server_ != nullptr) { - VLOG(1) << "Sending execution request to fork server: " << VV(timeout_); + VLOG(1) << "Sending execution request to fork server: " + << VV(options_.timeout); if (const auto status = VerifyForkServerIsHealthy(); !status.ok()) { LogProblemInfo(absl::StrCat("Fork server should be running, but isn't: ", @@ -331,7 +332,7 @@ int Command::Execute() { // execution result to it). struct pollfd poll_fd = {}; int poll_ret = -1; - auto poll_deadline = absl::Now() + timeout_; + auto poll_deadline = absl::Now() + options_.timeout; // The `poll()` syscall can get interrupted: it sets errno==EINTR in that // case. We should tolerate that. do { @@ -351,7 +352,7 @@ int Command::Execute() { if (poll_ret == 0) { LogProblemInfo( absl::StrCat("Timeout while waiting for fork server: timeout is ", - absl::FormatDuration(timeout_))); + absl::FormatDuration(options_.timeout))); } else { LogProblemInfo(absl::StrCat( "Error while waiting for fork server: poll() returned ", poll_ret)); @@ -423,8 +424,8 @@ int Command::Execute() { std::string Command::ReadRedirectedStdout() const { std::string ret; - if (!out_.empty()) { - ReadFromLocalFile(out_, ret); + if (!options_.stdout_file.empty()) { + ReadFromLocalFile(options_.stdout_file, ret); if (ret.empty()) ret = ""; } return ret; @@ -432,11 +433,12 @@ std::string Command::ReadRedirectedStdout() const { std::string Command::ReadRedirectedStderr() const { std::string ret; - if (!err_.empty()) { - if (err_ == "2>&1" || err_ == out_) { + if (!options_.stderr_file.empty()) { + if (options_.stderr_file == "2>&1" || + options_.stderr_file == options_.stdout_file) { ret = ""; } else { - ReadFromLocalFile(err_, ret); + ReadFromLocalFile(options_.stderr_file, ret); if (ret.empty()) ret = ""; } } diff --git a/centipede/command.h b/centipede/command.h index 48c8557e..b4dddb6f 100644 --- a/centipede/command.h +++ b/centipede/command.h @@ -27,38 +27,51 @@ namespace centipede { class Command final { public: + struct Options { + // Arguments to pass to the executed command. The command is executed by the + // shell, so the arguments need to be shell-escaped. + // TODO(b/381910257): Escape the arguments for passing to the shell. + std::vector args; + // Environment variables/values in the form "KEY=VALUE" to set in the + // subprocess executing the command. These are added to the environment + // variables inherited from the parent process. + std::vector env_add; + // Environment variables to unset in the subprocess executing the command. + std::vector env_remove; + // Redirect stdout to this file. If empty, use parent's STDOUT. + std::string stdout_file; + // Redirect stderr to this file. If empty, use parent's STDERR. If `out` == + // `err` and both are non-empty, stdout/stderr are combined. + std::string stderr_file; + // Terminate a fork server execution attempt after this duration. + absl::Duration timeout = absl::InfiniteDuration(); + // "@@" in the command will be replaced with `temp_file_path`. + std::string temp_file_path; + }; + + // Constructs a command to run the binary at `path` with the given `options`. + // The path can contain "@@" which will be replaced with + // `options.temp_file_path`. + explicit Command(std::string_view path, Options options); + + // Constructs a command to run the binary at `path` with default options. + explicit Command(std::string_view path); + // Move-constructible only. Command(const Command& other) = delete; Command& operator=(const Command& other) = delete; Command(Command&& other) noexcept; Command& operator=(Command&& other) noexcept = delete; - // Constructs a command: - // `path`: path to the binary. - // `args`: arguments. - // `env`: environment variables/values (in the form "KEY=VALUE"). - // `out`: stdout redirect path (empty means use parent's STDOUT). - // `err`: stderr redirect path (empty means use parent's STDERR). - // `timeout`: terminate a fork server execution attempt after this duration. - // `temp_file_path`: "@@" in `path` will be replaced with `temp_file_path`. - // If `out` == `err` and both are non-empty, stdout/stderr are combined. - // TODO(ussuri): The number of parameters became untenable and error-prone. - // Use the Options or Builder pattern instead. - explicit Command(std::string_view path, std::vector args = {}, - std::vector env = {}, std::string_view out = "", - std::string_view err = "", - absl::Duration timeout = absl::InfiniteDuration(), - std::string_view temp_file_path = ""); - // Cleans up the fork server, if that was created. ~Command(); // Returns a string representing the command, e.g. like this - // "ENV1=VAL1 path arg1 arg2 > out 2>& err" + // "env -u ENV1 ENV2=VAL2 path arg1 arg2 > out 2>& err" std::string ToString() const; // Executes the command, returns the exit status. // Can be called more than once. - // If interrupted, may call RequestEarlyExit(). + // If interrupted, may call `RequestEarlyStop()` (see stop.h). int Execute(); // Attempts to start a fork server, returns true on success. @@ -86,19 +99,14 @@ class Command final { std::string ReadRedirectedStderr() const; // Possibly logs information about a crash, starting with `message`, followed // by the the command line, followed by the redirected stdout and stderr read - // from `out_` and `err_` files, if any. + // from `options_.out` and `options_.err` files, if any. void LogProblemInfo(std::string_view message) const; // Just as `LogCrashInfo()`, but logging occurs only when the VLOG level (set // via `--v` or its equivalents) is >= `min_vlog`. void VlogProblemInfo(std::string_view message, int vlog_level) const; const std::string path_; - const std::vector args_; - const std::vector env_; - const std::string out_; - const std::string err_; - const absl::Duration timeout_; - const std::string temp_file_path_; + const Options options_; const std::string command_line_ = ToString(); std::unique_ptr fork_server_; diff --git a/centipede/command_test.cc b/centipede/command_test.cc index 2578e0ad..96864d11 100644 --- a/centipede/command_test.cc +++ b/centipede/command_test.cc @@ -34,17 +34,22 @@ namespace centipede { namespace { TEST(CommandTest, ToString) { - EXPECT_EQ(Command("x").ToString(), "x"); - EXPECT_EQ(Command("path", {"arg1", "arg2"}).ToString(), - "path \\\narg1 \\\narg2"); - EXPECT_EQ(Command("x", {}, {"K1=V1", "K2=V2"}).ToString(), - "K1=V1 \\\nK2=V2 \\\nx"); - EXPECT_EQ(Command("x", {}, {}, "out").ToString(), "x \\\n> out"); - EXPECT_EQ(Command("x", {}, {}, "", "err").ToString(), "x \\\n2> err"); - EXPECT_EQ(Command("x", {}, {}, "out", "err").ToString(), - "x \\\n> out \\\n2> err"); - EXPECT_EQ(Command("x", {}, {}, "out", "out").ToString(), - "x \\\n> out \\\n2>&1"); + EXPECT_EQ(Command("x").ToString(), "env \\\nx"); + EXPECT_EQ(Command("path", {.args = {"arg1", "arg2"}}).ToString(), + "env \\\npath \\\narg1 \\\narg2"); + EXPECT_EQ(Command("x", {.env_add = {"K1=V1", "K2=V2"}, .env_remove = {"K3"}}) + .ToString(), + "env \\\n-u K3 \\\nK1=V1 \\\nK2=V2 \\\nx"); + EXPECT_EQ(Command("x", {.stdout_file = "out"}).ToString(), + "env \\\nx \\\n> out"); + EXPECT_EQ(Command("x", {.stderr_file = "err"}).ToString(), + "env \\\nx \\\n2> err"); + EXPECT_EQ( + Command("x", {.stdout_file = "out", .stderr_file = "err"}).ToString(), + "env \\\nx \\\n> out \\\n2> err"); + EXPECT_EQ( + Command("x", {.stdout_file = "out", .stderr_file = "out"}).ToString(), + "env \\\nx \\\n> out \\\n2>&1"); } TEST(CommandTest, Execute) { @@ -74,9 +79,9 @@ TEST(CommandDeathTest, Execute) { } TEST(CommandTest, InputFileWildCard) { - std::string_view command_line = "foo bar @@ baz"; - Command cmd(command_line, {}, {}, "", "", absl::Seconds(2), "TEMP_FILE"); - EXPECT_EQ(cmd.ToString(), "foo bar TEMP_FILE baz"); + Command cmd("foo bar @@ baz", + {.timeout = absl::Seconds(2), .temp_file_path = "TEMP_FILE"}); + EXPECT_EQ(cmd.ToString(), "env \\\nfoo bar TEMP_FILE baz"); } TEST(CommandTest, ForkServer) { @@ -89,7 +94,8 @@ TEST(CommandTest, ForkServer) { { const std::string input = "success"; const std::string log = std::filesystem::path{test_tmpdir} / input; - Command cmd(helper, {input}, {}, log, log); + Command cmd(helper, + {.args = {input}, .stdout_file = log, .stderr_file = log}); EXPECT_TRUE(cmd.StartForkServer(test_tmpdir, "ForkServer")); EXPECT_EQ(cmd.Execute(), EXIT_SUCCESS); std::string log_contents; @@ -100,7 +106,8 @@ TEST(CommandTest, ForkServer) { { const std::string input = "fail"; const std::string log = std::filesystem::path{test_tmpdir} / input; - Command cmd(helper, {input}, {}, log, log); + Command cmd(helper, + {.args = {input}, .stdout_file = log, .stderr_file = log}); EXPECT_TRUE(cmd.StartForkServer(test_tmpdir, "ForkServer")); EXPECT_EQ(cmd.Execute(), EXIT_FAILURE); std::string log_contents; @@ -111,7 +118,8 @@ TEST(CommandTest, ForkServer) { { const std::string input = "ret42"; const std::string log = std::filesystem::path{test_tmpdir} / input; - Command cmd(helper, {input}, {}, log, log); + Command cmd(helper, + {.args = {input}, .stdout_file = log, .stderr_file = log}); EXPECT_TRUE(cmd.StartForkServer(test_tmpdir, "ForkServer")); EXPECT_EQ(cmd.Execute(), 42); std::string log_contents; @@ -122,7 +130,8 @@ TEST(CommandTest, ForkServer) { { const std::string input = "abort"; const std::string log = std::filesystem::path{test_tmpdir} / input; - Command cmd(helper, {input}, {}, log, log); + Command cmd(helper, + {.args = {input}, .stdout_file = log, .stderr_file = log}); EXPECT_TRUE(cmd.StartForkServer(test_tmpdir, "ForkServer")); // WTERMSIG() needs an lvalue on some platforms. const int ret = cmd.Execute(); @@ -136,7 +145,10 @@ TEST(CommandTest, ForkServer) { const std::string input = "hang"; const std::string log = std::filesystem::path{test_tmpdir} / input; constexpr auto kTimeout = absl::Seconds(2); - Command cmd(helper, {input}, {}, log, log, kTimeout); + Command cmd(helper, {.args = {input}, + .stdout_file = log, + .stderr_file = log, + .timeout = kTimeout}); ASSERT_TRUE(cmd.StartForkServer(test_tmpdir, "ForkServer")); EXPECT_EQ(cmd.Execute(), EXIT_FAILURE); std::string log_contents; diff --git a/centipede/control_flow.cc b/centipede/control_flow.cc index 78ebb9aa..729388ad 100644 --- a/centipede/control_flow.cc +++ b/centipede/control_flow.cc @@ -59,8 +59,9 @@ PCTable GetPcTableFromBinaryWithTracePC(std::string_view binary_path, std::string_view objdump_path, std::string_view tmp_path) { const std::string stderr_path = absl::StrCat(tmp_path, ".log"); - Command cmd(objdump_path, {"-d", std::string(binary_path)}, {}, tmp_path, - stderr_path); + Command cmd(objdump_path, {.args = {"-d", std::string(binary_path)}, + .stdout_file = std::string(tmp_path), + .stderr_file = stderr_path}); int exit_code = cmd.Execute(); if (exit_code != EXIT_SUCCESS) { std::string log_text; diff --git a/centipede/symbol_table.cc b/centipede/symbol_table.cc index ad849f44..073951de 100644 --- a/centipede/symbol_table.cc +++ b/centipede/symbol_table.cc @@ -115,18 +115,16 @@ void SymbolTable::GetSymbolsFromOneDso(absl::Span pc_infos, // Run the symbolizer. LOG(INFO) << "Symbolizing " << pc_infos.size() << " PCs from " << dso_basename; - Command cmd{ - symbolizer_path, - { - "--no-inlines", - "-e", - std::string(dso_path), - "<", - std::string(pcs_file.path()), - }, - /*env=*/{}, - symbols_file.path(), - }; + Command cmd{symbolizer_path, + {.args = + { + "--no-inlines", + "-e", + std::string(dso_path), + "<", + std::string(pcs_file.path()), + }, + .stdout_file = std::string(symbols_file.path())}}; int exit_code = cmd.Execute(); if (exit_code != EXIT_SUCCESS) { LOG(ERROR) << "Symbolization failed, debug symbols will not be used: "