Further improvements in Centipede::ReportCrash()

centipede.cc

@@ -112,6 +112,12 @@ Centipede::Centipede(const Environment &env, CentipedeCallbacks &user_callbacks,
     input_filter_cmd_.StartForkServer(TemporaryLocalDirPath(), "input_filter");
 }
 
+Centipede::~Centipede() {
+  LOG_IF(WARNING, num_crash_reports_ > env_.max_num_crash_reports)
+      << "Skipped " << (num_crash_reports_ - env_.max_num_crash_reports)
+      << "crash reports";
+}
+
 int Centipede::SaveCorpusToLocalDir(const Environment &env,
                                     std::string_view save_corpus_to_local_dir) {
   for (size_t shard = 0; shard < env.total_shards; shard++) {
@@ -692,13 +698,23 @@ void Centipede::ReportCrash(std::string_view binary,
                             const BatchResult &batch_result) {
   CHECK_EQ(input_vec.size(), batch_result.results().size());
 
-  if (num_crash_reports_ >= env_.max_num_crash_reports) return;
+  ++num_crash_reports_;
+  if (num_crash_reports_ == env_.max_num_crash_reports) {
+    LOG(INFO) << "Reached --max_num_crash_reports: last report below; further "
+                 "reports will be suppressed";
+  } else if (num_crash_reports_ > env_.max_num_crash_reports) {
+    return;
+  }
 
+  // The runner adds results to the returned value as it successfully executes
+  // inputs. It doesn't add anything for a crashing input (there is nothing to
+  // add!), and immediately returns to the engine. Therefore, the suspect
+  // crashing input's index is equal to the number of the returned results.
   const size_t suspect_input_idx = std::clamp<size_t>(
       batch_result.num_outputs_read(), 0, input_vec.size() - 1);
 
-  std::string log_prefix =
-      absl::StrCat("ReportCrash[", num_crash_reports_, "]: ");
+  const std::string log_prefix =
+      absl::StrCat("ReportCrash[", num_crash_reports_ - 1, "]: ");
 
   LOG(INFO) << log_prefix << "Batch execution failed:"
             << "\nBinary               : " << binary
@@ -714,37 +730,32 @@ void Centipede::ReportCrash(std::string_view binary,
   }
   LOG(INFO).NoPrefix() << "\n";
 
-  LOG_IF(INFO, ++num_crash_reports_ == env_.max_num_crash_reports)
-      << log_prefix
-      << "Reached --max_num_crash_reports: further reports will be suppressed";
+  if (batch_result.failure_description() == kExecutionFailurePerBatchTimeout) {
+    LOG(INFO)
+        << log_prefix
+        << "Failure applies to entire batch: not re-running inputs one-by-one";
+    return;
+  }
 
   // Determine the optimal order of the inputs to try to maximize the chances of
-  // finding the reproducer fast.
+  // finding the crasher fast: insert it in front of everything else; however,
+  // do keep it at the old place, too, in case the target was primed for a crash
+  // by the sequence of inputs that preceded the crasher.
   // TODO(b/274705740): When the bug is fixed, set `input_idxs_to_try`'s size to
   //  `suspect_input_idx + 1`.
   std::vector<size_t> input_idxs_to_try(input_vec.size() + 1);
   input_idxs_to_try.front() = suspect_input_idx;
   std::iota(input_idxs_to_try.begin() + 1, input_idxs_to_try.end(), 0);
-  // Prioritize the presumed crasher by inserting it in front of everything
-  // else. However, do keep it at the old location, too, in case the target was
-  // primed for a crash by the sequence of inputs that preceded the crasher.
-
-  if (batch_result.failure_description() == kExecutionFailurePerBatchTimeout) {
-    LOG(INFO) << log_prefix
-              << "Failure applies to entire batch: not executing inputs "
-                 "one-by-one, trying to find the reproducer";
-    return;
-  }
 
   // Try inputs one-by-one in the determined order.
-  LOG(INFO) << log_prefix
-            << "Executing inputs one-by-one, trying to find the reproducer";
+  // TODO(ussuri): Only re-run sequentially; offload all I/O to a thread pool.
+  LOG(INFO) << log_prefix << "Re-running inputs one-by-one to find reproducer";
   for (auto input_idx : input_idxs_to_try) {
     const auto &one_input = input_vec[input_idx];
     BatchResult one_input_batch_result;
     if (!user_callbacks_.Execute(binary, {one_input}, one_input_batch_result)) {
-      auto hash = Hash(one_input);
-      auto crash_dir = env_.MakeCrashReproducerDirPath();
+      const auto hash = Hash(one_input);
+      const auto crash_dir = env_.MakeCrashReproducerDirPath();
       RemoteMkdir(crash_dir);
       std::string file_path = std::filesystem::path(crash_dir).append(hash);
       LOG(INFO) << log_prefix << "Detected crash-reproducing input:"
@@ -754,16 +765,13 @@ void Centipede::ReportCrash(std::string_view binary,
                 << "\nFailure        : "
                 << one_input_batch_result.failure_description()
                 << "\nSaving input to: " << file_path;
-      auto *file = RemoteFileOpen(file_path, "w");  // overwrites existing file.
-      CHECK(file != nullptr) << log_prefix << "Failed to open " << file_path;
-      RemoteFileAppend(file, one_input);
-      RemoteFileClose(file);
+      RemoteFileSetContents(file_path, AsString(one_input));
       return;
     }
   }
 
   LOG(INFO) << log_prefix
-            << "Crash was not observed when running inputs one-by-one";
+            << "Crash was not observed when re-running inputs one-by-one";
 
   // There will be cases when several inputs collectively cause a crash, but no
   // single input does. Handle this by writing out all inputs from the batch.

centipede.h

@@ -42,7 +42,7 @@ class Centipede {
   Centipede(const Environment &env, CentipedeCallbacks &user_callbacks,
             const BinaryInfo &binary_info, CoverageLogger &coverage_logger,
             Stats &stats);
-  virtual ~Centipede() {}
+  ~Centipede();
 
   // Main loop.
   void FuzzingLoop();

testing/centipede_test.cc

@@ -12,11 +12,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#include <string.h>
-
 #include <algorithm>
 #include <cstdint>
 #include <cstdlib>
+#include <cstring>
 #include <filesystem>
 #include <set>
 #include <string>
@@ -47,7 +46,8 @@ namespace {
 // A mock for CentipedeCallbacks.
 class CentipedeMock : public CentipedeCallbacks {
  public:
-  CentipedeMock(const Environment &env) : CentipedeCallbacks(env) {}
+  explicit CentipedeMock(const Environment &env) : CentipedeCallbacks(env) {}
+
   // Doesn't execute anything
   // Sets `batch_result.results()` based on the values of `inputs`:
   // Collects various stats about the inputs, to be checked in tests.
@@ -58,7 +58,7 @@ class CentipedeMock : public CentipedeCallbacks {
     // i-th element is the number of bytes with the value 'i' in the input.
     // `counters` is converted to FeatureVec and added to
     // `batch_result.results()`.
-    for (auto &input : inputs) {
+    for (const auto &input : inputs) {
       ByteArray counters(256);
       for (uint8_t byte : input) {
         counters[byte]++;
@@ -84,6 +84,7 @@ class CentipedeMock : public CentipedeCallbacks {
     min_batch_size_ = std::min(min_batch_size_, inputs.size());
     return true;
   }
+
   // Makes predictable mutants:
   // first 255 mutations are 1-byte sequences {1} ... {255}.
   // (the value {0} is produced by DummyValidInput()).
@@ -104,6 +105,8 @@ class CentipedeMock : public CentipedeCallbacks {
     }
   }
 
+  // TODO(ussuri): Make all of these  private.
+
   absl::flat_hash_set<uint8_t> observed_1byte_inputs_;
   absl::flat_hash_set<uint16_t> observed_2byte_inputs_;
 
@@ -147,12 +150,16 @@ TEST(Centipede, MockTest) {
   EXPECT_EQ(mock.observed_2byte_inputs_.size(), 65536);  // all 2-byte seqs.
 }
 
-static size_t CountFilesInDir(std::string_view dir_path) {
+namespace {
+
+size_t CountFilesInDir(std::string_view dir_path) {
   const std::filesystem::directory_iterator dir_iter{dir_path};
   return std::distance(std::filesystem::begin(dir_iter),
                        std::filesystem::end(dir_iter));
 }
 
+}  // namespace
+
 // Tests fuzzing and distilling in multiple shards.
 TEST(Centipede, ShardsAndDistillTest) {
   TempCorpusDir tmp_dir{test_info_->name()};
@@ -236,27 +243,31 @@ TEST(Centipede, InputFilter) {
   EXPECT_TRUE(corpus_set.count({'c'}));
 }
 
+namespace {
+
 // Callbacks for MutateViaExternalBinary test.
 class MutateCallbacks : public CentipedeCallbacks {
  public:
   explicit MutateCallbacks(const Environment &env) : CentipedeCallbacks(env) {}
+
   // Will not be called.
   bool Execute(std::string_view binary, const std::vector<ByteArray> &inputs,
                BatchResult &batch_result) override {
-    CHECK(false);
-    return false;
+    ABSL_UNREACHABLE();
   }
 
   // Will not be called.
   void Mutate(const std::vector<ByteArray> &inputs, size_t num_mutants,
               std::vector<ByteArray> &mutants) override {
-    CHECK(false);
+    ABSL_UNREACHABLE();
   }
 
   // Redeclare a protected member function as public so the tests can call it.
   using CentipedeCallbacks::MutateViaExternalBinary;
 };
 
+}  // namespace
+
 TEST(Centipede, MutateViaExternalBinary) {
   // This binary contains a test-friendly custom mutator.
   const std::string binary_with_custom_mutator =
@@ -332,6 +343,8 @@ TEST(Centipede, MutateViaExternalBinary) {
   }
 }
 
+namespace {
+
 // A mock for MergeFromOtherCorpus test.
 class MergeMock : public CentipedeCallbacks {
  public:
@@ -366,6 +379,8 @@ class MergeMock : public CentipedeCallbacks {
   size_t number_of_mutations_ = 0;
 };
 
+}  // namespace
+
 TEST(Centipede, MergeFromOtherCorpus) {
   using Corpus = std::vector<ByteArray>;
 
@@ -411,6 +426,8 @@ TEST(Centipede, MergeFromOtherCorpus) {
   EXPECT_EQ(work_tmp_dir.GetCorpus(1), Corpus({{3}, {4}, {5}, {6}, {7}}));
 }
 
+namespace {
+
 // A mock for FunctionFilter test.
 class FunctionFilterMock : public CentipedeCallbacks {
  public:
@@ -428,7 +445,7 @@ class FunctionFilterMock : public CentipedeCallbacks {
   void Mutate(const std::vector<ByteArray> &inputs, size_t num_mutants,
               std::vector<ByteArray> &mutants) override {
     mutants.resize(num_mutants);
-    for (auto &input : inputs) {
+    for (const auto &input : inputs) {
       if (input != DummyValidInput()) {
         observed_inputs_.insert(input);
       }
@@ -455,8 +472,8 @@ class FunctionFilterMock : public CentipedeCallbacks {
 
 // Runs a short fuzzing session with the provided `function_filter`.
 // Returns a sorted array of observed inputs.
-static std::vector<ByteArray> RunWithFunctionFilter(
-    std::string_view function_filter, const TempDir &tmp_dir) {
+std::vector<ByteArray> RunWithFunctionFilter(std::string_view function_filter,
+                                             const TempDir &tmp_dir) {
   Environment env;
   env.workdir = tmp_dir.path();
   env.seed = 1;  // make the runs predictable.
@@ -480,6 +497,8 @@ static std::vector<ByteArray> RunWithFunctionFilter(
   return res;
 }
 
+}  // namespace
+
 // Tests --function_filter.
 TEST(Centipede, FunctionFilter) {
   // Run with empty function filter.
@@ -688,7 +707,9 @@ TEST(Centipede, UndetectedCrashingInput) {
   ASSERT_EQ(found_crash_file_names.size(), kCrashingInputIdxInBatch + 1);
 }
 
-static void WriteBlobsToFile(const std::vector<ByteArray> &blobs,
+namespace{
+
+void WriteBlobsToFile(const std::vector<ByteArray> &blobs,
                              const std::string_view path) {
   auto appender = DefaultBlobFileAppenderFactory();
   CHECK_OK(appender->Open(path));
@@ -697,6 +718,8 @@ static void WriteBlobsToFile(const std::vector<ByteArray> &blobs,
   }
 }
 
+}  // namespace
+
 TEST(Centipede, ShardReader) {
   ByteArray data1 = {1, 2, 3};
   ByteArray data2 = {3, 4, 5, 6};