Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Commit

Permalink
[contracts] Add debug buffer limit + enforcement (#12845)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
agryaznov authored Dec 13, 2022
1 parent 13664c3 commit 93fa104
Show file tree
Hide file tree
Showing 6 changed files with 86 additions and 30 deletions.
1 change: 1 addition & 0 deletions bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
84 changes: 65 additions & 19 deletions frame/contracts/src/exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand All @@ -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]
Expand Down
22 changes: 15 additions & 7 deletions frame/contracts/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand All @@ -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(),
}
}

Expand All @@ -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,
Expand All @@ -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(),
}
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions frame/contracts/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Expand Down
4 changes: 2 additions & 2 deletions frame/contracts/src/wasm/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions frame/contracts/src/wasm/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 93fa104

Please sign in to comment.