Overhaul fuzzing #1732
Overhaul fuzzing #1732
Conversation
82ad673 to
3497cfe
Compare
|
The fuzztests are now failing because the For now I'll just neuter the tests. But in future I'd like to replace |
fab7986 to
c58b61c
Compare
|
Rebased on master to get CI fix; removed the RustCrypto fuzztests and replaced them with simpler ones; fix removal of |
c58b61c to
0fc7fec
Compare
|
Rebase on #1733 so CI 8000 should finish faster; fix 1.41 fixes. I think it should actually pass now :) except that I'm unsure about the |
fuzz/fuzz-util.sh
Outdated
| targetFileToName() { | ||
| echo "$1" \ | ||
| | sed 's/^fuzz_targets\///' \ | ||
| | sed 's/.rs$//' \ |
There was a problem hiding this comment.
Strictly speaking the dot should be escaped but I guess it doesn't matter. Unless we create cars directory or something.
There was a problem hiding this comment.
Good catch. I'll fix this, I try to be fastidious about this but I guess after too many rebases I got sloppy.
fuzz/README.md
Outdated
| # Fuzzing | ||
|
|
||
| `bitcoin` and `bitcoin_hashes` have fuzzing harnesses setup for use with | ||
| honggfuzz. (There is also support for AFL, but we this is rarely tested |
| @@ -8,7 +8,7 @@ fn do_test(data: &[u8]) { | |||
| let w: Result<Witness, _> = deserialize(data); | |||
| if let Ok(witness) = w { | |||
| let serialized = serialize(&witness); | |||
| assert_eq!(data, serialized); | |||
| assert_eq!(data, &serialized[..]); | |||
There was a problem hiding this comment.
Do we really need this if we're bumping MSRV to 1.48 soon anyway?
There was a problem hiding this comment.
Good question. No, we do not. I'll remove it and wait for #1729.
|
|
||
| # Address Sanitizer | ||
| if [ "$DO_ASAN" = true ]; then | ||
| cargo clean |
There was a problem hiding this comment.
This shouldn't be needed unless something is seriously broken.
There was a problem hiding this comment.
Heh, agreed, but I started by copying the bitcoin one and then deleted stuff, and I figured it wouldn't hurt anything to leave in.
|
|
||
| #[cfg(all(test, fuzzing))] | ||
| mod tests { | ||
| fn extend_vec_from_hex(hex: &str, out: &mut Vec<u8>) { |
There was a problem hiding this comment.
IIUC this could be literally out.extend(HexIterator::new(hex).map(Result::unwrap))
There was a problem hiding this comment.
Good call. This code comes from an ancient version of rust-lightning and long predates us having any hex code :).
| @@ -33,6 +33,7 @@ listTargetNames() { | |||
| checkWindowsFiles() { | |||
| incorrectFilenames=$(find . -type f -name "*,*" -o -name "*:*" -o -name "*<*" -o -name "*>*" -o -name "*|*" -o -name "*\?*" -o -name "*\**" -o -name "*\"*" | wc -l) | |||
There was a problem hiding this comment.
I think nul, nul.* and some other magic names are missing.
There was a problem hiding this comment.
In practice it seems like honggfuzz never produces files with those characters, whereas the listed ones it does. I'm happy to add more, though I don't know how to escape nulls. But I don't think it's necessary.
|
|
||
| assert_eq!(&our_hash[..], &rc_hash[..]); | ||
| let hash = ripemd160::Hash::hash(data); | ||
| assert_eq!(&hash[..], &eng_hash[..]); |
There was a problem hiding this comment.
Isn't this test so trivial it's not worth fuzzing anymore?
There was a problem hiding this comment.
It's fuzzing that we don't panic when hashing stuff, I guess. And that we don't somehow let our engines get out of sync with our hash functions.
But in short, no, these fuzz tests have little value. I can can remove them entirely if you think they're too much noise.
|
@apoelstra, what is the motivation of bringing fuzz tests into main root directory? Does this mean we have to maintain MSRV support for hongfuzz tests? |
There was a problem hiding this comment.
Pretty cool. A few questions/comments
- Perhaps run
./generate-files.shagain - Why the camel case?
- Why the 2 character indentation?
And since its "pester Andrew with pointless questions" day, what column width do you use in your editor for text files? I was unable to work it out from reading the files (I've hit this problem before :)
fuzz/Cargo.toml
Outdated
| honggfuzz = { version = "0.5", default-features = false } | ||
| bitcoin = { version = "0.30.0", features = [ "serde" ] } | ||
|
|
||
| rust-crypto = "0.2" |
There was a problem hiding this comment.
We don't need this anymore, right?
There was a problem hiding this comment.
Correct. I've fixed it locally but haven't pushed, sorry.
|
@sanket1729 the motivation is that otherwise testing the fuzz tests involves special-case logic rather than just treating it like a worktree. No, we don't need to maintain MSRV support for the fuzztests ... but given that we currently do, and the tests themselves are all super simple, it would surprise me if this would be a problem. In particular, it is hard to build stuff with nix when there's a separate crate in a subdirectory that isn't a workspace, since this needs its own lockfile that needs to separately be maintained alongside the normal lockfiles, and I need to manually mess with the source location, etc., instead of letting crate2nixs workspace detection do it for me. Similarly our CI code has a separate cd fuzz; cargo test line rather than treating the fuzz harness like it does every other crate. It also creates the misleading impression that the fuzz harness is somehow tied to a specific crate, even though it treats the main crates as normal dependencies as is technically totally indepent of them. git rebase b260a310^ -x './fuzz/generate-files.sh && git diff --quiet -- fuzz/Cargo.toml runs clean on my local version of this PR. I think the pushed version is out of date. I will push a new one once #1729 is in. |
|
@tcharding, I suspect that you unintentionally edited Andrew's comment above to remove his responses to me? |
|
Oh bother, how on earth did I do that. Sorry man. |
Sweet.
Its ok, no need for total uniformity in shell scripts, I was just wondering where it came from.
Ah that would explain it. Cheers. |
|
There was no revert but the comment had a history of edits so I cut'n'pasted the content from @apoelstra last edit. Thanks for flagging it @sanket1729 |
|
I've been getting in the habit of editing peoples post to cut'n'paste so as to get access to all the markdown, perhaps I messed up while doing that, maybe its a bad habit. |
|
@tcharding if you select the relevant text, then click the ... button then "Quote Reply", Github will auto-fill your thing with just the text you selected, markdown included. Not sure how to quote multiple people this way. Lol @ you using maintainer powers to just get the text content...it never occurred to me I could do that. |
mmmm abuse of power :) |
That is sick. After many years I'm finally starting to warm to GitHub. |
14c76c5 to
13b5695
Compare
|
Because I always pick new contributors up on it so it feels remiss of me not to hold you to the same standards; commit |
|
Because this allows the fuzzer to be run at the crate root, the hfuzz files are created at crate root. Can you also them to .gitignore? |
diff --git a/.gitignore b/.gitignore
index 2ccfc05f..a70f2985 100644
--- a/.gitignore
+++ b/.gitignore
@@ -20,3 +20,5 @@ bitcoin/fuzz/hfuzz_target
bitcoin/fuzz/hfuzz_workspace
hashes/fuzz/hfuzz_target
hashes/fuzz/hfuzz_workspace
+hfuzz_workspace
+hfuzz_target
|
a68ba04 to
38569cb
Compare
|
Squashed the two commits @tcharding suggested and updated |
|
What is holding up this PR? I would like to remove the |
|
Reviewing this again today. But looks like something else broke in embedded world. Oops, I see that the fix has been merged in master |
This allows testing the fuzztests and making sure that they compile and tests pass, etc., just using `cargo test --all` in the root.
also remove `set -e` from `fuzz-util.sh` because the README suggests that users sometimes source this into their interactive shell.
This way you can run `cargo clippy --all` in the root, and `cargo hfuzz run` without modification.
AFAICT we literally never used this; it was available only on the bitcoin targets and not the honggfuzz ones; AFL has a broken dep tree (or at least, requires some more MSRV pins that I did not care to investigate). Just remove it entirely.
…r reproducing failures
We should probably restore this in the future, but we need to rethink how we fuzz hashes -- right now when cfg(fuzzing) is set, we break all the hash functions in a way that won't match any other library. We should probably make this breakage opt-in but this will require buy-in from rust-lightning and maybe others.
59d7ab6 to
2860aae
Compare
|
Rebased on |
|
ACK 2860aae |
|
@sanket1729 can you ACK using the github api? it looks like you just commented. |
|
Thanks! |
f50cbbb set honggfuzz dependency to 0.5.55 (Andrew Poelstra) 73bcffc cargo fmt new workspaces (bitcoin-tests, fuzz) (Andrew Poelstra) cd0abcd bitcoind-tests: find the bitcoind executable no matter where we are running from (Andrew Poelstra) bf98e2a bitcoind-tests: allow overriding the BITCOIND_EXE variable (Andrew Poelstra) 54d7657 promote bitcoind-tests and fuzz to workspaces (Andrew Poelstra) Pull request description: This brings the fuzztests and bitcoind-tests into workspaces along with the root crate. This means they are covered by `cargo test --all` and `cargo fmt --all` without any special action, and also means that `crate2nix` can test th B4B0 em independently without my manually messing around with directories. I run `cargo +nightly fmt` to bring the two new workspaces into proper formatting -- there were no changes that I'd consider controversial. It also allows the `bitcoin-tests` directory to be run with a custom `BITCOIND_EXE` variable, and computes the default version more flexibly so that you can run the tests from any directory. Finally I set the minimum honggfuzz version to 0.5.55, since the dep currently was set to 0.5.0, which definitely did not work. With these changes, everything in this repo should work with a lockfile generated by `cargo +nightly update -Z minimal-versions`, and I am able to run both the bitcoind and fuzz tests (for five minutes per unit locally). I'll push some code to my [local-nix-ci repo](https://github.com/apoelstra/local-nix-ci) tomorrow to demonstrate whan I'm doing. This PR does **not** do more dramatic rearrangemeent of the fuzztests ... though when rust-bitcoin/rust-bitcoin#1732 gets merged I will open a similar PR here which does that. ACKs for top commit: sanket1729: ACK f50cbbb. Tree-SHA512: a234f9f09ad059b051b2336738a1f8fb4d807af4523380037c8121173cfd05ab0deb567b92abfae3fc794d9ad91966f43d7396623a453df242bc5392a1bcb489
f50cbbb set honggfuzz dependency to 0.5.55 (Andrew Poelstra) 73bcffc cargo fmt new workspaces (bitcoin-tests, fuzz) (Andrew Poelstra) cd0abcd bitcoind-tests: find the bitcoind executable no matter where we are running from (Andrew Poelstra) bf98e2a bitcoind-tests: allow overriding the BITCOIND_EXE variable (Andrew Poelstra) 54d7657 promote bitcoind-tests and fuzz to workspaces (Andrew Poelstra) Pull request description: This brings the fuzztests and bitcoind-tests into workspaces along with the root crate. This means they are covered by `cargo test --all` and `cargo fmt --all` without any special action, and also means that `crate2nix` can test them independently without my manually messing around with directories. I run `cargo +nightly fmt` to bring the two new workspaces into proper formatting -- there were no changes that I'd consider controversial. It also allows the `bitcoin-tests` directory to be run with a custom `BITCOIND_EXE` variable, and computes the default version more flexibly so that you can run the tests from any directory. Finally I set the minimum honggfuzz version to 0.5.55, since the dep currently was set to 0.5.0, which definitely did not work. With these changes, everything in this repo should work with a lockfile generated by `cargo +nightly update -Z minimal-versions`, and I am able to run both the bitcoind and fuzz tests (for five minutes per unit locally). I'll push some code to my [local-nix-ci repo](https://github.com/apoelstra/local-nix-ci) tomorrow to demonstrate whan I'm doing. This PR does **not** do more dramatic rearrangemeent of the fuzztests ... though when rust-bitcoin/rust-bitcoin#1732 gets merged I will open a similar PR here which does that. ACKs for top commit: sanket1729: ACK f50cbbb. Tree-SHA512: a234f9f09ad059b051b2336738a1f8fb4d807af4523380037c8121173cfd05ab0deb567b92abfae3fc794d9ad91966f43d7396623a453df242bc5392a1bcb489
Several big changes here:
contrib/test.shetc so that CI will check that it compileshashes/fuzztests into workspaceSupercedes #1422
I suspect the hashes fuzztests will actually fail since we haven't touched them in so long. Will address that if CI fails here.