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

Add a Rust CLI #103

Merged
merged 15 commits into from
Apr 27, 2018
Merged

Add a Rust CLI #103

merged 15 commits into from
Apr 27, 2018

Conversation

killercup
Copy link
Member

No description provided.

@killercup
Copy link
Member Author

r? @PaulGrandperrin

@killercup
Copy link
Member Author

As a next step we should add a way to easily collect the results of the fuzzers in a convenient way.

.gitignore Outdated
@@ -6,3 +6,6 @@ fuzzer-*/src/bin
fuzzer-honggfuzz/hfuzz_target/
fuzzer-honggfuzz/hfuzz_workspace/
fuzzer-libfuzzer/crash-*

/target
**/*.rs.bk
Copy link
Member

Choose a reason for hiding this comment

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

these seem redundant with lines 1 and 3, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, cargo init probably added thos

README.md Outdated
- `cargo run -- list-targets` gives you a list of all fuzz targets
- `cargo run -- run pulldown_cmark_read` runs the `pulldown_cmark_read` target with the default fuzzer
- `cargo run -- run pulldown_cmark_read --fuzzer libfuzzer` runs the `pulldown_cmark_read` target with `libfuzzer`
- `cargo run -- continuous` runs all targets (you can overwrite timeout per target and change the fuzzer)
Copy link
Member

Choose a reason for hiding this comment

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

should we encourage cargo run --release? there might be a way to specify opt = 3 or whatever for the profile in cargo.toml, not sure

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think the performance of the CLI tool is that important, but this reminds me:

@PaulGrandperrin do the fuzzer's cargo subcommands have a --release flag? Should we pass it? Previously, I've always invoked libfuzzer with --release and debug assertions on.

fn run_honggfuzz(target: &str, timeout: Option<i32>) -> Result<(), Error> {
let fuzzer = Fuzzer::Honggfuzz;
write_fuzzer_target(fuzzer, target)?;
let dir = fuzzer.dir()?;
Copy link
Member

Choose a reason for hiding this comment

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

i tried checking out your branch and running cargo run -- run:

> cargo run -- run pulldown_cmark_read
    Finished dev [unoptimized + debuginfo] target(s) in 0.91 secs
     Running `target/debug/cli run pulldown_cmark_read`
No such file or directory (os error 2)

one of the downsides of the ? approach here is that it doesn't indicate what operation we tried to do when the error happened. i haven't done any investigation yet, so it's not on this line in particular, just thought i'd mention my thoughts

Copy link
Member

Choose a reason for hiding this comment

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

did a little investigating. this is the line that was returning the error:

https://github.com/rust-fuzz/targets/pull/103/files#diff-eaa2729b20be051a8cb0a93eb8dd2d9fR262

the bin directory isn't being created automatically, so i had to do mkdir fuzzer-honggfuzz/src/bin

Copy link
Member

Choose a reason for hiding this comment

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

also, related to my first point, i learned you can do this with failure:

diff --git a/cli.rs b/cli.rs
index cf48dc4..90ef062 100644
--- a/cli.rs
+++ b/cli.rs
@@ -13,7 +13,7 @@ use std::fs;
 use std::path::{Path, PathBuf};
 use std::process::Command;

-use failure::Error;
+use failure::{Error, ResultExt};
 use regex::Regex;
 use structopt::StructOpt;

@@ -259,7 +259,8 @@ fn write_fuzzer_target(fuzzer: Fuzzer, target: &str) -> Result<(), Error> {
         .write(true)
         .create(true)
         .truncate(true)
-        .open(&path)?;
+        .open(&path)
+        .context("Could not write fuzzer target")?;

     let source = template.replace("###TARGET###", &target);
     file.write_all(source.as_bytes())?;

not suggesting we should do that everywhere, just wanted to share

Copy link
Member Author

Choose a reason for hiding this comment

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

I've just sprinkled a bunch of error context's all over the place and also added automatically creating this */src/bin/ directories

Copy link
Member

Choose a reason for hiding this comment

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

Wow, I didn't about failure::ErrorExt, that's really neat!

@killercup
Copy link
Member Author

killercup commented Apr 26, 2018

Oh: We could rename the run subcommands a bit (mainly "run" to "target") and get rid of the -- in the docs (cargo supports this):

$ cargo run target
$ cargo run continuously
$ cargo run list-targets

Yeah, I'll go ahead with this.

`cargo run target pulldown_cmark_read` -- How nice is this?
Copied from clap -- thanks, Kevin!
@@ -1,60 +0,0 @@
brotli_read 1
Copy link
Member

Choose a reason for hiding this comment

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

I like the fact that now this list is automatically extracted from the code (less duplication = fewer potentials errors).
However, this file was also meant to be useful to specify a fuzzing duration multiplicator.
This is not important for this PR, but I just wanted to say somewhere that it's, I think, something important to have eventually.

@PaulGrandperrin
Copy link
Member

PaulGrandperrin commented Apr 26, 2018

I can't really review the structopt DSL as I have no experience with it but from a user perspective I think it works great!
I think the only feature "lost in translation" is the ability to put weights on each targets, but it can be done later.
You can merge as soon as you want, I'd like to start doing something about the "compile all targets at once" problem.

.filter(|x| filter.as_ref().map(|f| x.contains(f)).unwrap_or(true));

if infinite {
for target in targets.cycle() {
Copy link
Member

Choose a reason for hiding this comment

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

In the old script I was doing a cargo update at the end of each cycle. I think it can be useful as continuous runs can be long-lived.

@killercup
Copy link
Member Author

Either of you: Feel free to merge, we can easily add more stuff in follow up PRs.

I think the only feature "lost in translation" is the ability to put weights on each targets, but it can be done later.

Ah yes, I explicitly skipped this feature for now. I thought I had commented on that in a commit message, but it seems I forgot. Sorry about that.

What was you main goal with that? To make sure expensive fuzzers get some more time to get through a useful number of iterations?

I like the fact that now this list is automatically extracted from the code

It's currently done in quite a hack-y way (regex), and I'm not sure how best to add a multiplier there.

By the way, I was surprised to see there was no timeout to stop after ≥x seconds of no new branch found, by the way.

@frewsxcv
Copy link
Member

bors r+

🎉🎉🎉🎉🎉🎉

bors bot added a commit that referenced this pull request Apr 27, 2018
103: Add a Rust CLI r=frewsxcv a=killercup



Co-authored-by: Pascal Hertleif <killercup@gmail.com>
@bors
Copy link
Contributor

bors bot commented Apr 27, 2018

Build succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants