From 709bc4a8ef75d85d79654ee0c8e51798ef2271d7 Mon Sep 17 00:00:00 2001 From: Joaquin Gonzalez Date: Mon, 13 May 2024 15:54:56 -0300 Subject: [PATCH 01/14] chore(interpreter): optimisation for BYTE, SHL, SHR and SAR --- crates/interpreter/src/lib.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/interpreter/src/lib.rs b/crates/interpreter/src/lib.rs index 01db032788..cf94fdc4b4 100644 --- a/crates/interpreter/src/lib.rs +++ b/crates/interpreter/src/lib.rs @@ -44,3 +44,7 @@ pub use primitives::{MAX_CODE_SIZE, MAX_INITCODE_SIZE}; #[doc(hidden)] pub use revm_primitives as primitives; + +pub fn make_me_a_table() -> opcode::InstructionTable { + core::hint::black_box(opcode::make_instruction_table::<_, primitives::CancunSpec>()) +} From 74301675763473b450e92bb7deaa8260727397e2 Mon Sep 17 00:00:00 2001 From: Joaquin Gonzalez Date: Fri, 31 May 2024 12:35:03 -0300 Subject: [PATCH 02/14] test: add RETURNDATACOPY test and update RETURNDATALOAD --- crates/interpreter/src/instructions/system.rs | 58 ++++++++++++++++++- 1 file changed, 55 insertions(+), 3 deletions(-) diff --git a/crates/interpreter/src/instructions/system.rs b/crates/interpreter/src/instructions/system.rs index b00a82f617..ef19f393be 100644 --- a/crates/interpreter/src/instructions/system.rs +++ b/crates/interpreter/src/instructions/system.rs @@ -174,7 +174,7 @@ pub fn gas(interpreter: &mut Interpreter, _host: &mut H) { mod test { use super::*; use crate::{ - opcode::{make_instruction_table, RETURNDATALOAD}, + opcode::{make_instruction_table, RETURNDATACOPY, RETURNDATALOAD}, primitives::{bytes, Bytecode, PragueSpec}, DummyHost, Gas, }; @@ -210,8 +210,60 @@ mod test { ); let _ = interp.stack.pop(); - let _ = interp.stack.push(U256::from(2)); + let _ = interp.stack.push(U256::from(32)); interp.step(&table, &mut host); - assert_eq!(interp.instruction_result, InstructionResult::OutOfOffset); + assert_eq!(interp.instruction_result, InstructionResult::Continue); + assert_eq!( + interp.stack.data(), + &vec![U256::from_limbs([0x00, 0x00, 0x00, 0x00])] + ); + } + + #[test] + fn test_returdatalcopy() { + let table = make_instruction_table::<_, PragueSpec>(); + let mut host = DummyHost::default(); + + let mut interp = Interpreter::new_bytecode(Bytecode::LegacyRaw( + [RETURNDATACOPY, RETURNDATACOPY, RETURNDATACOPY].into(), + )); + interp.is_eof = true; + interp.gas = Gas::new(10000); + + interp.return_data_buffer = + bytes!("000000000000000400000000000000030000000000000002000000000000000100"); + + interp.shared_memory.resize(256); + + // Copying within bounds + interp.stack.push(U256::from(0)).unwrap(); + interp.stack.push(U256::from(0)).unwrap(); + interp.stack.push(U256::from(32)).unwrap(); + interp.step(&table, &mut host); + assert_eq!(interp.instruction_result, InstructionResult::Continue); + assert_eq!( + interp.shared_memory.slice(0, 32), + &interp.return_data_buffer[0..32] + ); + + // Copying with partial out-of-bounds (should zero pad) + interp.stack.push(U256::from(64)).unwrap(); + interp.stack.push(U256::from(16)).unwrap(); + interp.stack.push(U256::from(64)).unwrap(); + interp.step(&table, &mut host); + assert_eq!(interp.instruction_result, InstructionResult::Continue); + assert_eq!( + interp.shared_memory.slice(64, 16), + &interp.return_data_buffer[16..32] + ); + assert_eq!(&interp.shared_memory.slice(80, 48), &[0u8; 48]); + + // Completely out-of-bounds (should be all zeros) + interp.stack.push(U256::from(128)).unwrap(); + interp.stack.push(U256::from(96)).unwrap(); + interp.stack.push(U256::from(32)).unwrap(); + interp.step(&table, &mut host); + assert_eq!(interp.instruction_result, InstructionResult::Continue); + assert_eq!(&interp.shared_memory.slice(128, 32), &[0u8; 32]); } } From 9051655de1e5afbdf9602d1248fc4dc04da5ad0f Mon Sep 17 00:00:00 2001 From: Joaquin Gonzalez Date: Fri, 31 May 2024 13:03:21 -0300 Subject: [PATCH 03/14] feat: change oob behavior of returndataload --- crates/interpreter/src/instructions/system.rs | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/crates/interpreter/src/instructions/system.rs b/crates/interpreter/src/instructions/system.rs index ef19f393be..37deea0015 100644 --- a/crates/interpreter/src/instructions/system.rs +++ b/crates/interpreter/src/instructions/system.rs @@ -149,20 +149,20 @@ pub fn returndataload(interpreter: &mut Interpreter, _host: &m gas!(interpreter, gas::VERYLOW); pop_top!(interpreter, offset); let offset_usize = as_usize_or_fail!(interpreter, offset); - // TODO EOF needs to be padded with zeros, it is not correct to fail. - /* - if offset + 32 > len(returndata buffer) the result is zero-padded - (same behavior as CALLDATALOAD).see matching behavior of RETURNDATACOPY - in Modified Behavior section. - */ - if offset_usize.saturating_add(32) > interpreter.return_data_buffer.len() { - // TODO(EOF) proper error. - interpreter.instruction_result = InstructionResult::OutOfOffset; - return; - } - *offset = - B256::from_slice(&interpreter.return_data_buffer[offset_usize..offset_usize + 32]).into(); + let data = if offset_usize < interpreter.return_data_buffer.len() { + let available = interpreter.return_data_buffer.len() - offset_usize; + let mut padded = [0u8; 32]; + let copy_len = available.min(32); + padded[..copy_len].copy_from_slice( + &interpreter.return_data_buffer[offset_usize..offset_usize + copy_len], + ); + padded + } else { + [0u8; 32] + }; + + *offset = B256::from_slice(&data).into(); } pub fn gas(interpreter: &mut Interpreter, _host: &mut H) { From 74d9e8e02d9436e805f404311a8a85be58bf3995 Mon Sep 17 00:00:00 2001 From: Joaquin Gonzalez Date: Fri, 31 May 2024 14:05:24 -0300 Subject: [PATCH 04/14] fix: stack order and name on returndatacopy test --- crates/interpreter/src/instructions/system.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/interpreter/src/instructions/system.rs b/crates/interpreter/src/instructions/system.rs index 37deea0015..eb1888dea2 100644 --- a/crates/interpreter/src/instructions/system.rs +++ b/crates/interpreter/src/instructions/system.rs @@ -220,7 +220,7 @@ mod test { } #[test] - fn test_returdatalcopy() { + fn returndatacopy() { let table = make_instruction_table::<_, PragueSpec>(); let mut host = DummyHost::default(); @@ -236,9 +236,9 @@ mod test { interp.shared_memory.resize(256); // Copying within bounds + interp.stack.push(U256::from(32)).unwrap(); interp.stack.push(U256::from(0)).unwrap(); interp.stack.push(U256::from(0)).unwrap(); - interp.stack.push(U256::from(32)).unwrap(); interp.step(&table, &mut host); assert_eq!(interp.instruction_result, InstructionResult::Continue); assert_eq!( @@ -259,9 +259,9 @@ mod test { assert_eq!(&interp.shared_memory.slice(80, 48), &[0u8; 48]); // Completely out-of-bounds (should be all zeros) - interp.stack.push(U256::from(128)).unwrap(); - interp.stack.push(U256::from(96)).unwrap(); interp.stack.push(U256::from(32)).unwrap(); + interp.stack.push(U256::from(96)).unwrap(); + interp.stack.push(U256::from(128)).unwrap(); interp.step(&table, &mut host); assert_eq!(interp.instruction_result, InstructionResult::Continue); assert_eq!(&interp.shared_memory.slice(128, 32), &[0u8; 32]); From b302cdc46c515d699d525d8cbe2b4bf3954d7baa Mon Sep 17 00:00:00 2001 From: Joaquin Gonzalez Date: Fri, 31 May 2024 14:36:39 -0300 Subject: [PATCH 05/14] feat: change oob behavior of RETURNDATACOPY --- crates/interpreter/src/instructions/system.rs | 43 +++++++++++++------ 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/crates/interpreter/src/instructions/system.rs b/crates/interpreter/src/instructions/system.rs index eb1888dea2..5243216bd8 100644 --- a/crates/interpreter/src/instructions/system.rs +++ b/crates/interpreter/src/instructions/system.rs @@ -1,7 +1,7 @@ use crate::{ gas, primitives::{Spec, B256, KECCAK_EMPTY, U256}, - Host, InstructionResult, Interpreter, + Host, Interpreter, }; use core::ptr; @@ -123,23 +123,42 @@ pub fn returndatasize(interpreter: &mut Interprete /// EIP-211: New opcodes: RETURNDATASIZE and RETURNDATACOPY pub fn returndatacopy(interpreter: &mut Interpreter, _host: &mut H) { - check!(interpreter, BYZANTIUM); + check!(interpreter, PRAGUE); pop!(interpreter, memory_offset, offset, len); + let len = as_usize_or_fail!(interpreter, len); - gas_or_fail!(interpreter, gas::verylowcopy_cost(len as u64)); - let data_offset = as_usize_saturated!(offset); - let data_end = data_offset.saturating_add(len); - if data_end > interpreter.return_data_buffer.len() { - interpreter.instruction_result = InstructionResult::OutOfOffset; + if len == 0 { return; } - if len != 0 { - let memory_offset = as_usize_or_fail!(interpreter, memory_offset); - resize_memory!(interpreter, memory_offset, len); + + gas_or_fail!(interpreter, gas::verylowcopy_cost(len as u64)); + + let data_offset = as_usize_saturated!(offset); + let memory_offset = as_usize_or_fail!(interpreter, memory_offset); + + resize_memory!(interpreter, memory_offset, len); + + let return_data_buffer_len = interpreter.return_data_buffer.len(); + if data_offset < return_data_buffer_len { + let available_len = return_data_buffer_len - data_offset; + let copy_len = available_len.min(len); + interpreter.shared_memory.set( memory_offset, - &interpreter.return_data_buffer[data_offset..data_end], + &interpreter.return_data_buffer[data_offset..data_offset + copy_len], ); + + if copy_len < len { + interpreter + .shared_memory + .slice_mut(memory_offset + copy_len, len - copy_len) + .fill(0); + } + } else { + interpreter + .shared_memory + .slice_mut(memory_offset, len) + .fill(0); } } @@ -176,7 +195,7 @@ mod test { use crate::{ opcode::{make_instruction_table, RETURNDATACOPY, RETURNDATALOAD}, primitives::{bytes, Bytecode, PragueSpec}, - DummyHost, Gas, + DummyHost, Gas, InstructionResult, }; #[test] From d55c79633bbe11ad32ec5112769734fa6ad3c134 Mon Sep 17 00:00:00 2001 From: Joaquin Gonzalez Date: Fri, 31 May 2024 14:38:30 -0300 Subject: [PATCH 06/14] chore: remove unused function make_me_a_table --- crates/interpreter/src/lib.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/crates/interpreter/src/lib.rs b/crates/interpreter/src/lib.rs index cf94fdc4b4..01db032788 100644 --- a/crates/interpreter/src/lib.rs +++ b/crates/interpreter/src/lib.rs @@ -44,7 +44,3 @@ pub use primitives::{MAX_CODE_SIZE, MAX_INITCODE_SIZE}; #[doc(hidden)] pub use revm_primitives as primitives; - -pub fn make_me_a_table() -> opcode::InstructionTable { - core::hint::black_box(opcode::make_instruction_table::<_, primitives::CancunSpec>()) -} From 98882f7d9580532768976dfff07f350eeaa5d2d4 Mon Sep 17 00:00:00 2001 From: Joaquin Gonzalez Date: Fri, 31 May 2024 14:56:52 -0300 Subject: [PATCH 07/14] fix: revert interpreter check --- crates/interpreter/src/instructions/system.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/interpreter/src/instructions/system.rs b/crates/interpreter/src/instructions/system.rs index 5243216bd8..e26ec07919 100644 --- a/crates/interpreter/src/instructions/system.rs +++ b/crates/interpreter/src/instructions/system.rs @@ -123,7 +123,7 @@ pub fn returndatasize(interpreter: &mut Interprete /// EIP-211: New opcodes: RETURNDATASIZE and RETURNDATACOPY pub fn returndatacopy(interpreter: &mut Interpreter, _host: &mut H) { - check!(interpreter, PRAGUE); + check!(interpreter, BYZANTIUM); pop!(interpreter, memory_offset, offset, len); let len = as_usize_or_fail!(interpreter, len); From 4fc13da29d73e7fe660b705ce12e2fc5cbea79c5 Mon Sep 17 00:00:00 2001 From: Joaquin Gonzalez Date: Sun, 2 Jun 2024 16:59:30 -0300 Subject: [PATCH 08/14] chore: some tests --- crates/interpreter/src/instructions/system.rs | 89 +++++++++++++++++-- 1 file changed, 83 insertions(+), 6 deletions(-) diff --git a/crates/interpreter/src/instructions/system.rs b/crates/interpreter/src/instructions/system.rs index e26ec07919..0e8e847ddf 100644 --- a/crates/interpreter/src/instructions/system.rs +++ b/crates/interpreter/src/instructions/system.rs @@ -126,7 +126,7 @@ pub fn returndatacopy(interpreter: &mut Interprete check!(interpreter, BYZANTIUM); pop!(interpreter, memory_offset, offset, len); - let len = as_usize_or_fail!(interpreter, len); + let len = as_usize_saturated!(len); if len == 0 { return; } @@ -134,7 +134,7 @@ pub fn returndatacopy(interpreter: &mut Interprete gas_or_fail!(interpreter, gas::verylowcopy_cost(len as u64)); let data_offset = as_usize_saturated!(offset); - let memory_offset = as_usize_or_fail!(interpreter, memory_offset); + let memory_offset = as_usize_saturated!(memory_offset); resize_memory!(interpreter, memory_offset, len); @@ -167,7 +167,7 @@ pub fn returndataload(interpreter: &mut Interpreter, _host: &m require_eof!(interpreter); gas!(interpreter, gas::VERYLOW); pop_top!(interpreter, offset); - let offset_usize = as_usize_or_fail!(interpreter, offset); + let offset_usize = as_usize_saturated!(offset); let data = if offset_usize < interpreter.return_data_buffer.len() { let available = interpreter.return_data_buffer.len() - offset_usize; @@ -204,7 +204,15 @@ mod test { let mut host = DummyHost::default(); let mut interp = Interpreter::new_bytecode(Bytecode::LegacyRaw( - [RETURNDATALOAD, RETURNDATALOAD, RETURNDATALOAD].into(), + [ + RETURNDATALOAD, + RETURNDATALOAD, + RETURNDATALOAD, + RETURNDATALOAD, + RETURNDATALOAD, + RETURNDATALOAD, + ] + .into(), )); interp.is_eof = true; interp.gas = Gas::new(10000); @@ -236,6 +244,35 @@ mod test { interp.stack.data(), &vec![U256::from_limbs([0x00, 0x00, 0x00, 0x00])] ); + + // Large offset + let _ = interp.stack.pop(); + interp.stack.push(U256::MAX).unwrap(); + interp.step(&table, &mut host); + assert_eq!(interp.instruction_result, InstructionResult::Continue); + assert_eq!( + interp.stack.data(), + &vec![U256::from_limbs([0x00, 0x00, 0x00, 0x00])] + ); + + // Offset right at the boundary of the return data buffer size + let _ = interp.stack.pop(); + let _ = interp + .stack + .push(U256::from(interp.return_data_buffer.len())); + interp.step(&table, &mut host); + assert_eq!(interp.instruction_result, InstructionResult::Continue); + assert_eq!( + interp.stack.data(), + &vec![U256::from_limbs([0x00, 0x00, 0x00, 0x00])] + ); + + // Large length + let _ = interp.stack.pop(); + let _ = interp.stack.push(U256::from(0)); + interp.stack.push(U256::MAX).unwrap(); + interp.step(&table, &mut host); + assert_eq!(interp.instruction_result, InstructionResult::Continue); } #[test] @@ -244,14 +281,21 @@ mod test { let mut host = DummyHost::default(); let mut interp = Interpreter::new_bytecode(Bytecode::LegacyRaw( - [RETURNDATACOPY, RETURNDATACOPY, RETURNDATACOPY].into(), + [ + RETURNDATACOPY, + RETURNDATACOPY, + RETURNDATACOPY, + RETURNDATACOPY, + RETURNDATACOPY, + RETURNDATACOPY, + ] + .into(), )); interp.is_eof = true; interp.gas = Gas::new(10000); interp.return_data_buffer = bytes!("000000000000000400000000000000030000000000000002000000000000000100"); - interp.shared_memory.resize(256); // Copying within bounds @@ -284,5 +328,38 @@ mod test { interp.step(&table, &mut host); assert_eq!(interp.instruction_result, InstructionResult::Continue); assert_eq!(&interp.shared_memory.slice(128, 32), &[0u8; 32]); + + // Large offset + interp.stack.push(U256::from(32)).unwrap(); + interp.stack.push(U256::MAX).unwrap(); + interp.stack.push(U256::from(0)).unwrap(); + interp.step(&table, &mut host); + assert_eq!(interp.instruction_result, InstructionResult::Continue); + assert_eq!(&interp.shared_memory.slice(0, 32), &[0u8; 32]); + + // Offset just before the boundary of the return data buffer size + interp.stack.push(U256::from(32)).unwrap(); + interp + .stack + .push(U256::from(interp.return_data_buffer.len() - 32)) + .unwrap(); + interp.stack.push(U256::from(0)).unwrap(); + interp.step(&table, &mut host); + assert_eq!(interp.instruction_result, InstructionResult::Continue); + assert_eq!( + interp.shared_memory.slice(0, 32), + &interp.return_data_buffer[interp.return_data_buffer.len() - 32..] + ); + + // Offset right at the boundary of the return data buffer size + interp.stack.push(U256::from(32)).unwrap(); + interp + .stack + .push(U256::from(interp.return_data_buffer.len())) + .unwrap(); + interp.stack.push(U256::from(0)).unwrap(); + interp.step(&table, &mut host); + assert_eq!(interp.instruction_result, InstructionResult::Continue); + assert_eq!(&interp.shared_memory.slice(0, 32), &[0u8; 32]); } } From f1e29877064a1bbffbcf5d9f250eebfe5afbae30 Mon Sep 17 00:00:00 2001 From: Joaquin Gonzalez Date: Sun, 2 Jun 2024 18:37:46 -0300 Subject: [PATCH 09/14] chore: use as_usize_or_fail! macro instead of as_usize_saturated! --- crates/interpreter/src/instructions/system.rs | 28 +++---------------- 1 file changed, 4 insertions(+), 24 deletions(-) diff --git a/crates/interpreter/src/instructions/system.rs b/crates/interpreter/src/instructions/system.rs index 0e8e847ddf..5de6b944a3 100644 --- a/crates/interpreter/src/instructions/system.rs +++ b/crates/interpreter/src/instructions/system.rs @@ -126,15 +126,14 @@ pub fn returndatacopy(interpreter: &mut Interprete check!(interpreter, BYZANTIUM); pop!(interpreter, memory_offset, offset, len); - let len = as_usize_saturated!(len); + let len = as_usize_or_fail!(interpreter, len); + gas_or_fail!(interpreter, gas::verylowcopy_cost(len as u64)); if len == 0 { return; } - gas_or_fail!(interpreter, gas::verylowcopy_cost(len as u64)); - let data_offset = as_usize_saturated!(offset); - let memory_offset = as_usize_saturated!(memory_offset); + let memory_offset = as_usize_or_fail!(interpreter, memory_offset); resize_memory!(interpreter, memory_offset, len); @@ -167,7 +166,7 @@ pub fn returndataload(interpreter: &mut Interpreter, _host: &m require_eof!(interpreter); gas!(interpreter, gas::VERYLOW); pop_top!(interpreter, offset); - let offset_usize = as_usize_saturated!(offset); + let offset_usize = as_usize_or_fail!(interpreter, offset); let data = if offset_usize < interpreter.return_data_buffer.len() { let available = interpreter.return_data_buffer.len() - offset_usize; @@ -209,8 +208,6 @@ mod test { RETURNDATALOAD, RETURNDATALOAD, RETURNDATALOAD, - RETURNDATALOAD, - RETURNDATALOAD, ] .into(), )); @@ -245,16 +242,6 @@ mod test { &vec![U256::from_limbs([0x00, 0x00, 0x00, 0x00])] ); - // Large offset - let _ = interp.stack.pop(); - interp.stack.push(U256::MAX).unwrap(); - interp.step(&table, &mut host); - assert_eq!(interp.instruction_result, InstructionResult::Continue); - assert_eq!( - interp.stack.data(), - &vec![U256::from_limbs([0x00, 0x00, 0x00, 0x00])] - ); - // Offset right at the boundary of the return data buffer size let _ = interp.stack.pop(); let _ = interp @@ -266,13 +253,6 @@ mod test { interp.stack.data(), &vec![U256::from_limbs([0x00, 0x00, 0x00, 0x00])] ); - - // Large length - let _ = interp.stack.pop(); - let _ = interp.stack.push(U256::from(0)); - interp.stack.push(U256::MAX).unwrap(); - interp.step(&table, &mut host); - assert_eq!(interp.instruction_result, InstructionResult::Continue); } #[test] From 85cc8aadf359199fa5a577b90b189a87ffa15ecd Mon Sep 17 00:00:00 2001 From: Joaquin Gonzalez Date: Mon, 3 Jun 2024 19:57:19 -0300 Subject: [PATCH 10/14] fix: add backwards compatibility --- crates/interpreter/src/instructions/system.rs | 31 +++++++++++++------ 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/crates/interpreter/src/instructions/system.rs b/crates/interpreter/src/instructions/system.rs index 5de6b944a3..15ea20e5d2 100644 --- a/crates/interpreter/src/instructions/system.rs +++ b/crates/interpreter/src/instructions/system.rs @@ -1,7 +1,7 @@ use crate::{ gas, primitives::{Spec, B256, KECCAK_EMPTY, U256}, - Host, Interpreter, + Host, InstructionResult, Interpreter, }; use core::ptr; @@ -128,16 +128,23 @@ pub fn returndatacopy(interpreter: &mut Interprete let len = as_usize_or_fail!(interpreter, len); gas_or_fail!(interpreter, gas::verylowcopy_cost(len as u64)); - if len == 0 { - return; - } let data_offset = as_usize_saturated!(offset); let memory_offset = as_usize_or_fail!(interpreter, memory_offset); - resize_memory!(interpreter, memory_offset, len); let return_data_buffer_len = interpreter.return_data_buffer.len(); + let data_end = data_offset.saturating_add(len); + + if data_end > return_data_buffer_len && !interpreter.is_eof { + interpreter.instruction_result = InstructionResult::OutOfOffset; + return; + } + + if len == 0 { + return; + } + if data_offset < return_data_buffer_len { let available_len = return_data_buffer_len - data_offset; let copy_len = available_len.min(len); @@ -147,17 +154,21 @@ pub fn returndatacopy(interpreter: &mut Interprete &interpreter.return_data_buffer[data_offset..data_offset + copy_len], ); - if copy_len < len { + if interpreter.is_eof && copy_len < len { interpreter .shared_memory .slice_mut(memory_offset + copy_len, len - copy_len) .fill(0); } } else { - interpreter - .shared_memory - .slice_mut(memory_offset, len) - .fill(0); + if interpreter.is_eof { + interpreter + .shared_memory + .slice_mut(memory_offset, len) + .fill(0); + } else { + interpreter.instruction_result = InstructionResult::OutOfOffset; + } } } From 93c7b9430ec5059a01997166dec822e24a7cd9eb Mon Sep 17 00:00:00 2001 From: Joaquin Gonzalez Date: Mon, 3 Jun 2024 20:33:26 -0300 Subject: [PATCH 11/14] fix: collapsible-else-if --- crates/interpreter/src/instructions/system.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/crates/interpreter/src/instructions/system.rs b/crates/interpreter/src/instructions/system.rs index 15ea20e5d2..979e77272f 100644 --- a/crates/interpreter/src/instructions/system.rs +++ b/crates/interpreter/src/instructions/system.rs @@ -160,15 +160,13 @@ pub fn returndatacopy(interpreter: &mut Interprete .slice_mut(memory_offset + copy_len, len - copy_len) .fill(0); } + } else if interpreter.is_eof { + interpreter + .shared_memory + .slice_mut(memory_offset, len) + .fill(0); } else { - if interpreter.is_eof { - interpreter - .shared_memory - .slice_mut(memory_offset, len) - .fill(0); - } else { - interpreter.instruction_result = InstructionResult::OutOfOffset; - } + interpreter.instruction_result = InstructionResult::OutOfOffset; } } From 3d0d5d070b72d8b1025605697bd5b1d4b7dee073 Mon Sep 17 00:00:00 2001 From: Joaquin Gonzalez Date: Mon, 3 Jun 2024 23:03:57 -0300 Subject: [PATCH 12/14] self review --- crates/interpreter/src/instructions/system.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/crates/interpreter/src/instructions/system.rs b/crates/interpreter/src/instructions/system.rs index 979e77272f..a799529904 100644 --- a/crates/interpreter/src/instructions/system.rs +++ b/crates/interpreter/src/instructions/system.rs @@ -130,13 +130,9 @@ pub fn returndatacopy(interpreter: &mut Interprete gas_or_fail!(interpreter, gas::verylowcopy_cost(len as u64)); let data_offset = as_usize_saturated!(offset); - let memory_offset = as_usize_or_fail!(interpreter, memory_offset); - resize_memory!(interpreter, memory_offset, len); - let return_data_buffer_len = interpreter.return_data_buffer.len(); - let data_end = data_offset.saturating_add(len); - if data_end > return_data_buffer_len && !interpreter.is_eof { + if data_offset.saturating_add(len) > return_data_buffer_len && !interpreter.is_eof { interpreter.instruction_result = InstructionResult::OutOfOffset; return; } @@ -145,6 +141,8 @@ pub fn returndatacopy(interpreter: &mut Interprete return; } + let memory_offset = as_usize_or_fail!(interpreter, memory_offset); + resize_memory!(interpreter, memory_offset, len); if data_offset < return_data_buffer_len { let available_len = return_data_buffer_len - data_offset; let copy_len = available_len.min(len); From cc93fe66ab9459bcae51e62a8b7edfe58234d50e Mon Sep 17 00:00:00 2001 From: rakita Date: Sat, 8 Jun 2024 19:00:20 +0200 Subject: [PATCH 13/14] refactor code. use set_data in returndatacopy --- crates/interpreter/src/instructions/system.rs | 54 ++++++++----------- .../src/interpreter/shared_memory.rs | 2 +- 2 files changed, 23 insertions(+), 33 deletions(-) diff --git a/crates/interpreter/src/instructions/system.rs b/crates/interpreter/src/instructions/system.rs index a799529904..26395bfb9c 100644 --- a/crates/interpreter/src/instructions/system.rs +++ b/crates/interpreter/src/instructions/system.rs @@ -130,42 +130,32 @@ pub fn returndatacopy(interpreter: &mut Interprete gas_or_fail!(interpreter, gas::verylowcopy_cost(len as u64)); let data_offset = as_usize_saturated!(offset); + let data_end = data_offset.saturating_add(len); let return_data_buffer_len = interpreter.return_data_buffer.len(); - if data_offset.saturating_add(len) > return_data_buffer_len && !interpreter.is_eof { + // Old legacy behavior is to panic if data_end is out of scope of return buffer. + // This behavior is changed in EOF. + if data_end > return_data_buffer_len && !interpreter.is_eof { interpreter.instruction_result = InstructionResult::OutOfOffset; return; } + // if len is zero memory is not resized. if len == 0 { return; } + // resize memory let memory_offset = as_usize_or_fail!(interpreter, memory_offset); resize_memory!(interpreter, memory_offset, len); - if data_offset < return_data_buffer_len { - let available_len = return_data_buffer_len - data_offset; - let copy_len = available_len.min(len); - interpreter.shared_memory.set( - memory_offset, - &interpreter.return_data_buffer[data_offset..data_offset + copy_len], - ); - - if interpreter.is_eof && copy_len < len { - interpreter - .shared_memory - .slice_mut(memory_offset + copy_len, len - copy_len) - .fill(0); - } - } else if interpreter.is_eof { - interpreter - .shared_memory - .slice_mut(memory_offset, len) - .fill(0); - } else { - interpreter.instruction_result = InstructionResult::OutOfOffset; - } + // Note: this can't panic because we resized memory to fit. + interpreter.shared_memory.set_data( + memory_offset, + data_offset, + len, + &interpreter.return_data_buffer, + ); } /// Part of EOF ``. @@ -175,19 +165,19 @@ pub fn returndataload(interpreter: &mut Interpreter, _host: &m pop_top!(interpreter, offset); let offset_usize = as_usize_or_fail!(interpreter, offset); - let data = if offset_usize < interpreter.return_data_buffer.len() { - let available = interpreter.return_data_buffer.len() - offset_usize; - let mut padded = [0u8; 32]; + let mut output = [0u8; 32]; + if let Some(available) = interpreter + .return_data_buffer + .len() + .checked_sub(offset_usize) + { let copy_len = available.min(32); - padded[..copy_len].copy_from_slice( + output[..copy_len].copy_from_slice( &interpreter.return_data_buffer[offset_usize..offset_usize + copy_len], ); - padded - } else { - [0u8; 32] - }; + } - *offset = B256::from_slice(&data).into(); + *offset = B256::from(output).into(); } pub fn gas(interpreter: &mut Interpreter, _host: &mut H) { diff --git a/crates/interpreter/src/interpreter/shared_memory.rs b/crates/interpreter/src/interpreter/shared_memory.rs index cc63e1bf07..601379b553 100644 --- a/crates/interpreter/src/interpreter/shared_memory.rs +++ b/crates/interpreter/src/interpreter/shared_memory.rs @@ -256,7 +256,7 @@ impl SharedMemory { /// /// # Panics /// - /// Panics on out of bounds. + /// Panics if memory is out of bounds. #[inline] #[cfg_attr(debug_assertions, track_caller)] pub fn set_data(&mut self, memory_offset: usize, data_offset: usize, len: usize, data: &[u8]) { From f87e596f2fbbf16993f34bb9f5bf7e592708d9c0 Mon Sep 17 00:00:00 2001 From: rakita Date: Sat, 8 Jun 2024 19:02:24 +0200 Subject: [PATCH 14/14] remove local as it is used once --- crates/interpreter/src/instructions/system.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/interpreter/src/instructions/system.rs b/crates/interpreter/src/instructions/system.rs index 26395bfb9c..513d334e12 100644 --- a/crates/interpreter/src/instructions/system.rs +++ b/crates/interpreter/src/instructions/system.rs @@ -131,11 +131,10 @@ pub fn returndatacopy(interpreter: &mut Interprete let data_offset = as_usize_saturated!(offset); let data_end = data_offset.saturating_add(len); - let return_data_buffer_len = interpreter.return_data_buffer.len(); // Old legacy behavior is to panic if data_end is out of scope of return buffer. // This behavior is changed in EOF. - if data_end > return_data_buffer_len && !interpreter.is_eof { + if data_end > interpreter.return_data_buffer.len() && !interpreter.is_eof { interpreter.instruction_result = InstructionResult::OutOfOffset; return; }