Skip to content
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.

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

fuzz: Add test/fuzz/test_runner.py and run it in travis #15295

Merged
merged 2 commits into from Feb 14, 2019

Conversation

Projects
None yet
8 participants
@MarcoFalke
Copy link
Member

commented Jan 30, 2019

Can be run with ./test/fuzz/test_runner.py after building as described in doc/fuzzing.md

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1901-qaFuzz branch from d68255d to 5a1806c Jan 30, 2019

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

Concept ACK. Very nice!

Related: #10364 ("Feature request: Make Bitcoin libFuzzer-friendly and consider integration into the OSS-Fuzz project"). Feel free to collect the $20 000 USD bounty :-)

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1901-qaFuzz branch 4 times, most recently from 032ac70 to 4da6a56 Jan 30, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15134 (tests: Add a Travis ASan/LSan/UBSan job testing in a unsigned char environment (-funsigned-char) by practicalswift)
  • #15063 (GUI: If BIP70 is disabled, attempt to fall back to BIP21 parsing by luke-jr)
  • #10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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.

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1901-qaFuzz branch 2 times, most recently from 1074dc1 to 0ca2db9 Jan 30, 2019

@fanquake fanquake added the Tests label Jan 30, 2019

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1901-qaFuzz branch from 0ca2db9 to fa599f8 Jan 30, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

Can you please explain what you're adding here—
Will adding fuzzing to travis make the tests non-deterministic, by randomizing it? Or does it only verify the current corpus?

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2019

Indeed the script currently only supports running over all seeds exactly once

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

Concept ACK. I can't get fuzzing to work on macOS (not that I tried hard), so having it on Travis sounds like a safe idea, if only to ensure people don't accidentally break the build.

@bitcoin bitcoin deleted a comment from DrahtBot Feb 2, 2019

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1901-qaFuzz branch from fa599f8 to faa77d7 Feb 2, 2019

@DrahtBot DrahtBot removed the Needs rebase label Feb 2, 2019

Show resolved Hide resolved test/fuzz/test_runner.py Outdated
Show resolved Hide resolved test/fuzz/test_runner.py Outdated

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1901-qaFuzz branch from faa77d7 to fa1d155 Feb 5, 2019

Show resolved Hide resolved test/fuzz/test_runner.py Outdated

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1901-qaFuzz branch 3 times, most recently from 0491dcb to 03ba30a Feb 5, 2019

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1901-qaFuzz branch 4 times, most recently from d43de79 to fa7ce81 Feb 11, 2019

Show resolved Hide resolved .travis.yml
Show resolved Hide resolved test/fuzz/test_runner.py Outdated
Show resolved Hide resolved test/fuzz/test_runner.py Outdated
Show resolved Hide resolved doc/fuzzing.md Outdated

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1901-qaFuzz branch from fa7ce81 to fa7ca8e Feb 13, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

Thx, done

@practicalswift

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

utACK fa7ca8e

Let's get this merged :-)

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

Would be nice if at least some person other than myself and travis could test this. Just copy-paste the commands in the readme on a linux machine and tell me your computer didn't crash or something.

@jamesob

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

Tested changes (fa7ca8e) to doc/fuzzing.md on Linux 4.15.0-43-generic #46-Ubuntu SMP x86_64 with afl-fuzz 2.52b. Installed afl from scratch, rebuilt Bitcoin with requisite flags, and confirmed that running afl-fuzz with the transaction_deserialize works as intended.

Used new test_runner script which looks like an easy way to delegate to calling src/test/fuzz/*. Have yet to get any meaningful output back, but maybe this is intended?

$ ./test/fuzz/test_runner.py -l DEBUG ~/src/qa-assets/fuzz_seed_corpus transaction_deserialize

Fuzz targets found: ['address_deserialize', 'addrman_deserialize', 'banentry_deserialize', 'block_deserialize', 'blockheader_deserialize', 'blocklocator_deserialize', 'blockmerkleroot', 'blocktransactions_deserialize', 'blocktransactionsrequest_deserialize', 'blockundo_deserialize', 'bloomfilter_deserialize', 'coins_deserialize', 'diskblockindex_deserialize', 'inv_deserialize', 'messageheader_deserialize', 'netaddr_deserialize', 'service_deserialize', 'transaction_deserialize', 'txoutcompressor_deserialize', 'txundo_deserialize']
Fuzz targets selected: ['transaction_deserialize']
logging.error("Must be built with libFuzzer")
sys.exit(1)
except subprocess.TimeoutExpired:
logging.error("subprocess timed out: Currently only libFuzzer is supported")

This comment has been minimized.

Copy link
@jamesob

jamesob Feb 14, 2019

Member

Tested and works when bitcoin is built with afl.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Feb 14, 2019

Author Member

❤️

@jamesob

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

Tested ACK fa535af. Rebuilt with libfuzzer and tested the command that previously failed with afl:

$ ./test/fuzz/test_runner.py -l DEBUG ~/src/qa-assets/fuzz_seed_corpus transaction_deserialize

Fuzz targets found: ['address_deserialize', 'addrman_deserialize', 'banentry_deserialize', 'block_deserialize', 'blockheader_deserialize', 'blocklocator_deserialize', 'blockmerkleroot', 'blocktransactions_deserialize', 'blocktransactionsrequest_deserialize', 'blockundo_deserialize', 'bloomfilter_deserialize', 'coins_deserialize', 'diskblockindex_deserialize', 'inv_deserialize', 'messageheader_deserialize', 'netaddr_deserialize', 'service_deserialize', 'transaction_deserialize', 'txoutcompressor_deserialize', 'txundo_deserialize']
Fuzz targets selected: ['transaction_deserialize']
Run transaction_deserialize with args ['/home/james/src/bitcoin/src/test/fuzz/transaction_deserialize', '-runs=1', '/home/james/src/qa-assets/fuzz_seed_corpus/transaction_deserialize']
Output: INFO: Seed: 819372946
INFO: Loaded 1 modules   (2002 inline 8-bit counters): 2002 [0x55b9cec15620, 0x55b9cec15df2),
INFO: Loaded 1 PC tables (2002 PCs): 2002 [0x55b9cec15df8,0x55b9cec1db18),
INFO:      295 files found in /home/james/src/qa-assets/fuzz_seed_corpus/transaction_deserialize
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 52575 bytes
INFO: seed corpus: files: 295 min: 1b max: 52575b total: 491499b rss: 45Mb
#296    INITED cov: 240 ft: 938 corp: 277/475Kb exec/s: 0 rss: 106Mb
#296    DONE   cov: 240 ft: 938 corp: 277/475Kb exec/s: 0 rss: 106Mb
Done 296 runs in 0 second(s)

Worth noting that I had to bump clang from 4. to 6. in order for the -fsanitize=fuzzer flags to work.

@DrahtBot DrahtBot removed the Needs rebase label Feb 14, 2019

@MarcoFalke MarcoFalke merged commit fa535af into bitcoin:master Feb 14, 2019

1 check failed

continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details

MarcoFalke added a commit that referenced this pull request Feb 14, 2019

Merge #15295: fuzz: Add test/fuzz/test_runner.py and run it in travis
fa535af fuzz: test_runner: Better error message when built with afl (MarcoFalke)
fa7ca8e qa: Add test/fuzz/test_runner.py (MarcoFalke)

Pull request description:

  Can be run with `./test/fuzz/test_runner.py` after building as described in `doc/fuzzing.md`

Tree-SHA512: f6a3cd8165ec2de4b363be4fd0a936b4a60829cce923f93fe5d6a046b1bbd64c959cdf790440bf70c0e13b0bb1b956a746a24c6fd92bddeab15b837ed50ffad2

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1901-qaFuzz branch Feb 14, 2019

@@ -28,6 +27,8 @@ FUZZ_TARGETS = \

if ENABLE_FUZZ
noinst_PROGRAMS += $(FUZZ_TARGETS:=)
else
bin_PROGRAMS += test/test_bitcoin

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 22, 2019

Contributor

In commit "qa: Add test/fuzz/test_runner.py" (fa7ca8e)

@MarcoFalke Is this no longer building test_bitcoin if ENABLE_FUZZ is true? I wouldn't expect ENABLE_FUZZ to affect this, but if this is correct it'd be helpful to have a comment here explaining what this is doing.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Feb 22, 2019

Author Member

Yes, I think there is no reason to build test_bitcoin if you want to build the fuzz tests. Am I missing something?

Regardless of that, it would result in linker errors later in the build process, since you can't link libfuzzer to something that has a main function.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 22, 2019

Contributor

Oh ok, I don't know really anything about fuzzing, and I assumed enabling a fuzzing option would just build some new binaries, not affect existing binaries in any way. But I guess due to autotools lack of support for side-by-side build configurations, ENABLE_FUZZ affects every existing binary we build, as well as building new binaries? Probably not worth adding a comment here just for test_bitcoin in this case.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Feb 22, 2019

Author Member

Yes, ideally it should disable all other binaries.

Currently we hack around that by specifying it manually:

BITCOIN_CONFIG="--disable-wallet --disable-bench --with-utils=no --with-daemon=no --with-libs=no --with-gui=no --enable-fuzz --with-sanitizers=fuzzer,address CC=clang CXX=clang++"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.