build: build fuzz tests by default #20936

danben · 2021-01-14T16:26:04Z

This fixes issue #19388. The changes are as follows:

Add a new flag to configure, --enable-fuzz-binary, which allows building test/fuzz/fuzz regardless of whether we are building to do actual fuzzing
Set -DPROVIDE_MAIN_FUNCTION whenever --enable-fuzz is no
Add the following libraries to FUZZ_SUITE_LD_COMMON:
- LIBBITCOIN_WALLET
- SQLLITE_LIBS
- BDB_LIBS
- if necessary, some or all of:
  - NATPMP_LIBS
  - MINIUPNPC_LIBS
  - LIBBITCOIN_ZMQ / ZMQ_LIBS

Fixes #19388

practicalswift · 2021-01-20T10:03:23Z

Concept ACK on building fuzz tests by default, but needs squashing and fixing of failing CI jobs :)

Haven't reviewed any code or build changes.

fanquake

I'd suggest splitting up this PR, because it seems like there are at least two separate changes here. i.e f1be902 "Allow building the fuzz tests under windows" shouldn't be related to whether or not we build the fuzz tests by default.

In any case, you need to rebase and fix up your branch, so that it only contains your changes. You can't have merge commits like 04f5799. It looks like you've also got the same changes in multiple commits, i.e 622a2e7 and 0f2bdb1.

Please don't close and open a new PR.

configure.ac

DrahtBot · 2021-01-22T20:28:51Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

Use std::filesystem. Remove Boost Filesystem & System #20744 ([POC] Use std::filesystem. Remove Boost Filesystem & System by fanquake)
Fix Windows build with --enable-werror #20586 (Fix Windows build with --enable-werror by hebasto)
fuzz: Add fuzzing harness for LoadMempool(...) and DumpMempool(...) #19259 (fuzz: Add fuzzing harness for LoadMempool(...) and DumpMempool(...) by practicalswift)
test: add v0.20.1, v0.21.0 and v22.0 to backwards compatibility test #19013 (test: add v0.20.1 to backwards compatibility test by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

sipa · 2021-01-23T23:34:39Z

The standard "formula" for a generic swap is:

...
{
    using std::swap;
    swap(a, b);
}
...

Not sure what the problem is, but maybe that's worth trying.

danben · 2021-01-23T23:46:48Z

The standard "formula" for a generic swap is:
...
{
    using std::swap;
    swap(a, b);
}
...
Not sure what the problem is, but maybe that's worth trying.

Thanks, I just hadn't looked carefully enough - it was a swap where one side was a vector location, and Element happened to be bool. Fingers crossed that this fixed it and I can clean up for review.

danben · 2021-01-24T02:50:51Z

Well...almost there. I tried for a while to squash things down into a single commit, but I think Ive failed. The correct patch is at c7746f0 (except for the fact that I accidentally chopped off the first two lines of the commit message and now the linter is complaining), but I don't know how to get rid of the history (I used git rebase -i and manually squashed/dropped and this was the result). If someone could tell me how to fix this that would be much appreciated.

As far as the actual review, the substring match that I introduced feels kind of sketchy but I wasn't sure how else to accomplish what I wanted. The intent was to do some stuff only when libfuzzer was linked, which is what I thought the old check wanted to do as well but it still passed (that is, suppressed a main function etc) on e.g -fsanitize=thread. So I'm confused about what's going on there and would like feedback on that part in particular.

maflcko · 2021-01-24T07:59:31Z

If you want to squash everything into one commit:

Reset the state to the latest commit that is in your branch, but not one of your changes.
This is usually the latest commit from master that you merged in.
In your case, it is 32b191f.
Softly reset to that commit, so that all your changes are staged as one in the git staging area.

git reset --soft 32b191fb66e644c690c94cbfdae6ddbc754769d7

Now commit the staged changes with a message

git commit -m 'build: build fuzz tests by default'

push as usual ...

sipa · 2021-01-25T08:05:55Z

Code-wise, this looks pretty good, and in a short test it also seems to work.

I'm not entirely sure about the meaning of --{enable,disable}-fuzz that this introduces. There are really two distinct concepts now that are not exactly the same anymore:

Whether or not you're building for some form of actual fuzzing (whether through -fsanitize=fuzzer, AFL, or something else). Doing any of these should disable all other targets.
Whether or not you want to build the fuzz code/binary (which if fuzzing is enabled, will be what does the fuzzing; but otherwise is just a simple test binary that invokes the fuzz targets).

So I think this leads to problems if you'd want to fuzz with AFL. You'd need to specify (or get default) --enable-fuzz, and not enable -fsanitize=fuzzer, resulting in a situation where all the non-fuzz make targets aren't disabled. I don't think this is a big issue, and I'm perfectly fine with getting this merged as is, and iterate later. However, I think the following alternative may actually be less invasive change:

Add a new --{enable,disable}-fuzz-binary option (default on) to indicate the need to build a ./src/test/fuzz/fuzz binary
Leave --{enable,disable}-fuzz as option (default off) to indicate a fuzzing build (which disables all other make targets, and enables --enable-fuzz-binary)
The existing detection for whether a main function is needed can stay.

danben · 2021-01-28T21:34:04Z

@sipa Based on your comment I wasn't sure if I should make the change you suggested or wait for someone else to weigh in, but I wanted to make sure that I'm not dropping the ball on anything.

I agree that your suggestion would be easy to implement. If we don't want to add another flag (perhaps desirable from a usability perspective?), I could also just disable non-fuzz targets when building with AFL or Honggfuzz.

sipa · 2021-01-28T21:44:57Z

@danben Yeah, up to you. Feel free to keep the PR as-is.

I don't think there is much convenience difference; in general nobody should touch --disable-fuzz-binary in general (except maybe we do so for release builds).

maflcko

I couldn't build a library with afl++, though the binaries did compile.

I expect this issue to "fix itself" once you address #20936 (comment)

root@9d90aa3c2826:/bitcoin# make
Making all in src
make[1]: Entering directory '/bitcoin/src'
make[2]: Entering directory '/bitcoin/src'
make[3]: Entering directory '/bitcoin'
make[3]: Leaving directory '/bitcoin'
  CXXLD    libbitcoinconsensus.la
afl-cc ++3.01a by Michal Zalewski, Laszlo Szekeres, Marc Heuse - mode: LLVM-PCGUARD
Using unoptimized trace-pc-guard, upgrade to llvm 10.0.1+ for enhanced version.
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:80: multiple definition of `__afl_area_ptr'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:80: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:81: multiple definition of `__afl_area_ptr_backup'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:81: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:79: multiple definition of `__afl_area_ptr_dummy'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:79: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_auto_early':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1045: multiple definition of `__afl_auto_early'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1045: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_auto_first':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1088: multiple definition of `__afl_auto_first'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1088: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_auto_init':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1033: multiple definition of `__afl_auto_init'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1033: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_auto_second':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1057: multiple definition of `__afl_auto_second'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1057: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o:(.tbss+0x24): multiple definition of `__afl_cmp_counter'; /AFLplusplus/afl-compiler-rt.o:(.tbss+0x24): first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_coverage_discard':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1422: multiple definition of `__afl_coverage_discard'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1422: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_coverage_interesting':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1452: multiple definition of `__afl_coverage_interesting'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1452: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_coverage_off':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1400: multiple definition of `__afl_coverage_off'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1400: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_coverage_on':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1412: multiple definition of `__afl_coverage_on'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1412: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_coverage_discard':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1432: multiple definition of `__afl_coverage_skip'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1432: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:85: multiple definition of `__afl_fuzz_len'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:85: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_manual_init':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1004: multiple definition of `__afl_manual_init'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1004: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:88: multiple definition of `__afl_map_size'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:88: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_persistent_loop':
llvm_mode/instrumentation/afl-compiler-rt.o.c:945: multiple definition of `__afl_persistent_loop'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:945: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o:(.tbss+0x20): multiple definition of `__afl_prev_ctx'; /AFLplusplus/afl-compiler-rt.o:(.tbss+0x20): first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o:(.tbss+0x0): multiple definition of `__afl_prev_loc'; /AFLplusplus/afl-compiler-rt.o:(.tbss+0x0): first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:95: multiple definition of `__afl_selective_coverage_temp'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:95: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__afl_trace':
llvm_mode/instrumentation/afl-compiler-rt.o.c:135: multiple definition of `__afl_trace'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:135: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook1':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1212: multiple definition of `__cmplog_ins_hook1'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1212: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook2':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1235: multiple definition of `__cmplog_ins_hook2'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1235: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook4':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1256: multiple definition of `__cmplog_ins_hook4'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1256: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook8':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1277: multiple definition of `__cmplog_ins_hook8'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1277: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_rtn_hook':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1365: multiple definition of `__cmplog_rtn_hook'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1365: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook1':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1212: multiple definition of `__sanitizer_cov_trace_cmp1'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1212: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook2':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1235: multiple definition of `__sanitizer_cov_trace_cmp2'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1235: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook4':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1256: multiple definition of `__sanitizer_cov_trace_cmp4'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1256: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook8':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1277: multiple definition of `__sanitizer_cov_trace_cmp8'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1277: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook1':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1212: multiple definition of `__sanitizer_cov_trace_const_cmp1'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1212: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook2':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1235: multiple definition of `__sanitizer_cov_trace_const_cmp2'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1235: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook4':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1256: multiple definition of `__sanitizer_cov_trace_const_cmp4'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1256: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__cmplog_ins_hook8':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1277: multiple definition of `__sanitizer_cov_trace_const_cmp8'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1277: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__sanitizer_cov_trace_pc_guard':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1148: multiple definition of `__sanitizer_cov_trace_pc_guard'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1148: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__sanitizer_cov_trace_pc_guard_init':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1163: multiple definition of `__sanitizer_cov_trace_pc_guard_init'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1163: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `__sanitizer_cov_trace_switch':
llvm_mode/instrumentation/afl-compiler-rt.o.c:1326: multiple definition of `__sanitizer_cov_trace_switch'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:1326: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `at_exit':
llvm_mode/instrumentation/afl-compiler-rt.o.c:129: multiple definition of `at_exit'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:129: first defined here
/usr/bin/ld: /AFLplusplus/afl-compiler-rt.o: in function `send_forkserver_error':
llvm_mode/instrumentation/afl-compiler-rt.o.c:162: multiple definition of `send_forkserver_error'; /AFLplusplus/afl-compiler-rt.o:llvm_mode/instrumentation/afl-compiler-rt.o.c:162: first defined here
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [Makefile:5482: libbitcoinconsensus.la] Error 1
make[2]: Leaving directory '/bitcoin/src'
make[1]: *** [Makefile:15061: all-recursive] Error 1
make[1]: Leaving directory '/bitcoin/src'
make: *** [Makefile:809: all-recursive] Error 1

maflcko · 2021-01-25T06:52:08Z

src/cuckoocache.h

            // Can't std::swap a std::vector<bool>::reference and a bool&.
+            Element el = std::move(table[last_loc]);
+            table[last_loc] = std::move(e);
+            e = std::move(el);


I don't really like changing code in a build change. Maybe exclude the failing ci config via --disable-fuzz for now?

Actually I'm a bit baffled by this one - the reason for this change was the instantiation of a bool cache in src/test/fuzz/cuckoocache.cpp, which looks like it would have been built whenever enable-fuzz was true. I don't understand how that ever compiled, but if you don't know the answer offhand I'll investigate.

All compilers are broken (as in: They compile wrong code without error, or conversely compile correct code into an incorrect binary). But we should document this brokenness as best as we can, and fix incorrect code separate from feature changes or refactoring or build system changes. Mixing everything into one commit/patchset will make it harder to review and bisect later on.

configure.ac

maflcko · 2021-01-29T10:02:51Z

src/test/fuzz/fuzz.cpp

 __attribute__((weak)) int main(int argc, char** argv)
+#endif


I don't really like changing code in a build change. Maybe exclude the failing ci config via --disable-fuzz for now?

maflcko · 2021-01-29T10:03:08Z

src/test/fuzz/netaddress.cpp

 #include <netinet/in.h>
+#endif


I don't really like changing code in a build change. Maybe exclude the failing ci config via --disable-fuzz for now?

maflcko · 2021-01-29T10:03:14Z

src/test/fuzz/string.cpp

    (void)ShellEscape(random_string_1);
+    #endif


I don't really like changing code in a build change. Maybe exclude the failing ci config via --disable-fuzz for now?

maflcko · 2021-01-29T10:03:21Z

src/test/fuzz/util.h

        const in_addr v4_addr = {
            .s_addr = fuzzed_data_provider.ConsumeIntegral<uint32_t>()};
+        #endif


I don't really like changing code in a build change. Maybe exclude the failing ci config via --disable-fuzz for now?

So this should probably be --disable-fuzz-binary in the windows ci (instead of my suggestion --disable-fuzz, which might not go well with how configure is supposed to work)

…ests dee2d6f fuzz: Avoid designated initialization (C++20) in fuzz tests (practicalswift) Pull request description: Avoid designated initialization (C++20) in fuzz tests. Context: bitcoin/bitcoin#20197 (comment), bitcoin/bitcoin#20936 (comment) ACKs for top commit: MarcoFalke: cr ACK dee2d6f dhruv: code review ACK dee2d6f ajtowns: utACK dee2d6f Tree-SHA512: 5940fab6e97a2b11dd3b1475d2cffa2840dc2e6ec34bd9f9df90f948709cab98fd1c513d5dd104816d33a525a6e9710b8715b02db941e35d84f92bc211f56d1d

danben · 2021-02-02T00:50:45Z

Turned review back on. Two things to note:

I have no idea what this appveyor failure is trying to tell me. It involves a file that I didn't touch.
I left the new substring check for PROVIDE_MAIN_FUNCTION in order to be able to build some of the other sanitizer tests without disabling the fuzz binary. If anyone feels strongly that this should be reverted then I will.

… fuzz tests dee2d6f fuzz: Avoid designated initialization (C++20) in fuzz tests (practicalswift) Pull request description: Avoid designated initialization (C++20) in fuzz tests. Context: bitcoin#20197 (comment), bitcoin#20936 (comment) ACKs for top commit: MarcoFalke: cr ACK dee2d6f dhruv: code review ACK dee2d6f ajtowns: utACK dee2d6f Tree-SHA512: 5940fab6e97a2b11dd3b1475d2cffa2840dc2e6ec34bd9f9df90f948709cab98fd1c513d5dd104816d33a525a6e9710b8715b02db941e35d84f92bc211f56d1d

maflcko · 2021-02-02T10:25:37Z

configure.ac

@@ -1221,6 +1227,14 @@ dnl it would break GCC's #include_next.
 AC_DEFUN([SUPPRESS_WARNINGS],
         [$(echo $1 |${SED} -E -e 's/(^| )-I/\1-isystem /g' -e 's;-isystem /usr/include([/ ]|$);-I/usr/include\1;g')])

+AC_MSG_CHECKING([whether test/fuzz/fuzz main function is needed])
+if [[[ $use_sanitizers == *"fuzzer"* ]]]; then


I am pretty sure this will break honggfuzz. Is there anything wrong with how this was checked previously?

Ok, once you rebase I expect honggfuzz to be broken

maflcko

Concept ACK. Looks better now, except that it breaks honggfuzz

danben · 2021-02-02T17:28:58Z

@MarcoFalke Prior to reopening I built and ran honggfuzz (as per doc/fuzzing.md) with no issue. Perhaps it's something to do with my system; how does it break for you?

Re: your question above, I had changed it because (for reasons that I still don't quite understand) the check would pass for me even when specifying e.g. --fsanitize=thread which caused several CI tests to fail now that the fuzz targets are built. That said, I would have expected the old check to do the right thing so I'm fairly confused by this.

maflcko · 2021-02-03T07:39:17Z

Oh, sorry. I was testing master 🤦‍♂️ , which means honggfuzz is broken in master and fixed in this pull? I'll need to test some more...

Edit: Broken on master and this pr.

maflcko · 2021-02-04T12:26:57Z

That said, I would have expected the old check to do the right thing so I'm fairly confused by this.

I am incredibly confused by this as well. Tried to fix in #21080

DrahtBot · 2021-02-05T11:03:24Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

maflcko · 2021-02-05T18:46:38Z

I think what you want now is:

if ENABLE_FUZZ:
 * Run MAIN_FUNCTION configure check
else:
 * PROVIDE_MAIN_FUNCTION=yes
fi

danben · 2021-02-05T18:49:29Z

I think what you want now is:

if ENABLE_FUZZ:
 * Run MAIN_FUNCTION configure check
else:
 * PROVIDE_MAIN_FUNCTION=yes
fi

Yep, got it changed locally and just testing everything now. Should be ready soon.

danben · 2021-02-05T19:33:52Z

@MarcoFalke Strangely, now I'm seeing the same issue as in #19633. I reinstalled boost and tried a fresh build but it did not help. Pushing blindly for now.

maflcko

ACK on this commit if the ci is green. Will test tomorrow

configure.ac

maflcko · 2021-02-05T19:40:32Z

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

This fixes issue bitcoin#19388. The changes are as follows: - Add a new flag to configure, --enable-fuzz-binary, which allows building test/fuzz/fuzz regardless of whether we are building to do actual fuzzing - Set -DPROVIDE_MAIN_FUNCTION whenever --enable-fuzz is no - Add the following libraries to FUZZ_SUITE_LD_COMMON: - LIBBITCOIN_WALLET - SQLLITE_LIBS - BDB_LIBS - if necessary, some or all of: - NATPMP_LIBS - MINIUPNPC_LIBS - LIBBITCOIN_ZMQ / ZMQ_LIBS

maflcko · 2021-02-06T07:54:56Z

review ACK 32cbb06 📭

Show signature and timestamp

Signature:

Timestamp of file with hash 6ff121421f769a4f3402980038ef6e89d55175dfce07728a3f9adfac23f10030 -

maflcko · 2021-02-06T10:23:22Z

Upgrade to Tested ACK for libfuzzer and honggfuzz

maflcko

left some ideas for follow-ups

maflcko · 2021-02-06T08:10:28Z

src/Makefile.test.include

 endif

-if ENABLE_FUZZ
-
 FUZZ_SUITE_LDFLAGS_COMMON = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS) $(PTHREAD_FLAGS)


unrelated style nit: This is only used once, so you can inline it

maflcko · 2021-02-06T08:11:27Z

configure.ac

@@ -1271,6 +1278,8 @@ else
    QT_DBUS_INCLUDES=SUPPRESS_WARNINGS($QT_DBUS_INCLUDES)
    QT_TEST_INCLUDES=SUPPRESS_WARNINGS($QT_TEST_INCLUDES)
  fi
+
+  CPPFLAGS="$CPPFLAGS -DPROVIDE_MAIN_FUNCTION"


unrelated style nit: This can be called DPROVIDE_FUZZ_MAIN_FUNCTION for clarity

maflcko · 2021-02-08T09:07:57Z

ci/test/00_setup_env_native_qt5.sh

@@ -16,4 +16,5 @@ export RUN_UNIT_TESTS_SEQUENTIAL="true"
 export RUN_UNIT_TESTS="false"
 export GOAL="install"
 export PREVIOUS_RELEASES_TO_DOWNLOAD="v0.15.2 v0.16.3 v0.17.2 v0.18.1 v0.19.1"
-export BITCOIN_CONFIG="--enable-zmq --with-libs=no --with-gui=qt5 --enable-glibc-back-compat --enable-reduce-exports --enable-debug CFLAGS=\"-g0 -O2 -funsigned-char\" CXXFLAGS=\"-g0 -O2 -funsigned-char\" --with-boost-process"
+export BITCOIN_CONFIG="--enable-zmq --with-libs=no --with-gui=qt5 --enable-glibc-back-compat --enable-reduce-exports
+--enable-debug --disable-fuzz-binary  CFLAGS=\"-g0 -O2 -funsigned-char\" CXXFLAGS=\"-g0 -O2 -funsigned-char\" --with-boost-process"


feel free to follow up with pull requests to remove the temporary disable-fuzz-binary

maflcko · 2021-02-08T09:08:05Z

ci/test/00_setup_env_win64.sh

@@ -13,7 +13,7 @@ export PACKAGES="python3 nsis g++-mingw-w64-x86-64 wine-binfmt wine64 file"
 export RUN_FUNCTIONAL_TESTS=false
 export RUN_SECURITY_TESTS="true"
 export GOAL="deploy"
-export BITCOIN_CONFIG="--enable-reduce-exports --disable-gui-tests --without-boost-process"
+export BITCOIN_CONFIG="--enable-reduce-exports --disable-fuzz-binary --disable-gui-tests --without-boost-process"


hebasto · 2021-02-08T13:22:05Z

After merging this PR Windows cross compiling fails. Fixed in #21115.

723eb43 test: Fix Windows cross build (Hennadii Stepanov) Pull request description: On master (e51f6c4, after #20936 merge), Windows cross compiling fails: ``` $ make > /dev/null In file included from ./policy/fees.h:12, from policy/fees.cpp:6: policy/fees.cpp: In member function ‘unsigned int CBlockPolicyEstimator::HighestTargetTracked(FeeEstimateHorizon) const’: ./sync.h:232:104: warning: control reaches end of non-void function [-Wreturn-type] 232 | #define LOCK(cs) DebugLock<decltype(cs)> PASTE2(criticalblock, __COUNTER__)(cs, #cs, __FILE__, __LINE__) | ^ policy/fees.cpp:680:5: note: in expansion of macro ‘LOCK’ 680 | LOCK(m_cs_fee_estimator); | ^~~~ test/fuzz/netaddress.cpp:12:10: fatal error: netinet/in.h: No such file or directory 12 | #include <netinet/in.h> | ^~~~~~~~~~~~~~ compilation terminated. make[2]: *** [Makefile:13039: test/fuzz/fuzz-netaddress.o] Error 1 make[2]: *** Waiting for unfinished jobs.... libtool: warning: undefined symbols not allowed in x86_64-w64-mingw32 shared libraries; building static only test/fuzz/string.cpp: In function ‘void string_fuzz_target(FuzzBufferType)’: test/fuzz/string.cpp:81:11: error: ‘ShellEscape’ was not declared in this scope 81 | (void)ShellEscape(random_string_1); | ^~~~~~~~~~~ make[2]: *** [Makefile:13543: test/fuzz/fuzz-string.o] Error 1 make[1]: *** [Makefile:15078: all-recursive] Error 1 make: *** [Makefile:812: all-recursive] Error 1 ``` This PR fixes both of errors. ACKs for top commit: MarcoFalke: cr ACK 723eb43 Tree-SHA512: 5d2fba5ca806e64bf92011786d1f868c6624f786bfa753a10316feab7a802a28ec27a4bd25fc26dc289a399895a521c3878ffa1efeff0e540c7245cdb8e4942c

723eb43 test: Fix Windows cross build (Hennadii Stepanov) Pull request description: On master (e51f6c4, after bitcoin#20936 merge), Windows cross compiling fails: ``` $ make > /dev/null In file included from ./policy/fees.h:12, from policy/fees.cpp:6: policy/fees.cpp: In member function ‘unsigned int CBlockPolicyEstimator::HighestTargetTracked(FeeEstimateHorizon) const’: ./sync.h:232:104: warning: control reaches end of non-void function [-Wreturn-type] 232 | #define LOCK(cs) DebugLock<decltype(cs)> PASTE2(criticalblock, __COUNTER__)(cs, #cs, __FILE__, __LINE__) | ^ policy/fees.cpp:680:5: note: in expansion of macro ‘LOCK’ 680 | LOCK(m_cs_fee_estimator); | ^~~~ test/fuzz/netaddress.cpp:12:10: fatal error: netinet/in.h: No such file or directory 12 | #include <netinet/in.h> | ^~~~~~~~~~~~~~ compilation terminated. make[2]: *** [Makefile:13039: test/fuzz/fuzz-netaddress.o] Error 1 make[2]: *** Waiting for unfinished jobs.... libtool: warning: undefined symbols not allowed in x86_64-w64-mingw32 shared libraries; building static only test/fuzz/string.cpp: In function ‘void string_fuzz_target(FuzzBufferType)’: test/fuzz/string.cpp:81:11: error: ‘ShellEscape’ was not declared in this scope 81 | (void)ShellEscape(random_string_1); | ^~~~~~~~~~~ make[2]: *** [Makefile:13543: test/fuzz/fuzz-string.o] Error 1 make[1]: *** [Makefile:15078: all-recursive] Error 1 make: *** [Makefile:812: all-recursive] Error 1 ``` This PR fixes both of errors. ACKs for top commit: MarcoFalke: cr ACK 723eb43 Tree-SHA512: 5d2fba5ca806e64bf92011786d1f868c6624f786bfa753a10316feab7a802a28ec27a4bd25fc26dc289a399895a521c3878ffa1efeff0e540c7245cdb8e4942c

… fuzz tests dee2d6f fuzz: Avoid designated initialization (C++20) in fuzz tests (practicalswift) Pull request description: Avoid designated initialization (C++20) in fuzz tests. Context: bitcoin#20197 (comment), bitcoin#20936 (comment) ACKs for top commit: MarcoFalke: cr ACK dee2d6f dhruv: code review ACK dee2d6f ajtowns: utACK dee2d6f Tree-SHA512: 5940fab6e97a2b11dd3b1475d2cffa2840dc2e6ec34bd9f9df90f948709cab98fd1c513d5dd104816d33a525a6e9710b8715b02db941e35d84f92bc211f56d1d

fanquake added Build system Tests labels Jan 14, 2021

fanquake reviewed Jan 22, 2021

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

danben marked this pull request as draft January 22, 2021 22:51

DrahtBot mentioned this pull request Jan 22, 2021

External signer support - Wallet Box edition #16546

Merged

2 tasks

danben marked this pull request as ready for review January 24, 2021 02:42

danben force-pushed the build-fuzz-tests-by-default branch 2 times, most recently from d545738 to b50884e Compare January 24, 2021 18:13

DrahtBot mentioned this pull request Jan 26, 2021

ci: Fuzz with integer sanitizer #21012

Merged

DrahtBot added the Needs rebase label Jan 29, 2021

maflcko reviewed Jan 29, 2021

View reviewed changes

danben marked this pull request as draft January 29, 2021 19:03

practicalswift mentioned this pull request Jan 30, 2021

fuzz: Avoid designated initialization (C++20) in fuzz tests #21037

Merged

sipsorcery mentioned this pull request Feb 1, 2021

build: adds switch to enable/disable randomized base address in MSVC builds #21045

Merged

danben force-pushed the build-fuzz-tests-by-default branch 2 times, most recently from c7f35f7 to 44825f7 Compare February 1, 2021 22:58

danben marked this pull request as ready for review February 2, 2021 00:48

DrahtBot mentioned this pull request Feb 2, 2021

Use std::filesystem. Remove Boost Filesystem & System #20744

Merged

maflcko reviewed Feb 2, 2021

View reviewed changes

maflcko approved these changes Feb 2, 2021

View reviewed changes

DrahtBot mentioned this pull request Feb 4, 2021

fuzz: Configure check for main function (take 2) #21080

Merged

DrahtBot added the Needs rebase label Feb 5, 2021

danben force-pushed the build-fuzz-tests-by-default branch from 44825f7 to 0a6aa39 Compare February 5, 2021 17:56

danben marked this pull request as draft February 5, 2021 18:24

maflcko approved these changes Feb 5, 2021

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

danben force-pushed the build-fuzz-tests-by-default branch from 04fc96e to 32cbb06 Compare February 6, 2021 00:55

maflcko reviewed Feb 8, 2021

View reviewed changes

maflcko merged commit e51f6c4 into bitcoin:master Feb 8, 2021

hebasto mentioned this pull request Feb 8, 2021

test: Fix Windows cross build #21115

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 8, 2021

Merge bitcoin#20936: build: build fuzz tests by default

a62e1dd

danben deleted the build-fuzz-tests-by-default branch February 13, 2021 17:24

fanquake removed the Needs rebase label May 31, 2021

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

build: build fuzz tests by default #20936

build: build fuzz tests by default #20936

Conversation

danben commented Jan 14, 2021 • edited by maflcko

practicalswift commented Jan 20, 2021

fanquake left a comment

Choose a reason for hiding this comment

DrahtBot commented Jan 22, 2021 • edited

Conflicts

sipa commented Jan 23, 2021 • edited

danben commented Jan 23, 2021

danben commented Jan 24, 2021

maflcko commented Jan 24, 2021 • edited

sipa commented Jan 25, 2021

danben commented Jan 28, 2021

sipa commented Jan 28, 2021

maflcko left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danben commented Feb 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

danben commented Feb 2, 2021

maflcko commented Feb 3, 2021 • edited

maflcko commented Feb 4, 2021

DrahtBot commented Feb 5, 2021

maflcko commented Feb 5, 2021

danben commented Feb 5, 2021

danben commented Feb 5, 2021

maflcko left a comment

Choose a reason for hiding this comment

maflcko commented Feb 5, 2021

maflcko commented Feb 6, 2021

maflcko commented Feb 6, 2021

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto commented Feb 8, 2021

danben commented Jan 14, 2021 •

edited by maflcko

DrahtBot commented Jan 22, 2021 •

edited

sipa commented Jan 23, 2021 •

edited

maflcko commented Jan 24, 2021 •

edited

maflcko left a comment •

edited

maflcko commented Feb 3, 2021 •

edited