-
Notifications
You must be signed in to change notification settings - Fork 39k
fuzz: Make partially_downloaded_block more deterministic #32158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
fa7e931
fa82fe2
fa900bb
fa17cdb
fa51310
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -60,6 +60,7 @@ | |||||
| #include <walletinitinterface.h> | ||||||
|
|
||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Inline comment thread to not spam main)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I guess this is an unclean build, because it the command doesn't set the cxx flags, as explained in Also, this is building with sanitizers, which may or may not have an effect on runtime behavior and thus make the non-determinism manifest differently. Also, could you please share the full
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Log/diff with above build config: without_fix_400t.gz Will try with a fresh build directory. Would make sense that sanitizers change behavior.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like this may be a bug in libFuzzer where an input is run twice, instead of once, when it should only be run once? It is not visible in the output, as that is now hidden after your suggestion to hide the output, but it could make sense to reject libFuzzer in the script completely.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Maybe one could return the First run with fresh build tree and without the C++ thread fix from this PR failed (no determinism-issue detected). ✨ Second run did produce a diff containing both (Third run again detected no determinism). ✨ Fourth run again produced diff with expected substrings. ✨
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was using Had it output the thread id when printing the fuzz-output (
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the output, it seems to indicate that the fuzz target ran twice: 46| |FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb)
- 47| 2|{
- 48| 2| SeedRandomStateForTest(SeedRand::ZEROS);
- 49| 2| FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
- 50| 2| SetMockTime(ConsumeTime(fuzzed_data_provider));
+ 47| 1|{
+ 48| 1| SeedRandomStateForTest(SeedRand::ZEROS);
+ 49| 1| FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
+ 50| 1| SetMockTime(ConsumeTime(fuzzed_data_provider));
51| |Thus, it seems there are two options:
If you want to check this further, and if you want to check if the fuzz process itself is at fault, it should be possible by aborting it, if the process runs the target twice. (Also, could mock out the meat of the logic here): diff --git a/contrib/devtools/deterministic-fuzz-coverage/src/main.rs b/contrib/devtools/deterministic-fuzz-coverage/src/main.rs
index 9c1738396b..d5b7181c80 100644
--- a/contrib/devtools/deterministic-fuzz-coverage/src/main.rs
+++ b/contrib/devtools/deterministic-fuzz-coverage/src/main.rs
@@ -150,40 +150,6 @@ fn deterministic_coverage(
))?;
}
}
- if !Command::new(LLVM_PROFDATA)
- .arg("merge")
- .arg("--sparse")
- .arg(&profraw_file)
- .arg("-o")
- .arg(&profdata_file)
- .status()
- .map_err(|e| format!("{LLVM_PROFDATA} merge failed with {e}"))?
- .success()
- {
- Err(format!("{LLVM_PROFDATA} merge failed. This can be a sign of compiling without code coverage support."))?;
- }
- let cov_file = File::create(&cov_txt_path)
- .map_err(|e| format!("Failed to create coverage txt file ({e})"))?;
- if !Command::new(LLVM_COV)
- .args([
- "show",
- "--show-line-counts-or-regions",
- "--show-branches=count",
- "--show-expansions",
- "--show-instantiation-summary",
- "-Xdemangler=llvm-cxxfilt",
- &format!("--instr-profile={}", profdata_file.display()),
- ])
- .arg(fuzz_exe)
- .stdout(cov_file)
- .spawn()
- .map_err(|e| format!("{LLVM_COV} show failed with {e}"))?
- .wait()
- .map_err(|e| format!("{LLVM_COV} show failed with {e}"))?
- .success()
- {
- Err(format!("{LLVM_COV} show failed"))?;
- };
Ok(cov_txt_path)
};
let check_diff = |a: &Path, b: &Path, err: &str| -> AppResult {
@@ -221,11 +187,6 @@ The coverage was not deterministic between runs.
}
let cov_txt_base = run_single('a', &entry, thread_id)?;
let cov_txt_repeat = run_single('b', &entry, thread_id)?;
- check_diff(
- &cov_txt_base,
- &cov_txt_repeat,
- &format!("The fuzz target input was {}.", entry.display()),
- )?;
Ok(())
};
thread::scope(|s| -> AppResult {
@@ -254,18 +215,6 @@ The coverage was not deterministic between runs.
// This can catch issues where mutable global state is leaked from one fuzz input execution to
// the next.
println!("Check all fuzz inputs in one go ...");
- {
- if !corpus_dir.is_dir() {
- Err(format!("{} should be a folder", corpus_dir.display()))?;
- }
- let cov_txt_base = run_single('a', &corpus_dir, 0)?;
- let cov_txt_repeat = run_single('b', &corpus_dir, 0)?;
- check_diff(
- &cov_txt_base,
- &cov_txt_repeat,
- &format!("All fuzz inputs in {} were used.", corpus_dir.display()),
- )?;
- }
println!("✨ Coverage test passed for {fuzz_target}. ✨");
Ok(())
}
diff --git a/src/test/fuzz/partially_downloaded_block.cpp b/src/test/fuzz/partially_downloaded_block.cpp
index 8c6565e48b..466548ee35 100644
--- a/src/test/fuzz/partially_downloaded_block.cpp
+++ b/src/test/fuzz/partially_downloaded_block.cpp
@@ -45,6 +45,9 @@ PartiallyDownloadedBlock::CheckBlockFn FuzzedCheckBlock(std::optional<BlockValid
FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb)
{
+static bool g_once{true};
+Assert(g_once);
+g_once=false;
SeedRandomStateForTest(SeedRand::ZEROS);
FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
SetMockTime(ConsumeTime(fuzzed_data_provider));
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Result with provided patch: So it's within the same process. libFuzzer doesn't take Full error
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Thanks for checking. I'd say it would be nice to report this upstream to llvm (with a minimal reproducer), and then eventually fix it. Though, google stopped maintaining it, so they providing the fix is unlikely.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suspect the minimal reproducer would be: cat << EOF > /tmp/fuzz_once.cpp
#include <cstddef>
#include <cstdint>
#include <cstdlib>
extern "C" int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) {
static bool g_once{true};
if (g_once) {
g_once = false;
return 0;
}
std::abort(); // Ran twice :(
}
EOFand then: However, I can't reproduce. So it would be good if someone else checked this. |
||||||
| #include <algorithm> | ||||||
| #include <future> | ||||||
| #include <functional> | ||||||
| #include <stdexcept> | ||||||
|
|
||||||
|
|
@@ -230,6 +231,12 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, TestOpts opts) | |||||
| // Use synchronous task runner while fuzzing to avoid non-determinism | ||||||
| G_FUZZING ? std::make_unique<ValidationSignals>(std::make_unique<util::ImmediateTaskRunner>()) : | ||||||
| std::make_unique<ValidationSignals>(std::make_unique<SerialTaskRunner>(*m_node.scheduler)); | ||||||
| { | ||||||
| // Ensure deterministic coverage by waiting for m_service_thread to be running | ||||||
| std::promise<void> promise; | ||||||
| m_node.scheduler->scheduleFromNow([&promise] { promise.set_value(); }, 0ms); | ||||||
| promise.get_future().wait(); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| bilingual_str error{}; | ||||||
|
|
@@ -247,7 +254,8 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, TestOpts opts) | |||||
| .check_block_index = 1, | ||||||
| .notifications = *m_node.notifications, | ||||||
| .signals = m_node.validation_signals.get(), | ||||||
| .worker_threads_num = 2, | ||||||
| // Use no worker threads while fuzzing to avoid non-determinism | ||||||
| .worker_threads_num = G_FUZZING ? 0 : 2, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm worried this means we get decreased fuzz-coverage. Maybe could introduce another var?
Suggested change
Although being able to reproduce fuzz failures is also nice. :\
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure. If someone wants to fuzz test the script worker threads, a dedicated fuzz target seems ideal (See |
||||||
| }; | ||||||
| if (opts.min_validation_cache) { | ||||||
| chainman_opts.script_execution_cache_bytes = 0; | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Would be nice to get
For the negative case. Right now one only gets the
...and then just the omission of this warning.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May do if I have to re-touch for other reasons.