From 21c8d18c23f475a54d0c38ca163ba84a534fa2b6 Mon Sep 17 00:00:00 2001 From: Zeke Mostov <32168567+emostov@users.noreply.github.com> Date: Sun, 31 Oct 2021 22:10:13 +0100 Subject: [PATCH] Fuzzer for Pallet Bags List (#9851) * Fuzzer for Pallet Bags List * Some small updates * Fuzzer for Pallet Bags List This PR adds a fuzzer for the `SortedListProvider` API exposed by pallet-bags-list. * Feature gate code NOT used by fuzz feature * Create Enum for list actions * fix some small mistakes * try and make CI happy * fmt * Do not insert before updating * clean up some misc. comments * marginally improve Node::sanity_check * Change ID_RANGE to 25_000 * comma * try improve correct feature gating so no unused code Co-authored-by: thiolliere --- substrate/Cargo.lock | 10 +++ substrate/Cargo.toml | 1 + substrate/frame/bags-list/Cargo.toml | 6 ++ substrate/frame/bags-list/fuzzer/.gitignore | 2 + substrate/frame/bags-list/fuzzer/Cargo.toml | 22 +++++ substrate/frame/bags-list/fuzzer/src/main.rs | 88 ++++++++++++++++++++ substrate/frame/bags-list/src/lib.rs | 4 +- substrate/frame/bags-list/src/list/mod.rs | 14 ++-- substrate/frame/bags-list/src/mock.rs | 7 +- 9 files changed, 144 insertions(+), 10 deletions(-) create mode 100644 substrate/frame/bags-list/fuzzer/.gitignore create mode 100644 substrate/frame/bags-list/fuzzer/Cargo.toml create mode 100644 substrate/frame/bags-list/fuzzer/src/main.rs diff --git a/substrate/Cargo.lock b/substrate/Cargo.lock index 666fe6e451..3434e01d13 100644 --- a/substrate/Cargo.lock +++ b/substrate/Cargo.lock @@ -5180,6 +5180,16 @@ dependencies = [ "sp-tracing", ] +[[package]] +name = "pallet-bags-list-fuzzer" +version = "4.0.0-dev" +dependencies = [ + "frame-election-provider-support", + "honggfuzz", + "pallet-bags-list", + "rand 0.8.4", +] + [[package]] name = "pallet-balances" version = "4.0.0-dev" diff --git a/substrate/Cargo.toml b/substrate/Cargo.toml index 4a22820315..743e0f7066 100644 --- a/substrate/Cargo.toml +++ b/substrate/Cargo.toml @@ -135,6 +135,7 @@ members = [ "frame/utility", "frame/vesting", "frame/bags-list", + "frame/bags-list/fuzzer", "primitives/api", "primitives/api/proc-macro", "primitives/api/test", diff --git a/substrate/frame/bags-list/Cargo.toml b/substrate/frame/bags-list/Cargo.toml index cd06ce4a69..372dc87e21 100644 --- a/substrate/frame/bags-list/Cargo.toml +++ b/substrate/frame/bags-list/Cargo.toml @@ -63,4 +63,10 @@ runtime-benchmarks = [ "sp-tracing", "frame-election-provider-support/runtime-benchmarks", ] +fuzz = [ + "sp-core", + "sp-io", + "pallet-balances", + "sp-tracing", +] diff --git a/substrate/frame/bags-list/fuzzer/.gitignore b/substrate/frame/bags-list/fuzzer/.gitignore new file mode 100644 index 0000000000..3ebcb104d4 --- /dev/null +++ b/substrate/frame/bags-list/fuzzer/.gitignore @@ -0,0 +1,2 @@ +hfuzz_target +hfuzz_workspace diff --git a/substrate/frame/bags-list/fuzzer/Cargo.toml b/substrate/frame/bags-list/fuzzer/Cargo.toml new file mode 100644 index 0000000000..171e0e7af7 --- /dev/null +++ b/substrate/frame/bags-list/fuzzer/Cargo.toml @@ -0,0 +1,22 @@ +[package] +name = "pallet-bags-list-fuzzer" +version = "4.0.0-dev" +authors = ["Parity Technologies "] +edition = "2018" +license = "Apache-2.0" +homepage = "https://substrate.dev" +repository = "https://github.com/paritytech/substrate/" +description = "Fuzzer for FRAME pallet bags list" +readme = "README.md" +publish = false + +[dependencies] +honggfuzz = "0.5" +rand = { version = "0.8", features = ["std", "small_rng"] } + +pallet-bags-list = { version = "4.0.0-dev", features = ["fuzz"], path = ".." } +frame-election-provider-support = { version = "4.0.0-dev", path = "../../election-provider-support", features = ["runtime-benchmarks"] } + +[[bin]] +name = "bags-list" +path = "src/main.rs" diff --git a/substrate/frame/bags-list/fuzzer/src/main.rs b/substrate/frame/bags-list/fuzzer/src/main.rs new file mode 100644 index 0000000000..02a2003b9a --- /dev/null +++ b/substrate/frame/bags-list/fuzzer/src/main.rs @@ -0,0 +1,88 @@ +// This file is part of Substrate. + +// Copyright (C) 2021 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! # Running +//! Running this fuzzer can be done with `cargo hfuzz run bags-list`. `honggfuzz` CLI options can +//! be used by setting `HFUZZ_RUN_ARGS`, such as `-n 4` to use 4 threads. +//! +//! # Debugging a panic +//! Once a panic is found, it can be debugged with +//! `cargo hfuzz run-debug fixed_point hfuzz_workspace/bags_list/*.fuzz`. +//! +//! # More information +//! More information about `honggfuzz` can be found +//! [here](https://docs.rs/honggfuzz/). + +use frame_election_provider_support::{SortedListProvider, VoteWeight}; +use honggfuzz::fuzz; +use pallet_bags_list::mock::{AccountId, BagsList, ExtBuilder}; +use std::convert::From; + +const ID_RANGE: AccountId = 25_000; + +/// Actions of a `SortedListProvider` that we fuzz. +enum Action { + Insert, + Update, + Remove, +} + +impl From for Action { + fn from(v: u32) -> Self { + let num_variants = Self::Remove as u32 + 1; + match v % num_variants { + _x if _x == Action::Insert as u32 => Action::Insert, + _x if _x == Action::Update as u32 => Action::Update, + _x if _x == Action::Remove as u32 => Action::Remove, + _ => unreachable!(), + } + } +} + +fn main() { + ExtBuilder::default().build_and_execute(|| loop { + fuzz!(|data: (AccountId, VoteWeight, u32)| { + let (account_id_seed, vote_weight, action_seed) = data; + + let id = account_id_seed % ID_RANGE; + let action = Action::from(action_seed); + + match action { + Action::Insert => { + if BagsList::on_insert(id.clone(), vote_weight).is_err() { + // this was a duplicate id, which is ok. We can just update it. + BagsList::on_update(&id, vote_weight); + } + assert!(BagsList::contains(&id)); + }, + Action::Update => { + let already_contains = BagsList::contains(&id); + BagsList::on_update(&id, vote_weight); + if already_contains { + assert!(BagsList::contains(&id)); + } + }, + Action::Remove => { + BagsList::on_remove(&id); + assert!(!BagsList::contains(&id)); + }, + } + + assert!(BagsList::sanity_check().is_ok()); + }) + }); +} diff --git a/substrate/frame/bags-list/src/lib.rs b/substrate/frame/bags-list/src/lib.rs index 4202a4d499..10a692e8b3 100644 --- a/substrate/frame/bags-list/src/lib.rs +++ b/substrate/frame/bags-list/src/lib.rs @@ -59,8 +59,8 @@ use sp_std::prelude::*; mod benchmarks; mod list; -#[cfg(test)] -mod mock; +#[cfg(any(test, feature = "fuzz"))] +pub mod mock; #[cfg(test)] mod tests; pub mod weights; diff --git a/substrate/frame/bags-list/src/list/mod.rs b/substrate/frame/bags-list/src/list/mod.rs index 3f55f22271..057565e645 100644 --- a/substrate/frame/bags-list/src/list/mod.rs +++ b/substrate/frame/bags-list/src/list/mod.rs @@ -391,8 +391,8 @@ impl List { /// /// * there are no duplicate ids, /// * length of this list is in sync with `CounterForListNodes`, - /// * and sanity-checks all bags. This will cascade down all the checks and makes sure all bags - /// are checked per *any* update to `List`. + /// * and sanity-checks all bags and nodes. This will cascade down all the checks and makes sure + /// all bags and nodes are checked per *any* update to `List`. #[cfg(feature = "std")] pub(crate) fn sanity_check() -> Result<(), &'static str> { use frame_support::ensure; @@ -414,7 +414,6 @@ impl List { let thresholds = T::BagThresholds::get().iter().copied(); let thresholds: Vec = if thresholds.clone().last() == Some(VoteWeight::MAX) { // in the event that they included it, we don't need to make any changes - // Box::new(thresholds.collect() thresholds.collect() } else { // otherwise, insert it here. @@ -774,10 +773,13 @@ impl Node { "node does not exist in the expected bag" ); + let non_terminal_check = !self.is_terminal() && + expected_bag.head.as_ref() != Some(id) && + expected_bag.tail.as_ref() != Some(id); + let terminal_check = + expected_bag.head.as_ref() == Some(id) || expected_bag.tail.as_ref() == Some(id); frame_support::ensure!( - !self.is_terminal() || - expected_bag.head.as_ref() == Some(id) || - expected_bag.tail.as_ref() == Some(id), + non_terminal_check || terminal_check, "a terminal node is neither its bag head or tail" ); diff --git a/substrate/frame/bags-list/src/mock.rs b/substrate/frame/bags-list/src/mock.rs index a6ab35896b..45eb1d85ab 100644 --- a/substrate/frame/bags-list/src/mock.rs +++ b/substrate/frame/bags-list/src/mock.rs @@ -101,12 +101,13 @@ pub(crate) const GENESIS_IDS: [(AccountId, VoteWeight); 4] = [(1, 10), (2, 1_000), (3, 1_000), (4, 1_000)]; #[derive(Default)] -pub(crate) struct ExtBuilder { +pub struct ExtBuilder { ids: Vec<(AccountId, VoteWeight)>, } impl ExtBuilder { /// Add some AccountIds to insert into `List`. + #[cfg(test)] pub(crate) fn add_ids(mut self, ids: Vec<(AccountId, VoteWeight)>) -> Self { self.ids = ids; self @@ -126,18 +127,20 @@ impl ExtBuilder { ext } - pub(crate) fn build_and_execute(self, test: impl FnOnce() -> ()) { + pub fn build_and_execute(self, test: impl FnOnce() -> ()) { self.build().execute_with(|| { test(); List::::sanity_check().expect("Sanity check post condition failed") }) } + #[cfg(test)] pub(crate) fn build_and_execute_no_post_check(self, test: impl FnOnce() -> ()) { self.build().execute_with(test) } } +#[cfg(test)] pub(crate) mod test_utils { use super::*; use list::Bag;