tests: Provide main(...) function in fuzzer. Allow building uninstrumented harnesses with --enable-fuzz.

practicalswift commented at 9:48 pm on June 23, 2020: contributor

Provide main(...) function in fuzzer. Allow building uninstrumented harnesses with only --enable-fuzz.

This PR restores the behaviour to how things worked prior to #18008. #18008 worked around an macOS specific issue but did it in a way which unnecessarily affected platforms not in need of the workaround :)

Before this patch:

 0# Build uninstrumented fuzzing harness (no libFuzzer/AFL/other-fuzzer-instrumentation)
 1$ ./configure --enable-fuzz
 2$ make
 3  CXXLD    test/fuzz/span     
 4/usr/lib/gcc/x86_64-linux-gnu/7/../../../x86_64-linux-gnu/Scrt1.o: In function `_start':
 5(.text+0x20): undefined reference to `main'              
 6collect2: error: ld returned 1 exit status
 7Makefile:7244: recipe for target 'test/fuzz/span' failed                   
 8make[2]: *** [test/fuzz/span] Error 1
 9make[2]: *** Waiting for unfinished jobs....
10$

After this patch:

0# Build uninstrumented fuzzing harness (no libFuzzer/AFL/other-fuzzer-instrumentation)
1$ ./configure --enable-fuzz
2$ make
3$ echo foo | src/test/fuzz/span
4$

The examples above show the change in non-macOS functionality. macOS functionality is unaffected by this patch.

DrahtBot added the label Tests on Jun 23, 2020

practicalswift commented at 5:49 am on June 24, 2020: contributor

The wallet_basic.py --descriptors CI failure seems entirely unrelated :)

Reported as #19369.

MarcoFalke commented at 11:18 am on June 24, 2020: member

@sipa @theuni I think this is in reply to the recent IRC conversation?

For reference:

 0[12:33] <sipa> MarcoFalke: i'm finding it a bit annoying that codebase changes often break existing fuzz tests, which require travis or an entirely separate build to find out
 1[12:33] <sipa> what do you think about "building" the fuzz tests in normal make all mode (but without the actual fuzzing enabled)
 2[12:34] <sipa> one caveat is that the fuzz tests currently already need c++17
 3[12:48] <bitcoin-git> [bitcoin] hebasto closed pull request [#18710](/bitcoin-bitcoin/18710/): Add local thread pool to CCheckQueue (master...200419-thread-pool) [#18710](/bitcoin-bitcoin/18710/)
 4[12:49] <bitcoin-git> [bitcoin] hebasto reopened pull request [#18710](/bitcoin-bitcoin/18710/): Add local thread pool to CCheckQueue (master...200419-thread-pool) [#18710](/bitcoin-bitcoin/18710/)
 5[13:11] <cfields> sipa / MarcoFalke: See top 2 commits here for how that could work: https://github.com/theuni/bitcoin/commits/build-fuzz
 6[13:57] <wumpus> sipa: I'm not sure it should be the default, but I do agree it should be easier (e.g. a single configure option) to build the fuzz tests as well sa everything else
 7[13:57] <wumpus> having to do a second build with either-or exclusive options is kind of a time sink and easy to forget
 8[13:58] <sipa> wumpus: it seems that building the actual fuzz tests within one configure is very hard
 9[13:59] <wumpus> okay
10[13:59] <sipa> but just something that can test whether the fuzz tests build is great already, i think
11[13:59] <sipa> because it's really easy to introduce compile errors in them (and this will likely worsen as their coverage increases)
12[14:00] <wumpus> right
13[14:01] <sipa> we could even have a minimal main() for the fake-fuzz tests that reads a single input from stdin, and runs the test code on it
14[14:02] <sipa> or all entries in a directory... meaning it'd be actually useful as a test harness; you just wouldn't be able to fuzz with it
15[14:06] <wumpus> that's a neat idea, at least the building feels less like dead weight in that case
16[14:09] <sipa> as the travis fuzz build isn't actually doing any fuzzing, this may even be sufficient there
17[14:09] <bitcoin-git> [bitcoin] jnewbery opened pull request [#19364](/bitcoin-bitcoin/19364/): net processing: Move orphan reprocessing to a global (master...2020-06-global-orphans) [#19364](/bitcoin-bitcoin/19364/)
18[14:11] <cfields> it seems getting them to link as real progs will have linker issues due to missing libs. I guess the linker doesn't go looking for unresolved symbols with no main?
19[14:11] <sipa> cfields: hmm, what symbols are missing?
20[14:12] <cfields> sipa: zmq/miniupnpc/etc.
21[14:12] <sipa> oh
22[14:12] <sipa> that sounds fixable?
23[14:12] <cfields> We could tack them on, but that kinda sucks.
24[14:13] <cfields> Yeah, there's another approach though to avoid linking altogether though, 1 sec.
25[14:13] <sipa> or maybe it's just not an issue
26[14:13] <sipa> because if it's not a problem for real fuzzing, why would it here?
27[14:13] <sipa> perhaps it's due to not having main() indeed
28[14:17] <cfields> different approach: https://github.com/theuni/bitcoin/commit/d36b2b9cfe7d3916f261e88b63c03a7a3edd9ea5
29[14:17] <cfields> That one's verbose though, requires us to c/p the object names to that list.
30[14:20] <sipa> that's somewhat annoying (it's already annoying to need to add a blob to the makefile for every new fuzz test)
31[14:20] <sipa> but not terrible i guess
32[14:24] <cfields> I'll play with the linker errors a bit, see if there's a quick fix.
33[14:49] <wumpus> even just compiling and not linking would find the largest part of the errors introduced by changes and drift i guess
34[14:50] <wumpus> link-time errors are kind of rare

MarcoFalke commented at 11:27 am on June 24, 2020: member

I’d say they should be compiled by default, just like bench and the unit tests, and the gui, and the gui tests. No one is running the benchmarks to do actual benchmarking in a usual development workflow. They are merely compiled to be smoke-run once in make check. I don’t see why the fuzz tests should be different. They should be built and maybe even smoke tested (with the empty input or so) by default.

Similar to --disable-tests, --disable-gui-tests, --disable-bench, there should be a --disable-fuzz option.

The reason that fuzz tests are handled differently is because I am lacking the background in automake to do even the most simplest task. (I leave improvements in the build system for others to implement. E.g. #18527 :grimacing: )

Anyway, I think all the tests should be handled the same by the build system and make check.

laanwj commented at 12:07 pm on June 24, 2020: member

I’d say they should be compiled by default, just like bench and the unit tests, and the gui, and the gui tests. No one is running the benchmarks to do actual benchmarking in a usual development workflow. They are merely compiled to be smoke-run once in make check. I don’t see why the fuzz tests should be different. They should be built and maybe even smoke tested (with the empty input or so) by default.

Concept ACK. I think we’d want to skip building them in gitian (to save time for every platform, the result isn’t shipped), but for a normal developer build it’d make sense.

MarcoFalke commented at 12:29 pm on June 24, 2020: member

Jup, I think gitian/guix set --disable-bench, so they could also disable the fuzz tests

practicalswift commented at 1:20 pm on June 24, 2020: contributor

@MarcoFalke I hadn’t seen that conversation (I’m not on IRC), so this PR isn’t a reply to it :)

I’ve simply needed this myself many times when using the fuzzing harnesses as simple application entry points for concolic testing tools, abstract interpretation static analysis tools, etc which analyze the possible execution flow from main and onwards :)

In other words, being able to build the fuzz targets (non-instrumented using --enable-fuzz) is of much value also outside of fuzzing :)

This PR is restoring how things worked under Linux prior to the “macOS libFuzzer linker issue” fixed in #18008 (I’m upstreaming a local fix I’ve had for this).

Regarding the quoted IRC conversation: I fully agree that we should start building the fuzzing harnesses by default, but I think that is largely unrelated to this PR.

FWIW, the coverage of the fuzzing harnesses is quickly approaching the coverage reached by the unit tests (see unit test coverage and fuzz test coverage). Hopefully writing fuzz tests to show a basic level of robustness for new code will soon be almost as natural as writing unit or functional tests for new code :) Building the fuzzers by default will hopefully help us get there even sooner.

MarcoFalke commented at 1:27 pm on June 24, 2020: member

review ACK fdb933cf68f777bb5e0aaabc6216c5b54b4b28f1

practicalswift closed this on Jun 25, 2020

practicalswift reopened this on Jun 25, 2020

tests: Provide main(...) function in fuzzer 1087807b2b

practicalswift force-pushed on Jun 25, 2020

MarcoFalke commented at 6:38 pm on June 26, 2020: member

ACK 1087807b2bc56b9c7e7a5471c83f6ecfae79b048

MarcoFalke merged this on Jun 26, 2020

MarcoFalke closed this on Jun 26, 2020

jonatack commented at 12:15 pm on June 28, 2020: member

Post-merge Concept ACK.

I’d say they should be compiled by default, just like bench and the unit tests, and the gui, and the gui tests.

Anyway, I think all the tests should be handled the same by the build system and make check.

I agree. This may catch a number of inadvertent fuzz build issues we’ve been seeing in PRs.

Fabcien referenced this in commit 4ffa90d363 on Jan 19, 2021

practicalswift deleted the branch on Apr 10, 2021

PastaPastaPasta referenced this in commit 50d20fe788 on Apr 3, 2022

DrahtBot locked this on Aug 18, 2022

tests: Provide main(…) function in fuzzer. Allow building uninstrumented harnesses with –enable-fuzz. #19366