diff --git a/CHANGELOG.md b/CHANGELOG.md index 71543f99cca..e57fe62d5bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Fail when decoding from storage and not all bytes consumed - [#1897](https://github.com/paritytech/ink/pull/1897) +- Add storage value size assertion in `set_contract_storage` - [#1961](https://github.com/paritytech/ink/pull/1961) ### Added - Linter: `storage_never_freed` lint - [#1932](https://github.com/paritytech/ink/pull/1932) diff --git a/crates/env/src/engine/off_chain/impls.rs b/crates/env/src/engine/off_chain/impls.rs index f170a8ab537..c8c6c4465f6 100644 --- a/crates/env/src/engine/off_chain/impls.rs +++ b/crates/env/src/engine/off_chain/impls.rs @@ -196,8 +196,15 @@ impl EnvBackend for EnvInstance { V: Storable, { let mut v = vec![]; + Storable::encode(value, &mut v); - self.engine.set_storage(&key.encode(), &v[..]) + let encoded_key: Vec = key.encode(); + let encoded_value: &[u8] = &v[..]; + + if encoded_value.len() + encoded_key.len() > BUFFER_SIZE { + panic!("Value too large to be stored in contract storage, maximum size is {} bytes", BUFFER_SIZE); + } + self.engine.set_storage(&encoded_key, encoded_value) } fn get_contract_storage(&mut self, key: &K) -> Result> diff --git a/crates/ink/codegen/src/generator/trait_def/trait_registry.rs b/crates/ink/codegen/src/generator/trait_def/trait_registry.rs index 6d6471e53e3..b0fe1a25bdb 100644 --- a/crates/ink/codegen/src/generator/trait_def/trait_registry.rs +++ b/crates/ink/codegen/src/generator/trait_def/trait_registry.rs @@ -21,7 +21,9 @@ use super::TraitDefinition; use crate::{ - generator::{self,}, + generator::{ + self, + }, traits::GenerateCode, EnforcedErrors, }; diff --git a/crates/ink/ir/src/ir/attrs.rs b/crates/ink/ir/src/ir/attrs.rs index 4cd8320f434..100efa06548 100644 --- a/crates/ink/ir/src/ir/attrs.rs +++ b/crates/ink/ir/src/ir/attrs.rs @@ -32,7 +32,9 @@ use syn::{ }; use crate::{ - ast::{self,}, + ast::{ + self, + }, error::ExtError as _, ir, ir::{ diff --git a/crates/storage/src/lazy/mapping.rs b/crates/storage/src/lazy/mapping.rs index 22fd15bed85..892189212b8 100644 --- a/crates/storage/src/lazy/mapping.rs +++ b/crates/storage/src/lazy/mapping.rs @@ -171,7 +171,9 @@ where let value_size = ::encoded_size(value); - if key_size.saturating_add(value_size) > ink_env::BUFFER_SIZE { + // insert() will attempt to insert a tuple (u32, key), which increases + // the size of the key by 4. + if key_size.saturating_add(value_size).saturating_add(4) > ink_env::BUFFER_SIZE { return Err(ink_env::Error::BufferTooSmall) } @@ -379,6 +381,8 @@ const _: () = { #[cfg(test)] mod tests { + use std::panic; + use super::*; use crate::traits::ManualKey; @@ -483,10 +487,11 @@ mod tests { #[test] fn fallible_storage_works_for_fitting_data() { ink_env::test::run_test::(|_| { - let mut mapping: Mapping = Mapping::new(); + let mut mapping: Mapping = + Mapping::new(); let key = 0; - let value = [0u8; ink_env::BUFFER_SIZE - 1]; + let value = [0u8; ink_env::BUFFER_SIZE - 1 - 4]; assert_eq!(mapping.try_insert(key, &value), Ok(None)); assert_eq!(mapping.try_get(key), Some(Ok(value))); @@ -499,12 +504,13 @@ mod tests { } #[test] + #[should_panic] fn fallible_storage_fails_gracefully_for_overgrown_data() { ink_env::test::run_test::(|_| { - let mut mapping: Mapping = Mapping::new(); + let mut mapping: Mapping = Mapping::new(); let key = 0; - let value = [0u8; ink_env::BUFFER_SIZE]; + let value = [0u8; ink_env::BUFFER_SIZE - 4]; assert_eq!(mapping.try_get(0), None); assert_eq!( @@ -512,17 +518,7 @@ mod tests { Err(ink_env::Error::BufferTooSmall) ); - // The off-chain impl conveniently uses a Vec for encoding, - // allowing writing values exceeding the static buffer size. ink_env::set_contract_storage(&(&mapping.key(), key), &value); - assert_eq!( - mapping.try_get(key), - Some(Err(ink_env::Error::BufferTooSmall)) - ); - assert_eq!( - mapping.try_take(key), - Some(Err(ink_env::Error::BufferTooSmall)) - ); Ok(()) }) @@ -543,9 +539,10 @@ mod tests { Err(ink_env::Error::BufferTooSmall) ); - // The off-chain impl conveniently uses a Vec for encoding, - // allowing writing values exceeding the static buffer size. - ink_env::set_contract_storage(&(&mapping.key(), key), &value); + let result = panic::catch_unwind(|| { + ink_env::set_contract_storage(&(&mapping.key(), key), &value); + }); + assert!(result.is_err()); assert_eq!( mapping.try_get(key), Some(Err(ink_env::Error::BufferTooSmall)) diff --git a/crates/storage/src/lazy/mod.rs b/crates/storage/src/lazy/mod.rs index 058c975c0ec..7c201b54e1b 100644 --- a/crates/storage/src/lazy/mod.rs +++ b/crates/storage/src/lazy/mod.rs @@ -177,7 +177,9 @@ where /// /// Fails if `value` exceeds the static buffer size. pub fn try_set(&mut self, value: &V) -> ink_env::Result<()> { - if value.encoded_size() > ink_env::BUFFER_SIZE { + // set() will attempt to use a u32 key, which we need to account for + // here: + if value.encoded_size().saturating_add(4) > ink_env::BUFFER_SIZE { return Err(ink_env::Error::BufferTooSmall) }; @@ -317,9 +319,9 @@ mod tests { #[test] fn fallible_storage_works_for_fitting_data() { ink_env::test::run_test::(|_| { - let mut storage: Lazy<[u8; ink_env::BUFFER_SIZE]> = Lazy::new(); + let mut storage: Lazy<[u8; ink_env::BUFFER_SIZE - 4]> = Lazy::new(); - let value = [0u8; ink_env::BUFFER_SIZE]; + let value = [0u8; ink_env::BUFFER_SIZE - 4]; assert_eq!(storage.try_set(&value), Ok(())); assert_eq!(storage.try_get(), Some(Ok(value))); @@ -329,18 +331,16 @@ mod tests { } #[test] + #[should_panic] fn fallible_storage_fails_gracefully_for_overgrown_data() { ink_env::test::run_test::(|_| { - let mut storage: Lazy<[u8; ink_env::BUFFER_SIZE + 1]> = Lazy::new(); + let mut storage: Lazy<[u8; ink_env::BUFFER_SIZE - 4 + 1]> = Lazy::new(); - let value = [0u8; ink_env::BUFFER_SIZE + 1]; + let value = [0u8; ink_env::BUFFER_SIZE - 4 + 1]; assert_eq!(storage.try_get(), None); assert_eq!(storage.try_set(&value), Err(ink_env::Error::BufferTooSmall)); - // The off-chain impl conveniently uses a Vec for encoding, - // allowing writing values exceeding the static buffer size. ink_env::set_contract_storage(&storage.key(), &value); - assert_eq!(storage.try_get(), Some(Err(ink_env::Error::BufferTooSmall))); Ok(()) }) diff --git a/integration-tests/set-contract-storage/.gitignore b/integration-tests/set-contract-storage/.gitignore new file mode 100644 index 00000000000..bf910de10af --- /dev/null +++ b/integration-tests/set-contract-storage/.gitignore @@ -0,0 +1,9 @@ +# Ignore build artifacts from the local tests sub-crate. +/target/ + +# Ignore backup files creates by cargo fmt. +**/*.rs.bk + +# Remove Cargo.lock when creating an executable, leave it for libraries +# More information here http://doc.crates.io/guide.html#cargotoml-vs-cargolock +Cargo.lock \ No newline at end of file diff --git a/integration-tests/set-contract-storage/Cargo.toml b/integration-tests/set-contract-storage/Cargo.toml new file mode 100644 index 00000000000..ee7a9d906b2 --- /dev/null +++ b/integration-tests/set-contract-storage/Cargo.toml @@ -0,0 +1,27 @@ +[package] +name = "set-contract-storage" +version = "4.3.0" +authors = ["Parity Technologies "] +edition = "2021" +publish = false + +[dependencies] +ink = { path = "../../crates/ink", default-features = false } +scale = { package = "parity-scale-codec", version = "3", default-features = false, features = ["derive"] } +scale-info = { version = "2.5", default-features = false, features = ["derive"], optional = true } + +[dev-dependencies] +ink_e2e = { path = "../../crates/e2e" } + +[lib] +path = "lib.rs" + +[features] +default = ["std"] +std = [ + "ink/std", + "scale/std", + "scale-info/std" +] +ink-as-dependency = [] +e2e-tests = [] \ No newline at end of file diff --git a/integration-tests/set-contract-storage/lib.rs b/integration-tests/set-contract-storage/lib.rs new file mode 100644 index 00000000000..8305f95369a --- /dev/null +++ b/integration-tests/set-contract-storage/lib.rs @@ -0,0 +1,128 @@ +#![cfg_attr(not(feature = "std"), no_std, no_main)] + +#[ink::contract] +mod set_contract_storage { + use ink::env::set_contract_storage; + + const SIZE_LIMIT: usize = (1 << 14) - 4; + + #[ink(storage)] + pub struct SetContractStorage {} + + impl SetContractStorage { + /// Creates a new SetContractStorage contract. + #[ink(constructor)] + pub fn new() -> Self { + Self {} + } + + /// Stores an array that is JUST big enough to be validly allocated. + #[ink(message)] + pub fn set_storage_big(&self) { + println!("{}", SIZE_LIMIT.to_string()); + set_contract_storage(&42, &[42u8; SIZE_LIMIT]); + } + + /// Tries to store the smallest array that is too big to be validly + /// allocated. This function should always fail. + #[ink(message)] + pub fn set_storage_very_big(&self) { + println!("{}", SIZE_LIMIT.to_string()); + set_contract_storage(&42, &[42u8; SIZE_LIMIT + 1]); + } + } + + impl Default for SetContractStorage { + fn default() -> Self { + Self::new() + } + } + + #[cfg(test)] + mod tests { + use super::*; + + #[ink::test] + fn contract_storage_big() { + let contract = SetContractStorage::new(); + + contract.set_storage_big(); + + assert_eq!(0, 0); + } + + #[ink::test] + #[should_panic( + expected = "Value too large to be stored in contract storage, maximum size is 16384 bytes" + )] + fn contract_storage_too_big() { + let contract = SetContractStorage::new(); + + contract.set_storage_very_big(); + } + } + + #[cfg(all(test, feature = "e2e-tests"))] + mod e2e_tests { + use ink_e2e::ContractsBackend; + + use super::*; + + type E2EResult = std::result::Result>; + + #[ink_e2e::test] + async fn contract_storage_big( + mut client: ink_e2e::Client, + ) -> E2EResult<()> { + // given + let mut constructor = SetContractStorageRef::new(); + + let contract = client + .instantiate("set-contract-storage", &ink_e2e::alice(), &mut constructor) + .submit() + .await + .expect("instantiate failed"); + let call = contract.call::(); + + // when + let set_storage_big_call = call.set_storage_big(); + + let result = client + .call(&ink_e2e::alice(), &set_storage_big_call) + .submit() + .await; + + // then + assert!(result.is_ok(), "set_storage_big success"); + + Ok(()) + } + + #[ink_e2e::test] + async fn contract_storage_too_big( + mut client: Client, + ) -> E2EResult<()> { + // given + let mut constructor = SetContractStorageRef::new(); + + let contract = client + .instantiate("set-contract-storage", &ink_e2e::bob(), &mut constructor) + .submit() + .await + .expect("instantiate failed"); + let call = contract.call::(); + + // when + let set_storage_very_big_call = call.set_storage_very_big(); + + let result = client + .call(&ink_e2e::bob(), &set_storage_very_big_call) + .submit() + .await; + + assert!(result.is_err(), "set_storage_very_big failed"); + + Ok(()) + } + } +}