Return correct PanicReason on memory-related panics (#511)

* Return correct PanicReason on write_bytes without ownership * Fix and clarify MemoryOverflow errors * Further simplify memory management code * One last MaxAccessSize fix * Undo some formatting from too new rustfmt * More fixes * Add more edge case tests for store_byte * Simplify store_byte implementation * Remove dbg macro * Rework instruction fetch * Update changelog * Remove MEM_MAX_ACCESS_SIZE * Remove MIN_VM_MAX_RAM_USIZE_MAX * Fix compilation errors --------- Co-authored-by: green <xgreenx9999@gmail.com>
FuelLabs · Aug 6, 2023 · e24f188 · e24f188
1 parent 75022cb
commit e24f188
Show file tree

Hide file tree

Showing 20 changed files with 212 additions and 290 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -23,6 +23,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 - [#529](https://github.com/FuelLabs/fuel-vm/pull/529) [#534](https://github.com/FuelLabs/fuel-vm/pull/534): Enforcing async WASM initialization for all 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.

diff --git a/fuel-asm/src/encoding_tests.rs b/fuel-asm/src/encoding_tests.rs
@@ -44,27 +44,13 @@ fn opcode() {
 
     assert_eq!(instructions, instructions_from_bytes.unwrap());
 
-    let pairs = bytes.chunks(8).map(|chunk| {
-        let mut arr = [0; core::mem::size_of::<Word>()];
-        arr.copy_from_slice(chunk);
-        Word::from_be_bytes(arr)
-    });
-
-    let instructions_from_words: Vec<Instruction> = 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");
         let ins_de: Instruction =
             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
@@ -883,13 +883,6 @@ impl core::iter::FromIterator<Instruction> for Vec<u32> {
     }
 }
 
-/// 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-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/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
@@ -35,15 +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;
 
-/// 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
-} else {
-    usize::MAX as u64
-};
+static_assertions::const_assert!(VM_MAX_RAM < usize::MAX as u64);
 
 // no limits to heap for now.
 

diff --git 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;
@@ -487,39 +479,28 @@ 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)?;
+        if dst_range.end >= *self.hp as usize {
+            // Would make stack and heap overlap
+            return Err(PanicReason::MemoryOverflow.into())
+        }
 
-        // 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 length > self.contract_max_size as usize {
             return Err(PanicReason::MemoryOverflow.into())
         }
 
         // Clear memory
-        self.memory[memory_offset..memory_offset_end].fill(0);
+        self.memory[dst_range.usizes()].fill(0);
 
-        // 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.");
-
-        // Safety: Memory bounds are checked and consistent
-        let contract_id = ContractId::from_bytes_ref(contract_id);
-
-        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() {
@@ -531,10 +512,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);
@@ -550,23 +533,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)
@@ -674,28 +659,25 @@ 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())
-        }
-
-        let (a, c, d) = (a as usize, c as usize, d as usize);
-        let cd = cd as usize;
+        MemoryRange::new(a, d)?;
+        let c_range = MemoryRange::new(c, d)?;
 
         let contract = ContractId::from_bytes_ref(contract.read(self.memory));
 
         self.input_contracts.check(contract)?;
 
         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)
@@ -759,13 +741,9 @@ impl<'vm, S, I: Iterator<Item = &'vm ContractId>> 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)?;
@@ -954,10 +932,6 @@ where
             self.recipient_mem_address,
         )?;
 
-        if self.msg_data_len > MEM_MAX_ACCESS_SIZE {
-            return Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow))
-        }
-
         if self.msg_data_len > self.max_message_data_length {
             return Err(RuntimeError::Recoverable(PanicReason::MessageDataTooLong))
         }
@@ -1012,11 +986,9 @@ where
 }
 
 struct StateReadQWord {
-    /// The destination memory address is
-    /// stored in this range of memory.
-    destination_address_memory_range: Range<usize>,
-    /// 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,
@@ -1029,28 +1001,16 @@ impl StateReadQWord {
         num_slots: Word,
         ownership_registers: OwnershipRegisters,
     ) -> Result<Self, RuntimeError> {
-        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,
         })
@@ -1083,20 +1043,18 @@ 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)?;
 
     Ok(())
 }
 
 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<usize>,
+    /// The source data memory address is stored in this range of memory.
+    source_address_memory_range: MemoryRange,
 }
 
 impl StateWriteQWord {
@@ -1105,20 +1063,18 @@ impl StateWriteQWord {
         source_memory_address: Word,
         num_slots: Word,
     ) -> Result<Self, RuntimeError> {
-        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,
         })
     }
@@ -1135,7 +1091,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();