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

fuzz: let's do fuzz testing #2942

Merged
merged 10 commits into from
Apr 26, 2018
Merged

fuzz: let's do fuzz testing #2942

merged 10 commits into from
Apr 26, 2018

Conversation

overvenus
Copy link
Member

This PR introduces fuzz testing, fuzzing codec::bytes, and it already found a bug #2941.

Copy link
Contributor

@Hoverbear Hoverbear left a comment

Choose a reason for hiding this comment

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

Yes!! 😍 I tested it with cargo fuzz run codec_bytes and it indeed did get a panic.

Can you add documentation on using it?

Do you think we should add it to the CI?

src/lib.rs Outdated
@@ -23,7 +23,7 @@
#![cfg_attr(feature = "dev", feature(plugin))]
#![cfg_attr(feature = "dev", plugin(clippy))]
#![cfg_attr(not(feature = "dev"), allow(unknown_lints))]
#![recursion_limit = "100"]
#![recursion_limit = "200"]
Copy link
Member

Choose a reason for hiding this comment

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

why is this config changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Rustc reports:

error: recursion limit reached while expanding the macro `opts`
    --> /home/pingcap/stn/tikv/src/util/rocksdb/engine_metrics.rs:1305:9
     |
1305 | /         register_int_gauge_vec!(
1306 | |             "tikv_engine_oldest_snapshot_duration",
1307 | |             "Oldest unreleased snapshot duration in seconds",
1308 | |             &["db"]
1309 | |         ).unwrap();
     | |_________^
     |
     = help: consider adding a `#![recursion_limit="200"]` attribute to your crate
     = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error: Could not compile `tikv`.

@overvenus
Copy link
Member Author

Can you add documentation on using it?

Sure, I am going to add a README.

Do you think we should add it to the CI?

Good idea! Let's add it to jenkins CI, since it may take a long time to build. And fuzz it in our internal test platform.

@Hoverbear
Copy link
Contributor

@overvenus Can we maybe make Jenkins do a fuzz once per day or something? It would be sad to have unrelated fuzz failures on otherwise good PRs.

Hoverbear
Hoverbear previously approved these changes Apr 16, 2018
Copy link
Contributor

@Hoverbear Hoverbear left a comment

Choose a reason for hiding this comment

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

This looks good, can you open up a follow-up issue in SRE to add a Jenkins job for this?

fuzz/Cargo.toml Outdated
git = "https://github.com/pingcap/rust-protobuf.git"
branch = "fuzz-build"

# Prevent this from interfering with workspaces
Copy link
Member

Choose a reason for hiding this comment

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

Why not use workspace?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's automatically generated.

Copy link
Member

Choose a reason for hiding this comment

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

Then I think it's better to put it into workspace, so you don't need an additional lock file.

fuzz/Cargo.toml Outdated

[dependencies]
# HACK: A dummy dependency,
# it's only for building TiKV which depends on an unpublished protobuf.
Copy link
Member

Choose a reason for hiding this comment

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

If this is what you want, you can specify rev directly.

fuzz/.gitignore Outdated
@@ -0,0 +1,4 @@

target
corpus
Copy link
Member

Choose a reason for hiding this comment

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

Use the gitignore file in root directory instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's automatically generated.

Copy link
Member

Choose a reason for hiding this comment

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

So will it be regenerated again?

@@ -0,0 +1,18 @@

Copy link
Member

Choose a reason for hiding this comment

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

Why blank line?

Copy link
Member Author

Choose a reason for hiding this comment

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

fuzz/Cargo.toml Outdated
[package]
name = "tikv-fuzz"
version = "0.0.1"
authors = ["Automatically generated"]
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be updated.

[dependencies.tikv]
path = ".."
[dependencies.libfuzzer-sys]
git = "https://github.com/rust-fuzz/libfuzzer-sys.git"
Copy link
Member

Choose a reason for hiding this comment

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

Does it have a release version?

@@ -0,0 +1,14 @@
#![no_main]
#[macro_use] extern crate libfuzzer_sys;
extern crate tikv;
Copy link
Member

Choose a reason for hiding this comment

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

Does it mean everything we want to test with fuzz needs to be public?

@overvenus
Copy link
Member Author

PTAL

Copy link
Member

@BusyJay BusyJay left a comment

Choose a reason for hiding this comment

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

LGTM

@hicqu
Copy link
Contributor

hicqu commented Apr 25, 2018

LGTM

hicqu
hicqu previously approved these changes Apr 25, 2018
@overvenus overvenus merged commit ae28e9c into tikv:master Apr 26, 2018
@overvenus overvenus deleted the fuzz branch April 26, 2018 08:37
@Hoverbear Hoverbear mentioned this pull request Apr 28, 2018
1 task
@overvenus overvenus mentioned this pull request Jul 26, 2018
19 tasks
sticnarf pushed a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019
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.

5 participants