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

[Fuzzing] Create script to run all fuzz targets #943

Merged
merged 4 commits into from
Jun 3, 2020

Conversation

fleupold
Copy link
Contributor

@fleupold fleupold commented Jun 2, 2020

We currently have two fuzzing targets:

  • element_read_all
  • orderbook

Our goal is to run fuzzing continuously after each push. This PR adds a shell script that fetches the list of fuzz targets via cargo fuzz list and runs each target sequentially for a certain amount of time.

It loops over all targets indefinitely until it finds a crash.

Test Plan

cd pricegraph/fuzz
./fuzz_all_targets.sh 20

See targets getting fuzzed for 20s each indefinitely. Now, add a panic in one of the targets (e.g. orderbook.rs) and save. As soon as the target is compiled and run next everything should stop.

@fleupold fleupold requested review from e00E and a team June 2, 2020 11:08
@e00E
Copy link
Contributor

e00E commented Jun 2, 2020

I don't want to delete the existing targets. cargo fuzz is made with the intention of having multiple targets. I don't want to limit this because a level higher up (running the fuzzer) cannot handle this well. We should improve that layer instead.

I would rather have a more complicated kubernetes script and have to update it. We can get rid of manually updating by parsing the output of cargo +nightly fuzz list.

Copy link
Contributor

@nlordell nlordell left a comment

Choose a reason for hiding this comment

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

Cool!

@nlordell
Copy link
Contributor

nlordell commented Jun 2, 2020

I don't want to delete the existing targets. cargo fuzz is made with the intention of having multiple targets.

I agree, we can keep our old targets, but what is the issue of adding a third target to have a single entry point for kubernetes?

@e00E
Copy link
Contributor

e00E commented Jun 2, 2020

It's not unreasonable but it has the drawback of duplicating the code into that target. When we make changes to the original targets we have to update this target too. Or we could refactor the targets into functions in a module so the code does not have to be duplicated.

On the other hand writing a script that parses the list of fuzzing targets and then loops through them forever running each target for 5 minutes is the better solution imo.

@fleupold
Copy link
Contributor Author

fleupold commented Jun 2, 2020

I agree that having to add a target in two places is a bit error prone and annoying. We can use cargo fuzz list to get the targets. I'd probably write a rust binary similar to https://github.com/rust-fuzz/targets/blob/master/cli.rs that queries the targets and then spawns a command to run them (will try in parallel, but sequentially would also work).

I'll add this script into the services repo as it's inconvenient to have non-trivial code in the gnosis-staging repo which should really only contain configuration.

@e00E
Copy link
Contributor

e00E commented Jun 2, 2020

I like a rust binary. If you prefer a shell script you can try something like

cargo +nightly fuzz list | xargs -L1 -I{} cargo +nightly fuzz run {} -- -max_total_time=300

That runs each target for 300 seconds, once. Not sure how it behaves if the fuzzer finds a panic.

will try in parallel, but sequentially would also work

I don't think we need parallel, if that makes the script more complicated but if not then there is no drawback.

@fleupold
Copy link
Contributor Author

fleupold commented Jun 2, 2020

If you prefer a shell script you can try something like

I looked into that but I would have to do something like this to make xargs fail if one of the inner targets fail and wrap this in some endless loop running until the first failure. It's possible in bash but maybe a bit too magical, and it might be good to have a simple target that can run all fuzz targets locally as well.

@fleupold
Copy link
Contributor Author

fleupold commented Jun 2, 2020

It turns out that writing a rust binary to do this is actually harder than I thought (a binary that lives in the fuzz subfolder will automatically be considered a fuzzing target itself + I wasn't able to kill the processes after the timeout using Child.kill and thus the script become quite large and hacky)

I therefore ended up writing it in bash at the end 🙈

Much time wasted...

@fleupold fleupold changed the title [Fuzzing] Create single fuzz target to run continuously [Fuzzing] Create script to run all fuzz targets Jun 2, 2020
Copy link
Contributor

@bh2smith bh2smith left a comment

Choose a reason for hiding this comment

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

I find this shell script nice and easy to read!

@fleupold fleupold added the merge when green See: https://github.com/phstc/probot-merge-when-green/ label Jun 3, 2020
@merge-when-green merge-when-green bot merged commit 1c302cf into master Jun 3, 2020
@merge-when-green merge-when-green bot deleted the single_fuzz_target branch June 3, 2020 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge when green See: https://github.com/phstc/probot-merge-when-green/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants