Replace SIGALRM-based stopping with direct comparison against stop time.

SIGALRM-based stopping was unreliable: we observed cases where the signal never got triggered, causing fuzzing to run indefinitely. PiperOrigin-RevId: 695786141
google · Nov 12, 2024 · 73fe6d8 · 73fe6d8
1 parent 4aa4462
commit 73fe6d8
Show file tree

Hide file tree

Showing 11 changed files with 123 additions and 123 deletions.
diff --git a/centipede/BUILD b/centipede/BUILD
@@ -380,10 +380,10 @@ cc_library(
     hdrs = ["minimize_crash.h"],
     deps = [
         ":centipede_callbacks",
-        ":early_exit",
         ":environment",
         ":mutation_input",
         ":runner_result",
+        ":stop",
         ":thread_pool",
         ":util",
         ":workdir",
@@ -619,7 +619,7 @@ cc_library(
     hdrs = ["command.h"],
     visibility = EXTENDED_API_VISIBILITY,
     deps = [
-        ":early_exit",
+        ":stop",
         ":util",
         "@com_google_absl//absl/base:core_headers",
         "@com_google_absl//absl/log",
@@ -711,16 +711,17 @@ cc_library(
 )
 
 cc_library(
-    name = "early_exit",
-    srcs = ["early_exit.cc"],
-    hdrs = ["early_exit.h"],
+    name = "stop",
+    srcs = ["stop.cc"],
+    hdrs = ["stop.h"],
     linkopts = select({
         "@platforms//os:macos": [],
         "//conditions:default": [
             "-latomic",  # for std::atomic::load()/store().
         ],
     }),
     visibility = PUBLIC_API_VISIBILITY,
+    deps = ["@com_google_absl//absl/time"],
 )
 
 cc_library(
@@ -761,7 +762,6 @@ cc_library(
         ":corpus",
         ":corpus_io",
         ":coverage",
-        ":early_exit",
         ":environment",
         ":feature",
         ":feature_set",
@@ -771,6 +771,7 @@ cc_library(
         ":rusage_profiler",
         ":rusage_stats",
         ":stats",
+        ":stop",
         ":symbol_table",
         ":util",
         ":workdir",
@@ -810,14 +811,14 @@ cc_library(
         ":command",
         ":coverage",
         ":distill",
-        ":early_exit",
         ":environment",
         ":minimize_crash",
         ":pc_info",
         ":periodic_action",
         ":runner_result",
         ":seed_corpus_maker_lib",
         ":stats",
+        ":stop",
         ":thread_pool",
         ":util",
         ":workdir",
@@ -1615,7 +1616,7 @@ cc_test(
     data = [":command_test_helper"],
     deps = [
         ":command",
-        ":early_exit",
+        ":stop",
         ":util",
         "@com_google_absl//absl/log",
         "@com_google_absl//absl/strings",

diff --git a/centipede/centipede.cc b/centipede/centipede.cc
@@ -80,7 +80,6 @@
 #include "./centipede/control_flow.h"
 #include "./centipede/corpus_io.h"
 #include "./centipede/coverage.h"
-#include "./centipede/early_exit.h"
 #include "./centipede/environment.h"
 #include "./centipede/feature.h"
 #include "./centipede/feature_set.h"
@@ -89,6 +88,7 @@
 #include "./centipede/rusage_profiler.h"
 #include "./centipede/rusage_stats.h"
 #include "./centipede/stats.h"
+#include "./centipede/stop.h"
 #include "./centipede/util.h"
 #include "./centipede/workdir.h"
 #include "./common/blob_file.h"
@@ -377,14 +377,14 @@ bool Centipede::RunBatch(
   }
   if (!success && env_.exit_on_crash) {
     LOG(INFO) << "--exit_on_crash is enabled; exiting soon";
-    RequestEarlyExit(EXIT_FAILURE);
+    RequestEarlyStop(EXIT_FAILURE);
     return false;
   }
   CHECK_EQ(batch_result.results().size(), input_vec.size());
   num_runs_ += input_vec.size();
   bool batch_gained_new_coverage = false;
   for (size_t i = 0; i < input_vec.size(); i++) {
-    if (EarlyExitRequested()) break;
+    if (ShouldStop()) break;
     FeatureVec &fv = batch_result.results()[i].mutable_features();
     bool function_filter_passed = function_filter_.filter(fv);
     bool input_gained_new_coverage = fs_.PruneFeaturesAndCountUnseen(fv) != 0;
@@ -429,7 +429,7 @@ void Centipede::LoadShard(const Environment &load_env, size_t shard_index,
   std::vector<ByteArray> inputs_to_rerun;
   auto input_features_callback = [&](ByteArray input,
                                      FeatureVec input_features) {
-    if (EarlyExitRequested()) return;
+    if (ShouldStop()) return;
     if (input_features.empty()) {
       if (rerun) {
         inputs_to_rerun.emplace_back(std::move(input));
@@ -498,7 +498,7 @@ void Centipede::Rerun(std::vector<ByteArray> &to_rerun) {
   // Re-run all inputs for which we don't know their features.
   // Run in batches of at most env_.batch_size inputs each.
   while (!to_rerun.empty()) {
-    if (EarlyExitRequested()) break;
+    if (ShouldStop()) break;
     size_t batch_size = std::min(to_rerun.size(), env_.batch_size);
     std::vector<ByteArray> batch(to_rerun.end() - batch_size, to_rerun.end());
     to_rerun.resize(to_rerun.size() - batch_size);
@@ -748,7 +748,7 @@ void Centipede::FuzzingLoop() {
   size_t new_runs = 0;
   size_t corpus_size_at_last_prune = corpus_.NumActive();
   for (size_t batch_index = 0; batch_index < number_of_batches; batch_index++) {
-    if (EarlyExitRequested()) break;
+    if (ShouldStop()) break;
     CHECK_LT(new_runs, env_.num_runs);
     auto remaining_runs = env_.num_runs - new_runs;
     auto batch_size = std::min(env_.batch_size, remaining_runs);
@@ -809,7 +809,7 @@ void Centipede::ReportCrash(std::string_view binary,
                             const std::vector<ByteArray> &input_vec,
                             const BatchResult &batch_result) {
   CHECK_EQ(input_vec.size(), batch_result.results().size());
-  if (EarlyExitRequested()) return;
+  if (ShouldStop()) return;
 
   if (++num_crashes_ > env_.max_num_crash_reports) return;
 
@@ -867,7 +867,7 @@ void Centipede::ReportCrash(std::string_view binary,
   LOG(INFO) << log_prefix
             << "Executing inputs one-by-one, trying to find the reproducer";
   for (auto input_idx : input_idxs_to_try) {
-    if (EarlyExitRequested()) return;
+    if (ShouldStop()) return;
     const auto &one_input = input_vec[input_idx];
     BatchResult one_input_batch_result;
     if (!user_callbacks_.Execute(binary, {one_input}, one_input_batch_result)) {

diff --git a/centipede/centipede_interface.cc b/centipede/centipede_interface.cc
@@ -51,14 +51,14 @@
 #include "./centipede/command.h"
 #include "./centipede/coverage.h"
 #include "./centipede/distill.h"
-#include "./centipede/early_exit.h"
 #include "./centipede/environment.h"
 #include "./centipede/minimize_crash.h"
 #include "./centipede/pc_info.h"
 #include "./centipede/periodic_action.h"
 #include "./centipede/runner_result.h"
 #include "./centipede/seed_corpus_maker_lib.h"
 #include "./centipede/stats.h"
+#include "./centipede/stop.h"
 #include "./centipede/thread_pool.h"
 #include "./centipede/util.h"
 #include "./centipede/workdir.h"
@@ -74,37 +74,20 @@ namespace centipede {
 
 namespace {
 
-// Sets signal handler for SIGINT and SIGALRM.
-void SetSignalHandlers(absl::Time stop_at) {
-  for (int signum : {SIGINT, SIGALRM}) {
-    struct sigaction sigact = {};
-    sigact.sa_handler = [](int received_signum) {
-      if (received_signum == SIGINT) {
-        ABSL_RAW_LOG(INFO, "Ctrl-C pressed: winding down");
-        RequestEarlyExit(EXIT_FAILURE);  // => abnormal outcome
-      } else if (received_signum == SIGALRM) {
-        ABSL_RAW_LOG(INFO, "Reached --stop_at time: winding down");
-        RequestEarlyExit(EXIT_SUCCESS);  // => expected outcome
-      } else {
-        ABSL_UNREACHABLE();
-      }
-    };
-    sigaction(signum, &sigact, nullptr);
-  }
-
-  if (stop_at != absl::InfiniteFuture()) {
-    const absl::Duration stop_in = stop_at - absl::Now();
-    // Setting an alarm works only if the delay is longer than 1 second.
-    if (stop_in >= absl::Seconds(1)) {
-      LOG(INFO) << "Setting alarm for --stop_at time " << stop_at << " (in "
-                << stop_in << ")";
-      PCHECK(alarm(absl::ToInt64Seconds(stop_in)) == 0) << "Alarm already set";
-    } else {
-      LOG(WARNING) << "Already reached --stop_at time " << stop_at
-                   << " upon starting: winding down immediately";
-      RequestEarlyExit(EXIT_SUCCESS);  // => expected outcome
+// Sets signal handler for SIGINT.
+// TODO(b/378532202): Replace this with a more generic mechanism that allows
+// the called or `CentipedeMain()` to indicate when to stop.
+void SetSignalHandlers() {
+  struct sigaction sigact = {};
+  sigact.sa_handler = [](int received_signum) {
+    if (received_signum == SIGINT) {
+      LOG(INFO) << "Ctrl-C pressed: winding down";
+      RequestEarlyStop(EXIT_FAILURE);
+      return;
     }
-  }
+    ABSL_UNREACHABLE();
+  };
+  sigaction(SIGINT, &sigact, nullptr);
 }
 
 // Runs env.for_each_blob on every blob extracted from env.args.
@@ -135,7 +118,7 @@ int ForEachBlob(const Environment &env) {
       // If this flag gets active use, we may want to define special cases,
       // e.g. if for_each_blob=="cp %P /some/where" we can do it in-process.
       cmd.Execute();
-      if (EarlyExitRequested()) return ExitCode();
+      if (ShouldStop()) return ExitCode();
     }
   }
   return EXIT_SUCCESS;
@@ -577,8 +560,8 @@ int UpdateCorpusDatabaseForFuzzTests(
     LOG(INFO) << "Fuzzing " << fuzztest_config.fuzz_tests[i]
               << "\n\tTest binary: " << env.binary;
 
-    ClearEarlyExitRequest();
-    alarm(absl::ToInt64Seconds(fuzztest_config.GetTimeLimitPerTest()));
+    ClearEarlyStopRequestAndSetStopTime(/*stop_time=*/absl::Now() +
+                                        fuzztest_config.GetTimeLimitPerTest());
     Fuzz(env, binary_info, pcs_file_path, callbacks_factory);
     if (!stats_root_path.empty()) {
       const auto stats_dir = stats_root_path / fuzztest_config.fuzz_tests[i];
@@ -624,8 +607,8 @@ int UpdateCorpusDatabaseForFuzzTests(
 
 int CentipedeMain(const Environment &env,
                   CentipedeCallbacksFactory &callbacks_factory) {
-  ClearEarlyExitRequest();
-  SetSignalHandlers(env.stop_at);
+  ClearEarlyStopRequestAndSetStopTime(env.stop_at);
+  SetSignalHandlers();
 
   if (!env.corpus_to_files.empty()) {
     Centipede::CorpusToFiles(env, env.corpus_to_files);

diff --git a/centipede/command.cc b/centipede/command.cc
@@ -51,7 +51,7 @@
 #include "absl/synchronization/mutex.h"
 #include "absl/time/clock.h"
 #include "absl/time/time.h"
-#include "./centipede/early_exit.h"
+#include "./centipede/stop.h"
 #include "./centipede/util.h"
 #include "./common/logging.h"
 
@@ -399,7 +399,7 @@ int Command::Execute() {
   } else if (WIFSIGNALED(exit_code)) {
     const auto signal = WTERMSIG(exit_code);
     if (signal == SIGINT) {
-      RequestEarlyExit(EXIT_FAILURE);
+      RequestEarlyStop(EXIT_FAILURE);
       // When the user kills Centipede via ^C, they are unlikely to be
       // interested in any of the subprocesses' outputs. Also, ^C terminates all
       // the subprocesses, including all the runners, so all their outputs would

diff --git a/centipede/command_test.cc b/centipede/command_test.cc
@@ -26,7 +26,7 @@
 #include "absl/log/log.h"
 #include "absl/strings/substitute.h"
 #include "absl/time/time.h"
-#include "./centipede/early_exit.h"
+#include "./centipede/stop.h"
 #include "./centipede/util.h"
 #include "./common/test_util.h"
 
@@ -51,12 +51,12 @@ TEST(CommandTest, Execute) {
   // Check for default exit code.
   Command echo("echo");
   EXPECT_EQ(echo.Execute(), 0);
-  EXPECT_FALSE(EarlyExitRequested());
+  EXPECT_FALSE(ShouldStop());
 
   // Check for exit code 7.
   Command exit7("bash -c 'exit 7'");
   EXPECT_EQ(exit7.Execute(), 7);
-  EXPECT_FALSE(EarlyExitRequested());
+  EXPECT_FALSE(ShouldStop());
 }
 
 TEST(CommandDeathTest, Execute) {
@@ -65,12 +65,12 @@ TEST(CommandDeathTest, Execute) {
   const auto self_sigint_lambda = []() {
     Command self_sigint("bash -c 'kill -SIGINT $$'");
     self_sigint.Execute();
-    if (EarlyExitRequested()) {
-      LOG(INFO) << "Early exit requested";
+    if (ShouldStop()) {
+      LOG(INFO) << "Early stop requested";
       exit(ExitCode());
     }
   };
-  EXPECT_DEATH(self_sigint_lambda(), "Early exit requested");
+  EXPECT_DEATH(self_sigint_lambda(), "Early stop requested");
 }
 
 TEST(CommandTest, InputFileWildCard) {

diff --git a/centipede/early_exit.h b/centipede/early_exit.h
diff --git a/centipede/minimize_crash.cc b/centipede/minimize_crash.cc
@@ -27,10 +27,10 @@
 #include "absl/log/log.h"
 #include "absl/synchronization/mutex.h"
 #include "./centipede/centipede_callbacks.h"
-#include "./centipede/early_exit.h"
 #include "./centipede/environment.h"
 #include "./centipede/mutation_input.h"
 #include "./centipede/runner_result.h"
+#include "./centipede/stop.h"
 #include "./centipede/thread_pool.h"
 #include "./centipede/util.h"
 #include "./centipede/workdir.h"
@@ -97,7 +97,7 @@ static void MinimizeCrash(const Environment &env,
   size_t num_batches = env.num_runs / env.batch_size;
   for (size_t i = 0; i < num_batches; ++i) {
     LOG_EVERY_POW_2(INFO) << "[" << i << "] Minimizing... Interrupt to stop";
-    if (EarlyExitRequested()) break;
+    if (ShouldStop()) break;
     // Get up to kMaxNumCrashersToGet most recent crashers. We don't want just
     // the most recent crasher to avoid being stuck in local minimum.
     constexpr size_t kMaxNumCrashersToGet = 20;