Skip to content

Conversation

@dergoegge
Copy link
Member

Our coverage reports include coverage of initialization code, which can be misleading when trying to evaluate the coverage a fuzz harness achieves through fuzzing alone.

This PR proposes to make fuzz coverage reports more accurate by resetting coverage counters after initialization code has been run. This makes it easier to evaluate which code was actually reached through fuzzing (e.g. to spot fuzz blockers).

@DrahtBot
Copy link
Contributor

DrahtBot commented May 23, 2024

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK maflcko, brunoerg
Concept ACK TheCharlatan, hebasto, kevkevinpal, instagibbs

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

@DrahtBot DrahtBot added the Tests label May 23, 2024
@sedited
Copy link
Contributor

sedited commented May 23, 2024

Concept ACK

@dergoegge dergoegge force-pushed the 2024-05-cov-reset-counters branch from dc8c30b to 52506a0 Compare May 23, 2024 11:12
@hebasto
Copy link
Member

hebasto commented May 23, 2024

Concept ACK on improving coverage.

@brunoerg
Copy link
Contributor

Concept ACK

@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/25325963327

@kevkevinpal
Copy link
Contributor

Concept ACK 52506a0

Made a clean build of this change using

sudo make clean && ./autogen.sh &&  CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined && sudo make -j"$(($(nproc)+1))" install

Running fuzz tests twice

(master: f157785)

FUZZ=process_message src/test/fuzz/fuzz -runs=1 qa-assets/fuzz_seed_corpus/process_message
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 4218378445
INFO: Loaded 1 modules   (425045 inline 8-bit counters): 425045 [0x558dbe652ce0, 0x558dbe6ba935),
INFO: Loaded 1 PC tables (425045 PCs): 425045 [0x558dbe6ba938,0x558dbed36e88),
INFO:     2759 files found in qa-assets/fuzz_seed_corpus/process_message
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 814557 bytes
INFO: seed corpus: files: 2759 min: 1b max: 814557b total: 14744612b rss: 218Mb
#1024   pulse  cov: 11518 ft: 23753 corp: 583/80Kb exec/s: 341 rss: 454Mb
#2048   pulse  cov: 13725 ft: 35158 corp: 1025/223Kb exec/s: 256 rss: 454Mb
#3698   INITED cov: 14297 ft: 52528 corp: 1782/8638Kb exec/s: 160 rss: 581Mb
#3698   DONE   cov: 14297 ft: 52528 corp: 1782/8638Kb lim: 630378 exec/s: 160 rss: 581Mb
Done 3698 runs in 23 second(s)

FUZZ=process_message src/test/fuzz/fuzz -runs=1 qa-assets/fuzz_seed_corpus/process_message
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 555650094
INFO: Loaded 1 modules   (425045 inline 8-bit counters): 425045 [0x557c8bfd0ce0, 0x557c8c038935),
INFO: Loaded 1 PC tables (425045 PCs): 425045 [0x557c8c038938,0x557c8c6b4e88),
INFO:     2759 files found in qa-assets/fuzz_seed_corpus/process_message
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 814557 bytes
INFO: seed corpus: files: 2759 min: 1b max: 814557b total: 14744612b rss: 219Mb
#3696   INITED cov: 14264 ft: 52459 corp: 1784/8568Kb exec/s: 160 rss: 583Mb
#3696   DONE   cov: 14264 ft: 52459 corp: 1784/8568Kb lim: 630378 exec/s: 160 rss: 583Mb
Done 3696 runs in 23 second(s)

(dergoegge:2024-05-cov-reset-counters: 52506a0)

FUZZ=process_message src/test/fuzz/fuzz -runs=1 qa-assets/fuzz_seed_corpus/process_message
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 1724170080
INFO: Loaded 1 modules   (424990 inline 8-bit counters): 424990 [0x55773dc1cc20, 0x55773dc8483e),
INFO: Loaded 1 PC tables (424990 PCs): 424990 [0x55773dc84840,0x55773e300a20),
INFO:     2759 files found in qa-assets/fuzz_seed_corpus/process_message
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 814557 bytes
INFO: seed corpus: files: 2759 min: 1b max: 814557b total: 14744612b rss: 219Mb
#1024   pulse  cov: 11499 ft: 23802 corp: 589/81Kb exec/s: 341 rss: 455Mb
#2048   pulse  cov: 13711 ft: 35251 corp: 1034/226Kb exec/s: 256 rss: 455Mb
#3701   INITED cov: 14277 ft: 52623 corp: 1793/8700Kb exec/s: 160 rss: 582Mb
#3701   DONE   cov: 14277 ft: 52623 corp: 1793/8700Kb lim: 630378 exec/s: 160 rss: 582Mb
Done 3701 runs in 23 second(s)

FUZZ=process_message src/test/fuzz/fuzz -runs=1 qa-assets/fuzz_seed_corpus/process_message
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3858639904
INFO: Loaded 1 modules   (424990 inline 8-bit counters): 424990 [0x562169278c20, 0x5621692e083e),
INFO: Loaded 1 PC tables (424990 PCs): 424990 [0x5621692e0840,0x56216995ca20),
INFO:     2759 files found in qa-assets/fuzz_seed_corpus/process_message
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 814557 bytes
INFO: seed corpus: files: 2759 min: 1b max: 814557b total: 14744612b rss: 219Mb
#1024   pulse  cov: 11480 ft: 23795 corp: 578/80Kb exec/s: 341 rss: 456Mb
#2048   pulse  cov: 13698 ft: 35272 corp: 1017/222Kb exec/s: 256 rss: 456Mb
#3700   INITED cov: 14266 ft: 52596 corp: 1769/8685Kb exec/s: 160 rss: 582Mb
#3700   DONE   cov: 14266 ft: 52596 corp: 1769/8685Kb lim: 630378 exec/s: 160 rss: 582Mb
Done 3700 runs in 23 second(s)

@instagibbs
Copy link
Member

concept ACK

@dergoegge dergoegge force-pushed the 2024-05-cov-reset-counters branch from 52506a0 to 949abeb Compare May 23, 2024 16:26
@dergoegge
Copy link
Member Author

@maflcko
Copy link
Member

maflcko commented May 24, 2024

utACK 949abeb

@dergoegge
Copy link
Member Author

As an example for the minisketch harness, files reported as reached by the fuzzer:

master:

src/coins.h
src/common/args.cpp
src/common/system.cpp
src/compat/byteswap.h
src/compat/endian.h
src/crypto/chacha20.cpp
src/crypto/chacha20.h
src/crypto/common.h
src/crypto/sha256.cpp
src/crypto/sha512.cpp
src/crypto/sha512.h
src/crypto/siphash.cpp
src/cuckoocache.h
src/hash.cpp
src/hash.h
src/logging.cpp
src/logging.h
src/minisketch/include/minisketch.h
src/minisketch/src/fields/generic_4bytes.cpp
src/minisketch/src/fields/generic_common_impl.h
src/minisketch/src/int_utils.h
src/minisketch/src/lintrans.h
src/minisketch/src/minisketch.cpp
src/minisketch/src/sketch.h
src/minisketch/src/sketch_impl.h
src/net.cpp
src/netaddress.cpp
src/netbase.h
src/policy/feerate.h
src/prevector.h
src/primitives/transaction.h
src/pubkey.cpp
src/random.cpp
src/randomenv.cpp
src/rpc/client.cpp
src/rpc/server.cpp
src/rpc/server.h
src/rpc/util.cpp
src/rpc/util.h
src/script/miniscript.cpp
src/script/miniscript.h
src/script/script.h
src/script/sigcache.cpp
src/script/sign.cpp
src/script/solver.cpp
src/secp256k1/src/hash_impl.h
src/secp256k1/src/secp256k1.c
src/secp256k1/src/selftest.h
src/secp256k1/src/util.h
src/serialize.h
src/span.h
src/support/allocators/secure.h
src/support/cleanse.cpp
src/support/lockedpool.cpp
src/support/lockedpool.h
src/sync.h
src/test/fuzz/FuzzedDataProvider.h
src/test/fuzz/fuzz.cpp
src/test/fuzz/fuzz.h
src/test/fuzz/minisketch.cpp
src/test/fuzz/script_assets_test_minimizer.cpp
src/test/fuzz/txrequest.cpp
src/test/fuzz/util.h
src/test/util/script.h
src/test/util/setup_common.cpp
src/uint256.h
src/univalue/include/univalue.h
src/univalue/lib/univalue.cpp
src/univalue/lib/univalue_read.cpp
src/util/check.h
src/util/fs.cpp
src/util/fs.h
src/util/spanparsing.h
src/util/strencodings.cpp
src/util/strencodings.h
src/util/string.h
src/util/threadinterrupt.cpp
src/util/time.cpp
src/util/time.h
src/util/vector.h

pull:

src/minisketch/include/minisketch.h
src/minisketch/src/fields/generic_4bytes.cpp
src/minisketch/src/fields/generic_common_impl.h
src/minisketch/src/int_utils.h
src/minisketch/src/lintrans.h
src/minisketch/src/minisketch.cpp
src/minisketch/src/sketch.h
src/minisketch/src/sketch_impl.h
src/span.h
src/test/fuzz/FuzzedDataProvider.h
src/test/fuzz/fuzz.cpp
src/test/fuzz/minisketch.cpp
src/test/fuzz/util.h
src/util/check.h
src/util/fs.cpp
src/util/fs.h
src/util/time.h

Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, utACK 949abeb

@dergoegge
Copy link
Member Author

rfm?

@fanquake fanquake merged commit 417b6ce into bitcoin:master May 29, 2024
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 22, 2025
949abeb [fuzz] Avoid collecting initialization coverage (dergoegge)

Pull request description:

  Our coverage reports include coverage of initialization code, which can be misleading when trying to evaluate the coverage a fuzz harness achieves through fuzzing alone.

  This PR proposes to make fuzz coverage reports more accurate by resetting coverage counters after initialization code has been run. This makes it easier to evaluate which code was actually reached through fuzzing (e.g. to spot fuzz blockers).

ACKs for top commit:
  maflcko:
    utACK 949abeb
  brunoerg:
    nice, utACK 949abeb

Tree-SHA512: c8579bda4f3d71d199b9331fbe6316fce375a906743d0bc216bb94958dc03fdc9a951ea50cfeb487494a75668ae3c16471a82f7e5fdd912d781dc29d063e2c5b
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Apr 23, 2025
1e04c56 Merge bitcoin#30556: doc: multisig-tutorial: remove obsolete mention and link to closed PR (merge-script)
fd43510 Merge bitcoin#30453: test: Non-Shy version sender (glozow)
e3c3a11 Merge bitcoin#30327: build: Drop redundant `sys/sysctl.h` header check (merge-script)
808a77d Merge bitcoin#30156: fuzz: More accurate coverage reports (merge-script)
3ca42ba Merge bitcoin#28874: doc: fixup help output for -upnp and -natpmp (merge-script)
ea32090 Merge bitcoin#28461: build: Windows SSP roundup (fanquake)
e71c422 Merge bitcoin#28151: build: use `-muse-unaligned-vector-move` for Windows builds (fanquake)
077bbb4 Merge bitcoin#28131: test: Add UBSan `-fsanitize=integer` suppressions for `src/secp256k1` subtree (fanquake)
de5a2d1 Merge bitcoin#27940: test: Add implicit-signed-integer-truncation:*/include/c++/ suppression (fanquake)

Pull request description:

  ## Issue being fixed or feature implemented
  Batch of trivial backports

  ## How Has This Been Tested?
  Built locally

  ## Breaking Changes

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 1e04c56
  knst:
    utACK 1e04c56

Tree-SHA512: 5e9a3fc4ac2ea06e8da48952bbdb43e7ed0c3d9ab3fdae3d8753bbe10b957c6cbb06e01b9860db4cd5ade91c8cd419dbbc8ee76073d00b4d6ff0f6ae6a4cbfd2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants