From c837250eff25692c597e339a0ef58d39a5e60f84 Mon Sep 17 00:00:00 2001 From: Ivan Litteri <67517699+ilitteri@users.noreply.github.com> Date: Mon, 2 Dec 2024 21:25:07 -0300 Subject: [PATCH] fix(levm): next opcode handling (#1368) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes: - If there's no opcode present at the given PC (PC > bytecode length), then we stop the execution (as [it is done in the execution specs](https://github.com/ethereum/execution-specs/blob/master/src/ethereum/cancun/vm/interpreter.py#L293-L303)) - If at the address given by the JUMP instruction there's a valid opcode present then it's a valid jump otherwise it isn't. --------- Co-authored-by: Jeremías Salomón <48994069+JereSalo@users.noreply.github.com> --- crates/vm/levm/src/call_frame.rs | 41 +++++++++---------- crates/vm/levm/src/errors.rs | 2 + .../stack_memory_storage_flow.rs | 12 +++--- crates/vm/levm/src/opcodes.rs | 16 +++----- crates/vm/levm/src/operations.rs | 4 +- crates/vm/levm/src/vm.rs | 3 +- 6 files changed, 36 insertions(+), 42 deletions(-) diff --git a/crates/vm/levm/src/call_frame.rs b/crates/vm/levm/src/call_frame.rs index 0602e26da..d8bc21019 100644 --- a/crates/vm/levm/src/call_frame.rs +++ b/crates/vm/levm/src/call_frame.rs @@ -119,10 +119,11 @@ impl CallFrame { } } - pub fn next_opcode(&mut self) -> Result, VMError> { - let opcode = self.opcode_at(self.pc); - self.increment_pc()?; - Ok(opcode) + pub fn next_opcode(&mut self) -> Opcode { + match self.bytecode.get(self.pc).copied().map(Opcode::from) { + Some(opcode) => opcode, + None => Opcode::STOP, + } } pub fn increment_pc_by(&mut self, count: usize) -> Result<(), VMError> { @@ -142,29 +143,27 @@ impl CallFrame { } /// Jump to the given address, returns false if the jump position wasn't a JUMPDEST - pub fn jump(&mut self, jump_address: U256) -> Result { + pub fn jump(&mut self, jump_address: U256) -> Result<(), VMError> { let jump_address_usize = jump_address .try_into() .map_err(|_err| VMError::VeryLargeNumber)?; - if !self.valid_jump(jump_address_usize) { - return Ok(false); - } + self.validate_jump(jump_address_usize)?; self.pc = jump_address_usize; - Ok(true) - } - - fn valid_jump(&self, jump_address: usize) -> bool { - self.opcode_at(jump_address) - .map(|opcode| opcode.eq(&Opcode::JUMPDEST)) - .is_some_and(|is_jumpdest| is_jumpdest) + Ok(()) } - fn opcode_at(&self, offset: usize) -> Option { - self.bytecode - .get(offset) - .copied() - .map(Opcode::try_from)? - .ok() + fn validate_jump(&self, to_jump_address: usize) -> Result<(), VMError> { + if matches!( + self.bytecode + .get(to_jump_address) + .copied() + .map(Opcode::try_from), + Some(Ok(Opcode::JUMPDEST)) + ) { + Ok(()) + } else { + Err(VMError::InvalidJump) + } } } diff --git a/crates/vm/levm/src/errors.rs b/crates/vm/levm/src/errors.rs index 2f6a4300a..c75e1e746 100644 --- a/crates/vm/levm/src/errors.rs +++ b/crates/vm/levm/src/errors.rs @@ -174,6 +174,8 @@ pub enum InternalError { ExcessBlobGasShouldNotBeNone, #[error("Error in utils file")] UtilsError, + #[error("PC out of bounds")] + PCOutOfBounds, } #[derive(Debug, Clone)] diff --git a/crates/vm/levm/src/opcode_handlers/stack_memory_storage_flow.rs b/crates/vm/levm/src/opcode_handlers/stack_memory_storage_flow.rs index 11f968d34..c58e66dd2 100644 --- a/crates/vm/levm/src/opcode_handlers/stack_memory_storage_flow.rs +++ b/crates/vm/levm/src/opcode_handlers/stack_memory_storage_flow.rs @@ -289,9 +289,7 @@ impl VM { self.increase_consumed_gas(current_call_frame, gas_cost::JUMP)?; let jump_address = current_call_frame.stack.pop()?; - if !current_call_frame.jump(jump_address)? { - return Err(VMError::InvalidJump); - } + current_call_frame.jump(jump_address)?; Ok(OpcodeSuccess::Continue) } @@ -304,11 +302,11 @@ impl VM { let jump_address = current_call_frame.stack.pop()?; let condition = current_call_frame.stack.pop()?; - if condition != U256::zero() && !current_call_frame.jump(jump_address)? { - return Err(VMError::InvalidJump); - } - self.increase_consumed_gas(current_call_frame, gas_cost::JUMPI)?; + + if !condition.is_zero() { + current_call_frame.jump(jump_address)?; + } Ok(OpcodeSuccess::Continue) } diff --git a/crates/vm/levm/src/opcodes.rs b/crates/vm/levm/src/opcodes.rs index db6662e65..e919b82ba 100644 --- a/crates/vm/levm/src/opcodes.rs +++ b/crates/vm/levm/src/opcodes.rs @@ -1,5 +1,3 @@ -use crate::errors::VMError; - #[derive(Debug, PartialEq, Eq, Clone, PartialOrd)] pub enum Opcode { // Stop and Arithmetic Operations @@ -174,11 +172,9 @@ pub enum Opcode { impl Copy for Opcode {} -impl TryFrom for Opcode { - type Error = VMError; - - fn try_from(byte: u8) -> Result { - let op = match byte { +impl From for Opcode { + fn from(byte: u8) -> Self { + match byte { 0x00 => Opcode::STOP, 0x01 => Opcode::ADD, 0x16 => Opcode::AND, @@ -326,11 +322,9 @@ impl TryFrom for Opcode { 0xF4 => Opcode::DELEGATECALL, 0xFA => Opcode::STATICCALL, 0xFD => Opcode::REVERT, - 0xFE => Opcode::INVALID, 0xFF => Opcode::SELFDESTRUCT, - _ => return Err(VMError::InvalidOpcode), - }; - Ok(op) + _ => Opcode::INVALID, + } } } diff --git a/crates/vm/levm/src/operations.rs b/crates/vm/levm/src/operations.rs index 03e99f91a..3bd71dcdb 100644 --- a/crates/vm/levm/src/operations.rs +++ b/crates/vm/levm/src/operations.rs @@ -188,9 +188,9 @@ impl Operation { ) .ok_or(VMError::Internal(InternalError::SlicingError))?; assert_eq!(value_to_push.len(), n_usize); - let opcode = Opcode::try_from((u8::from(Opcode::PUSH0)).checked_add(*n).ok_or( + let opcode = Opcode::from((u8::from(Opcode::PUSH0)).checked_add(*n).ok_or( VMError::Internal(InternalError::ArithmeticOperationOverflow), - )?)?; + )?); let mut bytes = vec![u8::from(opcode)]; bytes.extend_from_slice(value_to_push); diff --git a/crates/vm/levm/src/vm.rs b/crates/vm/levm/src/vm.rs index a16073a0c..96e84b771 100644 --- a/crates/vm/levm/src/vm.rs +++ b/crates/vm/levm/src/vm.rs @@ -180,7 +180,8 @@ impl VM { ); loop { - let opcode = current_call_frame.next_opcode()?.unwrap_or(Opcode::STOP); // This will execute opcode stop if there are no more opcodes, there are other ways of solving this but this is the simplest and doesn't change VM behavior. + let opcode = current_call_frame.next_opcode(); + current_call_frame.increment_pc()?; let op_result: Result = match opcode { Opcode::STOP => Ok(OpcodeSuccess::Result(ResultReason::Stop)),