Apply suggestion from the review.

Also updated teh blobs logic to use the `read` function as well.

fuel-vm/src/interpreter/blob.rs

@@ -2,13 +2,15 @@ use fuel_asm::{
     RegisterId,
     Word,
 };
-use fuel_storage::StorageSize;
 use fuel_tx::PanicReason;
 use fuel_types::BlobId;
 
 use crate::{
     error::IoResult,
-    prelude::*,
+    interpreter::{
+        contract::blob_size,
+        memory::copy_from_storage_zero_fill,
+    },
     storage::{
         BlobData,
         InterpreterStorage,
@@ -17,7 +19,6 @@ use crate::{
 
 use super::{
     internal::inc_pc,
-    memory::copy_from_slice_zero_fill,
     split_registers,
     GetRegMut,
     Interpreter,
@@ -46,9 +47,7 @@ where
 
         let blob_id = BlobId::from(self.memory.as_ref().read_bytes(blob_id_ptr)?);
 
-        let size = <S as StorageSize<BlobData>>::size_of_value(&self.storage, &blob_id)
-            .map_err(RuntimeError::Storage)?
-            .ok_or(PanicReason::BlobNotFound)?;
+        let size = blob_size(&self.storage, &blob_id)?;
 
         self.dependent_gas_charge_without_base(gas_cost, size as Word)?;
         let (SystemRegisters { pc, .. }, mut w) = split_registers(&mut self.registers);
@@ -74,23 +73,19 @@ where
         let blob_id = BlobId::from(self.memory.as_ref().read_bytes(blob_id_ptr)?);
         let owner = self.ownership_registers();
 
-        let size = <S as StorageSize<BlobData>>::size_of_value(&self.storage, &blob_id)
-            .map_err(RuntimeError::Storage)?
-            .ok_or(PanicReason::BlobNotFound)?;
-        self.dependent_gas_charge_without_base(gas_cost, len.max(size as Word))?;
-
-        let blob = <S as StorageInspect<BlobData>>::get(&self.storage, &blob_id)
-            .map_err(RuntimeError::Storage)?
-            .ok_or(PanicReason::BlobNotFound)?;
-        let blob = blob.as_ref().as_ref();
+        let blob_len = blob_size(&self.storage, &blob_id)?;
+        let charge_len = len.max(blob_len as Word);
+        self.dependent_gas_charge_without_base(gas_cost, charge_len)?;
 
-        copy_from_slice_zero_fill(
+        copy_from_storage_zero_fill::<BlobData, _>(
             self.memory.as_mut(),
             owner,
-            blob,
+            &self.storage,
             dst_ptr,
-            blob_offset,
             len,
+            &blob_id,
+            blob_offset,
+            blob_len,
         )?;
 
         Ok(inc_pc(self.registers.pc_mut())?)

fuel-vm/src/interpreter/blockchain.rs

@@ -13,6 +13,7 @@ use crate::{
         contract::{
             balance,
             balance_decrease,
+            blob_size,
             contract_size,
         },
         gas::{
@@ -28,7 +29,7 @@ use crate::{
             tx_id,
         },
         memory::{
-            copy_from_slice_zero_fill,
+            copy_from_storage_zero_fill,
             OwnershipRegisters,
         },
         receipts::ReceiptsCtx,
@@ -53,10 +54,7 @@ use fuel_asm::{
     Imm06,
     PanicReason,
 };
-use fuel_storage::{
-    StorageInspect,
-    StorageSize,
-};
+use fuel_storage::StorageSize;
 use fuel_tx::{
     consts::BALANCE_ENTRY_SIZE,
     BlobId,
@@ -648,25 +646,16 @@ where
         *self.sp = new_sp;
         *self.ssp = new_sp;
 
-        // Get the slice where to read the contract code into
-        let contract_buffer = self.memory.write(owner, region_start, length)?;
-
-        if contract_offset >= contract_len.into() {
-            contract_buffer[..].fill(0);
-        } else {
-            // The conract offset is within the contract bounds, which
-            // is guaranteed to be at most `u32::MAX`. As such, it is safe
-            // to convert `contract_offset` to `usize`, which is required
-            // to invoke `self.storage.read_contract`. Furthermore,
-            // the call to `self.storage.read_contract` cannot fail with
-            // error `StorageError::`
-            #[allow(clippy::cast_possible_truncation)]
-            self.storage
-                .read_contract(&contract_id, contract_offset as usize, contract_buffer)
-                .transpose()
-                .ok_or(PanicReason::ContractNotFound)?
-                .map_err(RuntimeError::Storage)?;
-        }
+        copy_from_storage_zero_fill::<ContractsRawCode, _>(
+            self.memory,
+            owner,
+            self.storage,
+            region_start,
+            length,
+            &contract_id,
+            contract_offset,
+            contract_len,
+        )?;
 
         // Update frame code size, if we have a stack frame (i.e. fp > 0)
         if self.context.is_internal() {
@@ -719,10 +708,7 @@ where
 
         let length = bytes::padded_len_word(length_unpadded).unwrap_or(Word::MAX);
 
-        let blob_len =
-            <S as StorageSize<BlobData>>::size_of_value(self.storage, &blob_id)
-                .map_err(RuntimeError::Storage)?
-                .ok_or(PanicReason::BlobNotFound)?;
+        let blob_len = blob_size(self.storage, &blob_id)?;
 
         // Fetch the storage blob
         let profiler = ProfileGas {
@@ -739,10 +725,6 @@ where
             self.gas_cost,
             charge_len,
         )?;
-        let blob = <S as StorageInspect<BlobData>>::get(self.storage, &blob_id)
-            .map_err(RuntimeError::Storage)?
-            .ok_or(PanicReason::BlobNotFound)?;
-        let blob_bytes = blob.as_ref().as_ref();
 
         let new_sp = ssp.saturating_add(length);
         self.memory.grow_stack(new_sp)?;
@@ -756,13 +738,15 @@ where
         *self.ssp = new_sp;
 
         // Copy the code.
-        copy_from_slice_zero_fill(
+        copy_from_storage_zero_fill::<BlobData, _>(
             self.memory,
             owner,
-            blob_bytes,
+            self.storage,
             region_start,
-            blob_offset,
             length,
+            &blob_id,
+            blob_offset,
+            blob_len,
         )?;
 
         // Update frame code size, if we have a stack frame (i.e. fp > 0)
@@ -1002,7 +986,6 @@ where
         self.memory.write(self.owner, dst_addr, length)?;
         self.input_contracts.check(&contract_id)?;
 
-        let contract_bytes = self.memory.write(self.owner, dst_addr, length)?;
         let contract_len = contract_size(&self.storage, &contract_id)?;
         let charge_len = core::cmp::max(contract_len as u64, length);
         let profiler = ProfileGas {
@@ -1019,22 +1002,16 @@ where
             charge_len,
         )?;
 
-        if contract_offset >= contract_len.into() {
-            contract_bytes[..].fill(0);
-        } else {
-            // The conract offset is within the contract bounds, which
-            // is guaranteed to be at most `u32::MAX`. As such, it is safe
-            // to convert `contract_offset` to `usize`, which is required
-            // to invoke `self.storage.read_contract`. Furthermore,
-            // the call to `self.storage.read_contract` cannot fail with
-            // error `StorageError::`
-            #[allow(clippy::cast_possible_truncation)]
-            self.storage
-                .read_contract(&contract_id, contract_offset as usize, contract_bytes)
-                .transpose()
-                .ok_or(PanicReason::ContractNotFound)?
-                .map_err(RuntimeError::Storage)?;
-        }
+        copy_from_storage_zero_fill::<ContractsRawCode, _>(
+            self.memory,
+            self.owner,
+            self.storage,
+            dst_addr,
+            length,
+            &contract_id,
+            contract_offset,
+            contract_len,
+        )?;
 
         Ok(inc_pc(self.pc)?)
     }

fuel-vm/src/interpreter/contract.rs

@@ -32,6 +32,7 @@ use crate::{
     },
     prelude::Profiler,
     storage::{
+        BlobData,
         ContractsAssetsStorage,
         ContractsRawCode,
         InterpreterStorage,
@@ -50,6 +51,7 @@ use fuel_tx::{
 use fuel_types::{
     Address,
     AssetId,
+    BlobId,
     Bytes32,
     ContractId,
 };
@@ -369,15 +371,26 @@ impl<'vm, S, Tx> TransferCtx<'vm, S, Tx> {
 pub(crate) fn contract_size<S>(
     storage: &S,
     contract: &ContractId,
-) -> IoResult<u32, S::Error>
+) -> IoResult<usize, S::Error>
 where
     S: StorageSize<ContractsRawCode> + ?Sized,
 {
     let size = storage
         .size_of_value(contract)
         .map_err(RuntimeError::Storage)?
         .ok_or(PanicReason::ContractNotFound)?;
-    Ok(u32::try_from(size).map_err(|_| PanicReason::MemoryOverflow)?)
+    Ok(size)
+}
+
+pub(crate) fn blob_size<S>(storage: &S, blob_id: &BlobId) -> IoResult<usize, S::Error>
+where
+    S: StorageSize<BlobData> + ?Sized,
+{
+    let size = storage
+        .size_of_value(blob_id)
+        .map_err(RuntimeError::Storage)?
+        .ok_or(PanicReason::BlobNotFound)?;
+    Ok(size)
 }
 
 pub(crate) fn balance<S>(

fuel-vm/src/interpreter/memory.rs

@@ -35,10 +35,18 @@ use core::ops::{
     RangeTo,
 };
 
+use crate::error::{
+    IoResult,
+    RuntimeError,
+};
 use alloc::{
     vec,
     vec::Vec,
 };
+use fuel_storage::{
+    Mappable,
+    StorageRead,
+};
 
 #[cfg(test)]
 mod tests;
@@ -1027,34 +1035,46 @@ impl OwnershipRegisters {
     }
 }
 
-/// Attempt copy from slice to memory, filling zero bytes when exceeding slice boundaries.
-/// Performs overflow and memory range checks, but no ownership checks.
+/// Attempt copy from the storage to memory, filling zero bytes when exceeding slice
+/// boundaries. Performs overflow and memory range checks, but no ownership checks.
 /// Note that if `src_offset` is larger than `src.len()`, the whole range will be
 /// zero-filled.
-pub(crate) fn copy_from_slice_zero_fill<A: ToAddr, B: ToAddr>(
+#[allow(clippy::too_many_arguments)]
+pub(crate) fn copy_from_storage_zero_fill<M, S>(
     memory: &mut MemoryInstance,
     owner: OwnershipRegisters,
-    src: &[u8],
-    dst_addr: A,
-    src_offset: Word,
-    len: B,
-) -> SimpleResult<()> {
-    let range = memory.write(owner, dst_addr, len)?;
+    storage: &S,
+    dst_addr: Word,
+    dst_len: Word,
+    src_id: &M::Key,
+    src_offset: u64,
+    src_len: usize,
+) -> IoResult<(), S::Error>
+where
+    M: Mappable,
+    S: StorageRead<M>,
+{
+    let write_buffer = memory.write(owner, dst_addr, dst_len)?;
+    let mut empty_offset = 0;
 
-    // Special-case the ranges that are completely out of bounds,
-    // to avoid platform-dependenct usize conversion.
-    if src_offset >= src.len() as Word {
-        range[..].fill(0);
-    } else {
-        // Safety: since we check above that this is not larger than `src.len()`,
-        // which is `usize`, the cast never truncates.
-        #[allow(clippy::cast_possible_truncation)]
-        let src_offset = src_offset as usize;
-        let src_end = src_offset.saturating_add(range.len()).min(src.len());
-        let data = src.get(src_offset..src_end).unwrap_or(&[]);
-
-        range[..data.len()].copy_from_slice(data);
-        range[data.len()..].fill(0);
+    if src_offset < src_len as Word {
+        let src_offset =
+            u32::try_from(src_offset).map_err(|_| PanicReason::MemoryOverflow)?;
+
+        let src_read_length = src_len.saturating_sub(src_offset as usize);
+        let src_read_length = src_read_length.min(write_buffer.len());
+
+        let (src_read_buffer, _) = write_buffer.split_at_mut(src_read_length);
+        storage
+            .read(src_id, src_offset as usize, src_read_buffer)
+            .transpose()
+            .ok_or(PanicReason::ContractNotFound)?
+            .map_err(RuntimeError::Storage)?;
+
+        empty_offset = src_read_length;
     }
+
+    write_buffer[empty_offset..].fill(0);
+
     Ok(())
 }

fuel-vm/src/interpreter/memory/tests.rs

@@ -7,6 +7,7 @@ use super::*;
 use crate::{
     interpreter::InterpreterParams,
     prelude::*,
+    storage::ContractsRawCode,
 };
 use fuel_asm::op;
 use fuel_tx::ConsensusParameters;
@@ -268,20 +269,30 @@ fn test_mem_write(
 #[test_case(1, 2, 1, &[] => (true, [0xff, 0, 0, 0xff, 0xff]))]
 #[test_case(1, 2, 2, &[] => (true, [0xff, 0, 0, 0xff, 0xff]))]
 #[test_case(1, 2, 3, &[] => (true, [0xff, 0, 0, 0xff, 0xff]))]
-fn test_copy_from_slice_zero_fill(
+fn test_copy_from_storage_zero_fill(
     addr: usize,
     len: usize,
     src_offset: Word,
     src_data: &[u8],
 ) -> (bool, [u8; 5]) {
+    let contract_id = ContractId::zeroed();
+    let contract = Contract::from(src_data);
+    let contract_size = src_data.len();
+    let mut storage = MemoryStorage::default();
+    storage
+        .storage_contract_insert(&contract_id, &contract)
+        .unwrap();
+
     let mut memory: MemoryInstance = vec![0xffu8; MEM_SIZE].try_into().unwrap();
-    let r = copy_from_slice_zero_fill(
+    let r = copy_from_storage_zero_fill::<ContractsRawCode, _>(
         &mut memory,
         OwnershipRegisters::test_full_stack(),
-        src_data,
-        addr,
+        &storage,
+        addr as Word,
+        len as Word,
+        &contract_id,
         src_offset,
-        len,
+        contract_size,
     )
     .is_ok();
     let memory: [u8; 5] = memory[..5].try_into().unwrap();