From 9a39621653fc21e27fca970632903a01dd124735 Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Fri, 14 Jul 2023 22:12:03 +0300 Subject: [PATCH 01/14] Return correct PanicReason on write_bytes without ownership --- fuel-vm/src/interpreter/memory.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fuel-vm/src/interpreter/memory.rs b/fuel-vm/src/interpreter/memory.rs index 785bed204..e0dad8bea 100644 --- a/fuel-vm/src/interpreter/memory.rs +++ b/fuel-vm/src/interpreter/memory.rs @@ -575,7 +575,7 @@ pub(crate) fn write_bytes( ) -> Result<(), RuntimeError> { let range = MemoryRange::new_const::<_, COUNT>(addr)?; if !owner.has_ownership_range(&range) { - return Err(PanicReason::MemoryOverflow.into()) + return Err(PanicReason::MemoryOwnership.into()) } memory[range.usizes()].copy_from_slice(&bytes); From d58536d9877c19f33b8e9c34b8155bd7b408a791 Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Fri, 14 Jul 2023 23:46:36 +0300 Subject: [PATCH 02/14] Fix and clarify MemoryOverflow errors --- fuel-vm/src/constraints/reg_key.rs | 8 +- fuel-vm/src/interpreter/blockchain.rs | 168 +++++++----------- fuel-vm/src/interpreter/blockchain/test.rs | 2 + fuel-vm/src/interpreter/contract.rs | 20 +-- fuel-vm/src/interpreter/executors/main.rs | 5 - fuel-vm/src/interpreter/internal.rs | 4 +- fuel-vm/src/interpreter/memory.rs | 88 +++++---- .../interpreter/memory/allocation_tests.rs | 6 +- fuel-vm/src/interpreter/memory/tests.rs | 2 +- fuel-vm/src/tests/blockchain.rs | 5 +- 10 files changed, 146 insertions(+), 162 deletions(-) diff --git a/fuel-vm/src/constraints/reg_key.rs b/fuel-vm/src/constraints/reg_key.rs index 21ec13d55..bdd12d9e3 100644 --- a/fuel-vm/src/constraints/reg_key.rs +++ b/fuel-vm/src/constraints/reg_key.rs @@ -288,7 +288,9 @@ impl<'r> ProgramRegisters<'r> { // Translate the `a` absolute register index to a program register index. let a = a.translate(); // Split the array at the first register which is a. - let [i, rest @ ..] = &mut self.0[a..] else { return None }; + let [i, rest @ ..] = &mut self.0[a..] else { + return None + }; // Translate the `b` absolute register index to a program register index. // Subtract 1 because the first register is `a`. // Subtract `a` registers because we split the array at `a`. @@ -303,7 +305,9 @@ impl<'r> ProgramRegisters<'r> { // Translate the `b` absolute register index to a program register index. let b = b.translate(); // Split the array at the first register which is b. - let [i, rest @ ..] = &mut self.0[b..] else { return None }; + let [i, rest @ ..] = &mut self.0[b..] else { + return None + }; // Translate the `a` absolute register index to a program register index. // Subtract 1 because the first register is `b`. // Subtract `b` registers because we split the array at `b`. diff --git a/fuel-vm/src/interpreter/blockchain.rs b/fuel-vm/src/interpreter/blockchain.rs index 493dbd506..d2c42f444 100644 --- a/fuel-vm/src/interpreter/blockchain.rs +++ b/fuel-vm/src/interpreter/blockchain.rs @@ -19,6 +19,7 @@ use super::{ AppendReceipt, }, memory::{ + read_bytes, try_mem_write, try_zeroize, OwnershipRegisters, @@ -29,13 +30,7 @@ use super::{ RuntimeBalances, }; use crate::{ - arith, - arith::{ - add_usize, - checked_add_usize, - checked_add_word, - checked_sub_word, - }, + arith::checked_add_word, call::CallFrame, constraints::{ reg_key::*, @@ -83,10 +78,7 @@ use fuel_types::{ Word, }; -use std::{ - borrow::Borrow, - ops::Range, -}; +use std::borrow::Borrow; #[cfg(test)] mod code_tests; @@ -481,39 +473,32 @@ impl<'vm, S, I> LoadContractCodeCtx<'vm, S, I> { return Err(PanicReason::ExpectedUnallocatedStack.into()) } - let contract_id = a as usize; - let contract_id_end = checked_add_usize(ContractId::LEN, contract_id)?; + // Validate arguments + let contract_id = ContractId::from(read_bytes(self.memory, a)?); let contract_offset = b as usize; let length = bytes::padded_len_usize(c as usize); + let dst_range = MemoryRange::new(ssp, length)?; - let memory_offset = ssp as usize; - let memory_offset_end = checked_add_usize(memory_offset, length)?; - - // Validate arguments - if memory_offset_end >= *self.hp as usize - || contract_id_end as Word > VM_MAX_RAM - || length > MEM_MAX_ACCESS_SIZE as usize - || length > self.contract_max_size as usize - { + if dst_range.end >= *self.hp as usize { + // Would make stack and heap overlap return Err(PanicReason::MemoryOverflow.into()) } - // Clear memory - self.memory[memory_offset..memory_offset_end].fill(0); + if length > self.contract_max_size as usize { + return Err(PanicReason::MemoryOverflow.into()) + } - // Fetch the contract id - let contract_id: &[u8; ContractId::LEN] = &self.memory - [contract_id..contract_id_end] - .try_into() - .expect("This can't fail, because we checked the bounds above."); + if length > MEM_MAX_ACCESS_SIZE as usize { + return Err(PanicReason::MaxMemoryAccess.into()) + } - // Safety: Memory bounds are checked and consistent - let contract_id = ContractId::from_bytes_ref(contract_id); + // Clear memory + self.memory[dst_range.usizes()].fill(0); - self.input_contracts.check(contract_id)?; + self.input_contracts.check(&contract_id)?; // fetch the storage contract - let contract = super::contract::contract(self.storage, contract_id)?; + let contract = super::contract::contract(self.storage, &contract_id)?; let contract = contract.as_ref().as_ref(); if contract_offset > contract.len() { @@ -525,10 +510,12 @@ impl<'vm, S, I> LoadContractCodeCtx<'vm, S, I> { let code = &contract[..len]; + let dst_range = dst_range.truncated(code.len()); + let memory = self .memory - .get_mut(memory_offset..arith::checked_add_usize(memory_offset, len)?) - .ok_or(PanicReason::MemoryOverflow)?; + .get_mut(dst_range.usizes()) + .expect("Checked memory access"); // perform the code copy memory.copy_from_slice(code); @@ -544,23 +531,25 @@ impl<'vm, S, I> LoadContractCodeCtx<'vm, S, I> { // update frame pointer, if we have a stack frame (e.g. fp > 0) if fp > 0 { - let fp_code_size = add_usize(fp, CallFrame::code_size_offset()); - let fp_code_size_end = add_usize(fp_code_size, WORD_SIZE); - - if fp_code_size_end > self.memory.len() { - Err(PanicReason::MemoryOverflow)?; - } + let fp_code_size = MemoryRange::new_overflowing_op( + usize::overflowing_add, + fp, + CallFrame::code_size_offset(), + WORD_SIZE, + )?; - let length = Word::from_be_bytes( - self.memory[fp_code_size..fp_code_size_end] + let old_code_size = Word::from_be_bytes( + self.memory[fp_code_size.usizes()] .try_into() .expect("`fp_code_size_end` is `WORD_SIZE`"), - ) - .checked_add(length as Word) - .ok_or(PanicReason::MemoryOverflow)?; + ); + + let new_code_size = old_code_size + .checked_add(length as Word) + .ok_or(PanicReason::MemoryOverflow)?; - self.memory[fp_code_size..fp_code_size_end] - .copy_from_slice(&length.to_be_bytes()); + self.memory[fp_code_size.usizes()] + .copy_from_slice(&new_code_size.to_be_bytes()); } inc_pc(self.pc) @@ -668,17 +657,13 @@ impl<'vm, S, I> CodeCopyCtx<'vm, S, I> { S: InterpreterStorage, { let contract = CheckedMemConstLen::<{ ContractId::LEN }>::new(b)?; - let cd = checked_add_word(c, d)?; - if d > MEM_MAX_ACCESS_SIZE - || a > checked_sub_word(VM_MAX_RAM, d)? - || cd > VM_MAX_RAM - { - return Err(PanicReason::MemoryOverflow.into()) - } + MemoryRange::new(a, d)?; + let c_range = MemoryRange::new(c, d)?; - let (a, c, d) = (a as usize, c as usize, d as usize); - let cd = cd as usize; + if d > MEM_MAX_ACCESS_SIZE { + return Err(PanicReason::MaxMemoryAccess.into()) + } let contract = ContractId::from_bytes_ref(contract.read(self.memory)); @@ -686,10 +671,15 @@ impl<'vm, S, I> CodeCopyCtx<'vm, S, I> { let contract = super::contract::contract(self.storage, contract)?.into_owned(); - if contract.as_ref().len() < d { + if contract.as_ref().len() < d as usize { try_zeroize(a, d, self.owner, self.memory)?; } else { - try_mem_write(a, &contract.as_ref()[c..cd], self.owner, self.memory)?; + try_mem_write( + a, + &contract.as_ref()[c_range.usizes()], + self.owner, + self.memory, + )?; } inc_pc(self.pc) @@ -753,13 +743,9 @@ impl<'vm, S, I: Iterator> CodeRootCtx<'vm, S, I> { where S: InterpreterStorage, { - let ax = checked_add_word(a, Bytes32::LEN as Word)?; + MemoryRange::new(a, Bytes32::LEN)?; let contract_id = CheckedMemConstLen::<{ ContractId::LEN }>::new(b)?; - if ax > VM_MAX_RAM { - return Err(PanicReason::MemoryOverflow.into()) - } - let contract_id = ContractId::from_bytes_ref(contract_id.read(self.memory)); self.input_contracts.check(contract_id)?; @@ -949,7 +935,7 @@ where )?; if self.msg_data_len > MEM_MAX_ACCESS_SIZE { - return Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)) + return Err(RuntimeError::Recoverable(PanicReason::MaxMemoryAccess)) } if self.msg_data_len > self.max_message_data_length { @@ -1006,11 +992,9 @@ where } struct StateReadQWord { - /// The destination memory address is - /// stored in this range of memory. - destination_address_memory_range: Range, - /// The starting storage key location is stored - /// in this range of memory. + /// The destination memory address is stored in this range of memory. + destination_address_memory_range: MemoryRange, + /// The starting storage key location is stored in this range of memory. origin_key_memory_range: CheckedMemConstLen<{ Bytes32::LEN }>, /// Number of slots to read. num_slots: Word, @@ -1023,28 +1007,16 @@ impl StateReadQWord { num_slots: Word, ownership_registers: OwnershipRegisters, ) -> Result { - let mem_range = MemoryRange::new( + let destination_address_memory_range = MemoryRange::new( destination_memory_address, (Bytes32::LEN as Word).saturating_mul(num_slots), )?; - if !ownership_registers.has_ownership_range(&mem_range) { - return Err(PanicReason::MemoryOwnership.into()) - } - if ownership_registers.context.is_external() { - return Err(PanicReason::ExpectedInternalContext.into()) - } - let dest_end = checked_add_word( - destination_memory_address, - Bytes32::LEN.saturating_mul(num_slots as usize) as Word, - )?; + ownership_registers.verify_ownership(&destination_address_memory_range)?; + ownership_registers.verify_internal_context()?; let origin_key_memory_range = CheckedMemConstLen::<{ Bytes32::LEN }>::new(origin_key_memory_address)?; - if dest_end > VM_MAX_RAM { - return Err(PanicReason::MemoryOverflow.into()) - } Ok(Self { - destination_address_memory_range: (destination_memory_address as usize) - ..(dest_end as usize), + destination_address_memory_range, origin_key_memory_range, num_slots, }) @@ -1077,7 +1049,7 @@ fn state_read_qword( *result_register = all_set as Word; - memory[input.destination_address_memory_range].copy_from_slice(&result); + memory[input.destination_address_memory_range.usizes()].copy_from_slice(&result); inc_pc(pc)?; @@ -1085,12 +1057,10 @@ fn state_read_qword( } struct StateWriteQWord { - /// The starting storage key location is stored - /// in this range of memory. + /// The starting storage key location is stored in this range of memory. starting_storage_key_memory_range: CheckedMemConstLen<{ Bytes32::LEN }>, - /// The source data memory address is - /// stored in this range of memory. - source_address_memory_range: Range, + /// The source data memory address is stored in this range of memory. + source_address_memory_range: MemoryRange, } impl StateWriteQWord { @@ -1099,20 +1069,18 @@ impl StateWriteQWord { source_memory_address: Word, num_slots: Word, ) -> Result { - let source_end = checked_add_word( + let source_address_memory_range = MemoryRange::new( source_memory_address, - Bytes32::LEN.saturating_mul(num_slots as usize) as Word, + (Bytes32::LEN as Word).saturating_mul(num_slots), )?; + let starting_storage_key_memory_range = CheckedMemConstLen::<{ Bytes32::LEN }>::new( starting_storage_key_memory_address, )?; - if source_end > VM_MAX_RAM { - return Err(PanicReason::MemoryOverflow.into()) - } + Ok(Self { - source_address_memory_range: (source_memory_address as usize) - ..(source_end as usize), + source_address_memory_range, starting_storage_key_memory_range, }) } @@ -1129,7 +1097,7 @@ fn state_write_qword( let destination_key = Bytes32::from_bytes_ref(input.starting_storage_key_memory_range.read(memory)); - let values: Vec<_> = memory[input.source_address_memory_range] + let values: Vec<_> = memory[input.source_address_memory_range.usizes()] .chunks_exact(Bytes32::LEN) .flat_map(|chunk| Some(Bytes32::from(<[u8; 32]>::try_from(chunk).ok()?))) .collect(); diff --git a/fuel-vm/src/interpreter/blockchain/test.rs b/fuel-vm/src/interpreter/blockchain/test.rs index 7f39be064..9939cad21 100644 --- a/fuel-vm/src/interpreter/blockchain/test.rs +++ b/fuel-vm/src/interpreter/blockchain/test.rs @@ -1,3 +1,5 @@ +use std::ops::Range; + use crate::{ context::Context, interpreter::memory::Memory, diff --git a/fuel-vm/src/interpreter/contract.rs b/fuel-vm/src/interpreter/contract.rs index 0a175a056..4729cd3fb 100644 --- a/fuel-vm/src/interpreter/contract.rs +++ b/fuel-vm/src/interpreter/contract.rs @@ -9,6 +9,7 @@ use super::{ }, ExecutableTransaction, Interpreter, + MemoryRange, RuntimeBalances, }; use crate::{ @@ -211,24 +212,13 @@ impl<'vm, S, Tx> TransferCtx<'vm, S, Tx> { S: ContractsAssetsStorage, >::Error: Into, { - let ax = a - .checked_add(ContractId::LEN as Word) - .ok_or(PanicReason::ArithmeticOverflow)?; - - let cx = c - .checked_add(AssetId::LEN as Word) - .ok_or(PanicReason::ArithmeticOverflow)?; - - // if above usize::MAX then it cannot be safely cast to usize, - // check the tighter bound between VM_MAX_RAM and usize::MAX - if ax > MIN_VM_MAX_RAM_USIZE_MAX || cx > MIN_VM_MAX_RAM_USIZE_MAX { - return Err(PanicReason::MemoryOverflow.into()) - } + let a_range = MemoryRange::new(a, ContractId::LEN)?; + let c_range = MemoryRange::new(c, AssetId::LEN)?; let amount = b; - let destination = ContractId::try_from(&self.memory[a as usize..ax as usize]) + let destination = ContractId::try_from(&self.memory[a_range.usizes()]) .expect("Unreachable! Checked memory range"); - let asset_id = AssetId::try_from(&self.memory[c as usize..cx as usize]) + let asset_id = AssetId::try_from(&self.memory[c_range.usizes()]) .expect("Unreachable! Checked memory range"); InputContracts::new(self.tx.input_contracts(), panic_context) diff --git a/fuel-vm/src/interpreter/executors/main.rs b/fuel-vm/src/interpreter/executors/main.rs index 47bc92b5b..633ebf2ec 100644 --- a/fuel-vm/src/interpreter/executors/main.rs +++ b/fuel-vm/src/interpreter/executors/main.rs @@ -7,7 +7,6 @@ use crate::{ IntoChecked, ParallelExecutor, }, - consts::*, context::Context, error::{ Bug, @@ -617,10 +616,6 @@ where pub(crate) fn run_program(&mut self) -> Result { loop { - if self.registers[RegId::PC] >= VM_MAX_RAM { - return Err(InterpreterError::Panic(PanicReason::MemoryOverflow)) - } - // Check whether the instruction will be executed in a call context let in_call = !self.frames.is_empty(); diff --git a/fuel-vm/src/interpreter/internal.rs b/fuel-vm/src/interpreter/internal.rs index 343393de6..ef1e43287 100644 --- a/fuel-vm/src/interpreter/internal.rs +++ b/fuel-vm/src/interpreter/internal.rs @@ -230,7 +230,9 @@ pub(crate) fn set_flag( pc: RegMut, a: Word, ) -> Result<(), RuntimeError> { - let Some(flags) = Flags::from_bits(a) else { return Err(PanicReason::ErrorFlag.into()) }; + let Some(flags) = Flags::from_bits(a) else { + return Err(PanicReason::ErrorFlag.into()) + }; *flag = flags.bits(); diff --git a/fuel-vm/src/interpreter/memory.rs b/fuel-vm/src/interpreter/memory.rs index e0dad8bea..8d69a9e6f 100644 --- a/fuel-vm/src/interpreter/memory.rs +++ b/fuel-vm/src/interpreter/memory.rs @@ -109,6 +109,31 @@ impl MemoryRange { Self::new(address, SIZE) } + /// Uses a overflow-capturing operator to combine `base` and `offset` + /// into the start of a memory range, returning an error on overflow, + /// and then checks that the resulting range is within the VM memory. + pub fn new_overflowing_op( + overflowing_op: F, + base: T, + offset: T, + size: A, + ) -> Result + where + F: FnOnce(T, T) -> (T, bool), + { + let (addr, overflow) = overflowing_op(base, offset); + if overflow { + return Err(PanicReason::MemoryOverflow.into()) + } + Self::new(addr, size) + } + + /// Truncate a memory range to a new size if it's smaller then current one. + pub fn truncated(&self, limit: usize) -> Self { + Self::new(self.start, self.len().min(limit)) + .expect("Cannot overflow on truncation") + } + /// Return `true` if the length is `0`. pub fn is_empty(&self) -> bool { self.len() == 0 @@ -327,18 +352,10 @@ pub(crate) fn store_byte( b: Word, c: Word, ) -> Result<(), RuntimeError> { - let (ac, overflow) = a.overflowing_add(c); - let range = ac..(ac + 1); - if overflow - || ac >= VM_MAX_RAM - || !(owner.has_ownership_stack(&range) || owner.has_ownership_heap(&range)) - { - Err(PanicReason::MemoryOverflow.into()) - } else { - memory[ac as usize] = b as u8; - - inc_pc(pc) - } + let range = MemoryRange::new_overflowing_op(Word::overflowing_add, a, c, 1u64)?; + owner.verify_ownership(&range)?; + memory[range.start] = b as u8; + inc_pc(pc) } pub(crate) fn store_word( @@ -381,7 +398,8 @@ pub(crate) fn memclear( b: Word, ) -> Result<(), RuntimeError> { let range = MemoryRange::new(a, b)?; - if b > MEM_MAX_ACCESS_SIZE || !owner.has_ownership_range(&range) { + owner.verify_ownership(&range)?; + if b > MEM_MAX_ACCESS_SIZE { Err(PanicReason::MemoryOverflow.into()) } else { memory[range.usizes()].fill(0); @@ -404,9 +422,7 @@ pub(crate) fn memcopy( return Err(PanicReason::MemoryOverflow.into()) } - if !owner.has_ownership_range(&dst_range) { - return Err(PanicReason::MemoryOwnership.into()) - } + owner.verify_ownership(&dst_range)?; if dst_range.start <= src_range.start && src_range.start < dst_range.end || src_range.start <= dst_range.start && dst_range.start < src_range.end @@ -474,6 +490,25 @@ impl OwnershipRegisters { } } + pub(crate) fn verify_ownership( + &self, + range: &MemoryRange, + ) -> Result<(), PanicReason> { + if self.has_ownership_range(range) { + Ok(()) + } else { + Err(PanicReason::MemoryOwnership) + } + } + + pub(crate) fn verify_internal_context(&self) -> Result<(), PanicReason> { + if self.context.is_internal() { + Ok(()) + } else { + Err(PanicReason::ExpectedInternalContext) + } + } + pub(crate) fn has_ownership_range(&self, range: &MemoryRange) -> bool { let range = range.words(); self.has_ownership_stack(&range) || self.has_ownership_heap(&range) @@ -520,15 +555,11 @@ impl OwnershipRegisters { pub(crate) fn try_mem_write( addr: A, data: &[u8], - registers: OwnershipRegisters, + owner: OwnershipRegisters, memory: &mut [u8; MEM_SIZE], ) -> Result<(), RuntimeError> { let range = MemoryRange::new(addr, data.len())?; - - if !registers.has_ownership_range(&range) { - return Err(PanicReason::MemoryOwnership.into()) - } - + owner.verify_ownership(&range)?; memory[range.usizes()].copy_from_slice(data); Ok(()) } @@ -536,15 +567,11 @@ pub(crate) fn try_mem_write( pub(crate) fn try_zeroize( addr: A, len: B, - registers: OwnershipRegisters, + owner: OwnershipRegisters, memory: &mut [u8; MEM_SIZE], ) -> Result<(), RuntimeError> { let range = MemoryRange::new(addr, len)?; - - if !registers.has_ownership_range(&range) { - return Err(PanicReason::MemoryOwnership.into()) - } - + owner.verify_ownership(&range)?; memory[range.usizes()].fill(0); Ok(()) } @@ -574,10 +601,7 @@ pub(crate) fn write_bytes( bytes: [u8; COUNT], ) -> Result<(), RuntimeError> { let range = MemoryRange::new_const::<_, COUNT>(addr)?; - if !owner.has_ownership_range(&range) { - return Err(PanicReason::MemoryOwnership.into()) - } - + owner.verify_ownership(&range)?; memory[range.usizes()].copy_from_slice(&bytes); Ok(()) } diff --git a/fuel-vm/src/interpreter/memory/allocation_tests.rs b/fuel-vm/src/interpreter/memory/allocation_tests.rs index 55cf4da3a..7a40fac92 100644 --- a/fuel-vm/src/interpreter/memory/allocation_tests.rs +++ b/fuel-vm/src/interpreter/memory/allocation_tests.rs @@ -19,7 +19,7 @@ fn test_malloc(mut hp: Word, sp: Word, a: Word) -> Result { } #[test_case(true, 1, 10 => Ok(()); "Can clear some bytes")] -#[test_case(false, 1, 10 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "No ownership")] +#[test_case(false, 1, 10 => Err(RuntimeError::Recoverable(PanicReason::MemoryOwnership)); "No ownership")] #[test_case(true, 0, 10 => Ok(()); "Memory range starts at 0")] #[test_case(true, MEM_SIZE as Word - 10, 10 => Ok(()); "Memory range ends at last address")] #[test_case(true, 1, MEM_MAX_ACCESS_SIZE + 1 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Memory range size exceeds limit")] @@ -208,7 +208,7 @@ fn test_load_word(b: Word, c: Word) -> Result<(), RuntimeError> { #[test_case(true, 20, 30, 40 => Ok(()); "Can store a byte")] #[test_case(false, VM_MAX_RAM - 1, 100, 2 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Memory overflow on heap")] -#[test_case(false, 0, 100, VM_MAX_RAM - 1 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Memory overflow on stack")] +#[test_case(false, 0, 100, VM_MAX_RAM - 1 => Err(RuntimeError::Recoverable(PanicReason::MemoryOwnership)); "Memory overflow on stack")] #[test_case(true, VM_MAX_RAM, 1, 1 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Memory overflow by address range")] fn test_store_byte( has_ownership: bool, @@ -242,7 +242,7 @@ fn test_store_byte( #[test_case(true, 20, 30, 40 => Ok(()); "Can store a word")] #[test_case(true, 20, 30, VM_MAX_RAM => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Fails due to memory overflow")] -#[test_case(false, 20, 30, 40 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Fails due to not having ownership of the range")] +#[test_case(false, 20, 30, 40 => Err(RuntimeError::Recoverable(PanicReason::MemoryOwnership)); "Fails due to not having ownership of the range")] fn test_store_word( has_ownership: bool, a: Word, diff --git a/fuel-vm/src/interpreter/memory/tests.rs b/fuel-vm/src/interpreter/memory/tests.rs index 7341781f8..e56e8cb89 100644 --- a/fuel-vm/src/interpreter/memory/tests.rs +++ b/fuel-vm/src/interpreter/memory/tests.rs @@ -180,7 +180,7 @@ fn stack_alloc_ownership() { fn test_ownership(reg: OwnershipRegisters, range: Range) -> bool { let range = MemoryRange::new(range.start, range.end - range.start).expect("Invalid range"); - reg.has_ownership_range(&range) + reg.verify_ownership(&range).is_ok() } fn set_index(index: usize, val: u8, mut array: [u8; 100]) -> [u8; 100] { diff --git a/fuel-vm/src/tests/blockchain.rs b/fuel-vm/src/tests/blockchain.rs index 5ced29d65..30d56a2d8 100644 --- a/fuel-vm/src/tests/blockchain.rs +++ b/fuel-vm/src/tests/blockchain.rs @@ -32,7 +32,6 @@ use fuel_asm::{ op, Instruction, PanicReason::{ - ArithmeticOverflow, ContractNotInInputs, ErrorFlag, ExpectedUnallocatedStack, @@ -689,7 +688,7 @@ fn code_root_a_plus_32_overflow() { op::croo(reg_a, RegId::ZERO), ]; - check_expected_reason_for_instructions(code_root, ArithmeticOverflow); + check_expected_reason_for_instructions(code_root, MemoryOverflow); } #[test] @@ -1148,7 +1147,7 @@ fn state_w_qword_b_plus_32_over() { op::swwq(RegId::ZERO, SET_STATUS_REG, reg_a, RegId::ONE), ]; - check_expected_reason_for_instructions(state_write_qword, ArithmeticOverflow); + check_expected_reason_for_instructions(state_write_qword, MemoryOverflow); } #[test] From 27a94fc2d62d2647e9dbcef44c25dc4cbcdb05aa Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Sat, 15 Jul 2023 00:20:31 +0300 Subject: [PATCH 03/14] Further simplify memory management code --- fuel-vm/src/interpreter/contract.rs | 21 +++------------ fuel-vm/src/interpreter/crypto.rs | 41 ++++++++--------------------- fuel-vm/src/interpreter/flow.rs | 9 +++---- fuel-vm/src/interpreter/internal.rs | 2 +- fuel-vm/src/interpreter/log.rs | 11 ++++---- fuel-vm/src/interpreter/memory.rs | 39 +++++++++------------------ 6 files changed, 38 insertions(+), 85 deletions(-) diff --git a/fuel-vm/src/interpreter/contract.rs b/fuel-vm/src/interpreter/contract.rs index 4729cd3fb..879d647c7 100644 --- a/fuel-vm/src/interpreter/contract.rs +++ b/fuel-vm/src/interpreter/contract.rs @@ -7,6 +7,7 @@ use super::{ set_variable_output, AppendReceipt, }, + memory::read_bytes, ExecutableTransaction, Interpreter, MemoryRange, @@ -282,26 +283,10 @@ impl<'vm, S, Tx> TransferCtx<'vm, S, Tx> { S: ContractsAssetsStorage, >::Error: Into, { - let ax = a - .checked_add(ContractId::LEN as Word) - .ok_or(PanicReason::ArithmeticOverflow)?; - - let dx = d - .checked_add(AssetId::LEN as Word) - .ok_or(PanicReason::ArithmeticOverflow)?; - - // if above usize::MAX then it cannot be safely cast to usize, - // check the tighter bound between VM_MAX_RAM and usize::MAX - if ax > MIN_VM_MAX_RAM_USIZE_MAX || dx > MIN_VM_MAX_RAM_USIZE_MAX { - return Err(PanicReason::MemoryOverflow.into()) - } - + let to = Address::from(read_bytes(self.memory, a)?); let out_idx = b as usize; - let to = Address::try_from(&self.memory[a as usize..ax as usize]) - .expect("Unreachable! Checked memory range"); - let asset_id = AssetId::try_from(&self.memory[d as usize..dx as usize]) - .expect("Unreachable! Checked memory range"); let amount = c; + let asset_id = AssetId::from(read_bytes(self.memory, d)?); let internal_context = match internal_contract(self.context, self.fp, self.memory) { diff --git a/fuel-vm/src/interpreter/crypto.rs b/fuel-vm/src/interpreter/crypto.rs index 81ceab4c3..9edcee829 100644 --- a/fuel-vm/src/interpreter/crypto.rs +++ b/fuel-vm/src/interpreter/crypto.rs @@ -17,13 +17,9 @@ use crate::{ constraints::reg_key::*, consts::*, error::RuntimeError, + prelude::MemoryRange, }; -use crate::arith::{ - checked_add_word, - checked_sub_word, -}; -use fuel_asm::PanicReason; use fuel_crypto::{ Hasher, Message, @@ -186,21 +182,10 @@ pub(crate) fn keccak256( Digest, Keccak256, }; - - let bc = checked_add_word(b, c)?; - - if a > checked_sub_word(VM_MAX_RAM, Bytes32::LEN as Word)? - || c > MEM_MAX_ACCESS_SIZE - || bc > MIN_VM_MAX_RAM_USIZE_MAX - { - return Err(PanicReason::MemoryOverflow.into()) - } - - let (a, b, bc) = (a as usize, b as usize, bc as usize); + let src_range = MemoryRange::new(b, c)?; let mut h = Keccak256::new(); - - h.update(&memory[b..bc]); + h.update(&memory[src_range.usizes()]); try_mem_write(a, h.finalize().as_slice(), owner, memory)?; @@ -215,18 +200,14 @@ pub(crate) fn sha256( b: Word, c: Word, ) -> Result<(), RuntimeError> { - let bc = checked_add_word(b, c)?; - - if a > checked_sub_word(VM_MAX_RAM, Bytes32::LEN as Word)? - || c > MEM_MAX_ACCESS_SIZE - || bc > MIN_VM_MAX_RAM_USIZE_MAX - { - return Err(PanicReason::MemoryOverflow.into()) - } - - let (a, b, bc) = (a as usize, b as usize, bc as usize); - - try_mem_write(a, Hasher::hash(&memory[b..bc]).as_ref(), owner, memory)?; + let src_range = MemoryRange::new(b, c)?; + + try_mem_write( + a, + Hasher::hash(&memory[src_range.usizes()]).as_ref(), + owner, + memory, + )?; inc_pc(pc) } diff --git a/fuel-vm/src/interpreter/flow.rs b/fuel-vm/src/interpreter/flow.rs index 6f1560a71..aad997849 100644 --- a/fuel-vm/src/interpreter/flow.rs +++ b/fuel-vm/src/interpreter/flow.rs @@ -224,18 +224,17 @@ impl RetCtx<'_> { } pub(crate) fn ret_data(self, a: Word, b: Word) -> Result { - if b > MEM_MAX_ACCESS_SIZE || a > VM_MAX_RAM - b { - return Err(PanicReason::MemoryOverflow.into()) + let range = MemoryRange::new(a, b)?; + if b > MEM_MAX_ACCESS_SIZE { + return Err(PanicReason::MaxMemoryAccess.into()) } - let ab = (a + b) as usize; - let receipt = Receipt::return_data( self.current_contract.unwrap_or_else(ContractId::zeroed), a, self.registers[RegId::PC], self.registers[RegId::IS], - self.append.memory[a as usize..ab].to_vec(), + self.append.memory[range.usizes()].to_vec(), ); let digest = *receipt .digest() diff --git a/fuel-vm/src/interpreter/internal.rs b/fuel-vm/src/interpreter/internal.rs index ef1e43287..c40a27c7b 100644 --- a/fuel-vm/src/interpreter/internal.rs +++ b/fuel-vm/src/interpreter/internal.rs @@ -241,7 +241,7 @@ pub(crate) fn set_flag( pub(crate) fn inc_pc(mut pc: RegMut) -> Result<(), RuntimeError> { pc.checked_add(Instruction::SIZE as Word) - .ok_or_else(|| PanicReason::ArithmeticOverflow.into()) + .ok_or_else(|| PanicReason::MemoryOverflow.into()) .map(|i| *pc = i) } diff --git a/fuel-vm/src/interpreter/log.rs b/fuel-vm/src/interpreter/log.rs index 85b9e541b..659bcce54 100644 --- a/fuel-vm/src/interpreter/log.rs +++ b/fuel-vm/src/interpreter/log.rs @@ -8,6 +8,7 @@ use super::{ receipts::ReceiptsCtx, ExecutableTransaction, Interpreter, + MemoryRange, }; use crate::{ constraints::reg_key::*, @@ -124,11 +125,11 @@ impl LogInput<'_> { c: Word, d: Word, ) -> Result<(), RuntimeError> { - if d > MEM_MAX_ACCESS_SIZE || c > VM_MAX_RAM - d { - return Err(PanicReason::MemoryOverflow.into()) - } + let range = MemoryRange::new(c, d)?; - let cd = (c + d) as usize; + if d > MEM_MAX_ACCESS_SIZE { + return Err(PanicReason::MaxMemoryAccess.into()) + } let receipt = Receipt::log_data( internal_contract_or_default(self.context, self.fp, self.memory), @@ -137,7 +138,7 @@ impl LogInput<'_> { c, *self.pc, *self.is, - self.memory[c as usize..cd].to_vec(), + self.memory[range.usizes()].to_vec(), ); append_receipt( diff --git a/fuel-vm/src/interpreter/memory.rs b/fuel-vm/src/interpreter/memory.rs index 8d69a9e6f..7d7c3ee33 100644 --- a/fuel-vm/src/interpreter/memory.rs +++ b/fuel-vm/src/interpreter/memory.rs @@ -43,7 +43,7 @@ pub trait ToAddr { impl ToAddr for usize { fn to_addr(self) -> Result { - if self >= MEM_SIZE { + if self > MEM_SIZE { return Err(PanicReason::MemoryOverflow.into()) } Ok(self) @@ -319,15 +319,9 @@ pub(crate) fn load_byte( b: Word, c: Word, ) -> Result<(), RuntimeError> { - let bc = b.saturating_add(c) as usize; - - if bc >= VM_MAX_RAM as RegisterId { - Err(PanicReason::MemoryOverflow.into()) - } else { - *result = memory[bc] as Word; - - inc_pc(pc) - } + let range = MemoryRange::new_overflowing_op(Word::overflowing_add, b, c, 1)?; + *result = memory[range.start] as Word; + inc_pc(pc) } pub(crate) fn load_word( @@ -400,7 +394,7 @@ pub(crate) fn memclear( let range = MemoryRange::new(a, b)?; owner.verify_ownership(&range)?; if b > MEM_MAX_ACCESS_SIZE { - Err(PanicReason::MemoryOverflow.into()) + Err(PanicReason::MaxMemoryAccess.into()) } else { memory[range.usizes()].fill(0); inc_pc(pc) @@ -452,15 +446,13 @@ pub(crate) fn memeq( c: Word, d: Word, ) -> Result<(), RuntimeError> { - let (bd, overflow) = b.overflowing_add(d); - let (cd, of) = c.overflowing_add(d); - let overflow = overflow || of; + let range1 = MemoryRange::new(b, d)?; + let range2 = MemoryRange::new(c, d)?; - if overflow || bd > VM_MAX_RAM || cd > VM_MAX_RAM || d > MEM_MAX_ACCESS_SIZE { - Err(PanicReason::MemoryOverflow.into()) + if d > MEM_MAX_ACCESS_SIZE { + Err(PanicReason::MaxMemoryAccess.into()) } else { - *result = - (memory[b as usize..bd as usize] == memory[c as usize..cd as usize]) as Word; + *result = (memory[range1.usizes()] == memory[range2.usizes()]) as Word; inc_pc(pc) } @@ -582,14 +574,9 @@ pub(crate) fn read_bytes( memory: &[u8; MEM_SIZE], addr: Word, ) -> Result<[u8; COUNT], RuntimeError> { - let addr = addr as usize; - let (end, overflow) = addr.overflowing_add(COUNT); - - if overflow || end > VM_MAX_RAM as RegisterId { - return Err(PanicReason::MemoryOverflow.into()) - } - - Ok(<[u8; COUNT]>::try_from(&memory[addr..end]).unwrap_or_else(|_| unreachable!())) + let range = MemoryRange::new_const::<_, COUNT>(addr)?; + Ok(<[u8; COUNT]>::try_from(&memory[range.usizes()]) + .unwrap_or_else(|_| unreachable!())) } /// Writes a constant-sized byte array to memory, performing overflow, memory range and From 380c4ba5a6ca962d416a0655b7867b4f47f72c27 Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Sat, 15 Jul 2023 00:23:48 +0300 Subject: [PATCH 04/14] One last MaxAccessSize fix --- fuel-vm/src/interpreter/memory.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fuel-vm/src/interpreter/memory.rs b/fuel-vm/src/interpreter/memory.rs index 7d7c3ee33..fd160592d 100644 --- a/fuel-vm/src/interpreter/memory.rs +++ b/fuel-vm/src/interpreter/memory.rs @@ -413,7 +413,7 @@ pub(crate) fn memcopy( let src_range = MemoryRange::new(b, c)?; if c > MEM_MAX_ACCESS_SIZE { - return Err(PanicReason::MemoryOverflow.into()) + return Err(PanicReason::MaxMemoryAccess.into()) } owner.verify_ownership(&dst_range)?; From 011c058bc270915cc3955df10dcd90c1e6948aa6 Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Sat, 15 Jul 2023 00:28:28 +0300 Subject: [PATCH 05/14] Undo some formatting from too new rustfmt --- fuel-vm/src/constraints/reg_key.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/fuel-vm/src/constraints/reg_key.rs b/fuel-vm/src/constraints/reg_key.rs index bdd12d9e3..21ec13d55 100644 --- a/fuel-vm/src/constraints/reg_key.rs +++ b/fuel-vm/src/constraints/reg_key.rs @@ -288,9 +288,7 @@ impl<'r> ProgramRegisters<'r> { // Translate the `a` absolute register index to a program register index. let a = a.translate(); // Split the array at the first register which is a. - let [i, rest @ ..] = &mut self.0[a..] else { - return None - }; + let [i, rest @ ..] = &mut self.0[a..] else { return None }; // Translate the `b` absolute register index to a program register index. // Subtract 1 because the first register is `a`. // Subtract `a` registers because we split the array at `a`. @@ -305,9 +303,7 @@ impl<'r> ProgramRegisters<'r> { // Translate the `b` absolute register index to a program register index. let b = b.translate(); // Split the array at the first register which is b. - let [i, rest @ ..] = &mut self.0[b..] else { - return None - }; + let [i, rest @ ..] = &mut self.0[b..] else { return None }; // Translate the `a` absolute register index to a program register index. // Subtract 1 because the first register is `b`. // Subtract `b` registers because we split the array at `b`. From 65499ae79792c120958f8912d62cab483c234b51 Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Sat, 15 Jul 2023 00:34:29 +0300 Subject: [PATCH 06/14] More fixes --- fuel-vm/src/interpreter/contract.rs | 10 ++-------- fuel-vm/src/interpreter/memory.rs | 2 +- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/fuel-vm/src/interpreter/contract.rs b/fuel-vm/src/interpreter/contract.rs index 879d647c7..e69c1eaa9 100644 --- a/fuel-vm/src/interpreter/contract.rs +++ b/fuel-vm/src/interpreter/contract.rs @@ -10,7 +10,6 @@ use super::{ memory::read_bytes, ExecutableTransaction, Interpreter, - MemoryRange, RuntimeBalances, }; use crate::{ @@ -213,14 +212,9 @@ impl<'vm, S, Tx> TransferCtx<'vm, S, Tx> { S: ContractsAssetsStorage, >::Error: Into, { - let a_range = MemoryRange::new(a, ContractId::LEN)?; - let c_range = MemoryRange::new(c, AssetId::LEN)?; - + let destination = ContractId::from(read_bytes(self.memory, a)?); let amount = b; - let destination = ContractId::try_from(&self.memory[a_range.usizes()]) - .expect("Unreachable! Checked memory range"); - let asset_id = AssetId::try_from(&self.memory[c_range.usizes()]) - .expect("Unreachable! Checked memory range"); + let asset_id = AssetId::from(read_bytes(self.memory, c)?); InputContracts::new(self.tx.input_contracts(), panic_context) .check(&destination)?; diff --git a/fuel-vm/src/interpreter/memory.rs b/fuel-vm/src/interpreter/memory.rs index fd160592d..313699dc9 100644 --- a/fuel-vm/src/interpreter/memory.rs +++ b/fuel-vm/src/interpreter/memory.rs @@ -319,7 +319,7 @@ pub(crate) fn load_byte( b: Word, c: Word, ) -> Result<(), RuntimeError> { - let range = MemoryRange::new_overflowing_op(Word::overflowing_add, b, c, 1)?; + let range = MemoryRange::new_overflowing_op(Word::overflowing_add, b, c, 1u64)?; *result = memory[range.start] as Word; inc_pc(pc) } From 917c3d6dad39d2d1d48283cd8d4b836b5b61d1d8 Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Sat, 15 Jul 2023 00:51:46 +0300 Subject: [PATCH 07/14] Add more edge case tests for store_byte --- .../interpreter/memory/allocation_tests.rs | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/fuel-vm/src/interpreter/memory/allocation_tests.rs b/fuel-vm/src/interpreter/memory/allocation_tests.rs index 7a40fac92..b73e11bd8 100644 --- a/fuel-vm/src/interpreter/memory/allocation_tests.rs +++ b/fuel-vm/src/interpreter/memory/allocation_tests.rs @@ -240,6 +240,43 @@ fn test_store_byte( Ok(()) } +#[rstest::rstest] +fn test_store_byte_more( + #[values(0, 1, VM_MAX_RAM - 1, VM_MAX_RAM)] a: Word, + #[values(0, 1, 0xff, 0x100)] b: Word, + #[values(0, 1, 2)] c: Word, +) -> Result<(), RuntimeError> { + let mut memory: Memory = vec![1u8; MEM_SIZE].try_into().unwrap(); + let mut pc = 4; + + // Full ownership in heap + let owner = OwnershipRegisters { + sp: 0, + ssp: 0, + hp: 0, + prev_hp: VM_MAX_RAM, + context: Context::Script { + block_height: Default::default(), + }, + }; + + let is_error = a+c >= memory.len() as u64; + match store_byte(&mut memory, owner, RegMut::new(&mut pc), a, b, c) { + Ok(_) => { + assert!(!is_error); + assert_eq!(memory[(a + c) as usize], b as u8); + }, + Err(e) => { + dbg!(&e); + assert!(is_error); + assert_eq!(e, RuntimeError::Recoverable(PanicReason::MemoryOverflow)); + }, + } + + + Ok(()) +} + #[test_case(true, 20, 30, 40 => Ok(()); "Can store a word")] #[test_case(true, 20, 30, VM_MAX_RAM => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Fails due to memory overflow")] #[test_case(false, 20, 30, 40 => Err(RuntimeError::Recoverable(PanicReason::MemoryOwnership)); "Fails due to not having ownership of the range")] From d92c7207c1a20f8087b17929775fabc0ded71396 Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Sat, 15 Jul 2023 00:53:35 +0300 Subject: [PATCH 08/14] Simplify store_byte implementation --- fuel-vm/src/interpreter/memory.rs | 4 +--- fuel-vm/src/interpreter/memory/allocation_tests.rs | 7 +++---- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/fuel-vm/src/interpreter/memory.rs b/fuel-vm/src/interpreter/memory.rs index 313699dc9..661d0f9a8 100644 --- a/fuel-vm/src/interpreter/memory.rs +++ b/fuel-vm/src/interpreter/memory.rs @@ -346,9 +346,7 @@ pub(crate) fn store_byte( b: Word, c: Word, ) -> Result<(), RuntimeError> { - let range = MemoryRange::new_overflowing_op(Word::overflowing_add, a, c, 1u64)?; - owner.verify_ownership(&range)?; - memory[range.start] = b as u8; + write_bytes(memory, owner, a.saturating_add(c), [b as u8])?; inc_pc(pc) } diff --git a/fuel-vm/src/interpreter/memory/allocation_tests.rs b/fuel-vm/src/interpreter/memory/allocation_tests.rs index b73e11bd8..81236cda0 100644 --- a/fuel-vm/src/interpreter/memory/allocation_tests.rs +++ b/fuel-vm/src/interpreter/memory/allocation_tests.rs @@ -260,20 +260,19 @@ fn test_store_byte_more( }, }; - let is_error = a+c >= memory.len() as u64; + let is_error = a + c >= memory.len() as u64; match store_byte(&mut memory, owner, RegMut::new(&mut pc), a, b, c) { Ok(_) => { assert!(!is_error); assert_eq!(memory[(a + c) as usize], b as u8); - }, + } Err(e) => { dbg!(&e); assert!(is_error); assert_eq!(e, RuntimeError::Recoverable(PanicReason::MemoryOverflow)); - }, + } } - Ok(()) } From eefea50a90607cad38b20aec33a77b6a85af970a Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Sat, 15 Jul 2023 00:57:09 +0300 Subject: [PATCH 09/14] Remove dbg macro --- fuel-vm/src/interpreter/memory/allocation_tests.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/fuel-vm/src/interpreter/memory/allocation_tests.rs b/fuel-vm/src/interpreter/memory/allocation_tests.rs index 81236cda0..8f8e19b38 100644 --- a/fuel-vm/src/interpreter/memory/allocation_tests.rs +++ b/fuel-vm/src/interpreter/memory/allocation_tests.rs @@ -267,7 +267,6 @@ fn test_store_byte_more( assert_eq!(memory[(a + c) as usize], b as u8); } Err(e) => { - dbg!(&e); assert!(is_error); assert_eq!(e, RuntimeError::Recoverable(PanicReason::MemoryOverflow)); } From c478bda70aa76e367318074c87e294974baa0f8e Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Thu, 27 Jul 2023 08:04:43 +0300 Subject: [PATCH 10/14] Rework instruction fetch --- fuel-asm/src/encoding_tests.rs | 14 ------- fuel-asm/src/lib.rs | 7 ---- .../src/interpreter/executors/instruction.rs | 37 ++++++++----------- 3 files changed, 15 insertions(+), 43 deletions(-) diff --git a/fuel-asm/src/encoding_tests.rs b/fuel-asm/src/encoding_tests.rs index 6b990a2f7..5d7f201ce 100644 --- a/fuel-asm/src/encoding_tests.rs +++ b/fuel-asm/src/encoding_tests.rs @@ -44,18 +44,6 @@ fn opcode() { assert_eq!(instructions, instructions_from_bytes.unwrap()); - let pairs = bytes.chunks(8).map(|chunk| { - let mut arr = [0; core::mem::size_of::()]; - arr.copy_from_slice(chunk); - Word::from_be_bytes(arr) - }); - - let instructions_from_words: Vec = pairs - .into_iter() - .flat_map(raw_instructions_from_word) - .map(|raw| Instruction::try_from(raw).unwrap()) - .collect(); - #[cfg(feature = "serde")] for ins in &instructions { let ins_ser = bincode::serialize(ins).expect("Failed to serialize opcode"); @@ -63,8 +51,6 @@ fn opcode() { bincode::deserialize(&ins_ser).expect("Failed to serialize opcode"); assert_eq!(ins, &ins_de); } - - assert_eq!(instructions, instructions_from_words); } #[test] diff --git a/fuel-asm/src/lib.rs b/fuel-asm/src/lib.rs index e547f7b62..221bb808c 100644 --- a/fuel-asm/src/lib.rs +++ b/fuel-asm/src/lib.rs @@ -882,13 +882,6 @@ impl core::iter::FromIterator for Vec { } } -/// Produce two raw instructions from a word's hi and lo parts. -pub fn raw_instructions_from_word(word: Word) -> [RawInstruction; 2] { - let hi = (word >> 32) as RawInstruction; - let lo = word as RawInstruction; - [hi, lo] -} - /// Given an iterator yielding bytes, produces an iterator yielding `Instruction`s. /// /// This function assumes each consecutive 4 bytes aligns with an instruction. diff --git a/fuel-vm/src/interpreter/executors/instruction.rs b/fuel-vm/src/interpreter/executors/instruction.rs index f27565850..f2ded5bdf 100644 --- a/fuel-vm/src/interpreter/executors/instruction.rs +++ b/fuel-vm/src/interpreter/executors/instruction.rs @@ -1,5 +1,4 @@ use crate::{ - consts::*, error::{ InterpreterError, RuntimeError, @@ -33,32 +32,26 @@ where S: InterpreterStorage, Tx: ExecutableTransaction, { - /// Execute the current instruction pair located in `$m[$pc]`. + /// Execute the current instruction located in `$m[$pc]`. pub fn execute(&mut self) -> Result { - // Safety: `chunks_exact` is guaranteed to return a well-formed slice - let [hi, lo] = self.memory[self.registers[RegId::PC] as usize..] - .chunks_exact(WORD_SIZE) - .next() - .map(|b| b.try_into().expect("Has to be correct size slice")) - .map(Word::from_be_bytes) - .map(fuel_asm::raw_instructions_from_word) - .ok_or(InterpreterError::Panic(PanicReason::MemoryOverflow))?; - - // Store the expected `$pc` after executing `hi` - let pc = self.registers[RegId::PC] + Instruction::SIZE as Word; - let state = self.instruction(hi)?; - - // TODO optimize - // Should execute `lo` only if there is no rupture in the flow - that means - // either a breakpoint or some instruction that would skip `lo` such as - // `RET`, `JI` or `CALL` - if self.registers[RegId::PC] == pc && state.should_continue() { - self.instruction(lo) + if let Some(raw_instruction) = self.fetch_instruction() { + self.instruction(raw_instruction) } else { - Ok(state) + Err(InterpreterError::Panic(PanicReason::MemoryOverflow)) } } + /// Reads the current instruction located in `$m[$pc]`, + /// returning `None` on any memory access violation. + fn fetch_instruction(&self) -> Option { + let start: usize = self.registers[RegId::PC].try_into().ok()?; + let end = start.checked_add(Instruction::SIZE)?; + let bytes = self.memory.get(start..end)?; + Some(RawInstruction::from_be_bytes( + bytes.try_into().expect("Slice len mismatch"), + )) + } + /// Execute a provided instruction pub fn instruction + Copy>( &mut self, From e5aee88da941a3ea279cc272f17216f6a8277661 Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Thu, 27 Jul 2023 08:16:29 +0300 Subject: [PATCH 11/14] Update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f7fd2c742..4c522ef23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - [#529](https://github.com/FuelLabs/fuel-vm/pull/529): Fix WASM initialization for NPM wrapper packages. +- [#511](https://github.com/FuelLabs/fuel-vm/pull/511): Changes multiple panic reasons to be more accurate, and internally refactors instruction fetch logic to be less error-prone. + #### Breaking - [#527](https://github.com/FuelLabs/fuel-vm/pull/527): The balances are empty during predicate estimation/verification. From 84cfbcac19e58729ff6d39c596b248875e005b01 Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Fri, 28 Jul 2023 00:13:14 +0300 Subject: [PATCH 12/14] Remove MEM_MAX_ACCESS_SIZE --- fuel-asm/src/panic_reason.rs | 4 ++-- fuel-vm/src/consts.rs | 3 --- fuel-vm/src/interpreter/blockchain.rs | 12 ---------- fuel-vm/src/interpreter/flow.rs | 3 --- fuel-vm/src/interpreter/flow/ret_tests.rs | 2 +- fuel-vm/src/interpreter/log.rs | 5 ----- fuel-vm/src/interpreter/memory.rs | 22 ++++--------------- .../interpreter/memory/allocation_tests.rs | 6 ++--- 8 files changed, 10 insertions(+), 47 deletions(-) diff --git a/fuel-asm/src/panic_reason.rs b/fuel-asm/src/panic_reason.rs index 858e0b18e..13c94f0fc 100644 --- a/fuel-asm/src/panic_reason.rs +++ b/fuel-asm/src/panic_reason.rs @@ -74,8 +74,8 @@ enum_from! { InvalidImmediateValue = 0x13, /// The provided transaction input is not of type `Coin`. ExpectedCoinInput = 0x14, - /// The requested memory access exceeds the limits of the interpreter. - MaxMemoryAccess = 0x15, + /// This entry is no longer used, and can be repurposed. + Unused0x15 = 0x15, /// Two segments of the interpreter memory should not intersect for write operations. MemoryWriteOverlap = 0x16, /// The requested contract is not listed in the transaction inputs. diff --git a/fuel-vm/src/consts.rs b/fuel-vm/src/consts.rs index 72516dfd5..5e8171007 100644 --- a/fuel-vm/src/consts.rs +++ b/fuel-vm/src/consts.rs @@ -35,9 +35,6 @@ pub const VM_MAX_RAM: u64 = 1024 * 1024 * FUEL_MAX_MEMORY_SIZE; /// Size of the VM memory, in bytes. pub const MEM_SIZE: usize = VM_MAX_RAM as usize; -/// Maximum memory access size, in bytes. -pub const MEM_MAX_ACCESS_SIZE: u64 = VM_MAX_RAM; - /// Tighter of the two bounds for VM_MAX_RAM and usize::MAX pub const MIN_VM_MAX_RAM_USIZE_MAX: u64 = if VM_MAX_RAM < usize::MAX as u64 { VM_MAX_RAM diff --git a/fuel-vm/src/interpreter/blockchain.rs b/fuel-vm/src/interpreter/blockchain.rs index 18ca76c23..3687f099d 100644 --- a/fuel-vm/src/interpreter/blockchain.rs +++ b/fuel-vm/src/interpreter/blockchain.rs @@ -494,10 +494,6 @@ impl<'vm, S, I> LoadContractCodeCtx<'vm, S, I> { return Err(PanicReason::MemoryOverflow.into()) } - if length > MEM_MAX_ACCESS_SIZE as usize { - return Err(PanicReason::MaxMemoryAccess.into()) - } - // Clear memory self.memory[dst_range.usizes()].fill(0); @@ -667,10 +663,6 @@ impl<'vm, S, I> CodeCopyCtx<'vm, S, I> { MemoryRange::new(a, d)?; let c_range = MemoryRange::new(c, d)?; - if d > MEM_MAX_ACCESS_SIZE { - return Err(PanicReason::MaxMemoryAccess.into()) - } - let contract = ContractId::from_bytes_ref(contract.read(self.memory)); self.input_contracts.check(contract)?; @@ -940,10 +932,6 @@ where self.recipient_mem_address, )?; - if self.msg_data_len > MEM_MAX_ACCESS_SIZE { - return Err(RuntimeError::Recoverable(PanicReason::MaxMemoryAccess)) - } - if self.msg_data_len > self.max_message_data_length { return Err(RuntimeError::Recoverable(PanicReason::MessageDataTooLong)) } diff --git a/fuel-vm/src/interpreter/flow.rs b/fuel-vm/src/interpreter/flow.rs index 6d20cfe3d..0b99005b1 100644 --- a/fuel-vm/src/interpreter/flow.rs +++ b/fuel-vm/src/interpreter/flow.rs @@ -229,9 +229,6 @@ impl RetCtx<'_> { pub(crate) fn ret_data(self, a: Word, b: Word) -> Result { let range = MemoryRange::new(a, b)?; - if b > MEM_MAX_ACCESS_SIZE { - return Err(PanicReason::MaxMemoryAccess.into()) - } let receipt = Receipt::return_data( self.current_contract.unwrap_or_else(ContractId::zeroed), diff --git a/fuel-vm/src/interpreter/flow/ret_tests.rs b/fuel-vm/src/interpreter/flow/ret_tests.rs index 7c863b87f..c59beb93d 100644 --- a/fuel-vm/src/interpreter/flow/ret_tests.rs +++ b/fuel-vm/src/interpreter/flow/ret_tests.rs @@ -102,7 +102,7 @@ fn test_return() { &mut memory, &mut context, ) - .ret_data(0, MEM_MAX_ACCESS_SIZE + 1); + .ret_data(0, VM_MAX_RAM + 1); assert_eq!( r, Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)) diff --git a/fuel-vm/src/interpreter/log.rs b/fuel-vm/src/interpreter/log.rs index 9c27ef975..f8d789da0 100644 --- a/fuel-vm/src/interpreter/log.rs +++ b/fuel-vm/src/interpreter/log.rs @@ -17,7 +17,6 @@ use crate::{ error::RuntimeError, }; -use fuel_asm::PanicReason; use fuel_tx::{ Receipt, Script, @@ -129,10 +128,6 @@ impl LogInput<'_> { ) -> Result<(), RuntimeError> { let range = MemoryRange::new(c, d)?; - if d > MEM_MAX_ACCESS_SIZE { - return Err(PanicReason::MaxMemoryAccess.into()) - } - let receipt = Receipt::log_data( internal_contract_or_default(self.context, self.fp, self.memory), a, diff --git a/fuel-vm/src/interpreter/memory.rs b/fuel-vm/src/interpreter/memory.rs index 661d0f9a8..0bad779d2 100644 --- a/fuel-vm/src/interpreter/memory.rs +++ b/fuel-vm/src/interpreter/memory.rs @@ -391,12 +391,8 @@ pub(crate) fn memclear( ) -> Result<(), RuntimeError> { let range = MemoryRange::new(a, b)?; owner.verify_ownership(&range)?; - if b > MEM_MAX_ACCESS_SIZE { - Err(PanicReason::MaxMemoryAccess.into()) - } else { - memory[range.usizes()].fill(0); - inc_pc(pc) - } + memory[range.usizes()].fill(0); + inc_pc(pc) } pub(crate) fn memcopy( @@ -410,10 +406,6 @@ pub(crate) fn memcopy( let dst_range = MemoryRange::new(a, c)?; let src_range = MemoryRange::new(b, c)?; - if c > MEM_MAX_ACCESS_SIZE { - return Err(PanicReason::MaxMemoryAccess.into()) - } - owner.verify_ownership(&dst_range)?; if dst_range.start <= src_range.start && src_range.start < dst_range.end @@ -446,14 +438,8 @@ pub(crate) fn memeq( ) -> Result<(), RuntimeError> { let range1 = MemoryRange::new(b, d)?; let range2 = MemoryRange::new(c, d)?; - - if d > MEM_MAX_ACCESS_SIZE { - Err(PanicReason::MaxMemoryAccess.into()) - } else { - *result = (memory[range1.usizes()] == memory[range2.usizes()]) as Word; - - inc_pc(pc) - } + *result = (memory[range1.usizes()] == memory[range2.usizes()]) as Word; + inc_pc(pc) } #[derive(Debug)] diff --git a/fuel-vm/src/interpreter/memory/allocation_tests.rs b/fuel-vm/src/interpreter/memory/allocation_tests.rs index 8f8e19b38..4c09eac31 100644 --- a/fuel-vm/src/interpreter/memory/allocation_tests.rs +++ b/fuel-vm/src/interpreter/memory/allocation_tests.rs @@ -22,7 +22,7 @@ fn test_malloc(mut hp: Word, sp: Word, a: Word) -> Result { #[test_case(false, 1, 10 => Err(RuntimeError::Recoverable(PanicReason::MemoryOwnership)); "No ownership")] #[test_case(true, 0, 10 => Ok(()); "Memory range starts at 0")] #[test_case(true, MEM_SIZE as Word - 10, 10 => Ok(()); "Memory range ends at last address")] -#[test_case(true, 1, MEM_MAX_ACCESS_SIZE + 1 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Memory range size exceeds limit")] +#[test_case(true, 1, VM_MAX_RAM + 1 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Memory range size exceeds limit")] fn test_memclear(has_ownership: bool, a: Word, b: Word) -> Result<(), RuntimeError> { let mut memory: Memory = vec![1u8; MEM_SIZE].try_into().unwrap(); let mut pc = 4; @@ -92,11 +92,11 @@ fn test_memcopy( #[test_case(1, 20, 10 => Ok(()); "Can compare some bytes")] #[test_case(MEM_SIZE as Word, 1, 2 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "b+d > MAX_RAM")] #[test_case(1, MEM_SIZE as Word, 2 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "c+d > MAX_RAM")] -#[test_case(1, 1, MEM_MAX_ACCESS_SIZE + 1 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "d > MEM_MAX_ACCESS_SIZE")] +#[test_case(1, 1, VM_MAX_RAM + 1 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "d > VM_MAX_RAM")] #[test_case(u64::MAX/2, 1, u64::MAX/2 + 1 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "b+d overflows")] #[test_case(1, u64::MAX/2, u64::MAX/2 + 1 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "c+d overflows")] #[test_case(0, 0, 0 => Ok(()); "smallest input values")] -#[test_case(0, MEM_MAX_ACCESS_SIZE/2, MEM_MAX_ACCESS_SIZE/2 => Ok(()); "maximum range of addressable memory")] +#[test_case(0, VM_MAX_RAM/2, VM_MAX_RAM/2 => Ok(()); "maximum range of addressable memory")] fn test_memeq(b: Word, c: Word, d: Word) -> Result<(), RuntimeError> { let mut memory: Memory = vec![1u8; MEM_SIZE].try_into().unwrap(); let r = (b as usize).min(MEM_SIZE) From f0e53676d9bc642bed2719d0d8f500ef3b852071 Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Fri, 28 Jul 2023 00:19:09 +0300 Subject: [PATCH 13/14] Remove MIN_VM_MAX_RAM_USIZE_MAX --- fuel-vm/Cargo.toml | 1 + fuel-vm/src/consts.rs | 7 +------ 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/fuel-vm/Cargo.toml b/fuel-vm/Cargo.toml index 247a47567..0c97a25b8 100644 --- a/fuel-vm/Cargo.toml +++ b/fuel-vm/Cargo.toml @@ -30,6 +30,7 @@ primitive-types = { version = "0.12", default-features = false } rand = { version = "0.8", optional = true } serde = { version = "1.0", features = ["derive", "rc"], optional = true } sha3 = "0.10" +static_assertions = "1.1" strum = { version = "0.24", features = ["derive"], optional = true } tai64 = "4.0" thiserror = "1.0" diff --git a/fuel-vm/src/consts.rs b/fuel-vm/src/consts.rs index 5e8171007..619966589 100644 --- a/fuel-vm/src/consts.rs +++ b/fuel-vm/src/consts.rs @@ -35,12 +35,7 @@ pub const VM_MAX_RAM: u64 = 1024 * 1024 * FUEL_MAX_MEMORY_SIZE; /// Size of the VM memory, in bytes. pub const MEM_SIZE: usize = VM_MAX_RAM as usize; -/// Tighter of the two bounds for VM_MAX_RAM and usize::MAX -pub const MIN_VM_MAX_RAM_USIZE_MAX: u64 = if VM_MAX_RAM < usize::MAX as u64 { - VM_MAX_RAM -} else { - usize::MAX as u64 -}; +static_assertions::const_assert!(VM_MAX_RAM < usize::MAX as u64); // no limits to heap for now. From 5656e0df719e5e487bcedb6eb4c51a0412394ebf Mon Sep 17 00:00:00 2001 From: green Date: Sun, 6 Aug 2023 12:53:55 +0100 Subject: [PATCH 14/14] Fix compilation errors --- fuel-vm/src/interpreter/contract.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/fuel-vm/src/interpreter/contract.rs b/fuel-vm/src/interpreter/contract.rs index 749464802..24a9cc54f 100644 --- a/fuel-vm/src/interpreter/contract.rs +++ b/fuel-vm/src/interpreter/contract.rs @@ -52,7 +52,6 @@ use fuel_types::{ ContractId, }; -use crate::interpreter::memory::read_bytes; use std::borrow::Cow; #[cfg(test)]