Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 26, 2020
Merged

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

merged 1 commit into from
Jun 26, 2020

Conversation

practicalswift
Copy link
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:

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

After this patch:

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

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

@DrahtBot DrahtBot added the Tests label Jun 23, 2020
@practicalswift
Copy link
Contributor Author

practicalswift commented Jun 24, 2020 •
edited
Loading

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

Reported as #19369.

@maflcko
Copy link
Member

maflcko commented Jun 24, 2020

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

For reference:

[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
[12:33] <sipa> what do you think about "building" the fuzz tests in normal make all mode (but without the actual fuzzing enabled)
[12:34] <sipa> one caveat is that the fuzz tests currently already need c++17
[12:48] <bitcoin-git> [bitcoin] hebasto closed pull request #18710: Add local thread pool to CCheckQueue (master...200419-thread-pool) https://github.com/bitcoin/bitcoin/pull/18710
[12:49] <bitcoin-git> [bitcoin] hebasto reopened pull request #18710: Add local thread pool to CCheckQueue (master...200419-thread-pool) https://github.com/bitcoin/bitcoin/pull/18710
[13:11] <cfields> sipa / MarcoFalke: See top 2 commits here for how that could work: https://github.com/theuni/bitcoin/commits/build-fuzz
[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
[13:57] <wumpus> having to do a second build with either-or exclusive options is kind of a time sink and easy to forget
[13:58] <sipa> wumpus: it seems that building the actual fuzz tests within one configure is very hard
[13:59] <wumpus> okay
[13:59] <sipa> but just something that can test whether the fuzz tests build is great already, i think
[13:59] <sipa> because it's really easy to introduce compile errors in them (and this will likely worsen as their coverage increases)
[14:00] <wumpus> right
[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: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
[14:06] <wumpus> that's a neat idea, at least the building feels less like dead weight in that case
[14:09] <sipa> as the travis fuzz build isn't actually doing any fuzzing, this may even be sufficient there
[14:09] <bitcoin-git> [bitcoin] jnewbery opened pull request #19364: net processing: Move orphan reprocessing to a global (master...2020-06-global-orphans) https://github.com/bitcoin/bitcoin/pull/19364
[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?
[14:11] <sipa> cfields: hmm, what symbols are missing?
[14:12] <cfields> sipa: zmq/miniupnpc/etc.
[14:12] <sipa> oh
[14:12] <sipa> that sounds fixable?
[14:12] <cfields> We could tack them on, but that kinda sucks.
[14:13] <cfields> Yeah, there's another approach though to avoid linking altogether though, 1 sec.
[14:13] <sipa> or maybe it's just not an issue
[14:13] <sipa> because if it's not a problem for real fuzzing, why would it here?
[14:13] <sipa> perhaps it's due to not having main() indeed
[14:17] <cfields> different approach: https://github.com/theuni/bitcoin/commit/d36b2b9cfe7d3916f261e88b63c03a7a3edd9ea5
[14:17] <cfields> That one's verbose though, requires us to c/p the object names to that list.
[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)
[14:20] <sipa> but not terrible i guess
[14:24] <cfields> I'll play with the linker errors a bit, see if there's a quick fix.
[14:49] <wumpus> even just compiling and not linking would find the largest part of the errors introduced by changes and drift i guess
[14:50] <wumpus> link-time errors are kind of rare

@maflcko
Copy link
Member

maflcko commented Jun 24, 2020

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 😬 )

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

@laanwj
Copy link
Member

laanwj commented Jun 24, 2020

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.

@maflcko
Copy link
Member

maflcko commented Jun 24, 2020

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

@practicalswift
Copy link
Contributor Author

practicalswift commented Jun 24, 2020 •
edited
Loading

@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.

@maflcko
Copy link
Member

maflcko commented Jun 24, 2020

review ACK fdb933c

@practicalswift practicalswift force-pushed the provide-main-function-in-fuzzer branch from fdb933c to 1087807 Compare June 25, 2020 21:03
@maflcko
Copy link
Member

maflcko commented Jun 26, 2020

ACK 1087807

@maflcko maflcko mentioned this pull request Jun 26, 2020
Closed
@maflcko maflcko merged commit 3bbd822 into bitcoin:master Jun 26, 2020
@jonatack
Copy link
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 pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 19, 2021
@fjahr @Fabcien
Make libfuzzer to work on macOS
4ffa90d 
Summary:
This makes it possible to build with libfuzzer on macOS ([[bitcoin/bitcoin#18008 | PR18008]]), fix
the issue that this PR introduced for other platforms ([[bitcoin/bitcoin#19366 | PR19366]]) and add
the corresponding documentation ([[bitcoin/bitcoin#17942 | PR17942]]).

This is a backport of core [[bitcoin/bitcoin#17942 | PR17942]], [[bitcoin/bitcoin#18008 | PR18008]] and [[bitcoin/bitcoin#19366 | PR19366]].

Test Plan:
On macOS, read the documentation and follow the instructions.
I only ran the instructions for libfuzzer.

  ninja bitcoin-fuzzers
  ./test/fuzz/test_runner.py <path_to_corpus>

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D8956
@practicalswift practicalswift deleted the provide-main-function-in-fuzzer branch April 10, 2021 19:42
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 3, 2022
@PastaPastaPasta
Merge bitcoin#19366: tests: Provide main(...) function in fuzzer. All…
50d20fe 
…ow building uninstrumented harnesses with --enable-fuzz.

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

Pull request description:

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

  This PR restores the behaviour to how things worked prior to bitcoin#18008. bitcoin#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:

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

  After this patch:

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

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

ACKs for top commit:
  MarcoFalke:
    ACK 1087807

Tree-SHA512: 9c16ea32ffd378057c4fae9d9124636d11e3769374d340f68a1b761b9e3e3b8a33579e60425293c96b8911405d8b96ac3ed378e669ea4c47836af06892aca73d
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@practicalswift @maflcko @laanwj @jonatack @DrahtBot