From 929e6170af79c9aaeb145784cabd2497d2e1e5dc Mon Sep 17 00:00:00 2001 From: Alexander Gryaznov Date: Fri, 16 Dec 2022 15:14:11 +0200 Subject: [PATCH 1/4] make debug buffer work like a FIFO pipe --- frame/contracts/src/exec.rs | 61 ++++++++++++++--------------- frame/contracts/src/wasm/mod.rs | 4 +- frame/contracts/src/wasm/runtime.rs | 4 +- 3 files changed, 33 insertions(+), 36 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 945095dc20329..2d871ae30f8c5 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -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) -> Result; + fn append_debug_buffer(&mut self, msg: &str) -> bool; /// Call some dispatchable and return the result. fn call_runtime(&self, call: ::RuntimeCall) -> DispatchResultWithPostInfo; @@ -1328,16 +1328,19 @@ where &mut self.top_frame_mut().nested_gas } - fn append_debug_buffer(&mut self, msg: &str) -> Result { + fn append_debug_buffer(&mut self, msg: &str) -> bool { if let Some(buffer) = &mut self.debug_message { if !msg.is_empty() { + if buffer.len() + msg.len() > DebugBufferVec::::bound() { + buffer.drain(0..msg.len()); + } buffer .try_extend(&mut msg.bytes()) - .map_err(|_| Error::::DebugBufferExhausted)?; + .expect("Debug buffer has enough space for the message, it's been truncated if needed; qed"); } - Ok(true) + true } else { - Ok(false) + false } } @@ -2505,12 +2508,8 @@ mod tests { #[test] fn printing_works() { let code_hash = MockLoader::insert(Call, |ctx, _| { - 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!"); + ctx.ext.append_debug_buffer("This is a test"); + ctx.ext.append_debug_buffer("More text"); exec_success() }); @@ -2543,12 +2542,8 @@ mod tests { #[test] fn printing_works_on_fail() { let code_hash = MockLoader::insert(Call, |ctx, _| { - 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!"); + ctx.ext.append_debug_buffer("This is a test"); + ctx.ext.append_debug_buffer("More text"); exec_trapped() }); @@ -2581,7 +2576,7 @@ mod tests { #[test] fn debug_buffer_is_limited() { let code_hash = MockLoader::insert(Call, move |ctx, _| { - ctx.ext.append_debug_buffer("overflowing bytes")?; + ctx.ext.append_debug_buffer("overflowing bytes"); exec_success() }); @@ -2596,20 +2591,22 @@ mod tests { 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::::DebugBufferExhausted + MockStack::run_call( + ALICE, + BOB, + &mut gas_meter, + &mut storage_meter, + &schedule, + 0, + vec![], + Some(&mut debug_buffer), + Determinism::Deterministic, + ) + .unwrap(); + assert_eq!( + &String::from_utf8(debug_buffer[DebugBufferVec::::bound() - 17..].to_vec()) + .unwrap(), + "overflowing bytes" ); }); } diff --git a/frame/contracts/src/wasm/mod.rs b/frame/contracts/src/wasm/mod.rs index d85dac95cc712..e9e6b42dc3f8a 100644 --- a/frame/contracts/src/wasm/mod.rs +++ b/frame/contracts/src/wasm/mod.rs @@ -594,9 +594,9 @@ mod tests { fn gas_meter(&mut self) -> &mut GasMeter { &mut self.gas_meter } - fn append_debug_buffer(&mut self, msg: &str) -> Result { + fn append_debug_buffer(&mut self, msg: &str) -> bool { self.debug_buffer.extend(msg.as_bytes()); - Ok(true) + true } fn call_runtime( &self, diff --git a/frame/contracts/src/wasm/runtime.rs b/frame/contracts/src/wasm/runtime.rs index 50ad9996e6eb6..b933688eb61ec 100644 --- a/frame/contracts/src/wasm/runtime.rs +++ b/frame/contracts/src/wasm/runtime.rs @@ -2395,11 +2395,11 @@ pub mod env { str_len: u32, ) -> Result { 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(|_| >::DebugMessageInvalidUTF8)?; - ctx.ext.append_debug_buffer(msg)?; + ctx.ext.append_debug_buffer(msg); return Ok(ReturnCode::Success) } Ok(ReturnCode::LoggingDisabled) From 0d2948b9f28ef4f99f3584ccc21ef87a895cceac Mon Sep 17 00:00:00 2001 From: Alexander Gryaznov Date: Fri, 16 Dec 2022 18:30:40 +0200 Subject: [PATCH 2/4] remove unused Error type --- frame/contracts/src/lib.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index b76acf9d1db08..01cd8125a3db2 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -868,9 +868,6 @@ 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. From f282c15afb48b5d920cc914d6ce7efcedd73243c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Thu, 22 Dec 2022 17:25:31 +0100 Subject: [PATCH 3/4] Remove panics --- frame/contracts/src/exec.rs | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 2d871ae30f8c5..1f954c4abb015 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -1330,14 +1330,29 @@ where fn append_debug_buffer(&mut self, msg: &str) -> bool { if let Some(buffer) = &mut self.debug_message { - if !msg.is_empty() { - if buffer.len() + msg.len() > DebugBufferVec::::bound() { - buffer.drain(0..msg.len()); - } - buffer - .try_extend(&mut msg.bytes()) - .expect("Debug buffer has enough space for the message, it's been truncated if needed; qed"); - } + let mut msg = msg.bytes(); + let num_drain = { + let capacity = DebugBufferVec::::bound().checked_sub(buffer.len()).expect( + " + `buffer` is of type `DebugBufferVec`, + `DebugBufferVec` is a `BoundedVec`, + `BoundedVec::bound()` <= `BoundedVec::len()`; + qed + ", + ); + msg.len().saturating_sub(capacity).min(buffer.len()) + }; + buffer.drain(0..num_drain); + buffer + .try_extend(&mut msg) + .map_err(|_| { + log::debug!( + target: "runtime::contracts", + "Debug message to big (size={}) for debug buffer (bound={})", + msg.len(), DebugBufferVec::::bound(), + ); + }) + .ok(); true } else { false From 1b9e901cb297a084a297e02a0e247bbb2bc42ed4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Tue, 27 Dec 2022 08:24:16 +0100 Subject: [PATCH 4/4] Update frame/contracts/src/exec.rs Co-authored-by: Sasha Gryaznov --- frame/contracts/src/exec.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index fec7d28537a8d..df1dd24856a8a 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -1342,7 +1342,7 @@ where " `buffer` is of type `DebugBufferVec`, `DebugBufferVec` is a `BoundedVec`, - `BoundedVec::bound()` <= `BoundedVec::len()`; + `BoundedVec::len()` <= `BoundedVec::bound()`; qed ", );