Disallow building fuzz binary without `-DBUILD_FOR

dergoegge commented at 12:49 pm on October 8, 2024: member

Without -DBUILD_FOR_FUZZING the fuzz binary is less suited for testing puporses, because it won’t crash on Assume and it won’t work around fuzz blockers with FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION. Afaict, the only place where the fuzz binary is currently used without -DBUILD_FOR_FUZZING is in the macOS and windows CI jobs.

I would like to propose that we split the CI jobs into separate fuzz-only jobs that use -DBUILD_FOR_FUZZING and that we remove support for building the fuzz binary without it.

Historically, we used to build most binaries by default (including the fuzz binary) to ensure that compile failures are caught early by devs. With the switch to CMake this is no longer the case and imo that’s fine since compile failures would be caught by the CI or other devs that regularly fuzz.

This would also avoid the need for workarounds like #31028.

dergoegge commented at 12:49 pm on October 8, 2024: member

cc @maflcko @marcofleon

marcofleon commented at 1:42 pm on October 8, 2024: contributor

This makes sense to me. Could be missing something, but I don’t see a strong reason for building a fuzz binary without the specfic behaviors and optimizations we would like for fuzzing. If there’s support for this, I could close #31028 and help out with this instead.

maflcko commented at 3:17 pm on October 8, 2024: member

Historically, we used to build most binaries by default (including the fuzz binary) to ensure that compile failures are caught early by devs. With the switch to CMake this is no longer the case

Why would it no longer be the case with cmake? Are you proposing that every switches to a multi-config build?

compile failures would be caught by the CI or other devs that regularly fuzz.

How would the CI catch compile or runtime failures on Windows or macOS, when the fuzz exe isn’t compiled and no one regularly fuzzes on them? Are you proposing to drop the CI coverage?

maflcko added the label Tests on Oct 8, 2024

dergoegge commented at 3:30 pm on October 8, 2024: member

Why would it no longer be the case with cmake? Are you proposing that every switches to a multi-config build?

What I meant, is that we no longer build the fuzz binary by default i.e. -DBUILD_FOR_FUZZING=ON or -DBUILD_FUZZ_BINARY=ON has to be added. Prior to cmake ./configure && make would produce the fuzz binary, while now cmake -B build/ && cmake --build build/ will not.

How would the CI catch compile or runtime failures on Windows or macOS, when the fuzz exe isn’t compiled and no one regularly fuzzes on them? Are you proposing to drop the CI coverage?

From the issue description: “I would like to propose that we split the CI jobs into separate fuzz-only jobs that use -DBUILD_FOR_FUZZING”

maflcko commented at 3:34 pm on October 8, 2024: member

Why would it no longer be the case with cmake? Are you proposing that every switches to a multi-config build?

What I meant, is that we no longer build the fuzz binary by default i.e. -DBUILD_FOR_FUZZING=ON or -DBUILD_FUZZ_BINARY=ON has to be added. Prior to cmake ./configure && make would produce the fuzz binary, while now cmake -B build/ && cmake --build build/ will not.

I think the vanilla cmake -B build is for some imaginary end-user, not for developers. I imagine devs use something like https://github.com/bitcoin/bitcoin/blob/a9f6a57b6918b2f92c7d6662e8f5892bf57cc127/CMakePresets.json#L65, so going forward they’d have to switch to a multi-config build to achieve the same?

ryanofsky commented at 5:39 pm on October 8, 2024: contributor

@dergoegge can you clarify what change is being proposed and how it would affect me if I am building with:

0BUILD_FOR_FUZZING:BOOL=OFF
1BUILD_FUZZ_BINARY:BOOL=ON

I build with these options to be able to be able to know if changes not related to fuzzing will break the build.

ryanofsky commented at 6:36 pm on October 8, 2024: contributor

Looking into windows timeout issue #30950 and the workaround #31028 disabling a subset of fuzz tests when BUILD_FOR_FUZZING is false, I wonder if we can’t just go with a simpler solution of fuzz testing normal libraries instead of building all the libraries specially for fuzz testing. Would a fix like the following not work?

 0--- a/CMakeLists.txt
 1+++ b/CMakeLists.txt
 2@@ -240,11 +240,6 @@ if(BUILD_FOR_FUZZING)
 3   set(BUILD_GUI_TESTS OFF)
 4   set(BUILD_BENCH OFF)
 5   set(BUILD_FUZZ_BINARY ON)
 6-
 7-  target_compile_definitions(core_interface INTERFACE
 8-    ABORT_ON_FAILED_ASSUME
 9-    FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
10-  )
11 endif()
12 
13 include(ProcessConfigurations)
14--- a/src/pow.cpp
15+++ b/src/pow.cpp
16@@ -9,6 +9,7 @@
17 #include <chain.h>
18 #include <primitives/block.h>
19 #include <uint256.h>
20+#include <util/check.h>
21 
22 unsigned int GetNextWorkRequired(const CBlockIndex* pindexLast, const CBlockHeader *pblock, const Consensus::Params& params)
23 {
24@@ -138,11 +139,8 @@ bool PermittedDifficultyTransition(const Consensus::Params& params, int64_t heig
25 // the most signficant bit of the last byte of the hash is set.
26 bool CheckProofOfWork(uint256 hash, unsigned int nBits, const Consensus::Params& params)
27 {
28-#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
29-    return (hash.data()[31] & 0x80) == 0;
30-#else
31+    if (g_fuzzing) return (hash.data()[31] & 0x80) == 0;
32     return CheckProofOfWorkImpl(hash, nBits, params);
33-#endif
34 }
35 
36 bool CheckProofOfWorkImpl(uint256 hash, unsigned int nBits, const Consensus::Params& params)
37--- a/src/test/fuzz/fuzz.cpp
38+++ b/src/test/fuzz/fuzz.cpp
39@@ -102,6 +102,8 @@ void ResetCoverageCounters() {}
40 
41 void initialize()
42 {
43+    g_fuzzing = true;
44+
45     // By default, make the RNG deterministic with a fixed seed. This will affect all
46     // randomness during the fuzz test, except:
47     // - GetStrongRandBytes(), which is used for the creation of private key material.
48--- a/src/util/check.cpp
49+++ b/src/util/check.cpp
50@@ -14,6 +14,8 @@
51 #include <string>
52 #include <string_view>
53 
54+bool g_fuzzing = false;
55+
56 std::string StrFormatInternalBug(std::string_view msg, std::string_view file, int line, std::string_view func)
57 {
58     return strprintf("Internal bug detected: %s\n%s:%d (%s)\n"
59--- a/src/util/check.h
60+++ b/src/util/check.h
61@@ -13,6 +13,8 @@
62 #include <string_view>
63 #include <utility>
64 
65+extern bool g_fuzzing;
66+
67 std::string StrFormatInternalBug(std::string_view msg, std::string_view file, int line, std::string_view func);
68 
69 class NonFatalCheckError : public std::runtime_error
70@@ -50,7 +52,7 @@ T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] const char* f
71         if (!val) {
72             assertion_fail(file, line, func, assertion);
73         }
74-    }
75+    } else if (g_fuzzing && !val) abort();
76     return std::forward<T>(val);
77 }
78

dergoegge commented at 10:31 am on October 9, 2024: member

I build with these options to be able to be able to know if changes not related to fuzzing will break the build.

I assumed (incorrectly) that almost no one would be doing this anymore since building the fuzz binary is no longer the default behavior. What I’m proposing would require you to do a separate build with -DBUILD_FOR_FUZZING=ON, which is of course annoying if you just want to check that the fuzz binary compiles.

Another assumption I have (perhaps also incorrect) is that no one (besides the CI atm) executes the fuzz binary that wasn’t build with -DBUILD_FOR_FUZZING=ON. To reproduce fuzzing issues one often has to turn on sanitizers or enable Assume crashes, which requires recompilation in any case.

The problem I’m trying to address is that we currently produce two different fuzz binaries i.e. the binary produced with -DBUILD_FOR_FUZZING=ON is different from the binary produced -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=OFF.

The solution I’m proposing is to get rid of the latter, because imo it is odd to produce a fuzz binary that is not “build for fuzzing” i.e. it doesn’t have all the bells and whistles that one would want when executing the binary (e.g. to reproduce an issue). We can also make both binaries behave the same by turning the compile-time checks into run-time checks (i.e. your suggestion #31057 (comment)).

My solution is flawed in that it makes checking for build breakage quite annoying. Perhaps there is a third approach that still allows to check for build breakage while not actually producing a binary that won’t be executed (which would let us keep the compile time differences). I’d prefer to go with compile-time checks if we can.

theuni commented at 8:07 pm on October 9, 2024: member

FWIW, I build same as @ryanofsky in order to know that I haven’t broken anything. @dergoegge We could use FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION to avoid registering any targets (rather than attempting to being selective like in #31028), and just exit(0) with a helpful message.

Or we could wrap it in a static lib and only produce a binary if BUILD_FOR_FUZZING.

ryanofsky commented at 8:40 pm on October 9, 2024: contributor

I think I’m not clear on what are the drawbacks of the suggestion in #31057 (comment). It seems like that would fix the issues, simplify the build, make meaning of the cmake options clearer.

I also like cory’s suggestion of building fuzzing code just as a static library instead of as an executable if it really is not useful for any runtime testing.

Perhaps there is a third approach that still allows to check for build breakage while not actually producing a binary that won’t be executed (which would let us keep the compile time differences). I’d prefer to go with compile-time checks if we can.

One approach I considered instead of defining global variable bool g_fuzzing = false; and setting g_fuzzing = true; in the fuzz binary was defining a global constant with weak linkage extern const bool IS_FUZZING __attribute__((weak)) = false; that could be overridden in the fuzz binary extern const bool IS_FUZZING = true;. This just seemed more complicated and less portable.

In general it seems like a good thing to avoid unnecessary differences when fuzzing and nonfuzzing, and to try to keep any differences that exist simple and easy to understand.

fanquake commented at 8:42 am on October 10, 2024: member

From the issue description: “I would like to propose that we split the CI jobs into separate fuzz-only jobs that use -DBUILD_FOR_FUZZING”

I think regardless of other changes we make with any build options, we should do this in any case, and have a fuzz job per Linux/Windows/macOS.

dergoegge commented at 10:42 am on October 16, 2024: member

I opened #31093, implementing the suggestion from #31057 (comment).

fanquake referenced this in commit 1c7ca6e64d on Oct 28, 2024

maflcko closed this on Oct 28, 2024

Disallow building fuzz binary without -DBUILD_FOR_FUZZING #31057

Disallow building fuzz binary without `-DBUILD_FOR_FUZZING` #31057