diff --git a/CHANGELOG.md b/CHANGELOG.md index 55d330edf38..38527123b7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Make `set_code_hash` generic - [#1906](https://github.com/paritytech/ink/pull/1906) - Clean E2E configuration parsing - [#1922](https://github.com/paritytech/ink/pull/1922) +### Changed +- Fail when decoding from storage and not all bytes consumed - [#1897](https://github.com/paritytech/ink/pull/1897) + ### Added - Linter: `storage_never_freed` lint - [#1932](https://github.com/paritytech/ink/pull/1932) diff --git a/crates/engine/src/ext.rs b/crates/engine/src/ext.rs index 44fd531fa84..387eb57127c 100644 --- a/crates/engine/src/ext.rs +++ b/crates/engine/src/ext.rs @@ -34,8 +34,6 @@ use crate::{ use scale::Encode; use std::panic::panic_any; -type Result = core::result::Result<(), Error>; - macro_rules! define_error_codes { ( $( @@ -56,7 +54,7 @@ macro_rules! define_error_codes { Unknown, } - impl From for Result { + impl From for Result<(), Error> { #[inline] fn from(return_code: ReturnCode) -> Self { match return_code.0 { @@ -177,7 +175,7 @@ impl Default for Engine { impl Engine { /// Transfers value from the contract to the destination account. - pub fn transfer(&mut self, account_id: &[u8], mut value: &[u8]) -> Result { + pub fn transfer(&mut self, account_id: &[u8], mut value: &[u8]) -> Result<(), Error> { // Note that a transfer of `0` is allowed here let increment = ::decode(&mut value) .map_err(|_| Error::TransferFailed)?; @@ -240,33 +238,27 @@ impl Engine { .map(|v| ::try_from(v.len()).expect("usize to u32 conversion failed")) } - /// Returns the decoded contract storage at the key if any. - pub fn get_storage(&mut self, key: &[u8], output: &mut &mut [u8]) -> Result { + /// Returns the contract storage bytes at the key if any. + pub fn get_storage(&mut self, key: &[u8]) -> Result<&[u8], Error> { let callee = self.get_callee(); let account_id = AccountId::from_bytes(&callee[..]); self.debug_info.inc_reads(account_id); match self.database.get_from_contract_storage(&callee, key) { - Some(val) => { - set_output(output, val); - Ok(()) - } + Some(val) => Ok(val), None => Err(Error::KeyNotFound), } } /// Removes the storage entries at the given key, /// returning previously stored value at the key if any. - pub fn take_storage(&mut self, key: &[u8], output: &mut &mut [u8]) -> Result { + pub fn take_storage(&mut self, key: &[u8]) -> Result, Error> { let callee = self.get_callee(); let account_id = AccountId::from_bytes(&callee[..]); self.debug_info.inc_writes(account_id); match self.database.remove_contract_storage(&callee, key) { - Some(val) => { - set_output(output, &val); - Ok(()) - } + Some(val) => Ok(val), None => Err(Error::KeyNotFound), } } @@ -425,7 +417,7 @@ impl Engine { _out_address: &mut &mut [u8], _out_return_value: &mut &mut [u8], _salt: &[u8], - ) -> Result { + ) -> Result<(), Error> { unimplemented!("off-chain environment does not yet support `instantiate`"); } @@ -436,7 +428,7 @@ impl Engine { _value: &[u8], _input: &[u8], _output: &mut &mut [u8], - ) -> Result { + ) -> Result<(), Error> { unimplemented!("off-chain environment does not yet support `call`"); } @@ -475,7 +467,7 @@ impl Engine { signature: &[u8; 65], message_hash: &[u8; 32], output: &mut [u8; 33], - ) -> Result { + ) -> Result<(), Error> { use secp256k1::{ ecdsa::{ RecoverableSignature, diff --git a/crates/engine/src/test_api.rs b/crates/engine/src/test_api.rs index 6615e5aa19b..2633abb8c0c 100644 --- a/crates/engine/src/test_api.rs +++ b/crates/engine/src/test_api.rs @@ -334,17 +334,16 @@ mod tests { // given let mut engine = Engine::new(); let key: &[u8; 32] = &[0x42; 32]; - let mut buf = [0_u8; 32]; // when engine.set_callee(vec![1; 32]); engine.set_storage(key, &[0x05_u8; 5]); engine.set_storage(key, &[0x05_u8; 6]); - engine.get_storage(key, &mut &mut buf[..]).unwrap(); + engine.get_storage(key).unwrap(); engine.set_callee(vec![2; 32]); engine.set_storage(key, &[0x07_u8; 7]); - engine.get_storage(key, &mut &mut buf[..]).unwrap(); + engine.get_storage(key).unwrap(); // then assert_eq!(engine.count_writes(), 3); diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index ba0a99f668c..b9359ab187a 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -40,17 +40,16 @@ fn store_load_clear() { let mut engine = Engine::new(); engine.set_callee(vec![1; 32]); let key: &[u8; 32] = &[0x42; 32]; - let output = &mut &mut get_buffer()[..]; - let res = engine.get_storage(key, output); + let res = engine.get_storage(key); assert_eq!(res, Err(Error::KeyNotFound)); engine.set_storage(key, &[0x05_u8; 5]); - let res = engine.get_storage(key, output); - assert_eq!(res, Ok(()),); - assert_eq!(output[..5], [0x05; 5]); + let res = engine.get_storage(key); + assert!(res.is_ok()); + assert_eq!(res.unwrap()[..5], [0x05; 5]); engine.clear_storage(key); - let res = engine.get_storage(key, output); + let res = engine.get_storage(key); assert_eq!(res, Err(Error::KeyNotFound)); } @@ -180,26 +179,6 @@ fn value_transferred() { assert_eq!(output, value); } -#[test] -#[should_panic( - expected = "the output buffer is too small! the decoded storage is of size 16 bytes, but the output buffer has only room for 8." -)] -fn must_panic_when_buffer_too_small() { - // given - let mut engine = Engine::new(); - engine.set_callee(vec![1; 32]); - let key: &[u8; 32] = &[0x42; 32]; - engine.set_storage(key, &[0x05_u8; 16]); - - // when - let mut small_buffer = [0; 8]; - let output = &mut &mut small_buffer[..]; - let _ = engine.get_storage(key, output); - - // then - unreachable!("`get_storage` must already have panicked"); -} - #[test] fn ecdsa_recovery_test_from_contracts_pallet() { // given diff --git a/crates/env/src/engine/off_chain/impls.rs b/crates/env/src/engine/off_chain/impls.rs index 3ab693839ab..f170a8ab537 100644 --- a/crates/env/src/engine/off_chain/impls.rs +++ b/crates/env/src/engine/off_chain/impls.rs @@ -46,7 +46,10 @@ use ink_engine::{ ext, ext::Engine, }; -use ink_storage_traits::Storable; +use ink_storage_traits::{ + decode_all, + Storable, +}; use schnorrkel::{ PublicKey, Signature, @@ -202,14 +205,14 @@ impl EnvBackend for EnvInstance { K: scale::Encode, R: Storable, { - let mut output: [u8; 9600] = [0; 9600]; - match self.engine.get_storage(&key.encode(), &mut &mut output[..]) { - Ok(_) => (), - Err(ext::Error::KeyNotFound) => return Ok(None), + match self.engine.get_storage(&key.encode()) { + Ok(res) => { + let decoded = decode_all(&mut &res[..])?; + Ok(Some(decoded)) + } + Err(ext::Error::KeyNotFound) => Ok(None), Err(_) => panic!("encountered unexpected error"), } - let decoded = Storable::decode(&mut &output[..])?; - Ok(Some(decoded)) } fn take_contract_storage(&mut self, key: &K) -> Result> @@ -217,17 +220,14 @@ impl EnvBackend for EnvInstance { K: scale::Encode, R: Storable, { - let mut output: [u8; 9600] = [0; 9600]; - match self - .engine - .take_storage(&key.encode(), &mut &mut output[..]) - { - Ok(_) => (), - Err(ext::Error::KeyNotFound) => return Ok(None), + match self.engine.take_storage(&key.encode()) { + Ok(output) => { + let decoded = decode_all(&mut &output[..])?; + Ok(Some(decoded)) + } + Err(ext::Error::KeyNotFound) => Ok(None), Err(_) => panic!("encountered unexpected error"), } - let decoded = Storable::decode(&mut &output[..])?; - Ok(Some(decoded)) } fn contains_contract_storage(&mut self, key: &K) -> Option diff --git a/crates/env/src/engine/on_chain/impls.rs b/crates/env/src/engine/on_chain/impls.rs index 06b6fec8465..3c5df7613ea 100644 --- a/crates/env/src/engine/on_chain/impls.rs +++ b/crates/env/src/engine/on_chain/impls.rs @@ -48,7 +48,10 @@ use crate::{ ReturnFlags, TypedEnvBackend, }; -use ink_storage_traits::Storable; +use ink_storage_traits::{ + decode_all, + Storable, +}; impl CryptoHash for Blake2x128 { fn hash(input: &[u8], output: &mut ::Type) { @@ -236,7 +239,7 @@ impl EnvBackend for EnvInstance { Err(ExtError::KeyNotFound) => return Ok(None), Err(_) => panic!("encountered unexpected error"), } - let decoded = Storable::decode(&mut &output[..])?; + let decoded = decode_all(&mut &output[..])?; Ok(Some(decoded)) } @@ -253,7 +256,7 @@ impl EnvBackend for EnvInstance { Err(ExtError::KeyNotFound) => return Ok(None), Err(_) => panic!("encountered unexpected error"), } - let decoded = Storable::decode(&mut &output[..])?; + let decoded = decode_all(&mut &output[..])?; Ok(Some(decoded)) } diff --git a/crates/storage/traits/src/lib.rs b/crates/storage/traits/src/lib.rs index ad0ed6d697a..465676ea257 100644 --- a/crates/storage/traits/src/lib.rs +++ b/crates/storage/traits/src/lib.rs @@ -47,6 +47,7 @@ pub use self::{ ResolverKey, }, storage::{ + decode_all, AutoStorableHint, Packed, Storable, diff --git a/crates/storage/traits/src/storage.rs b/crates/storage/traits/src/storage.rs index a0e3c4951dd..05d92373edd 100644 --- a/crates/storage/traits/src/storage.rs +++ b/crates/storage/traits/src/storage.rs @@ -44,6 +44,19 @@ where } } +/// Decode and consume all of the given input data. +/// +/// If not all data is consumed, an error is returned. +pub fn decode_all(input: &mut &[u8]) -> Result { + let res = ::decode(input)?; + + if input.is_empty() { + Ok(res) + } else { + Err("Input buffer has still data left after decoding!".into()) + } +} + pub(crate) mod private { /// Seals the implementation of `Packed`. pub trait Sealed {} diff --git a/integration-tests/contract-storage/.gitignore b/integration-tests/contract-storage/.gitignore new file mode 100755 index 00000000000..8de8f877e47 --- /dev/null +++ b/integration-tests/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 diff --git a/integration-tests/contract-storage/Cargo.toml b/integration-tests/contract-storage/Cargo.toml new file mode 100755 index 00000000000..efbbd475132 --- /dev/null +++ b/integration-tests/contract-storage/Cargo.toml @@ -0,0 +1,23 @@ +[package] +name = "contract-storage" +version = "4.2.0" +authors = ["Parity Technologies "] +edition = "2021" +publish = false + +[dependencies] +ink = { path = "../../crates/ink", default-features = false } + +[dev-dependencies] +ink_e2e = { path = "../../crates/e2e" } + +[lib] +path = "lib.rs" + +[features] +default = ["std"] +std = [ + "ink/std", +] +ink-as-dependency = [] +e2e-tests = [] diff --git a/integration-tests/contract-storage/e2e_tests.rs b/integration-tests/contract-storage/e2e_tests.rs new file mode 100644 index 00000000000..d5655139bd8 --- /dev/null +++ b/integration-tests/contract-storage/e2e_tests.rs @@ -0,0 +1,122 @@ +use super::contract_storage::*; +use ink_e2e::ContractsBackend; + +type E2EResult = std::result::Result>; + +#[ink_e2e::test] +async fn get_contract_storage_consumes_entire_buffer( + mut client: Client, +) -> E2EResult<()> { + // given + let mut constructor = ContractStorageRef::new(); + let contract = client + .instantiate("contract-storage", &ink_e2e::alice(), &mut constructor) + .submit() + .await + .expect("instantiate failed"); + let call = contract.call::(); + + // when + let result = client + .call( + &ink_e2e::alice(), + &call.set_and_get_storage_all_data_consumed(), + ) + .submit() + .await + .expect("Calling `insert_balance` failed") + .return_value(); + + assert!(result.is_ok()); + + Ok(()) +} + +#[ink_e2e::test] +async fn get_contract_storage_fails_when_extra_data( + mut client: Client, +) -> E2EResult<()> { + // given + let mut constructor = ContractStorageRef::new(); + let contract = client + .instantiate("contract-storage", &ink_e2e::alice(), &mut constructor) + .submit() + .await + .expect("instantiate failed"); + let call = contract.call::(); + + // when + let result = client + .call( + &ink_e2e::alice(), + &call.set_and_get_storage_partial_data_consumed(), + ) + .submit() + .await; + + assert!( + result.is_err(), + "Expected the contract to revert when only partially consuming the buffer" + ); + + Ok(()) +} + +#[ink_e2e::test] +async fn take_contract_storage_consumes_entire_buffer( + mut client: Client, +) -> E2EResult<()> { + // given + let mut constructor = ContractStorageRef::new(); + let contract = client + .instantiate("contract-storage", &ink_e2e::alice(), &mut constructor) + .submit() + .await + .expect("instantiate failed"); + let call = contract.call::(); + + // when + let result = client + .call( + &ink_e2e::alice(), + &call.set_and_take_storage_all_data_consumed(), + ) + .submit() + .await + .expect("Calling `insert_balance` failed") + .return_value(); + + assert!(result.is_ok()); + + Ok(()) +} + +#[ink_e2e::test] +async fn take_contract_storage_fails_when_extra_data( + mut client: Client, +) -> E2EResult<()> { + // given + let mut constructor = ContractStorageRef::new(); + let contract = client + .instantiate("contract-storage", &ink_e2e::alice(), &mut constructor) + .submit() + .await + .expect("instantiate failed"); + let call = contract.call::(); + + // when + let result = client + .call( + &ink_e2e::alice(), + &call.set_and_take_storage_partial_data_consumed(), + ) + .submit() + .await; + + assert!( + result.is_err(), + "Expected the contract to revert when only partially consuming the buffer" + ); + + Ok(()) +} diff --git a/integration-tests/contract-storage/lib.rs b/integration-tests/contract-storage/lib.rs new file mode 100755 index 00000000000..1404711f13c --- /dev/null +++ b/integration-tests/contract-storage/lib.rs @@ -0,0 +1,76 @@ +//! A smart contract to test reading and writing contract storage + +#![cfg_attr(not(feature = "std"), no_std, no_main)] + +#[ink::contract] +mod contract_storage { + use ink::prelude::{ + format, + string::String, + }; + + /// A contract for testing reading and writing contract storage. + #[ink(storage)] + #[derive(Default)] + pub struct ContractStorage; + + impl ContractStorage { + #[ink(constructor)] + pub fn new() -> Self { + Self {} + } + + /// Read from the contract storage slot, consuming all the data from the buffer. + #[ink(message)] + pub fn set_and_get_storage_all_data_consumed(&self) -> Result<(), String> { + let key = 0u32; + let value = [0x42; 32]; + ink::env::set_contract_storage(&key, &value); + let loaded_value = ink::env::get_contract_storage(&key) + .map_err(|e| format!("get_contract_storage failed: {:?}", e))?; + assert_eq!(loaded_value, Some(value)); + Ok(()) + } + + /// Read from the contract storage slot, only partially consuming data from the + /// buffer. + #[ink(message)] + pub fn set_and_get_storage_partial_data_consumed(&self) -> Result<(), String> { + let key = 0u32; + let value = [0x42; 32]; + ink::env::set_contract_storage(&key, &value); + // Only attempt to read the first byte (the `u8`) of the storage value data + let _loaded_value: Option = ink::env::get_contract_storage(&key) + .map_err(|e| format!("get_contract_storage failed: {:?}", e))?; + Ok(()) + } + + /// Read from the contract storage slot, consuming all the data from the buffer. + #[ink(message)] + pub fn set_and_take_storage_all_data_consumed(&self) -> Result<(), String> { + let key = 0u32; + let value = [0x42; 32]; + ink::env::set_contract_storage(&key, &value); + let loaded_value = ink::env::take_contract_storage(&key) + .map_err(|e| format!("get_contract_storage failed: {:?}", e))?; + assert_eq!(loaded_value, Some(value)); + Ok(()) + } + + /// Read from the contract storage slot, only partially consuming data from the + /// buffer. + #[ink(message)] + pub fn set_and_take_storage_partial_data_consumed(&self) -> Result<(), String> { + let key = 0u32; + let value = [0x42; 32]; + ink::env::set_contract_storage(&key, &value); + // Only attempt to read the first byte (the `u8`) of the storage value data + let _loaded_value: Option = ink::env::take_contract_storage(&key) + .map_err(|e| format!("get_contract_storage failed: {:?}", e))?; + Ok(()) + } + } +} + +#[cfg(all(test, feature = "e2e-tests"))] +mod e2e_tests;