Fuzzing related configuration/build options can be improved

vasild commented at 4:48 am on June 21, 2024: contributor

Please describe the feature you’d like to see added.

The fuzzing related configuration options are somewhat confusing and redundant and can be simplified.

TLDR: currently --enable-fuzz does not enable fuzzing.

Thanks for the discussion yesterday. Correct me if I am wrong below. There are the following options (to ./configure and in the coming CMake replacement):

--enable-fuzz (default no), note that this option does not engage the fuzzing framework. That is, it does not enable fuzzing :exclamation::question: :
- disables compiling all other targets (e.g. src/bitcoind); other targets should be disabled because the fuzzing framework provides its own main() function
- forces --enable-fuzz-binary=yes
- enables ABORT_ON_FAILED_ASSUME
--enable-fuzz-binary (default yes): whether to compile src/test/fuzz/fuzz
--with-sanitizers=fuzzer (default no sanitizers): engage the fuzzing framework, ie enable fuzzing.

Without the fuzzing framework (--with-sanitizers=fuzzer) src/test/fuzz/fuzz cannot do fuzzing but it can run existent inputs from e.g. https://github.com/bitcoin-core/qa-assets/tree/main/fuzz_seed_corpus (phony fuzz binary). So currently all of the below produce such a phony fuzz binary:

compiling by default without any options
compiling with --enable-fuzz
compiling with --enable-fuzz-binary

Describe the solution you’d like

Ideally there should be one boolean option which enables/disables fuzzing instead of 3:

when enabled: engage the fuzzing harness (compile with -fsanitize=fuzzer), force compiling src/test/fuzz/fuzz (if it is even possible to disable it via another option), force disable all other targets
when disabled: don’t use -fsanitize=fuzzer, leave alone src/test/fuzz/fuzz and other build targets to be decided by other options. If src/test/fuzz/fuzz is to be build, then create a “phony fuzz” binary which cannot do fuzzing but can execute existent fuzz inputs.

Describe any alternatives you’ve considered

The “enabled/disabled” option can be implicit - if --with-sanitizers=...,fuzzer,... is used then do as “when enabled” above.

vasild added the label Feature on Jun 21, 2024

maflcko commented at 6:07 am on June 21, 2024: member

when enabled: engage the fuzzing harness (compile with -fsanitize=fuzzer), force compiling src/test/fuzz/fuzz (if it is even possible to disable it via another option), force disable all other targets

More than one sanitizer exists. For example, the CI system uses more than one sanitizer. So afterwards you’d have two redundant and confusing ways to set sanitizers. Not sure that is an improvement.

vasild commented at 7:22 am on June 21, 2024: contributor

Feel free to propose better solutions, the one in “Describe the solution you’d like” in OP is just an idea.

--with-sanitizers can be used to enable additional sanitizers. Why would that be confusing? Because when the fuzzing is enabled, then -fsanitize=fuzzer would be used for compilation and then it is redundant if one does --with-sanitizers=fuzzer in addition? That would be harmless and produce the expected results. Much like if one uses --enable-werror and in addition sets CXXFLAGS=-Werror.

maflcko commented at 7:37 am on June 21, 2024: member

That would be harmless and produce the expected results.

No objection if a duplicate -fsanitize=fuzzer is harmless and works in all build settings.

maflcko added the label Build system on Jun 21, 2024

maflcko added the label Tests on Jun 21, 2024

dergoegge commented at 9:29 am on June 21, 2024: member

There are a few things that --enable-fuzz does that we definitely want it to do:

As you said, this option disables all other binaries besides the fuzz binary. This is nice when you only want to do fuzzing and nothing else (e.g. oss-fuzz). It is also required for the use of libFuzzer (--with-sanitizers=fuzzer i.e. -fsanitize=fuzzer with clang), because the other binaries provide a main function.
libFuzzer is not the only fuzzing engine. We would like to support as many engines as possible and we don’t want to enable -fsanitize=fuzzer by default because that is not guaranteed to work with other engines. For example, to build for fuzzing with afl++: CC=afl-clang-fast CXX=afl-clang-fast++ ./configure --enable-fuzz && make. We have a custom harness for fuzzing with afl++ (see here).

We could rename the option if that helps but we shouldn’t change the way it works. I’m not even sure if a rename is worth it given that it would break every fuzzing script. Our docs explain how to properly setup fuzzing as well (including instructions for various other engines).

vasild commented at 10:06 am on June 21, 2024: contributor

Ok, a few takeaways:

Don’t enable -fsanitize=fuzzer automatically by e.g. --enable-fuzz because that would conflict with other fuzzers.
A better name for --enable-fuzz would be --disable-all-build-targets-except-fuzz or --build-only-fuzz or --enable-fuzz-binary=yes|no|exclusive, but renaming it would break every fuzzing script. Maybe the transition to CMake is a good time to improve this since scripts would have to be adjusted anyway.

maflcko commented at 10:14 am on June 21, 2024: member

--enable-fuzz-binary=yes|no|exclusive, but renaming it would break every fuzzing script. Maybe the transition to CMake is a good time to improve this since scripts would have to be adjusted anyway.

Seems fine to do, if -DBUILD_FUZZ_BINARY=ON/OFF/EXCL... is easy to implement.

maflcko added the label Needs CMake port on Jun 22, 2024

maflcko commented at 3:12 pm on July 16, 2024: member

However, it would be good, to implement this before #30454 is merged. Otherwise there will be two breaking changes for each fuzz infra deployment.

hebasto commented at 3:14 pm on July 16, 2024: member

--enable-fuzz-binary=yes|no|exclusive, but renaming it would break every fuzzing script. Maybe the transition to CMake is a good time to improve this since scripts would have to be adjusted anyway.

Seems fine to do, if -DBUILD_FUZZ_BINARY=ON/OFF/EXCL... is easy to implement.

It’s not a problem at all.

hebasto commented at 8:37 am on July 20, 2024: member

There are a few things that --enable-fuzz does that we definitely want it to do:

As you said, this option disables all other binaries besides the fuzz binary. This is nice when you only want to do fuzzing and nothing else (e.g. oss-fuzz). It is also required for the use of libFuzzer (--with-sanitizers=fuzzer i.e. -fsanitize=fuzzer with clang), because the other binaries provide a main function.

This functionality does not require any build options. The fuzz binary can be built explicitly in both build systems:

Autotools:

0$ ./configure
1$ make -C src test/fuzz/fuzz

CMake:

0$ cmake -B build -DBUILD_FUZZ_BINARY=ON
1$ cmake --build build -t fuzz

maflcko commented at 8:32 am on July 22, 2024: member

--enable-fuzz-binary=yes|no|exclusive, but renaming it would break every fuzzing script. Maybe the transition to CMake is a good time to improve this since scripts would have to be adjusted anyway.

Seems fine to do, if -DBUILD_FUZZ_BINARY=ON/OFF/EXCL... is easy to implement.

It’s not a problem at all.

On thing that would be confusing, is that EXCL... (or whatever the third state is) would have to also enable -DABORT_ON_FAILED_ASSUME. At this point, I question which approach is less confusing.

vasild commented at 12:50 pm on July 22, 2024: contributor

Ok, so in summary -DENABLE_FUZZ=ON does 1. force BUILD_FUZZ_BINARY=ON, 2. disable all other targets, 3. enable ABORT_ON_FAILED_ASSUME. Just that. It does not enable fuzzing. @hebasto mentioned, “This functionality does not require any build options”, so to avoid this confusing option, what about:

Remove -DENABLE_FUZZ and introduce -DABORT_ON_FAILED_ASSUME=ON|OFF? Then the current workflow:

0cmake -DSANITIZERS=fuzzer -DENABLE_FUZZ=ON -B /tmp/build
1cmake --build /tmp/build

would become

0cmake -DSANITIZERS=fuzzer -DABORT_ON_FAILED_ASSUME=ON -B /tmp/build
1cmake --build /tmp/build --target fuzz

maflcko commented at 12:55 pm on July 22, 2024: member

Remove -DENABLE_FUZZ and introduce -DABORT_ON_FAILED_ASSUME=ON|OFF? Then the current workflow:

I’d still want a fuzz-specific build option to enable fuzz specific build flags. Otherwise, if more fuzz-specific build flags are added in the future, it will be a global breaking change again.

would become

The problem would be that someone forgetting to specify --target fuzz would run into link errors in the other targets. (I think it is fine, but I wanted to mention it).

vasild commented at 1:28 pm on July 22, 2024: contributor

@maflcko, there are no “fuzz specific build flags”. ABORT_ON_FAILED_ASSUME is not fuzz specific, it is also enabled for -DCMAKE_BUILD_TYPE=Debug. Anyway, I guess you mean “fuzz needed”, right? Would you want to

Have an option to enable ABORT_ON_FAILED_ASSUME plus any other future options that would be needed for fuzzing? or
The above 1. plus force BUILD_FUZZ_BINARY=ON and disable all other targets?

maflcko commented at 1:44 pm on July 22, 2024: member

there are no “fuzz specific build flags”

I am not a native speaker, but I wanted to say “build flags that make sense in the context of fuzzing”. They are not “needed”, rather “recommended”.

vasild commented at 4:41 pm on July 22, 2024: contributor

Which are those flags?

maflcko commented at 6:38 am on July 23, 2024: member

Which are those flags?

DABORT_ON_FAILED_ASSUME right now, but there can be more in the future. See for example https://duckduckgo.com/?q=FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION . So having a build system flag to enable all of them is useful. Otherwise, if more fuzz-recommended build flags are added in the future, it will be a global breaking change again.

vasild commented at 8:04 am on July 23, 2024: contributor

Ok, that would be 1. from above #30318 (comment). Just to clarify - this option would not be responsible for setting up build targets and would not add -fsanitize=fuzzer, right?

maflcko commented at 8:22 am on July 23, 2024: member

Just to clarify - this option would not be responsible for setting up build targets

I don’t care, for me anything works here. Just as a reminder, if the build targets aren’t set up at all, someone forgetting to specify --target fuzz during the build manually would run into link errors in the other targets (Because there could be conflicts between the fuzz engine’s main, and the other targets’ main). (I think those errors are fine, but I wanted to mention it). Also, forgetting --target fuzz could lead to accidentally longer build times.

and would not add -fsanitize=fuzzer, right?

Correct. libFuzzer is just one fuzz engine. The dev may want to pick a different one.

vasild referenced this in commit e93df1a1be on Jul 23, 2024

vasild commented at 11:23 am on July 23, 2024: contributor

Opened https://github.com/hebasto/bitcoin/pull/276 to address this. I decided to preserve the behavior of the option controlling ABORT_ON_FAILED_ASSUME plus to set up the build targets because that will hopefully result in less friction with people that are already familiar with the existent workflow. Just rename the option.

As a fancy extension, just an idea, maybe detect if -DSANITIZERS=fuzzer has been used and enable the option automatically.

vasild referenced this in commit fa89f1c155 on Jul 24, 2024

hebasto referenced this in commit 38f75be6e8 on Jul 24, 2024

hebasto closed this on Jul 24, 2024

hebasto referenced this in commit 8853a2e2ca on Jul 24, 2024

maflcko removed the label Needs CMake port on Jul 25, 2024

Fuzzing related configuration/build options can be improved #30318

Please describe the feature you’d like to see added.

Is your feature related to a problem, if so please describe it.

Describe the solution you’d like

Describe any alternatives you’ve considered