[contracts] Add debug buffer limit + enforcement (paritytech#12845)

* Add debug buffer limit + enforcement

Add debug buffer limit + enforcement

* use BoundedVec for the debug buffer

* revert schedule (debug buf len limit not needed anymore)

* return DispatchError

* addressed review comments

bin/node/runtime/src/lib.rs

@@ -1216,6 +1216,7 @@ impl pallet_contracts::Config for Runtime {
 	type MaxCodeLen = ConstU32<{ 128 * 1024 }>;
 	type MaxStorageKeyLen = ConstU32<128>;
 	type UnsafeUnstableInterface = ConstBool<false>;
+	type MaxDebugBufferLen = ConstU32<{ 2 * 1024 * 1024 }>;
 }
 
 impl pallet_sudo::Config for Runtime {

frame/contracts/src/exec.rs

@@ -18,8 +18,8 @@
 use crate::{
 	gas::GasMeter,
 	storage::{self, Storage, WriteOutcome},
-	BalanceOf, CodeHash, Config, ContractInfo, ContractInfoOf, Determinism, Error, Event, Nonce,
-	Pallet as Contracts, Schedule,
+	BalanceOf, CodeHash, Config, ContractInfo, ContractInfoOf, DebugBufferVec, Determinism, Error,
+	Event, Nonce, Pallet as Contracts, Schedule,
 };
 use frame_support::{
 	crypto::ecdsa::ECDSAExt,
@@ -279,7 +279,7 @@ pub trait Ext: sealing::Sealed {
 	/// when the code is executing on-chain.
 	///
 	/// Returns `true` if debug message recording is enabled. Otherwise `false` is returned.
-	fn append_debug_buffer(&mut self, msg: &str) -> bool;
+	fn append_debug_buffer(&mut self, msg: &str) -> Result<bool, DispatchError>;
 
 	/// Call some dispatchable and return the result.
 	fn call_runtime(&self, call: <Self::T as Config>::RuntimeCall) -> DispatchResultWithPostInfo;
@@ -409,7 +409,7 @@ pub struct Stack<'a, T: Config, E> {
 	///
 	/// All the bytes added to this field should be valid UTF-8. The buffer has no defined
 	/// structure and is intended to be shown to users as-is for debugging purposes.
-	debug_message: Option<&'a mut Vec<u8>>,
+	debug_message: Option<&'a mut DebugBufferVec<T>>,
 	/// The determinism requirement of this call stack.
 	determinism: Determinism,
 	/// No executable is held by the struct but influences its behaviour.
@@ -617,7 +617,7 @@ where
 		schedule: &'a Schedule<T>,
 		value: BalanceOf<T>,
 		input_data: Vec<u8>,
-		debug_message: Option<&'a mut Vec<u8>>,
+		debug_message: Option<&'a mut DebugBufferVec<T>>,
 		determinism: Determinism,
 	) -> Result<ExecReturnValue, ExecError> {
 		let (mut stack, executable) = Self::new(
@@ -652,7 +652,7 @@ where
 		value: BalanceOf<T>,
 		input_data: Vec<u8>,
 		salt: &[u8],
-		debug_message: Option<&'a mut Vec<u8>>,
+		debug_message: Option<&'a mut DebugBufferVec<T>>,
 	) -> Result<(T::AccountId, ExecReturnValue), ExecError> {
 		let (mut stack, executable) = Self::new(
 			FrameArgs::Instantiate {
@@ -681,7 +681,7 @@ where
 		storage_meter: &'a mut storage::meter::Meter<T>,
 		schedule: &'a Schedule<T>,
 		value: BalanceOf<T>,
-		debug_message: Option<&'a mut Vec<u8>>,
+		debug_message: Option<&'a mut DebugBufferVec<T>>,
 		determinism: Determinism,
 	) -> Result<(Self, E), ExecError> {
 		let (first_frame, executable, nonce) = Self::new_frame(
@@ -1328,14 +1328,16 @@ where
 		&mut self.top_frame_mut().nested_gas
 	}
 
-	fn append_debug_buffer(&mut self, msg: &str) -> bool {
+	fn append_debug_buffer(&mut self, msg: &str) -> Result<bool, DispatchError> {
 		if let Some(buffer) = &mut self.debug_message {
 			if !msg.is_empty() {
-				buffer.extend(msg.as_bytes());
+				buffer
+					.try_extend(&mut msg.bytes())
+					.map_err(|_| Error::<T>::DebugBufferExhausted)?;
 			}
-			true
+			Ok(true)
 		} else {
-			false
+			Ok(false)
 		}
 	}
 
@@ -2503,12 +2505,16 @@ mod tests {
 	#[test]
 	fn printing_works() {
 		let code_hash = MockLoader::insert(Call, |ctx, _| {
-			ctx.ext.append_debug_buffer("This is a test");
-			ctx.ext.append_debug_buffer("More text");
+			ctx.ext
+				.append_debug_buffer("This is a test")
+				.expect("Maximum allowed debug buffer size exhausted!");
+			ctx.ext
+				.append_debug_buffer("More text")
+				.expect("Maximum allowed debug buffer size exhausted!");
 			exec_success()
 		});
 
-		let mut debug_buffer = Vec::new();
+		let mut debug_buffer = DebugBufferVec::<Test>::try_from(Vec::new()).unwrap();
 
 		ExtBuilder::default().build().execute_with(|| {
 			let min_balance = <Test as Config>::Currency::minimum_balance();
@@ -2531,18 +2537,22 @@ mod tests {
 			.unwrap();
 		});
 
-		assert_eq!(&String::from_utf8(debug_buffer).unwrap(), "This is a testMore text");
+		assert_eq!(&String::from_utf8(debug_buffer.to_vec()).unwrap(), "This is a testMore text");
 	}
 
 	#[test]
 	fn printing_works_on_fail() {
 		let code_hash = MockLoader::insert(Call, |ctx, _| {
-			ctx.ext.append_debug_buffer("This is a test");
-			ctx.ext.append_debug_buffer("More text");
+			ctx.ext
+				.append_debug_buffer("This is a test")
+				.expect("Maximum allowed debug buffer size exhausted!");
+			ctx.ext
+				.append_debug_buffer("More text")
+				.expect("Maximum allowed debug buffer size exhausted!");
 			exec_trapped()
 		});
 
-		let mut debug_buffer = Vec::new();
+		let mut debug_buffer = DebugBufferVec::<Test>::try_from(Vec::new()).unwrap();
 
 		ExtBuilder::default().build().execute_with(|| {
 			let min_balance = <Test as Config>::Currency::minimum_balance();
@@ -2565,7 +2575,43 @@ mod tests {
 			assert!(result.is_err());
 		});
 
-		assert_eq!(&String::from_utf8(debug_buffer).unwrap(), "This is a testMore text");
+		assert_eq!(&String::from_utf8(debug_buffer.to_vec()).unwrap(), "This is a testMore text");
+	}
+
+	#[test]
+	fn debug_buffer_is_limited() {
+		let code_hash = MockLoader::insert(Call, move |ctx, _| {
+			ctx.ext.append_debug_buffer("overflowing bytes")?;
+			exec_success()
+		});
+
+		// Pre-fill the buffer up to its limit
+		let mut debug_buffer =
+			DebugBufferVec::<Test>::try_from(vec![0u8; DebugBufferVec::<Test>::bound()]).unwrap();
+
+		ExtBuilder::default().build().execute_with(|| {
+			let schedule: Schedule<Test> = <Test as Config>::Schedule::get();
+			let min_balance = <Test as Config>::Currency::minimum_balance();
+			let mut gas_meter = GasMeter::<Test>::new(GAS_LIMIT);
+			set_balance(&ALICE, min_balance * 10);
+			place_contract(&BOB, code_hash);
+			let mut storage_meter = storage::meter::Meter::new(&ALICE, Some(0), 0).unwrap();
+			assert_err!(
+				MockStack::run_call(
+					ALICE,
+					BOB,
+					&mut gas_meter,
+					&mut storage_meter,
+					&schedule,
+					0,
+					vec![],
+					Some(&mut debug_buffer),
+					Determinism::Deterministic,
+				)
+				.map_err(|e| e.error),
+				Error::<Test>::DebugBufferExhausted
+			);
+		});
 	}
 
 	#[test]

frame/contracts/src/lib.rs

@@ -142,6 +142,7 @@ type BalanceOf<T> =
 type CodeVec<T> = BoundedVec<u8, <T as Config>::MaxCodeLen>;
 type RelaxedCodeVec<T> = WeakBoundedVec<u8, <T as Config>::MaxCodeLen>;
 type AccountIdLookupOf<T> = <<T as frame_system::Config>::Lookup as StaticLookup>::Source;
+type DebugBufferVec<T> = BoundedVec<u8, <T as Config>::MaxDebugBufferLen>;
 
 /// Used as a sentinel value when reading and writing contract memory.
 ///
@@ -344,6 +345,10 @@ pub mod pallet {
 		/// Do **not** set to `true` on productions chains.
 		#[pallet::constant]
 		type UnsafeUnstableInterface: Get<bool>;
+
+		/// The maximum length of the debug buffer in bytes.
+		#[pallet::constant]
+		type MaxDebugBufferLen: Get<u32>;
 	}
 
 	#[pallet::hooks]
@@ -863,6 +868,9 @@ pub mod pallet {
 		CodeRejected,
 		/// An indetermistic code was used in a context where this is not permitted.
 		Indeterministic,
+		/// The debug buffer size used during contract execution exceeded the limit determined by
+		/// the `MaxDebugBufferLen` pallet config parameter.
+		DebugBufferExhausted,
 	}
 
 	/// A mapping from an original code hash to the original code, untouched by instrumentation.
@@ -961,7 +969,7 @@ where
 		debug: bool,
 		determinism: Determinism,
 	) -> ContractExecResult<BalanceOf<T>> {
-		let mut debug_message = if debug { Some(Vec::new()) } else { None };
+		let mut debug_message = if debug { Some(DebugBufferVec::<T>::default()) } else { None };
 		let output = Self::internal_call(
 			origin,
 			dest,
@@ -977,7 +985,7 @@ where
 			gas_consumed: output.gas_meter.gas_consumed(),
 			gas_required: output.gas_meter.gas_required(),
 			storage_deposit: output.storage_deposit,
-			debug_message: debug_message.unwrap_or_default(),
+			debug_message: debug_message.unwrap_or_default().to_vec(),
 		}
 	}
 
@@ -1003,7 +1011,7 @@ where
 		salt: Vec<u8>,
 		debug: bool,
 	) -> ContractInstantiateResult<T::AccountId, BalanceOf<T>> {
-		let mut debug_message = if debug { Some(Vec::new()) } else { None };
+		let mut debug_message = if debug { Some(DebugBufferVec::<T>::default()) } else { None };
 		let output = Self::internal_instantiate(
 			origin,
 			value,
@@ -1022,7 +1030,7 @@ where
 			gas_consumed: output.gas_meter.gas_consumed(),
 			gas_required: output.gas_meter.gas_required(),
 			storage_deposit: output.storage_deposit,
-			debug_message: debug_message.unwrap_or_default(),
+			debug_message: debug_message.unwrap_or_default().to_vec(),
 		}
 	}
 
@@ -1113,7 +1121,7 @@ where
 		gas_limit: Weight,
 		storage_deposit_limit: Option<BalanceOf<T>>,
 		data: Vec<u8>,
-		debug_message: Option<&mut Vec<u8>>,
+		debug_message: Option<&mut DebugBufferVec<T>>,
 		determinism: Determinism,
 	) -> InternalCallOutput<T> {
 		let mut gas_meter = GasMeter::new(gas_limit);
@@ -1156,7 +1164,7 @@ where
 		code: Code<CodeHash<T>>,
 		data: Vec<u8>,
 		salt: Vec<u8>,
-		mut debug_message: Option<&mut Vec<u8>>,
+		mut debug_message: Option<&mut DebugBufferVec<T>>,
 	) -> InternalInstantiateOutput<T> {
 		let mut storage_deposit = Default::default();
 		let mut gas_meter = GasMeter::new(gas_limit);
@@ -1172,7 +1180,7 @@ where
 						TryInstantiate::Skip,
 					)
 					.map_err(|(err, msg)| {
-						debug_message.as_mut().map(|buffer| buffer.extend(msg.as_bytes()));
+						debug_message.as_mut().map(|buffer| buffer.try_extend(&mut msg.bytes()));
 						err
 					})?;
 					// The open deposit will be charged during execution when the

frame/contracts/src/tests.rs

@@ -415,6 +415,7 @@ impl Config for Test {
 	type MaxCodeLen = ConstU32<{ 128 * 1024 }>;
 	type MaxStorageKeyLen = ConstU32<128>;
 	type UnsafeUnstableInterface = UnstableInterface;
+	type MaxDebugBufferLen = ConstU32<{ 2 * 1024 * 1024 }>;
 }
 
 pub const ALICE: AccountId32 = AccountId32::new([1u8; 32]);

frame/contracts/src/wasm/mod.rs

@@ -594,9 +594,9 @@ mod tests {
 		fn gas_meter(&mut self) -> &mut GasMeter<Self::T> {
 			&mut self.gas_meter
 		}
-		fn append_debug_buffer(&mut self, msg: &str) -> bool {
+		fn append_debug_buffer(&mut self, msg: &str) -> Result<bool, DispatchError> {
 			self.debug_buffer.extend(msg.as_bytes());
-			true
+			Ok(true)
 		}
 		fn call_runtime(
 			&self,

frame/contracts/src/wasm/runtime.rs

@@ -2395,11 +2395,11 @@ pub mod env {
 		str_len: u32,
 	) -> Result<ReturnCode, TrapReason> {
 		ctx.charge_gas(RuntimeCosts::DebugMessage)?;
-		if ctx.ext.append_debug_buffer("") {
+		if ctx.ext.append_debug_buffer("")? {
 			let data = ctx.read_sandbox_memory(memory, str_ptr, str_len)?;
 			let msg =
 				core::str::from_utf8(&data).map_err(|_| <Error<E::T>>::DebugMessageInvalidUTF8)?;
-			ctx.ext.append_debug_buffer(msg);
+			ctx.ext.append_debug_buffer(msg)?;
 			return Ok(ReturnCode::Success)
 		}
 		Ok(ReturnCode::LoggingDisabled)