Fuzz testing integration

[darosior]

This introduces coverage-guided fuzz testing using libfuzzer.

Why

We've been talking about it for some time now, and i'm sure it could help us to uncover trivial bugs with minimal overhead, more sneaky ones more easily than with unit tests, and finally to avoid regressions by maintaining a seed corpus to ran patches against.

Going further, structure-aware fuzzing could help us with Bitcoin Script creation or even with the state machine which while more intrusive because crypto seems plausible because of the daemons separation and messages.

For now, this PR only introduces the basis for integration with primitives and rudimentary targets (i focused on common/). Still, this found some bugs (eg amounts string parsing / formatting) and new targets could help with e.g. our JSON parser or the gossip store.

Integration

This extends our tests/ directory by moving the Python functional tests to the tests/functional/ subdirectory (sorry for the conflicts..) and creates a new sub: tests/fuzz/.

Inspired by the unit tests, all targets are named fuzz-[target_name].
Not inspired by the unit tests, all targets are built in tests/fuzz/. Even though there are only common/-specific targets for now, i think it makes sense to keep them contained in a single directory.

A libfuzz.h abstracts out the interface to libFuzzer for potential future support for other fuzzers, and run.py allows to manage them in batches. This was inspired by the integration in bitcoin-core but i guess it's a pretty common pattern.

What next

• a)
  I propose to create a new repository (eg in https://github.com/lightningd/ ?) for tracking the seed corpus that we could update if we generate new coverage locally.
  This could be pulled in a new CI job that checks PRs against each [target + seeds] once to check for regression. What do you think about using Github Action for this ?
• b)
  Write more targets
• c)
  Generate more coverage
• d)
  Go to b) :)

[niftynei]

Cool! Bunch of small nits. I think my biggest complaint is that there's no real reason to move all the existing CI tests down a directory. There's no reason we can't have tests/fuzzing exist while also leaving all the other tests where they are.

Didn't really give the Makefiles a good looksee, as that's not really my particular wheelhouse.

[niftynei]

this feels like it belongs in the previous commit

[cdecker]

Awesome PR @darosior, I've long wanted to look into fuzzing, and with this I have almost a guided tour to help me learn. Haven't looked too closely yet, but I'll grab a coco and start learning asap.

However, I share @niftynei's feeling re moving all tests down one directory level:

1. It breaks almost all PRs
2. Those are likely the most edited files in the whole repo, hiding them in ya directory means some more typing 😜

[darosior]

Ok, will remove the move!

[cdecker]

Just some minor performance tips :-)

[darosior]

Thanks for the reviews so far! Amended as per latest one.

(Will check about the process pool soon ™️)

[darosior]

What do you think about having a repo in lightningd/ to store corpora ?

[cdecker]

What do you think about having a repo in lightningd/ to store corpora ?

Sounds like something we could consider. From what I've seen so far the corpus is not going to be huge (<= 1GB by my estimate) so that can work. I wonder however if the version control brings anything new to the table, given that the corpus consists mostly of binary files, so maybe a linear storage model can work as well? The big advantage of using git + github is the reuse of infra and processes to collaborate on building up a corpus.

(edit: what I'm saying is that I would have no idea on how to judge a pull request xD)

[darosior]

Yeah, really just thinking of copying the workflow at github.com/bitcoin-core/qa-assets :-)

EDIT:

(edit: what I'm saying is that I would have no idea on how to judge a pull request xD)

:) I'd just ask (trust?) the contributor to have ran --merge on the seed dir. Could also be checked locally (wonder it's deterministic hmm). In any case i don't expect a lot of contributions there (as in bitcoind's qa-assets).

(<= 1GB by my estimate)

Been running one for almost a week it's 8.3M atm. But we really only have few simple targets.

I wonder however if the version control brings anything new to the table

A place which we can easily contribute to, but you still have control over since it's pulled in your CI. I'd say that the platform, Github, is more of interest than the version control tool, git, for this one :)

[jsarenik]

@darosior that qa-assets repo looks ugly to me. Exactly because of what @cdecker wrote earlier.

Here is my offer: If you agree to put the binary data out of git, i will happily help with any and all automation required (which will be itself properly kept in a git repository; e.g. building the corpuses, uploading them somewhere (or sharing them somehow), etc.).

Yes, i learned a bit about Torrent in last weeks and am strongly biased against any binary blob in repositories designed for source code.

https://git-annex.branchable.com/ or https://git-lfs.github.com/ can be used to track the binary blobs.

EDIT: This offer is valid also for serving the binary data over BitTorrent. No cloud needed.

[jsarenik]

Just for reference here is one repository that uses binary fuzz files, even inside its main repository: https://github.com/rust-bitcoin/rust-bitcoin/tree/master/fuzz. The proposed separate repository (like they have in https://github.com/bitcoin-core/qa-assets) is a better idea compared to in-tree blobs.

[cdecker]

@darosior that qa-assets repo looks ugly to me. Exactly because of what @cdecker wrote earlier.

Here is my offer: If you agree to put the binary data out of git, i will happily help with any and all automation required (which will be itself properly kept in a git repository; e.g. building the corpuses, uploading them somewhere (or sharing them somehow), etc.).

Yes, i learned a bit about Torrent in last weeks and am strongly biased against any binary blob in repositories designed for source code.

https://git-annex.branchable.com/ or https://git-lfs.github.com/ can be used to track the binary blobs.

EDIT: This offer is valid also for serving the binary data over BitTorrent. No cloud needed.

I think the main issue is collecting and managing the corpus, the serving is secondary (happy to host the corpus on GCS for a couple of cents each month). For the code review and addition we have a really good process, that's no longer true if we start managing things outside the git tree.

I absolutely share your dislike of binary artifacts in git, however it might be easiest to get started with an in-tree solution, and move that out at a later point in time. Or the separate corpus repo could be a cleaner solution that doesn't taint the main repo with these binaries.

Going a bit back and forth between the solutions tbh.

[darosior]

Yeah, management on a server would be awful, discussions around it would be mixed up. I can already imagine having to upload my new corpus to (a VPS / a torrent seeder / whatever) and bug cdecker on IRC for him to pull it and eventually merge it with the existing one on its GCS for it to be pulled by our CI... Wow. Let's just use Github?

[cdecker]

Ok, let's use a corpus repo then on the lightningd org. That unblocks this PR which I'd like to merge, and keeps our main tree clean, while allowing us to come up with a better solution at a later point in time.

[cdecker]

ACK acef7ea

[jsarenik]

ACK acef7ea (have learned a lot about Fuzzing in the process, thank you!)

Recommended reading (because the word fuzzing is quite fuzzy until the real meaning of it is seen):
https://en.wikipedia.org/wiki/Fuzzing

[darosior]

@rustyrussell could you look at the amount parsing commit ? iirc i stumbled upon something like that in the past and you corrected my fix so just would like your input on this one.

[jsarenik]

@darosior This PR has been merged. Is your comment relevant or is it here by mistake? Any link to that commit you mention?