Use stop condition to pass exit code; Separate early stop reset from stop time. The separation is because that we don't want to clean up the stop request when resetting the stop time, otherwise there is a time window between checking early stop and resetting the early stop where the requests issued in-between (mainly from the signal handlers) would be ignored. (Note that it's not a problem in centipede_adaptor.cc due to the additional Runtime::termination_requested() check.) PiperOrigin-RevId: 947783297
diff --git a/centipede/centipede_interface.cc b/centipede/centipede_interface.cc index 99cace6..3305045 100644 --- a/centipede/centipede_interface.cc +++ b/centipede/centipede_interface.cc
@@ -80,7 +80,7 @@ // Runs env.for_each_blob on every blob extracted from env.args. // Returns EXIT_SUCCESS on success, EXIT_FAILURE otherwise. -int ForEachBlob(const Environment& env, StopCondition& stop_condition) { +void ForEachBlob(const Environment& env, StopCondition& stop_condition) { auto tmpdir = TemporaryLocalDirPath(); CreateLocalDirRemovedAtExit(tmpdir); std::string tmpfile = std::filesystem::path(tmpdir).append("t"); @@ -91,7 +91,8 @@ absl::Status open_status = blob_reader->Open(arg); if (!open_status.ok()) { FUZZTEST_LOG(INFO) << "Failed to open " << arg << ": " << open_status; - return EXIT_FAILURE; + stop_condition.RequestEarlyStop(EXIT_FAILURE); + return; } ByteSpan blob; while (blob_reader->Read(blob) == absl::OkStatus()) { @@ -103,10 +104,9 @@ // 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 (stop_condition.ShouldStop()) return stop_condition.ExitCode(); + if (stop_condition.ShouldStop()) return; } } - return EXIT_SUCCESS; } // Loads corpora from work dirs provided in `env.args`, if there are two args @@ -177,10 +177,10 @@ return envs; } -int Fuzz(const Environment& env, const BinaryInfo& binary_info, - std::string_view pcs_file_path, - CentipedeCallbacksFactory& callbacks_factory, - StopCondition& stop_condition) { +void Fuzz(const Environment& env, const BinaryInfo& binary_info, + std::string_view pcs_file_path, + CentipedeCallbacksFactory& callbacks_factory, + StopCondition& stop_condition) { CoverageLogger coverage_logger(binary_info.pc_table, binary_info.symbols); std::vector<Environment> envs = @@ -256,8 +256,6 @@ } if (!env.knobs_file.empty()) PrintRewardValues(stats_vec, std::cerr); - - return stop_condition.ExitCode(); } TestShard SetUpTestSharding() { @@ -339,9 +337,9 @@ PeriodicAction::ZeroDelayConstInterval(absl::Seconds(15))}; } -int UpdateCorpusDatabase(Environment env, - CentipedeCallbacksFactory& callbacks_factory, - StopCondition& stop_condition) { +void UpdateCorpusDatabase(Environment env, + CentipedeCallbacksFactory& callbacks_factory, + StopCondition& stop_condition) { FUZZTEST_LOG(INFO) << "Starting the update of the corpus database for:" << "\nFuzz test: " << env.test_name << "\nBinary: " << env.binary @@ -389,13 +387,10 @@ FUZZTEST_LOG(INFO) << "Test shard index: " << test_shard_index << " Total test shards: " << total_test_shards; - int exit_code = EXIT_SUCCESS; - // Step 2: Run the fuzz test. - // Clean up previous stop requests. stop_time will be set later. - stop_condition.ClearEarlyStopRequestAndSetStopTime( - /*stop_time=*/absl::InfiniteFuture()); + // Unset stop time. stop_time will be set later. + stop_condition.SetStopTime(absl::InfiniteFuture()); if (!is_workdir_specified) { env.workdir = base_workdir_path / env.test_name; @@ -421,7 +416,7 @@ if (!RemotePathExists(WorkDir{env}.CoverageDirPath())) { FUZZTEST_LOG(INFO) << "Skipping running the fuzz test " << env.test_name; - return exit_code; + return; } // If execution IDs match and the previous coverage exists, it means // the same workflow got interrupted when running the test. So we resume @@ -489,15 +484,15 @@ is_resuming = false; if (stop_condition.EarlyStopRequested()) { - if (stop_condition.ExitCode() != EXIT_SUCCESS) { - exit_code = stop_condition.ExitCode(); + if (const auto exit_code = stop_condition.ExitCode(); + exit_code != EXIT_SUCCESS) { FUZZTEST_LOG(ERROR) << "Early stop requested for test " << env.test_name << " with failure exit code " << exit_code; } else { FUZZTEST_LOG(INFO) << "Skipping test " << env.test_name << " due to early stop requested without failure."; } - return exit_code; + return; } FUZZTEST_LOG(INFO) << (env.fuzztest_only_replay ? "Replaying " : "Fuzzing ") @@ -505,8 +500,7 @@ << "\n\tTest binary: " << env.binary; const absl::Time start_time = absl::Now(); - stop_condition.ClearEarlyStopRequestAndSetStopTime(/*stop_time=*/start_time + - time_limit); + stop_condition.SetStopTime(start_time + time_limit); PeriodicAction record_fuzzing_time = RecordFuzzingTime(fuzzing_time_file, start_time - time_spent); Fuzz(env, binary_info, pcs_file_path, callbacks_factory, stop_condition); @@ -522,25 +516,24 @@ } if (stop_condition.EarlyStopRequested()) { - if (stop_condition.ExitCode() != EXIT_SUCCESS) { - exit_code = stop_condition.ExitCode(); + if (const auto exit_code = stop_condition.ExitCode(); + exit_code != EXIT_SUCCESS) { FUZZTEST_LOG(ERROR) << "Early stop requested for test " << env.test_name << " with failure exit code " << exit_code; } else { FUZZTEST_LOG(INFO) << "Skip updating corpus database due to early stop " "requested without failure."; } - return exit_code; + return; } // The test time limit does not apply for the rest of the steps. - stop_condition.ClearEarlyStopRequestAndSetStopTime( - /*stop_time=*/absl::InfiniteFuture()); + stop_condition.SetStopTime(absl::InfiniteFuture()); // TODO(xinhaoyuan): Have a separate flag to skip corpus updating instead // of checking whether workdir is specified or not. const bool skip_corpus_db_update = env.fuzztest_only_replay || is_workdir_specified; - if (skip_corpus_db_update && !env.report_crash_summary) return exit_code; + if (skip_corpus_db_update && !env.report_crash_summary) return; // Deduplicate and optionally update the crashing inputs. CrashSummary crash_summary{env.fuzztest_binary_identifier, env.test_name}; @@ -555,7 +548,7 @@ crash_signature, crash_details.description}); } crash_summary.Report(&std::cerr); - return exit_code; + return; } OrganizeCrashingInputs(regression_dir, fuzztest_db_path / "crashing", env, callbacks_factory, crashes_by_signature, crash_summary, @@ -580,8 +573,6 @@ FUZZTEST_CHECK_OK( RemoteFileRename(corpus_file, (coverage_dir / file_name).c_str())); } - - return exit_code; } int ListCrashIds(const Environment& env) { @@ -611,9 +602,9 @@ return EXIT_SUCCESS; } -int ReplayCrash(const Environment& env, - CentipedeCallbacksFactory& callbacks_factory, - StopCondition& stop_condition) { +void ReplayCrash(const Environment& env, + CentipedeCallbacksFactory& callbacks_factory, + StopCondition& stop_condition) { FUZZTEST_CHECK(!env.crash_id.empty()) << "Need crash_id to be set for replay a crash"; FUZZTEST_CHECK(!env.test_name.empty()) @@ -641,8 +632,7 @@ crash_corpus_config, env.binary_name, env.binary_hash)); Environment run_crash_env = env; run_crash_env.load_shards_only = true; - int fuzz_result = - Fuzz(run_crash_env, {}, "", callbacks_factory, stop_condition); + Fuzz(run_crash_env, {}, "", callbacks_factory, stop_condition); if (env.report_crash_summary) { CrashSummary crash_summary{env.fuzztest_binary_identifier, env.test_name}; const absl::flat_hash_map<std::string, CrashDetails> crashes_by_signature = @@ -655,7 +645,6 @@ } crash_summary.Report(&std::cerr); } - return fuzz_result; } int ExportCrash(const Environment& env) { @@ -696,7 +685,7 @@ if (stop_condition == nullptr) { stop_condition = &default_stop_condition; } - stop_condition->ClearEarlyStopRequestAndSetStopTime(env.stop_at); + stop_condition->SetStopTime(env.stop_at); if (!env.corpus_to_files.empty()) { Centipede::CorpusToFiles(env, env.corpus_to_files); @@ -711,12 +700,16 @@ return EXIT_FAILURE; } - if (!env.for_each_blob.empty()) return ForEachBlob(env, *stop_condition); + if (!env.for_each_blob.empty()) { + ForEachBlob(env, *stop_condition); + return stop_condition->ExitCode(); + } if (!env.minimize_crash_file_path.empty()) { ByteArray crashy_input; ReadFromLocalFile(env.minimize_crash_file_path, crashy_input); - return MinimizeCrash(crashy_input, env, callbacks_factory, *stop_condition); + MinimizeCrash(crashy_input, env, callbacks_factory, *stop_condition); + return stop_condition->ExitCode(); } // Just export the corpus from a local dir and exit. @@ -789,7 +782,8 @@ return ListCrashIds(updated_env); } if (env.replay_crash) { - return ReplayCrash(updated_env, callbacks_factory, *stop_condition); + ReplayCrash(updated_env, callbacks_factory, *stop_condition); + return stop_condition->ExitCode(); } if (env.export_crash) { return ExportCrash(updated_env); @@ -802,8 +796,8 @@ FUZZTEST_CHECK(updated_env.fuzztest_time_limit_per_test >= absl::Seconds(1)) << "Time limit per fuzz test must be at least 1 second."; - return UpdateCorpusDatabase(updated_env, callbacks_factory, - *stop_condition); + UpdateCorpusDatabase(updated_env, callbacks_factory, *stop_condition); + return stop_condition->ExitCode(); } } @@ -819,8 +813,9 @@ if (env.analyze) return Analyze(env); - return Fuzz(env, binary_info, pcs_file_path, callbacks_factory, - *stop_condition); + Fuzz(env, binary_info, pcs_file_path, callbacks_factory, *stop_condition); + return stop_condition->ExitCode(); + // TODO: fniksic - Report the crash summary here if requested. What are the // binary identifier and the fuzz test name here? }
diff --git a/centipede/centipede_test.cc b/centipede/centipede_test.cc index c485781..3e7a471 100644 --- a/centipede/centipede_test.cc +++ b/centipede/centipede_test.cc
@@ -1325,7 +1325,7 @@ StopCondition stop_condition; CentipedeDefaultCallbacks callbacks(env, stop_condition); const auto start = absl::Now(); - stop_condition.ClearEarlyStopRequestAndSetStopTime(start + absl::Seconds(3)); + stop_condition.SetStopTime(start + absl::Seconds(3)); std::vector<ByteArray> seeds; callbacks.GetSeeds(/*num_seeds=*/1, seeds); // Give it some slack to stop in 5s.
diff --git a/centipede/minimize_crash.cc b/centipede/minimize_crash.cc index 48ca00c..3a75160 100644 --- a/centipede/minimize_crash.cc +++ b/centipede/minimize_crash.cc
@@ -135,9 +135,9 @@ } } -int MinimizeCrash(ByteSpan crashy_input, const Environment& env, - CentipedeCallbacksFactory& callbacks_factory, - StopCondition& stop_condition) { +void MinimizeCrash(ByteSpan crashy_input, const Environment& env, + CentipedeCallbacksFactory& callbacks_factory, + StopCondition& stop_condition) { ScopedCentipedeCallbacks scoped_callback(callbacks_factory, env, stop_condition); auto callbacks = scoped_callback.callbacks(); @@ -148,7 +148,8 @@ ByteArray original_crashy_input(crashy_input.begin(), crashy_input.end()); if (callbacks->Execute(env.binary, {original_crashy_input}, batch_result)) { FUZZTEST_LOG(INFO) << "The original crashy input did not crash; exiting"; - return EXIT_FAILURE; + stop_condition.RequestEarlyStop(EXIT_FAILURE); + return; } FUZZTEST_LOG(INFO) << "Starting the crash minimization loop in " @@ -166,7 +167,10 @@ } } // The threads join here. - return queue.SmallerCrashesFound() ? EXIT_SUCCESS : EXIT_FAILURE; + if (stop_condition.EarlyStopRequested()) return; + if (!queue.SmallerCrashesFound()) { + stop_condition.RequestEarlyStop(EXIT_FAILURE); + } } } // namespace fuzztest::internal
diff --git a/centipede/minimize_crash.h b/centipede/minimize_crash.h index 2910437..a7e21d5 100644 --- a/centipede/minimize_crash.h +++ b/centipede/minimize_crash.h
@@ -24,14 +24,12 @@ // Tries to minimize `crashy_input`. // Uses `callbacks_factory` to create `env.num_threads` workers. -// Returns EXIT_SUCCESS if at least one smaller crasher was found, -// EXIT_FAILURE otherwise. -// Also returns EXIT_FAILURE if the original input didn't crash. -// Stores the newly found crashy inputs in -// `WorkDir{env}.CrashReproducerDirPath()`. -int MinimizeCrash(ByteSpan crashy_input, const Environment& env, - CentipedeCallbacksFactory& callbacks_factory, - StopCondition& stop_condition); +// Requests to stop with EXIT_FAILURE using `stop_condition` if no smaller +// crasher was found or if the original input didn't crash. Stores the newly +// found crashy inputs in `WorkDir{env}.CrashReproducerDirPath()`. +void MinimizeCrash(ByteSpan crashy_input, const Environment& env, + CentipedeCallbacksFactory& callbacks_factory, + StopCondition& stop_condition); } // namespace fuzztest::internal
diff --git a/centipede/minimize_crash_test.cc b/centipede/minimize_crash_test.cc index 9eba004..03c9ad4 100644 --- a/centipede/minimize_crash_test.cc +++ b/centipede/minimize_crash_test.cc
@@ -91,21 +91,23 @@ StopCondition stop_condition; // Test with a non-crashy input. - EXPECT_EQ(MinimizeCrash({1, 2, 3}, env, factory, stop_condition), - EXIT_FAILURE); + MinimizeCrash({1, 2, 3}, env, factory, stop_condition); + EXPECT_EQ(stop_condition.ExitCode(), EXIT_FAILURE); ByteArray expected_minimized = {'f', 'u', 'z'}; // Test with a crashy input that can't be minimized further. - EXPECT_EQ(MinimizeCrash(expected_minimized, env, factory, stop_condition), - EXIT_FAILURE); + stop_condition.ClearEarlyStopRequest(); + MinimizeCrash(expected_minimized, env, factory, stop_condition); + EXPECT_EQ(stop_condition.ExitCode(), EXIT_FAILURE); // Test the actual minimization. ByteArray original_crasher = {'f', '.', '.', '.', '.', '.', '.', '.', '.', '.', '.', 'u', '.', '.', '.', '.', '.', '.', '.', '.', '.', '.', 'z'}; - EXPECT_EQ(MinimizeCrash(original_crasher, env, factory, stop_condition), - EXIT_SUCCESS); + stop_condition.ClearEarlyStopRequest(); + MinimizeCrash(original_crasher, env, factory, stop_condition); + EXPECT_EQ(stop_condition.ExitCode(), EXIT_SUCCESS); // Collect the new crashers from the crasher dir. std::vector<ByteArray> crashers; for (auto const &dir_entry : std::filesystem::directory_iterator{
diff --git a/centipede/stop.cc b/centipede/stop.cc index 6b2f685..fb7204a 100644 --- a/centipede/stop.cc +++ b/centipede/stop.cc
@@ -25,8 +25,11 @@ return early_stop_.load(std::memory_order_acquire).is_requested; } -void StopCondition::ClearEarlyStopRequestAndSetStopTime(absl::Time stop_time) { +void StopCondition::ClearEarlyStopRequest() { early_stop_.store({}, std::memory_order_release); +} + +void StopCondition::SetStopTime(absl::Time stop_time) { stop_time_ = stop_time; }
diff --git a/centipede/stop.h b/centipede/stop.h index f000fd5..071fbeb 100644 --- a/centipede/stop.h +++ b/centipede/stop.h
@@ -32,12 +32,18 @@ StopCondition(StopCondition&&) = delete; StopCondition& operator=(StopCondition&&) = delete; - // Clears the request to stop early and sets the stop time. + // Clears the request to stop early. // // REQUIRES: Must be called before starting concurrent threads that may invoke - // the other methods on this object instance. In particular, calling this - // function concurrently with `ShouldStop()` is not thread-safe. - void ClearEarlyStopRequestAndSetStopTime(absl::Time stop_time); + // the other methods on this object instance. Specifically, calling this + // function concurrently with `EarlyStopRequested()` is not thread-safe. + void ClearEarlyStopRequest(); + + // Returns whether `RequestEarlyStop()` was called or not since the most + // recent call to `ClearEarlyStopRequest()` (if any). + // + // ENSURES: Thread-safe unless with `ClearEarlyStopRequest()`. + bool EarlyStopRequested() const; // Requests that Centipede soon stops whatever it is doing (fuzzing, // minimizing reproducer, etc.), with `exit_code` indicating success (zero) or @@ -46,11 +52,12 @@ // ENSURES: Thread-safe and safe to call from signal handlers. void RequestEarlyStop(int exit_code); - // Returns whether `RequestEarlyStop()` was called or not since the most - // recent call to `ClearEarlyStopRequestAndSetStopTime()` (if any). + // Sets the stop time. // - // ENSURES: Thread-safe. - bool EarlyStopRequested() const; + // REQUIRES: Must be called before starting concurrent threads that may invoke + // the functions defined in this class. Specifically, calling this function + // concurrently with `ShouldStop()` and `GetStopTime()` is not thread-safe. + void SetStopTime(absl::Time stop_time); // Returns true iff it is time to stop, either because the stopping time has // been reached or `RequestEarlyStop()` was called since the most recent call
diff --git a/fuzztest/internal/centipede_adaptor.cc b/fuzztest/internal/centipede_adaptor.cc index 7c23407..276a331 100644 --- a/fuzztest/internal/centipede_adaptor.cc +++ b/fuzztest/internal/centipede_adaptor.cc
@@ -436,8 +436,8 @@ << "Termination status must be Exited if not Signaled"; return static_cast<int>(std::get<ExitCodeT>(status.Status())); } - global_stop_condition.ClearEarlyStopRequestAndSetStopTime( - absl::InfiniteFuture()); + global_stop_condition.ClearEarlyStopRequest(); + global_stop_condition.SetStopTime(absl::InfiniteFuture()); static absl::NoDestructor<DefaultCallbacksFactory<CentipedeDefaultCallbacks>> factory; return CentipedeMain(env, *factory, &global_stop_condition);