Fail when decoding from storage and not all bytes consumed (#1897)

* Add passing test to check reading and writing a value with the same size

* Add FAILING test for not consuming the entire buffer

* Add new DecodeAll trait, and use it.

* Fmt

* Use helper method instead of extension trait

* Space

* Storable

* offchain engine return populated buffer

* Fix up integration test

* Test for take_storage

* Fix tests, clippy

* Remove Result type alias

* CHANGELOG.md

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)
 

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<ReturnCode> for Result {
+        impl From<ReturnCode> 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 = <u128 as scale::Decode>::decode(&mut value)
             .map_err(|_| Error::TransferFailed)?;
@@ -240,33 +238,27 @@ impl Engine {
             .map(|v| <u32>::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<Vec<u8>, 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,

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);

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

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,32 +205,29 @@ 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<K, R>(&mut self, key: &K) -> Result<Option<R>>
     where
         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<K>(&mut self, key: &K) -> Option<u32>

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 <Self as HashOutput>::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))
     }
 

crates/storage/traits/src/lib.rs

@@ -47,6 +47,7 @@ pub use self::{
         ResolverKey,
     },
     storage::{
+        decode_all,
         AutoStorableHint,
         Packed,
         Storable,

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<T: Storable>(input: &mut &[u8]) -> Result<T, scale::Error> {
+    let res = <T as Storable>::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 {}

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

integration-tests/contract-storage/Cargo.toml

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