Use checked math when calculating storage size (paritytech#7885)

frame/contracts/src/exec.rs

@@ -24,7 +24,7 @@ use sp_core::crypto::UncheckedFrom;
 use sp_std::prelude::*;
 use sp_runtime::traits::{Bounded, Zero, Convert, Saturating};
 use frame_support::{
-	dispatch::DispatchError,
+	dispatch::DispatchResult,
 	traits::{ExistenceRequirement, Currency, Time, Randomness},
 	weights::Weight,
 	ensure, StorageMap,
@@ -65,7 +65,7 @@ pub trait Ext {
 
 	/// Sets the storage entry by the given key to the specified value. If `value` is `None` then
 	/// the storage entry is deleted.
-	fn set_storage(&mut self, key: StorageKey, value: Option<Vec<u8>>);
+	fn set_storage(&mut self, key: StorageKey, value: Option<Vec<u8>>) -> DispatchResult;
 
 	/// Instantiate a contract from the given code.
 	///
@@ -85,7 +85,7 @@ pub trait Ext {
 		&mut self,
 		to: &AccountIdOf<Self::T>,
 		value: BalanceOf<Self::T>,
-	) -> Result<(), DispatchError>;
+	) -> DispatchResult;
 
 	/// Transfer all funds to `beneficiary` and delete the contract.
 	///
@@ -97,7 +97,7 @@ pub trait Ext {
 	fn terminate(
 		&mut self,
 		beneficiary: &AccountIdOf<Self::T>,
-	) -> Result<(), DispatchError>;
+	) -> DispatchResult;
 
 	/// Call (possibly transferring some amount of funds) into the specified account.
 	fn call(
@@ -121,7 +121,7 @@ pub trait Ext {
 		code_hash: CodeHash<Self::T>,
 		rent_allowance: BalanceOf<Self::T>,
 		delta: Vec<StorageKey>,
-	) -> Result<(), DispatchError>;
+	) -> DispatchResult;
 
 	/// Returns a reference to the account id of the caller.
 	fn caller(&self) -> &AccountIdOf<Self::T>;
@@ -454,7 +454,7 @@ fn transfer<'a, T: Config, V: Vm<T>, L: Loader<T>>(
 	dest: &T::AccountId,
 	value: BalanceOf<T>,
 	ctx: &mut ExecutionContext<'a, T, V, L>,
-) -> Result<(), DispatchError>
+) -> DispatchResult
 where
 	T::AccountId: UncheckedFrom<T::Hash> + AsRef<[u8]>,
 {
@@ -520,23 +520,19 @@ where
 		Storage::<T>::read(trie_id, key)
 	}
 
-	fn set_storage(&mut self, key: StorageKey, value: Option<Vec<u8>>) {
+	fn set_storage(&mut self, key: StorageKey, value: Option<Vec<u8>>) -> DispatchResult {
 		let trie_id = self.ctx.self_trie_id.as_ref().expect(
 			"`ctx.self_trie_id` points to an alive contract within the `CallContext`;\
 				it cannot be `None`;\
 				expect can't fail;\
 				qed",
 		);
-		if let Err(storage::ContractAbsentError) =
-			Storage::<T>::write(&self.ctx.self_account, trie_id, &key, value)
-		{
-			panic!(
-				"the contract must be in the alive state within the `CallContext`;\
-				the contract cannot be absent in storage;
-				write cannot return `None`;
-				qed"
-			);
-		}
+		// write panics if the passed account is not alive.
+		// the contract must be in the alive state within the `CallContext`;\
+		// the contract cannot be absent in storage;
+		// write cannot return `None`;
+		// qed
+		Storage::<T>::write(&self.ctx.self_account, trie_id, &key, value)
 	}
 
 	fn instantiate(
@@ -554,7 +550,7 @@ where
 		&mut self,
 		to: &T::AccountId,
 		value: BalanceOf<T>,
-	) -> Result<(), DispatchError> {
+	) -> DispatchResult {
 		transfer(
 			TransferCause::Call,
 			TransactorKind::Contract,
@@ -568,7 +564,7 @@ where
 	fn terminate(
 		&mut self,
 		beneficiary: &AccountIdOf<Self::T>,
-	) -> Result<(), DispatchError> {
+	) -> DispatchResult {
 		let self_id = self.ctx.self_account.clone();
 		let value = T::Currency::free_balance(&self_id);
 		if let Some(caller_ctx) = self.ctx.caller {
@@ -612,7 +608,7 @@ where
 		code_hash: CodeHash<Self::T>,
 		rent_allowance: BalanceOf<Self::T>,
 		delta: Vec<StorageKey>,
-	) -> Result<(), DispatchError> {
+	) -> DispatchResult {
 		if let Some(caller_ctx) = self.ctx.caller {
 			if caller_ctx.is_live(&self.ctx.self_account) {
 				return Err(Error::<T>::ReentranceDenied.into());

frame/contracts/src/lib.rs

@@ -421,6 +421,11 @@ decl_error! {
 		/// This can be returned from [`Module::claim_surcharge`] because the target
 		/// contract has enough balance to pay for its rent.
 		ContractNotEvictable,
+		/// A storage modification exhausted the 32bit type that holds the storage size.
+		///
+		/// This can either happen when the accumulated storage in bytes is too large or
+		/// when number of storage items is too large.
+		StorageExhausted,
 	}
 }
 

frame/contracts/src/storage.rs

@@ -49,8 +49,6 @@ pub struct DeletedContract {
 	trie_id: TrieId,
 }
 
-
-
 pub struct Storage<T>(PhantomData<T>);
 
 impl<T> Storage<T>
@@ -75,15 +73,19 @@ where
 	/// `read`, this function also requires the `account` ID.
 	///
 	/// If the contract specified by the id `account` doesn't exist `Err` is returned.`
+	///
+	/// # Panics
+	///
+	/// Panics iff the `account` specified is not alive and in storage.
 	pub fn write(
 		account: &AccountIdOf<T>,
 		trie_id: &TrieId,
 		key: &StorageKey,
 		opt_new_value: Option<Vec<u8>>,
-	) -> Result<(), ContractAbsentError> {
+	) -> DispatchResult {
 		let mut new_info = match <ContractInfoOf<T>>::get(account) {
 			Some(ContractInfo::Alive(alive)) => alive,
-			None | Some(ContractInfo::Tombstone(_)) => return Err(ContractAbsentError),
+			None | Some(ContractInfo::Tombstone(_)) => panic!("Contract not found"),
 		};
 
 		let hashed_key = blake2_256(key);
@@ -94,10 +96,12 @@ where
 		// Update the total number of KV pairs and the number of empty pairs.
 		match (&opt_prev_len, &opt_new_value) {
 			(Some(_), None) => {
-				new_info.pair_count -= 1;
+				new_info.pair_count = new_info.pair_count.checked_sub(1)
+					.ok_or_else(|| Error::<T>::StorageExhausted)?;
 			},
 			(None, Some(_)) => {
-				new_info.pair_count += 1;
+				new_info.pair_count = new_info.pair_count.checked_add(1)
+					.ok_or_else(|| Error::<T>::StorageExhausted)?;
 			},
 			(Some(_), Some(_)) => {},
 			(None, None) => {},
@@ -111,8 +115,9 @@ where
 			.unwrap_or(0);
 		new_info.storage_size = new_info
 			.storage_size
-			.saturating_add(new_value_len)
-			.saturating_sub(prev_value_len);
+			.checked_sub(prev_value_len)
+			.and_then(|val| val.checked_add(new_value_len))
+			.ok_or_else(|| Error::<T>::StorageExhausted)?;
 
 		new_info.last_write = Some(<frame_system::Module<T>>::block_number());
 		<ContractInfoOf<T>>::insert(&account, ContractInfo::Alive(new_info));

frame/contracts/src/wasm/mod.rs

@@ -18,15 +18,15 @@
 //! This module provides a means for executing contracts
 //! represented in wasm.
 
-use crate::{CodeHash, Schedule, Config};
-use crate::wasm::env_def::FunctionImplProvider;
-use crate::exec::Ext;
-use crate::gas::GasMeter;
-
+use crate::{
+	CodeHash, Schedule, Config,
+	wasm::env_def::FunctionImplProvider,
+	exec::Ext,
+	gas::GasMeter,
+};
 use sp_std::prelude::*;
 use sp_core::crypto::UncheckedFrom;
 use codec::{Encode, Decode};
-use sp_sandbox;
 
 #[macro_use]
 mod env_def;
@@ -172,7 +172,7 @@ mod tests {
 	use sp_core::H256;
 	use hex_literal::hex;
 	use sp_runtime::DispatchError;
-	use frame_support::weights::Weight;
+	use frame_support::{dispatch::DispatchResult, weights::Weight};
 	use assert_matches::assert_matches;
 	use pallet_contracts_primitives::{ExecReturnValue, ReturnFlags, ExecError, ErrorOrigin};
 
@@ -228,8 +228,9 @@ mod tests {
 		fn get_storage(&self, key: &StorageKey) -> Option<Vec<u8>> {
 			self.storage.get(key).cloned()
 		}
-		fn set_storage(&mut self, key: StorageKey, value: Option<Vec<u8>>) {
+		fn set_storage(&mut self, key: StorageKey, value: Option<Vec<u8>>) -> DispatchResult {
 			*self.storage.entry(key).or_insert(Vec::new()) = value.unwrap_or(Vec::new());
+			Ok(())
 		}
 		fn instantiate(
 			&mut self,
@@ -362,7 +363,7 @@ mod tests {
 		fn get_storage(&self, key: &[u8; 32]) -> Option<Vec<u8>> {
 			(**self).get_storage(key)
 		}
-		fn set_storage(&mut self, key: [u8; 32], value: Option<Vec<u8>>) {
+		fn set_storage(&mut self, key: [u8; 32], value: Option<Vec<u8>>) -> DispatchResult {
 			(**self).set_storage(key, value)
 		}
 		fn instantiate(

frame/contracts/src/wasm/runtime.rs

@@ -643,8 +643,7 @@ define_env!(Env, <E: Ext>,
 		let mut key: StorageKey = [0; 32];
 		ctx.read_sandbox_memory_into_buf(key_ptr, &mut key)?;
 		let value = Some(ctx.read_sandbox_memory(value_ptr, value_len)?);
-		ctx.ext.set_storage(key, value);
-		Ok(())
+		ctx.ext.set_storage(key, value).map_err(Into::into)
 	},
 
 	// Clear the value at the given key in the contract storage.
@@ -656,8 +655,7 @@ define_env!(Env, <E: Ext>,
 		ctx.charge_gas(RuntimeToken::ClearStorage)?;
 		let mut key: StorageKey = [0; 32];
 		ctx.read_sandbox_memory_into_buf(key_ptr, &mut key)?;
-		ctx.ext.set_storage(key, None);
-		Ok(())
+		ctx.ext.set_storage(key, None).map_err(Into::into)
 	},
 
 	// Retrieve the value under the given key from storage.