fuzz: Don't use afl++ deferred forkserver mode #28480

dergoegge commented at 1:46 PM on September 14, 2023: member

This makes our afl++ harness essentially behave like libFuzzer, with the exception that the whole program does fully reset every 100000 iterations. 100000 is somewhat arbitrary and we could also go with std::numeric_limits<unsigned in>::max() but a smaller limit does allow for the occasional reset to counter act some amount of instability in the fuzzing loop (e.g. non-determinism, statefulness).

It's a bit of a shame to do this just for the targets whose initial state can't be forked (e.g. threads) because other targets do benefit from not having to redo the state setup. An alternative would be #28469 (comment):

If the goal is to be maximally performant, the fork would need to happen for each fuzz target specifically.
I guess it can be achieved by wrapping __AFL_INIT(); into a helper function and then require all fuzz
target initialize() to call it?

DrahtBot commented at 1:46 PM on September 14, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	MarcoFalke

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on Sep 14, 2023

in src/test/fuzz/fuzz.cpp:204 in 13046e0f72 outdated

 201 |      // afl-clang-fast++. See fuzzing.md for details.
 202 |      __AFL_INIT();
 203 | -#endif
 204 |  
 205 | -#ifdef __AFL_LOOP
 206 | +    state_init();

maflcko commented at 2:03 PM on September 14, 2023:

Any reason to split this? Seems easier to just leave it as-is and call initialize() here? The additional overhead of calling std::getenv("FUZZ") on every reset seems trivial.

dergoegge commented at 2:26 PM on September 14, 2023:

Any reason to split this?

yea not really... we can probably just get rid of __AFL_INIT all together.

in src/test/fuzz/fuzz.cpp:199 in 13046e0f72 outdated

 207 | +
 208 |      // Enable AFL persistent mode. Requires compilation using afl-clang-fast++.
 209 |      // See fuzzing.md for details.
 210 |      const uint8_t* buffer = __AFL_FUZZ_TESTCASE_BUF;
 211 | -    while (__AFL_LOOP(1000)) {
 212 | +    while (__AFL_LOOP(100000)) {

maflcko commented at 2:04 PM on September 14, 2023:

I'd also be fine with increasing it even more. I don't think we've ever had a single case on non-determinism in the fuzz tests (at least when it comes to bug-triggering one), so relying on that seems fine.

dergoegge commented at 2:27 PM on September 14, 2023:

I've seen the afl++ stability metric drop for some targets, which apparently is a indicator for whether or not the loop count should/can be increased.

maflcko commented at 2:30 PM on September 14, 2023:

Ok, if there is true non-determinism, it should be investigated/fixed, but no strong opinion here.

dergoegge commented at 2:45 PM on September 14, 2023:

I thought we up it a little bit here, then investigate and then increase it more later.

This should already be a strict improvement as oss-fuzz can now fuzz these targets.

maflcko approved

[fuzz] Don't use afl++ deferred forkserver mode

Deferring the forkserver initialization doesn't make sense for some of
our targets since they involve state that can't be forked (e.g.
threads). We therefore remove the use of __AFL_INIT entirely.

We also increase the __AFL_LOOP count to 100000. Our fuzz targets are
meant to all be deterministic and stateless therefore this should be
fine.

508d05f8a7

dergoegge force-pushed on Sep 14, 2023

dergoegge renamed this:
~~fuzz: Initialize state after __AFL_INIT~~
fuzz: Don't use afl++ deferred forkserver mode
on Sep 14, 2023

maflcko commented at 4:15 PM on September 14, 2023: member

lgtm ACK 508d05f8a7b511dd53f543df8899813487eb03e5

I presume the fork will now happen right before int main()?

dergoegge commented at 5:17 PM on September 14, 2023: member

I presume the fork will now happen right before int main()?

Yes, from the docs: AFL++ tries to optimize performance by executing the targeted binary just once, stopping it just before main(), and then cloning this "main" process to get a steady supply of targets to fuzz..

DrahtBot added the label CI failed on Sep 14, 2023

fanquake merged this on Sep 15, 2023

fanquake closed this on Sep 15, 2023

maflcko commented at 12:31 PM on September 17, 2023: member

https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=62455

Step [#3](/bitcoin-bitcoin/3/) - "compile-afl-address-x86_64":   CXXLD    test/fuzz/fuzz
Step [#3](/bitcoin-bitcoin/3/) - "compile-afl-address-x86_64": /usr/bin/ld: libtest_fuzz_a-fuzz.o (symbol from plugin): in function `FuzzTargets()':
Step [#3](/bitcoin-bitcoin/3/) - "compile-afl-address-x86_64": (.text+0x0): multiple definition of `__afl_sharedmem_fuzzing'; /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x25
Step [#3](/bitcoin-bitcoin/3/) - "compile-afl-address-x86_64": /usr/lib/libFuzzingEngine.a(aflpp_driver.o):(.data+0x0): first defined here
Step [#3](/bitcoin-bitcoin/3/) - "compile-afl-address-x86_64": clang-15: [0;1;31merror: [0m[1mlinker command failed with exit code 1 (use -v to see invocation)[0m
Step [#3](/bitcoin-bitcoin/3/) - "compile-afl-address-x86_64": make[2]: *** [Makefile:7711: test/fuzz/fuzz] Error 1
Step [#3](/bitcoin-bitcoin/3/) - "compile-afl-address-x86_64": make[2]: Leaving directory '/src/bitcoin-core/src'
Step [#3](/bitcoin-bitcoin/3/) - "compile-afl-address-x86_64": make[1]: *** [Makefile:20107: all-recursive] Error 1
Step [#3](/bitcoin-bitcoin/3/) - "compile-afl-address-x86_64": make[1]: Leaving directory '/src/bitcoin-core/src'
Step [#3](/bitcoin-bitcoin/3/) - "compile-afl-address-x86_64": make: *** [Makefile:811: all-recursive] Error 1
Step [#3](/bitcoin-bitcoin/3/) - "compile-afl-address-x86_64": ********************************************************************************
Step [#3](/bitcoin-bitcoin/3/) - "compile-afl-address-x86_64": Failed to build.
Step [#3](/bitcoin-bitcoin/3/) - "compile-afl-address-x86_64": To reproduce, run:
Step [#3](/bitcoin-bitcoin/3/) - "compile-afl-address-x86_64": python infra/helper.py build_image bitcoin-core
Step [#3](/bitcoin-bitcoin/3/) - "compile-afl-address-x86_64": python infra/helper.py build_fuzzers --sanitizer address --engine afl --architecture x86_64 bitcoin-core
Step [#3](/bitcoin-bitcoin/3/) - "compile-afl-address-x86_64": ********************************************************************************
Finished Step [#3](/bitcoin-bitcoin/3/) - "compile-afl-address-x86_64"
ERROR
ERROR: build step 3 "gcr.io/cloud-builders/docker" failed: step exited with non-zero status: 1

Frank-GER referenced this in commit 0102f2a871 on Sep 19, 2023

PastaPastaPasta referenced this in commit a27b58c86a on Oct 24, 2024

PastaPastaPasta referenced this in commit ee62312017 on Oct 24, 2024

PastaPastaPasta referenced this in commit ca0225c0fd on Oct 24, 2024

PastaPastaPasta referenced this in commit 2aefe308f3 on Oct 24, 2024

PastaPastaPasta referenced this in commit ac3f0ec111 on Oct 24, 2024

bitcoin locked this on Dec 3, 2024