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

MarcoFalke commented at 9:12 PM on January 30, 2019: member

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

MarcoFalke force-pushed on Jan 30, 2019

practicalswift commented at 9:22 PM on January 30, 2019: contributor

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 force-pushed on Jan 30, 2019

DrahtBot commented at 10:30 PM on January 30, 2019: member

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 force-pushed on Jan 30, 2019

fanquake added the label Tests on Jan 30, 2019

MarcoFalke force-pushed on Jan 30, 2019

laanwj commented at 11:57 AM on January 31, 2019: member

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 commented at 3:14 PM on January 31, 2019: member

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

MarcoFalke added the label Needs gitian build on Jan 31, 2019

MarcoFalke removed the label Needs gitian build on Jan 31, 2019

MarcoFalke added the label Needs gitian build on Jan 31, 2019

DrahtBot removed the label Needs gitian build on Feb 1, 2019

Sjors commented at 5:55 PM on February 1, 2019: member

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.

DrahtBot added the label Needs rebase on Feb 1, 2019

MarcoFalke deleted a comment on Feb 2, 2019

MarcoFalke force-pushed on Feb 2, 2019

DrahtBot removed the label Needs rebase on Feb 2, 2019

in test/fuzz/test_runner.py:83 in faa77d7880 outdated

  78 | +        ],
  79 | +        check=True,
  80 | +        stderr=subprocess.PIPE,
  81 | +        universal_newlines=True,
  82 | +    ).stderr
  83 | +    if not "libFuzzer" in help_output:

practicalswift commented at 9:22 PM on February 4, 2019:

PEP-8 tells us to do membership tests using if X not in Y instead of if not X in Y :-)

in test/fuzz/test_runner.py:125 in faa77d7880 outdated

 120 | +    corpus = corpi.pop(0)
 121 | +    for t in test_list:
 122 | +        args = [
 123 | +            os.path.join(build_dir, 'src', 'test', 'fuzz', t),
 124 | +            '-merge=1',
 125 | +            '-runs=1',  #??

practicalswift commented at 9:24 PM on February 4, 2019:

Why #?? :-)

in test/fuzz/test_runner.py:96 in faa77d7880 outdated

  97 | +            test_list=test_list,
  98 | +            build_dir=config["environment"]["BUILDDIR"],
  99 | +        )
 100 | +
 101 | +
 102 | +def run_once(*, corpus, test_list, build_dir, export_coverage):

practicalswift commented at 9:25 PM on February 4, 2019:

export_coverage unused?

MarcoFalke commented at 9:47 PM on February 5, 2019:

Fixed

MarcoFalke force-pushed on Feb 5, 2019

in test/fuzz/test_runner.py:31 in fa1d155cb6 outdated

  26 | +        '--export_coverage',
  27 | +        action='store_true',
  28 | +        help='If true, export coverage information to files in the seed corpus',
  29 | +    )
  30 | +    parser.add_argument(
  31 | +        'corpi',

practicalswift commented at 9:53 PM on February 5, 2019:

Nit: Isn't corpora or corpuses the plural of corpus?

Perhaps the least unexpected choice is to go with "corpus" instead of "corpi" here?

laanwj commented at 8:48 PM on February 13, 2019:

s/corpi/corgi

MarcoFalke force-pushed on Feb 5, 2019

MarcoFalke force-pushed on Feb 11, 2019

in .travis.yml:104 in fa7ce81ee1 outdated

 100 | @@ -100,7 +101,7 @@ jobs:
 101 |          PACKAGES="python3-zmq qtbase5-dev qttools5-dev-tools protobuf-compiler libdbus-1-dev libharfbuzz-dev libprotobuf-dev"
 102 |          DEP_OPTS="NO_QT=1 NO_UPNP=1 DEBUG=1 ALLOW_HOST_PACKAGES=1"
 103 |          GOAL="install"
 104 | -        BITCOIN_CONFIG="--enable-zmq --with-gui=qt5 --enable-fuzz --enable-glibc-back-compat --enable-reduce-exports --enable-debug CXXFLAGS=\"-g0 -O2\""
 105 | +        BITCOIN_CONFIG="--enable-zmq --with-gui=qt5 --enable-glibc-back-compat --enable-reduce-exports --enable-debug CXXFLAGS=\"-g0 -O2\""

laanwj commented at 8:49 PM on February 13, 2019:

~~Why the build parameter change for this existing test run?~~

oh I see, only removing --enable-fuzz because it's separated out

in test/fuzz/test_runner.py:53 in fa7ce81ee1 outdated

  48 | +    # Read config generated by configure.
  49 | +    config = configparser.ConfigParser()
  50 | +    configfile = os.path.abspath(os.path.dirname(__file__)) + "/../config.ini"
  51 | +    config.read_file(open(configfile, encoding="utf8"))
  52 | +
  53 | +    # Build list of tests

laanwj commented at 9:15 PM on February 13, 2019:

I think it would be more readable to factor the "get a list of fuzz targets", for example, out to a function, this is one very long main function.

in test/fuzz/test_runner.py:83 in fa7ce81ee1 outdated

  78 | +    test_list_selection = list(set(test_list_all).intersection(set(args.target)))
  79 | +    if not test_list_selection:
  80 | +        logging.error("No fuzz targets selected")
  81 | +    logging.info("Fuzz targets selected: {}".format(test_list_selection))
  82 | +
  83 | +    if not config["components"].getboolean("ENABLE_FUZZ"):

laanwj commented at 9:17 PM on February 13, 2019:

Could check this immediately after reading the config, I guess, no need to do any of the other things then.

in doc/fuzzing.md:28 in fa7ce81ee1 outdated

  23 | +
  24 | +Only for AFL:
  25 | +
  26 | +```
  27 | +mkdir outputs
  28 | +AFLOUT=$PWD/outputs

laanwj commented at 9:19 PM on February 13, 2019:

no export for this one, but there is for DIR_FUZZ_IN, is that intentional?

qa: Add test/fuzz/test_runner.py fa7ca8ef58

MarcoFalke force-pushed on Feb 13, 2019

MarcoFalke commented at 10:13 PM on February 13, 2019: member

Thx, done

practicalswift commented at 8:09 AM on February 14, 2019: contributor

utACK fa7ca8ef58bf3e3b91d1f5a67fa42008e63b1f7b

Let's get this merged :-)

MarcoFalke commented at 4:45 PM on February 14, 2019: member

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.

DrahtBot added the label Needs rebase on Feb 14, 2019

jamesob commented at 8:34 PM on February 14, 2019: member

Tested changes (https://github.com/bitcoin/bitcoin/pull/15295/commits/fa7ca8ef58bf3e3b91d1f5a67fa42008e63b1f7b) to doc/fuzzing.md on Linux 4.15.0-43-generic [#46](/bitcoin-bitcoin/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']

fuzz: test_runner: Better error message when built with afl fa535af92c

in test/fuzz/test_runner.py:90 in fa535af92c

  96 | +        ).stderr
  97 | +        if "libFuzzer" not in help_output:
  98 | +            logging.error("Must be built with libFuzzer")
  99 | +            sys.exit(1)
 100 | +    except subprocess.TimeoutExpired:
 101 | +        logging.error("subprocess timed out: Currently only libFuzzer is supported")

jamesob commented at 8:52 PM on February 14, 2019:

Tested and works when bitcoin is built with afl.

MarcoFalke commented at 8:53 PM on February 14, 2019:

:heart:

jamesob commented at 9:08 PM on February 14, 2019: member

Tested ACK https://github.com/bitcoin/bitcoin/pull/15295/commits/fa535af92c179b0ffb9280e0b2dc5acfeb80964a. 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](/bitcoin-bitcoin/296/)    INITED cov: 240 ft: 938 corp: 277/475Kb exec/s: 0 rss: 106Mb
[#296](/bitcoin-bitcoin/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 removed the label Needs rebase on Feb 14, 2019

MarcoFalke merged this on Feb 14, 2019

MarcoFalke closed this on Feb 14, 2019

MarcoFalke referenced this in commit 31f7c6dd21 on Feb 14, 2019

MarcoFalke deleted the branch on Feb 14, 2019

in src/Makefile.test.include:31 in fa7ca8ef58 outdated

  26 | @@ -28,6 +27,8 @@ FUZZ_TARGETS = \
  27 |  
  28 |  if ENABLE_FUZZ
  29 |  noinst_PROGRAMS += $(FUZZ_TARGETS:=)
  30 | +else
  31 | +bin_PROGRAMS += test/test_bitcoin

ryanofsky commented at 4:25 PM on February 22, 2019:

In commit "qa: Add test/fuzz/test_runner.py" (fa7ca8ef58bf3e3b91d1f5a67fa42008e63b1f7b) @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.

MarcoFalke commented at 4:35 PM on February 22, 2019:

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.

ryanofsky commented at 5:08 PM on February 22, 2019:

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.

MarcoFalke commented at 6:16 PM on February 22, 2019:

Yes, ideally it should disable all other binaries.

Currently we hack around that by specifying it manually:

https://github.com/bitcoin/bitcoin/blob/169dced9a42bd741b3265adee23e6a8d1f852227/.travis.yml#L146

apoelstra commented at 3:09 PM on December 9, 2020:

It is surprising that --enable-fuzz silently stops building the test binary, especially as config.log continues to claim that test_bitcoin is being built.

MarcoFalke commented at 3:17 PM on December 9, 2020:

It is surprising that --enable-fuzz silently stops building the test binary

Might be fixed by #19388, which is blocked on #20560 . Though when building and linking with a fuzz engine, the test_bitcoin binary won't have any meaning. Not sure if it will even compile/link.

config.log continues to claim that test_bitcoin is being built.

I don't really understand build systems, so if this is an issue, I hope that someone will fix it

apoelstra commented at 3:37 PM on December 9, 2020:

I'll open a PR to change config to output something like

configure:27478: checking whether to build test_bitcoin-qt
configure:27481: result: no, because fuzzing is enabled
configure:27499: checking whether to build test_bitcoin
configure:27502: result: no, because fuzzing is enabled

It's not really surprising that enabling fuzzing would break all the other binaries, I was just misled by the config output.

apoelstra commented at 11:36 PM on December 9, 2020:

https://github.com/bitcoin/bitcoin/pull/20609

random-zebra referenced this in commit 44b5327e61 on May 28, 2021

kittywhiskers referenced this in commit 4acb3889e4 on Aug 2, 2021

kittywhiskers referenced this in commit 07c505db85 on Aug 5, 2021

kittywhiskers referenced this in commit 122ac8a987 on Aug 5, 2021

PastaPastaPasta referenced this in commit 05f477eab7 on Aug 6, 2021

kittywhiskers referenced this in commit 21293ced10 on Aug 8, 2021

kittywhiskers referenced this in commit d967c39222 on Aug 11, 2021

PastaPastaPasta referenced this in commit 90e7119a8b on Aug 11, 2021

5tefan referenced this in commit cf41a16e5e on Aug 12, 2021

DrahtBot locked this on Feb 15, 2022