Missing Limitation for Storage Size on Integration Testing for set_contract_storage()

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)

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<u8> = 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<K, R>(&mut self, key: &K) -> Result<Option<R>>

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,
 };

crates/ink/ir/src/ir/attrs.rs

@@ -32,7 +32,9 @@ use syn::{
 };
 
 use crate::{
-    ast::{self,},
+    ast::{
+        self,
+    },
     error::ExtError as _,
     ir,
     ir::{

crates/storage/src/lazy/mapping.rs

@@ -171,7 +171,9 @@ where
 
         let value_size = <R as Storable>::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::<ink_env::DefaultEnvironment, _>(|_| {
-            let mut mapping: Mapping<u8, [u8; ink_env::BUFFER_SIZE - 1]> = Mapping::new();
+            let mut mapping: Mapping<u8, [u8; ink_env::BUFFER_SIZE - 1 - 4]> =
+                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,30 +504,21 @@ mod tests {
     }
 
     #[test]
+    #[should_panic]
     fn fallible_storage_fails_gracefully_for_overgrown_data() {
         ink_env::test::run_test::<ink_env::DefaultEnvironment, _>(|_| {
-            let mut mapping: Mapping<u8, [u8; ink_env::BUFFER_SIZE]> = Mapping::new();
+            let mut mapping: Mapping<u8, [u8; ink_env::BUFFER_SIZE - 4]> = 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!(
                 mapping.try_insert(key, &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(&(&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))

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::<ink_env::DefaultEnvironment, _>(|_| {
-            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::<ink_env::DefaultEnvironment, _>(|_| {
-            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(())
         })

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

integration-tests/set-contract-storage/Cargo.toml

@@ -0,0 +1,27 @@
+[package]
+name = "set-contract-storage"
+version = "4.3.0"
+authors = ["Parity Technologies <[email protected]>"]
+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 = []

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<T> = std::result::Result<T, Box<dyn std::error::Error>>;
+
+        #[ink_e2e::test]
+        async fn contract_storage_big(
+            mut client: ink_e2e::Client<C, E>,
+        ) -> 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::<SetContractStorage>();
+
+            // 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<Client: E2EBackend>(
+            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::<SetContractStorage>();
+
+            // 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(())
+        }
+    }
+}