From 5effeafc602f36d31996487181a5173d3d87f540 Mon Sep 17 00:00:00 2001 From: rakita Date: Thu, 17 Feb 2022 13:43:40 +0100 Subject: [PATCH] [revm] pop_top and unsafe comments --- bins/revm-test/src/main.rs | 1 + bins/revme/src/debugger/ctrl/ctrl.rs | 1 + bins/revme/src/statetest/trace.rs | 2 + crates/revm/src/db/web3db.rs | 2 +- crates/revm/src/evm.rs | 4 +- crates/revm/src/instructions/i256.rs | 10 ++- crates/revm/src/instructions/macros.rs | 90 +++++++++++++++----------- crates/revm/src/instructions/misc.rs | 10 ++- crates/revm/src/instructions/system.rs | 3 +- crates/revm/src/lib.rs | 3 +- crates/revm/src/machine/machine.rs | 4 ++ crates/revm/src/machine/memory.rs | 3 +- crates/revm/src/machine/stack.rs | 54 +++++++++++++--- 13 files changed, 126 insertions(+), 61 deletions(-) diff --git a/bins/revm-test/src/main.rs b/bins/revm-test/src/main.rs index db180ba1fa..1d2a5dcee8 100644 --- a/bins/revm-test/src/main.rs +++ b/bins/revm-test/src/main.rs @@ -37,4 +37,5 @@ pub fn simple_example() { fn main() { println!("Hello, world!"); simple_example(); + println!("end!"); } diff --git a/bins/revme/src/debugger/ctrl/ctrl.rs b/bins/revme/src/debugger/ctrl/ctrl.rs index b0ae6e11b3..60e8be2654 100644 --- a/bins/revme/src/debugger/ctrl/ctrl.rs +++ b/bins/revme/src/debugger/ctrl/ctrl.rs @@ -103,6 +103,7 @@ impl Controller { } } } + /// impl Inspector for Controller { fn step( diff --git a/bins/revme/src/statetest/trace.rs b/bins/revme/src/statetest/trace.rs index ab8dc26309..e1345b1334 100644 --- a/bins/revme/src/statetest/trace.rs +++ b/bins/revme/src/statetest/trace.rs @@ -39,6 +39,8 @@ impl Inspector for CustomPrintTracer { data: &mut EVMData<'_, DB>, _is_static: bool, ) -> Return { + // Safety: casting. In analazis we are making this clame true that program counter will always + // point to bytecode of the contract. let opcode = unsafe { *machine.program_counter }; let opcode_str = opcode::OPCODE_JUMPMAP[opcode as usize]; diff --git a/crates/revm/src/db/web3db.rs b/crates/revm/src/db/web3db.rs index f6549bf433..fb040a3085 100644 --- a/crates/revm/src/db/web3db.rs +++ b/crates/revm/src/db/web3db.rs @@ -39,7 +39,7 @@ impl Web3DB { } /// internal utility function to call tokio feature and wait for output - fn block_on(&self, f: F) -> F::Output { + fn block_on(&self, f: F) -> F::Output { match &self.runtime { Some(runtime) => runtime.block_on(f), None => futures::executor::block_on(f), diff --git a/crates/revm/src/evm.rs b/crates/revm/src/evm.rs index 5a0f1ebb9d..871f425600 100644 --- a/crates/revm/src/evm.rs +++ b/crates/revm/src/evm.rs @@ -5,7 +5,7 @@ use crate::{ BerlinSpec, ByzantineSpec, Env, Inspector, IstanbulSpec, LatestSpec, Log, LondonSpec, NoOpInspector, Return, Spec, SpecId, TransactOut, }; -use alloc::boxed::Box; +use alloc::{boxed::Box, vec::Vec}; use revm_precompiles::Precompiles; /// Struct that takes Database and enabled transact to update state dirrectly to database. /// additionaly it allows user to set all environment parameters. @@ -23,6 +23,8 @@ use revm_precompiles::Precompiles; /// want to update anything on it. It enabled `transact_ref` and `inspect_ref` functions /// * Database+DatabaseCommit allow's dirrectly commiting changes of transaction. it enabled `transact_commit` /// and `inspect_commit` + +#[derive(Clone)] pub struct EVM { pub env: Env, pub db: Option, diff --git a/crates/revm/src/instructions/i256.rs b/crates/revm/src/instructions/i256.rs index 04f71dfb90..7cb795dfa5 100644 --- a/crates/revm/src/instructions/i256.rs +++ b/crates/revm/src/instructions/i256.rs @@ -29,7 +29,7 @@ pub struct I256(pub Sign, pub U256); #[inline(always)] pub fn i256_sign(val: &mut U256) -> Sign { - if unsafe { val.0.get_unchecked(3) } & SIGN_BITMASK_U64 == 0 { + if val.0[3] & SIGN_BITMASK_U64 == 0 { if val.is_zero() { Sign::Zero } else { @@ -45,9 +45,7 @@ pub fn i256_sign(val: &mut U256) -> Sign { #[inline(always)] fn u256_remove_sign(val: &mut U256) { - unsafe { - *val.0.get_unchecked_mut(3) = val.0.get_unchecked(3) & FLIPH_BITMASK_U64; - } + val.0[3] &= FLIPH_BITMASK_U64; } #[inline(always)] @@ -438,11 +436,11 @@ mod tests { let mut f = U256([1, 100, 1, 1]); let mut s = U256([0, 0, 10, 0]); - let time = std::time::Instant::now(); + //let time = std::time::Instant::now(); for i in 0..1_000_000 { f.0[1] = i; s.0[3] = div_u256::div_mod(f, s).0 .0[3]; } - println!("TIME:{:?}", time.elapsed()); + //println!("TIME:{:?}", time.elapsed()); } } diff --git a/crates/revm/src/instructions/macros.rs b/crates/revm/src/instructions/macros.rs index 6b31cf16f5..c3591a8aa4 100644 --- a/crates/revm/src/instructions/macros.rs +++ b/crates/revm/src/instructions/macros.rs @@ -77,6 +77,7 @@ macro_rules! pop_address { return Return::StackUnderflow; } let mut temp = H256::zero(); + // Safety: Length is checked above. let $x1: H160 = { unsafe { $machine @@ -93,6 +94,7 @@ macro_rules! pop_address { } let mut temp = H256::zero(); $x1: H160 = { + // Safety: Length is checked above. unsafe { $machine .stack @@ -103,6 +105,7 @@ macro_rules! pop_address { }; $x2: H160 = { temp = H256::zero(); + // Safety: Length is checked above. unsafe { $machine .stack @@ -119,18 +122,21 @@ macro_rules! pop { if $machine.stack.len() < 1 { return Return::StackUnderflow; } + // Safety: Length is checked above. let $x1 = unsafe { $machine.stack.pop_unsafe() }; }; ( $machine:expr, $x1:ident, $x2:ident) => { if $machine.stack.len() < 2 { return Return::StackUnderflow; } + // Safety: Length is checked above. let ($x1, $x2) = unsafe { $machine.stack.pop2_unsafe() }; }; ( $machine:expr, $x1:ident, $x2:ident, $x3:ident) => { if $machine.stack.len() < 3 { return Return::StackUnderflow; } + // Safety: Length is checked above. let ($x1, $x2, $x3) = unsafe { $machine.stack.pop3_unsafe() }; }; @@ -138,10 +144,35 @@ macro_rules! pop { if $machine.stack.len() < 4 { return Return::StackUnderflow; } + // Safety: Length is checked above. let ($x1, $x2, $x3, $x4) = unsafe { $machine.stack.pop4_unsafe() }; }; } +macro_rules! pop_top { + ( $machine:expr, $x1:ident) => { + if $machine.stack.len() < 1 { + return Return::StackUnderflow; + } + // Safety: Length is checked above. + let $x1 = unsafe { $machine.stack.top_unsafe() }; + }; + ( $machine:expr, $x1:ident, $x2:ident) => { + if $machine.stack.len() < 2 { + return Return::StackUnderflow; + } + // Safety: Length is checked above. + let ($x1, $x2) = unsafe { $machine.stack.pop_top_unsafe() }; + }; + ( $machine:expr, $x1:ident, $x2:ident, $x3:ident) => { + if $machine.stack.len() < 3 { + return Return::StackUnderflow; + } + // Safety: Length is checked above. + let ($x1, $x2, $x3) = unsafe { $machine.stack.pop2_top_unsafe() }; + }; +} + macro_rules! push_h256 { ( $machine:expr, $( $x:expr ),* ) => ( $( @@ -167,9 +198,8 @@ macro_rules! push { macro_rules! op1_u256_fn { ( $machine:expr, $op:path ) => {{ //gas!($machine, $gas); - pop!($machine, op1); - let ret = $op(op1); - push!($machine, ret); + pop_top!($machine, op1); + *op1 = $op(*op1); Return::Continue }}; @@ -178,9 +208,9 @@ macro_rules! op1_u256_fn { macro_rules! op2_u256_bool_ref { ( $machine:expr, $op:ident) => {{ //gas!($machine, $gas); - pop!($machine, op1, op2); + pop_top!($machine, op1, op2); let ret = op1.$op(&op2); - push!($machine, if ret { U256::one() } else { U256::zero() }); + *op2 = if ret { U256::one() } else { U256::zero() }; Return::Continue }}; @@ -189,10 +219,8 @@ macro_rules! op2_u256_bool_ref { macro_rules! op2_u256 { ( $machine:expr, $op:ident) => {{ //gas!($machine, $gas); - pop!($machine, op1, op2); - let ret = op1.$op(op2); - push!($machine, ret); - + pop_top!($machine, op1, op2); + *op2 = op1.$op(*op2); Return::Continue }}; } @@ -201,16 +229,16 @@ macro_rules! op2_u256_tuple { ( $machine:expr, $op:ident) => {{ //gas!($machine, $gas); - pop!($machine, op1, op2); - let (ret, ..) = op1.$op(op2); - push!($machine, ret); + pop_top!($machine, op1, op2); + let (ret, ..) = op1.$op(*op2); + *op2 = ret; Return::Continue }}; ( $machine:expr, $op:ident ) => {{ - pop!($machine, op1, op2); + pop_top!($machine, op1, op2); let (ret, ..) = op1.$op(op2); - push!($machine, ret); + *op2 = ret; Return::Continue }}; @@ -220,9 +248,8 @@ macro_rules! op2_u256_fn { ( $machine:expr, $op:path ) => {{ //gas!($machine, $gas); - pop!($machine, op1, op2); - let ret = $op(op1, op2); - push!($machine, ret); + pop_top!($machine, op1, op2); + *op2 = $op(op1, *op2); Return::Continue }}; @@ -236,9 +263,8 @@ macro_rules! op3_u256_fn { ( $machine:expr, $op:path) => {{ //gas!($machine, $gas); - pop!($machine, op1, op2, op3); - let ret = $op(op1, op2, op3); - push!($machine, ret); + pop_top!($machine, op1, op2, op3); + *op3 = $op(op1, op2, *op3); Return::Continue }}; @@ -250,40 +276,28 @@ macro_rules! op3_u256_fn { macro_rules! as_usize_saturated { ( $v:expr ) => {{ - if unsafe { - *$v.0.get_unchecked(1) != 0 - || *$v.0.get_unchecked(2) != 0 - || *$v.0.get_unchecked(3) != 0 - } { + if $v.0[1] != 0 || $v.0[2] != 0 || $v.0[3] != 0 { usize::MAX } else { - unsafe { *$v.0.get_unchecked(0) as usize } + $v.0[0] as usize } }}; } macro_rules! as_usize_or_fail { ( $v:expr ) => {{ - if unsafe { - *$v.0.get_unchecked(1) != 0 - || *$v.0.get_unchecked(2) != 0 - || *$v.0.get_unchecked(3) != 0 - } { + if $v.0[1] != 0 || $v.0[2] != 0 || $v.0[3] != 0 { return Return::OutOfGas; } - unsafe { *$v.0.get_unchecked(0) as usize } + $v.0[0] as usize }}; ( $v:expr, $reason:expr ) => {{ - if unsafe { - *$v.0.get_unchecked(1) != 0 - || *$v.0.get_unchecked(2) != 0 - || *$v.0.get_unchecked(3) != 0 - } { + if $v.0[1] != 0 || $v.0[2] != 0 || $v.0[3] != 0 { return $reason; } - unsafe { *$v.0.get_unchecked(0) as usize } + $v.0[0] as usize }}; } diff --git a/crates/revm/src/instructions/misc.rs b/crates/revm/src/instructions/misc.rs index 638cb06f47..087f551ca3 100644 --- a/crates/revm/src/instructions/misc.rs +++ b/crates/revm/src/instructions/misc.rs @@ -20,6 +20,7 @@ pub fn codecopy(machine: &mut Machine) -> Return { let code_offset = as_usize_saturated!(code_offset); memory_resize!(machine, memory_offset, len); + // Safety: set_data is unsafe function and memory_resize ensures us that it is safe to call it machine .memory .set_data(memory_offset, code_offset, len, &machine.contract.code); @@ -68,6 +69,7 @@ pub fn calldatacopy(machine: &mut Machine) -> Return { let data_offset = as_usize_saturated!(data_offset); memory_resize!(machine, memory_offset, len); + // Safety: set_data is unsafe function and memory_resize ensures us that it is safe to call it machine .memory .set_data(memory_offset, data_offset, len, &machine.contract.input); @@ -111,7 +113,7 @@ pub fn mstore8(machine: &mut Machine) -> Return { let index = as_usize_or_fail!(index, Return::OutOfGas); memory_resize!(machine, index, 1); let value = (value.low_u32() & 0xff) as u8; - // SAFETY: we resized our memory two lines above. + // Safety: we resized our memory two lines above. unsafe { machine.memory.set_byte(index, value) } Return::Continue } @@ -123,6 +125,8 @@ pub fn jump(machine: &mut Machine) -> Return { let dest = as_usize_or_fail!(dest, Return::InvalidJump); if machine.contract.is_valid_jump(dest) { + // Safety: In analazis we are checking create our jump table and we do check above to be + // sure that jump is safe to execute. machine.program_counter = unsafe { machine.contract.code.as_ptr().add(dest) }; Return::Continue } else { @@ -138,6 +142,8 @@ pub fn jumpi(machine: &mut Machine) -> Return { if !value.is_zero() { let dest = as_usize_or_fail!(dest, Return::InvalidJump); if machine.contract.is_valid_jump(dest) { + // Safety: In analazis we are checking if jump is valid destination and this if. + // make this unsafe block safe. machine.program_counter = unsafe { machine.contract.code.as_ptr().add(dest) }; Return::Continue } else { @@ -172,6 +178,8 @@ pub fn push(machine: &mut Machine) -> Return { //gas!(machine, gas::VERYLOW); let start = machine.program_counter; + // Safety: In Analazis we appended needed bytes for bytecode so that we are safe to just add without + // checking if it is out of bound. This makes both of our unsafes block safe to do. let ret = machine .stack .push_slice::(unsafe { core::slice::from_raw_parts(start, N) }); diff --git a/crates/revm/src/instructions/system.rs b/crates/revm/src/instructions/system.rs index 0193e9b6d4..43488a90e1 100644 --- a/crates/revm/src/instructions/system.rs +++ b/crates/revm/src/instructions/system.rs @@ -162,6 +162,7 @@ pub fn extcodecopy(machine: &mut Machine, host: &mut H) -> let code_offset = min(as_usize_saturated!(code_offset), code.len()); memory_resize!(machine, memory_offset, len); + // Safety: set_data is unsafe function and memory_resize ensures us that it is safe to call it machine .memory .set_data(memory_offset, code_offset, len, &code); @@ -289,8 +290,8 @@ pub fn log(machine: &mut Machine, n: u8, host: &mut H) -> R let mut topics = Vec::with_capacity(n); for _ in 0..(n) { - /*** SAFETY stack bounds already checked few lines above */ let mut t = H256::zero(); + // Sefety: stack bounds already checked few lines above unsafe { machine.stack.pop_unsafe().to_big_endian(t.as_bytes_mut()) }; topics.push(t); } diff --git a/crates/revm/src/lib.rs b/crates/revm/src/lib.rs index 297a610af3..a259b36e7e 100644 --- a/crates/revm/src/lib.rs +++ b/crates/revm/src/lib.rs @@ -1,6 +1,5 @@ #![allow(dead_code)] -//#![forbid(unsafe_code, unused_variables, unused_imports)] -//#![no_std] only blocker in auto_impl check: https://github.com/bluealloy/revm/issues/4 +//#![no_std] pub mod db; mod evm; diff --git a/crates/revm/src/machine/machine.rs b/crates/revm/src/machine/machine.rs index d87db91a1e..afbacc43a0 100644 --- a/crates/revm/src/machine/machine.rs +++ b/crates/revm/src/machine/machine.rs @@ -163,6 +163,7 @@ impl Machine { /// Return a reference of the program counter. pub fn program_counter(&self) -> usize { + // Sefety: this is just substraction of pointers, it is safe to do. unsafe { self.program_counter .offset_from(self.contract.code.as_ptr()) as usize @@ -186,6 +187,9 @@ impl Machine { } } let opcode = unsafe { *self.program_counter }; + // Safety: In analazis we are doing padding of bytecode so that we are sure that last. + // byte instruction is STOP so we are safe to just increment program_counter bcs on last instruction + // it will do noop and just stop execution of this contract self.program_counter = unsafe { self.program_counter.offset(1) }; ret = eval::(opcode, self, host); diff --git a/crates/revm/src/machine/memory.rs b/crates/revm/src/machine/memory.rs index c2f6477c7a..fbb825644e 100644 --- a/crates/revm/src/machine/memory.rs +++ b/crates/revm/src/machine/memory.rs @@ -58,7 +58,6 @@ impl Memory { } /// Set memory region at given offset. The offset and value are already checked - /// #[inline(always)] pub unsafe fn set_byte(&mut self, index: usize, byte: u8) { *self.data.get_unchecked_mut(index) = byte; @@ -84,6 +83,7 @@ impl Memory { if data_offset >= data.len() { // nulify all memory slots for i in memory_offset..memory_offset + len { + // Safety: Memory is assumed to be valid. And it is commented where that assumption is made unsafe { *self.data.get_unchecked_mut(i) = 0; } @@ -96,6 +96,7 @@ impl Memory { // nulify rest of memory slots for i in memory_data_end..memory_offset + len { + // Safety: Memory is assumed to be valid. And it is commented where that assumption is made unsafe { *self.data.get_unchecked_mut(i) = 0; } diff --git a/crates/revm/src/machine/stack.rs b/crates/revm/src/machine/stack.rs index d1a9e2bd19..fa61b5b6fa 100644 --- a/crates/revm/src/machine/stack.rs +++ b/crates/revm/src/machine/stack.rs @@ -9,9 +9,9 @@ pub struct Stack { data: Vec, } -use std::fmt::{Display, Error, Formatter}; -impl Display for Stack { - fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), Error> { +#[cfg(feature = "std")] +impl std::fmt::Display for Stack { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { if self.data.is_empty() { f.write_str("[]")?; } else { @@ -37,6 +37,7 @@ impl Stack { /// Create a new stack with given limit. pub fn new() -> Self { Self { + // Safety: A lot of functions assumes that capacity is STACK_LIMIT data: Vec::with_capacity(STACK_LIMIT), } } @@ -78,9 +79,7 @@ impl Stack { } #[inline(always)] - /**** SAFETY ******** - * caller is responsible to check length of array - */ + /// Safety: caller is responsible to check length of array pub unsafe fn pop_unsafe(&mut self) -> U256 { let mut len = self.data.len(); len -= 1; @@ -89,6 +88,37 @@ impl Stack { } #[inline(always)] + /// Safety: caller is responsible to check length of array + pub unsafe fn top_unsafe(&mut self) -> &mut U256 { + let len = self.data.len(); + self.data.get_unchecked_mut(len - 1) + } + + #[inline(always)] + /// Safety: caller is responsible to check length of array + pub unsafe fn pop_top_unsafe(&mut self) -> (U256, &mut U256) { + let mut len = self.data.len(); + let pop = *self.data.get_unchecked(len - 1); + len -= 1; + self.data.set_len(len); + + (pop, self.data.get_unchecked_mut(len - 1)) + } + + #[inline(always)] + /// Safety: caller is responsible to check length of array + pub unsafe fn pop2_top_unsafe(&mut self) -> (U256, U256, &mut U256) { + let mut len = self.data.len(); + let pop1 = *self.data.get_unchecked(len - 1); + len -= 2; + let pop2 = *self.data.get_unchecked(len); + self.data.set_len(len); + + (pop1, pop2, self.data.get_unchecked_mut(len - 1)) + } + + #[inline(always)] + /// Safety: caller is responsible to check length of array pub unsafe fn pop2_unsafe(&mut self) -> (U256, U256) { let mut len = self.data.len(); len -= 2; @@ -100,6 +130,7 @@ impl Stack { } #[inline(always)] + /// Safety: caller is responsible to check length of array pub unsafe fn pop3_unsafe(&mut self) -> (U256, U256, U256) { let mut len = self.data.len(); len -= 3; @@ -112,6 +143,7 @@ impl Stack { } #[inline(always)] + /// Safety: caller is responsible to check length of array pub unsafe fn pop4_unsafe(&mut self) -> (U256, U256, U256, U256) { let mut len = self.data.len(); len -= 4; @@ -166,10 +198,10 @@ impl Stack { } else if len + 1 > STACK_LIMIT { Return::StackOverflow } else { + // Safety: check for out of bounds is done above and it makes this safe to do. unsafe { *self.data.get_unchecked_mut(len) = *self.data.get_unchecked(len - N); - let new_len = len + 1; - self.data.set_len(new_len); + self.data.set_len(len + 1); } Return::Continue } @@ -181,7 +213,7 @@ impl Stack { if len <= N { return Return::StackUnderflow; } - // SAFETY: length is checked before so we are okay to switch bytes in unsafe way. + // Safety: length is checked before so we are okay to switch bytes in unsafe way. unsafe { let pa: *mut U256 = self.data.get_unchecked_mut(len - 1); let pb: *mut U256 = self.data.get_unchecked_mut(len - 1 - N); @@ -198,11 +230,13 @@ impl Stack { return Return::StackOverflow; } + let slot; + // Safety: check above ensures us that we are okey in increment len. unsafe { self.data.set_len(new_len); + slot = self.data.get_unchecked_mut(new_len - 1); } - let slot = self.data.get_mut(new_len - 1).unwrap(); slot.0 = [0u64; 4]; let mut dangling = [0u8; 8]; if N < 8 {