When building with
BUILD_FOR_FUZZING=OFF
BUILD_FUZZ_BINARY=ON
CMAKE_BUILD_TYPE=Debug
allow the fuzz binary to execute given test cases (without actual fuzzing) to make it easier to reproduce fuzz test failures in a more normal debug build.
In Debug builds, deterministic fuzz behaviour is controlled via a runtime variable, which is normally false, but set to true automatically in the fuzz binary, unless the FUZZ_NONDETERMINISM environment variable is set.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32113.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
I don’t think the current patch will work. G_FUZZING influences more than just the behavior of the asserts/assumes. For example:
So with this patch it looks like you are instead opting into non-reproducible fuzz behavior.
In debug mode the performance doesn’t matter, so a possible alternative fix could be to just make G_FUZZING a runtime bool again?
I don’t think the current patch will work.
G_FUZZINGinfluences more than just the behavior of the asserts/assumes. For example:* POW checks are different * The task runner is different * The random seeding is different
Yes – the same fuzz seed will in many cases execute different code paths for the fuzzer when compiled in Debug mode without BUILD_FOR_FUZZING set. But that’s a feature not a bug – the idea is to be able to still run the fuzz tests with a relatively normal production-like build that doesn’t completely special-case out POW checks, random seeding, etc.
So with this patch it looks like you are instead opting into non-reproducible fuzz behavior.
This is only for builds where fuzzing isn’t actually happening. ie, you’re getting a seed from a real fuzz run, then retry that seed against a more normal build.
In debug mode the performance doesn’t matter, so a possible alternative fix could be to just make G_FUZZING a runtime bool again?
You could make the rng, pow checks etc conditional on something like this, I suppose, which would get you optimal performance in both fuzzing and production builds, and I think pretty good performance in debug builds?
0inline bool FuzzDetermism()
1{
2 if constexpr (G_FUZZING) {
3 return true;
4 } else if constexpr (!G_ABORT_ON_FAILED_ASSUME) {
5 return false;
6 } else {
7 return DynamicFuzzDeterminism();
8 }
9}
10
11bool DynamicFuzzDeterminism()
12{
13 static bool fuzz_det = std::getenv("FUZZ_DETERMINISM");
14 return fuzz_det;
15}
But that’s a feature not a bug – the idea is to be able to still run the fuzz tests with a relatively normal production-like build that doesn’t completely special-case out POW checks, random seeding, etc.
My fear is that some bugs are not reproducible. (I know this is mostly theoretical, because we haven’t seen such a case yet, but still it would be a bit disappointing if someone spent hours trying to reproduce a fuzz run, when it is impossible in a normal debug build)
inline bool FuzzDetermism()
lgtm
lgtm
Okay, done. How’s that look?
32@@ -33,3 +33,9 @@ void assertion_fail(std::string_view file, int line, std::string_view func, std:
33 fwrite(str.data(), 1, str.size(), stderr);
34 std::abort();
35 }
36+
37+bool EnableFuzzDeterminismDynamic()
38+{
39+ static bool fuzz_determinism = std::getenv("FUZZ_DETERMINISM") != nullptr;
nit: Forgot to mention this, but to me it seems simpler to have the program automatically set this. Otherwise, it is just busy work for the dev.
in src/test/fuzz/fuzz.cpp you can set it to true, and default it to false, no?
This is also the code prior to the changes in https://github.com/bitcoin/bitcoin/pull/31191/files
157+ if (std::getenv("FUZZ_NONDETERMINISM")) {
158+ std::cerr << "Warning: FUZZ_NONDETERMINISM env var set, results may be inconsistent with fuzz build" << std::endl;
159+ } else {
160+ g_enable_dynamic_fuzz_determinism = true;
161+ assert(EnableFuzzDeterminism());
162+ }
Is there a use-case for FUZZ_NONDETERMINISM? IIRC the goal of this change was to achieve fuzz determinism, so adding non-determinism could be left for a follow-up? I’d even argue that it may be better to not add at all, because it is such an edge case that no one will be using it. However, it existing, implies that reviewers will have to take it into account every time, which doesn’t seem worth it.
If you remove it, you can also move the assert(EnableFuzzDeterminism()); before Assert(!g_test_one_input);, so that it is executed unconditionally.
edit: nit: The g_enable_dynamic_fuzz_determinism could probably be a plain bool (without atomic).
FUZZ_NONDETERMINISM set. I don’t see why reviewers would have to take this into account at all; it’s just an alternate way to exercise test code, it has no effect on production code.
lgtm ACK 1718da848a0a7591f0eab086e159f1e9e0f2c59b
This should also address the concern by @ryanofsky in #31191 (comment) to some extent IIUC. Of course, it is still impossible to execute a fuzz test case normally in a release-mode non-fuzz build, but this pull offers one workaround:
This workaround could also be used to revert 72ab35a6d09c4f115908a3335d24eb03fd154bf6 and merge the split CI tasks back into one.
34+inline bool EnableFuzzDeterminism()
35+{
36+ if constexpr (G_FUZZING) {
37+ return true;
38+ } else if constexpr (!G_ABORT_ON_FAILED_ASSUME) {
39+ return false;
In commit “fuzz: enable running fuzz test cases in Debug mode” (1718da848a0a7591f0eab086e159f1e9e0f2c59b)
I don’t think I the understand reason for ABORT_ON_FAILED_ASSUME affecting return value of EnableFuzzDeterminism(), and trying to force non-determinism when assume doesn’t abort. It seems like this behavior might cause the assert(EnableFuzzDeterminism()) to fail, but also this code might just be easier to understand if fuzz determinism only depended on the FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION build option and FUZZ_NONDETERMINISM environment variable and didn’t have this third factor.
Is the reason maybe that this is trying to optimize code and avoid needing to access the g_enable_dynamic_fuzz_determinism variable in release builds? IMO would be good to either remove the !G_ABORT_ON_FAILED_ASSUME condition or document why it’s here.
Is the reason maybe that this is trying to optimize code and avoid needing to access the
g_enable_dynamic_fuzz_determinismvariable in release builds?
Yes, that’s the reason as I understand it. I am happy to re-ack a simple doc fixup like:
0return false; // Allow optimized code for release builds (non-fuzz and non-debug).
I don’t think I the understand reason for ABORT_ON_FAILED_ASSUME affecting return value of EnableFuzzDeterminism(), and trying to force non-determinism when assume doesn’t abort.
The initial thought was actually more that the running the fuzz tests without Assumes being checked isn’t going to be anywhere near as thorough, particularly influenced by the logic documented in #32100. Added some comment text on this.
When building with
BUILD_FOR_FUZZING=OFF
BUILD_FUZZ_BINARY=ON
CMAKE_BUILD_TYPE=Debug
allow the fuzz binary to execute given test cases (without actual
fuzzing) to make it easier to reproduce fuzz test failures in a more
normal debug build.
In Debug builds, deterministic fuzz behaviour is controlled via a runtime
variable, which is normally false, but set to true automatically in the
fuzz binary, unless the FUZZ_NONDETERMINISM environment variable is set.
re-review-only-ACK 1f072cfb108d8f71daf6281d97353ff755af9569
Only change is a comment
But I only have a vague idea of how this should get used in practice. It would be helpful to add some documentation to https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md to say how/when you could run the fuzz binary in debug mode, and when you would might want to set the FUZZ_NONDETERMINISM environment variable.
I think there’s two reasons to do this:
It’s an easier way to reproduce most fuzzer crashes (ie, ones that don’t need a sanitizer to trigger); just grab your existing debug build, and run
0$ FUZZ=process_message build_debug/bin/fuzz corpus/1bc91feec9fc00b107d97dc225a9f2cdaa078eb6`
and it’s also compatible with gdb
0$ FUZZ=process_message gdb build_debug/bin/fuzz
1run corpus/1bc91feec9fc00b107d97dc225a9f2cdaa078eb6
It lets you include the fuzz test corpora (assuming they all pass) in a regular code coverage build, something like:
0PRINT_ALL_FUZZ_TARGETS_AND_ABORT=1 ../build/bin/fuzz |
1 while read tgt; do
2 d=qa-assets/fuzz_corpora/$tgt;
3 if [ -d "$d" ]; then
4 (echo "*** $tgt ***";
5 time FUZZ=$tgt ../build/bin/fuzz $d) >OUT_$tgt.log 2>&1 &
6 else
7 echo SKIPPED $tgt -- no corpus;
8 fi;
9 done
Not really sure what should go in the docs or not, hopefully the above is enough explanation if someone else wants to add something.
- It lets you include the fuzz test corpora (assuming they all pass) in a regular code coverage build, something like:
Maybe I am missing something, but I guess you meant “debug mode coverage build” instead of “regular code coverage build”, because the regular coverage build has neither fuzz, nor debug enabled. For me it looks something like:
0C++ compiler .......................... GNU 14.2.0, /usr/bin/g++
1CMAKE_BUILD_TYPE ...................... Coverage
2Preprocessor defined macros ...........
3C++ compiler flags .................... -fprofile-update=atomic -g -Og --coverage -std=c++20 -fPIC -fno-extended-identifiers -fdebug-prefix-map=/tmp/cirrus-ci-build/bitcoin-core/src=. -fmacro-prefix-map=/tmp/cirrus-ci-build/bitcoin-core/src=. -fstack-reuse=none -Wall -Wextra -Wformat -Wformat-security -Wvla -Wredundant-decls -Wdate-time -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wbidi-chars=any -Wundef -Wno-unused-parameter -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection
- It lets you include the fuzz test corpora (assuming they all pass) in a regular code coverage build, something like:
Maybe I am missing something, but I guess you meant “debug mode coverage build” instead of “regular code coverage build”, because the regular coverage build has neither fuzz, nor debug enabled. For me it looks something like:
Sure; I meant “regular” just in “it builds all the binaries and you can run non-fuzz tests”.
