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

Update arbitrary #53

Merged
merged 3 commits into from
Jan 10, 2020
Merged

Update arbitrary #53

merged 3 commits into from
Jan 10, 2020

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Jan 9, 2020

Updates arbitrary to its current unreleased master branch, and fixes compilation errors here.

This makes running the tests easier in local development.
Since it isn't released yet, we depend on the git version.
And then return. This is intended for use by `cargo fuzz` for getting nice info
about an `Arbitrary` input that caused a crash.

let $data: $dty = match Arbitrary::arbitrary(&mut buf) {
let $data = match data {
Copy link
Member

Choose a reason for hiding this comment

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

This should be first.

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 wanted to handle running a fuzz target with debug enabled on some input that doesn't parse as an arbitrary, so you still get some idea of what is going on rather than silent early exit. Does that make sense?

@Manishearth
Copy link
Member

Wait, won't this make rust-fuzz run debug for every single fuzz test? Won't that slow things down? I thought the plan was to parse the crashdump file.

@fitzgen
Copy link
Member Author

fitzgen commented Jan 10, 2020

Wait, won't this make rust-fuzz run debug for every single fuzz test? Won't that slow things down? I thought the plan was to parse the crashdump file.

If you look at rust-fuzz/cargo-fuzz#206 what's happening is this:

  • we run the fuzz target without RUST_LIBFUZZER_DEBUG_PATH until we get a crash
  • cargo fuzz finds the new crash artifacts
  • cargo fuzz re-runs the fuzz target on each crash artifact (should generally be just one) with RUST_LIBFUZZER_DEBUG_PATH set to a temp file
  • cargo fuzz reads the temp file into a string and adds it to its helpful output

Notably, we don't run fuzzing with the env var set, so we don't debug format and write to disk for every single input.

Sound good?

@Manishearth
Copy link
Member

Oh! I understand now! That's neat.

- cd ../example_arbitrary
- cargo rustc --release -- -Cpasses='sancov' -Cllvm-args=-sanitizer-coverage-level=4 -Cllvm-args=-sanitizer-coverage-trace-compares -Cllvm-args=-sanitizer-coverage-inline-8bit-counters -Cllvm-args=-sanitizer-coverage-stack-depth -Cllvm-args=-sanitizer-coverage-trace-geps -Cllvm-args=-sanitizer-coverage-prune-blocks=0 -Zsanitizer=address
- (! ./target/release/example -runs=10000000)
script: ./ci/script.sh
Copy link
Member

Choose a reason for hiding this comment

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

eventually should probably move to GHA

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@@ -13,7 +13,11 @@ edition = "2018"
members = ["."]

[dependencies]
arbitrary = "0.2"
# arbitrary = "0.3"
Copy link
Member

Choose a reason for hiding this comment

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

feel free to cut a release, you should have access

@Manishearth
Copy link
Member

We should also start using version numbers for libfuzzer-sys

@Manishearth
Copy link
Member

(We can wait till both PRs land, test stuff out, and then update things with version numbers)

@fitzgen
Copy link
Member Author

fitzgen commented Jan 10, 2020

(We can wait till both PRs land, test stuff out, and then update things with version numbers)

Yeah -- I think we should probably move everything to 1.0.0 after we do some pre-release testing.

Do you have thoughts on that?

@Manishearth
Copy link
Member

I'm okay with a 1.0, if we test it out first. We can even release a preview version of cargo fuzz that has a flag to use libfuzzer-sys:next, though that's probably not super necessary.

One leftover concern for 1.0 is it would be nice if we could stop doing the no_main stuff, but the current attempts to do that aren't that good.

@fitzgen
Copy link
Member Author

fitzgen commented Jan 10, 2020

Maybe we should do a 1.0-rc of both at the same time?

Honestly, I'm not too concerned with the no_main stuff. /me shrugs

@fitzgen
Copy link
Member Author

fitzgen commented Jan 10, 2020

It does probably make sense to keep using next as the main dev branch until we are confident in a 1.0, since all existing installs are using git master...

@fitzgen fitzgen merged commit e616f59 into rust-fuzz:next Jan 10, 2020
@fitzgen fitzgen deleted the update-arbitrary branch January 10, 2020 18:32
@Manishearth
Copy link
Member

Manishearth commented Jan 10, 2020 via email

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.

None yet

2 participants