-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add fuzzer integration for gc/mutatis #11290
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 fuzzer integration for gc/mutatis #11290
Conversation
fitzgen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but there are a handful of small things to address before we merge this. Thanks!
crates/fuzzing/src/oracles.rs
Outdated
| /// Fuzz target for table operations that reconstructs config from seed. | ||
| pub fn fuzz_table_ops((config_seed, ops): (u64, generators::table_ops::TableOps)) { | ||
| let mut buf = [0u8; 1024]; | ||
| let mut rng = rand::rngs::StdRng::seed_from_u64(config_seed); | ||
|
|
||
| for b in buf.iter_mut() { | ||
| *b = rng.r#gen() | ||
| } | ||
|
|
||
| let u = Unstructured::new(&buf); | ||
| let config = generators::Config::arbitrary_take_rest(u) | ||
| .expect("should be able to generate config from seed"); | ||
|
|
||
| let _ = crate::oracles::table_ops(config, ops); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have an oracle for fuzzing table ops, which is what this function is ultimately calling. This is really just a small adapter for when we need to use Arbitrary to generate a config, and it feels out of place here. The only place we need to actually do that is in the fuzz target itself, so lets just inline it into the fuzz_target! invocation in fuzz/fuzz_targets/table_ops.rs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea makes sense! I guess I just updated the previous function which was in this file
fuzz/Cargo.toml
Outdated
|
|
||
| [dependencies] | ||
| mutatis = { workspace = true } | ||
| bincode = "2.0.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use postcard instead of bincode here, because we already depend on it in the workspace and don't need to worry about auditing or anything.
| bincode = "2.0.1" | |
| postcard = { workspace = true } |
fuzz/fuzz_targets/table_ops.rs
Outdated
| bincode::decode_from_slice::<(u64, TableOps), _>(data, bincode::config::standard()) | ||
| .map_or_else(|_err| (0, TableOps::default()), |(tuple, _)| tuple); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slight simplification:
| bincode::decode_from_slice::<(u64, TableOps), _>(data, bincode::config::standard()) | |
| .map_or_else(|_err| (0, TableOps::default()), |(tuple, _)| tuple); | |
| bincode::decode_from_slice::<(u64, TableOps), _>(data, bincode::config::standard()) | |
| .ok() | |
| .unwrap_or_default(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice!
| /// Pops from the vector of the opcodes and returns bool | ||
| pub fn pop(&mut self) -> bool { | ||
| self.ops.pop().is_some() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicks:
- Newline between methods, as elsewhere in the file
- "returns bool" is not very descriptive, the doc comment should describe what the boolean result means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted it to pass the clippy haha. Thanks!
crates/fuzzing/Cargo.toml
Outdated
| wasmtime-test-util = { workspace = true, features = ['wast'] } | ||
|
|
||
| [dependencies] | ||
| bincode = "2.0.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
postcard here too:
| bincode = "2.0.1" | |
| postcard = { workspace = true } |
|
Thanks! Working on them! |
Subscribe to Label Actioncc @fitzgen
This issue or pull request has been labeled: "fuzzing"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
07f2601 to
2a725fb
Compare
fitzgen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very close!
We should also remove table_ops from the misc fuzz target as part of this PR, as it will be redundant.
With the comment below addressed, this should be good to merge.
Thanks!
fuzz/fuzz_targets/table_ops.rs
Outdated
| use wasmtime_fuzzing::generators::table_ops::TableOps; | ||
| use wasmtime_fuzzing::oracles::table_ops; | ||
|
|
||
| fuzz_target!(|input: (u64, TableOps)| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want the fuzz target to take structured types directly, as those get generated via Arbitrary, and we want to do our serialization- and mutation-based thing here.
Instead we should do something like this:
fuzz_target!(|data: &[u8]| {
let Ok((seed, ops)) = postcard::from_bytes::<(u64, TableOps)>(data) else {
return;
}
// As before: Make an RNG from the seed, fill the buf with random
// data, make an `Unstructured`, call `Config::arbitrary_take_rest`,
// and finally invoke `oracles::table_ops(config, ops)`.
});Aside: once this PR lands, we should probably remove the Arbitrary implementation for TableOps as we won't be using it anymore and removing it will make usage footguns like this one disappear.
…t gives some dependecy error. Will be addressed later
2e0cfbf to
b08fe60
Compare
|
@khagankhan it looks like the |
|
I ran them locally and passed. Let us see!! |
|
@fitzgen Done ✅ |
fitzgen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
|
Would it be possible to re-integrate the |
|
Unfortunately, we can't because it is using mutation-based fuzzing via |
|
Sounds good to me 👍 |
* Add fuzzer integration for gc/mutatis * Replace bincode with postcard. Inline fuzz_mutate. Address other small comments * Update Cargo.lock after rebase * Update rng.gen to rng.fill to avoid deprecation failure * Remove table_ops from misc. Update fuzz_target. Keep Arbitrary impl it gives some dependecy error. Will be addressed later * Update Cargo.lock to match upstream/main * Regenerate Cargo.lock after rebase and dependency changes * Reset Cargo.lock to upstream/main to preserve cargo-deny compatibility * Update dependencies --------- Co-authored-by: Khagan Karimov <[email protected]>
mutatiswit fuzzerserialize/deserializefor config instead useu64serialize/deserializeforfuzz_mutate