Remove out of bounds check in exec_v2

script/src/scheduler.rs

@@ -1,7 +1,7 @@
 use crate::cost_model::transferred_byte_cycles;
 use crate::syscalls::{
-    EXEC_LOAD_ELF_V2_CYCLES_BASE, INVALID_FD, MAX_FDS_CREATED, MAX_VMS_SPAWNED, OTHER_END_CLOSED,
-    SPAWN_EXTRA_CYCLES_BASE, SUCCESS, WAIT_FAILURE,
+    EXEC_LOAD_ELF_V2_CYCLES_BASE, INDEX_OUT_OF_BOUND, INVALID_FD, MAX_FDS_CREATED, MAX_VMS_SPAWNED,
+    OTHER_END_CLOSED, SPAWN_EXTRA_CYCLES_BASE, SUCCESS, WAIT_FAILURE,
 };
 use crate::types::MachineContext;
 use crate::verify::TransactionScriptsSyscallsGenerator;
@@ -15,6 +15,7 @@ use ckb_traits::{CellDataProvider, ExtensionProvider, HeaderProvider};
 use ckb_types::core::Cycle;
 use ckb_vm::snapshot2::Snapshot2Context;
 use ckb_vm::{
+    bytes::Bytes,
     cost_model::estimate_cycles,
     elf::parse_elf,
     machine::{CoreMachine, DefaultMachineBuilder, Pause, SupportMachine},
@@ -347,7 +348,7 @@ where
         for message in messages {
             match message {
                 Message::ExecV2(vm_id, args) => {
-                    let (_, old_machine) = self
+                    let (old_context, old_machine) = self
                         .instantiated
                         .get_mut(&vm_id)
                         .ok_or_else(|| Error::Unexpected("Unable to find VM Id".to_string()))?;
@@ -356,13 +357,33 @@ where
                         .add_cycles_no_checking(EXEC_LOAD_ELF_V2_CYCLES_BASE)?;
                     let old_cycles = old_machine.machine.cycles();
                     let max_cycles = old_machine.machine.max_cycles();
+                    let (program, _full_length) = {
+                        let mut sc = old_context.snapshot2_context().lock().expect("lock");
+                        match sc.load_data(
+                            &args.location.data_piece_id,
+                            args.location.offset,
+                            args.location.length,
+                        ) {
+                            Ok(val) => val,
+                            Err(Error::SnapshotDataLoadError) => {
+                                // This comes from TxData results in an out of bound error, to
+                                // mimic current behavior, we would return INDEX_OUT_OF_BOUND error.
+                                old_machine
+                                    .machine
+                                    .set_register(A0, INDEX_OUT_OF_BOUND as u64);
+                                continue;
+                            }
+                            Err(e) => return Err(e),
+                        }
+                    };
                     let (context, mut new_machine) = self.create_dummy_vm(&vm_id)?;
                     new_machine.set_max_cycles(max_cycles);
                     new_machine.machine.add_cycles_no_checking(old_cycles)?;
                     self.load_vm_program(
                         &context,
                         &mut new_machine,
                         &args.location,
+                        program,
                         Some((vm_id, args.argc, args.argv)),
                     )?;
                     // The insert operation removes the old vm instance and adds the new vm instance.
@@ -781,7 +802,11 @@ where
         let id = self.next_vm_id;
         self.next_vm_id += 1;
         let (context, mut machine) = self.create_dummy_vm(&id)?;
-        self.load_vm_program(&context, &mut machine, location, args)?;
+        let (program, _) = {
+            let mut sc = context.snapshot2_context().lock().expect("lock");
+            sc.load_data(&location.data_piece_id, location.offset, location.length)?
+        };
+        self.load_vm_program(&context, &mut machine, location, program, args)?;
         // Newly booted VM will be instantiated by default
         while self.instantiated.len() >= MAX_INSTANTIATED_VMS {
             // Instantiated is a BTreeMap, first_entry will maintain key order
@@ -804,11 +829,9 @@ where
         context: &MachineContext<DL>,
         machine: &mut Machine,
         location: &DataLocation,
+        program: Bytes,
         args: Option<(u64, u64, u64)>,
     ) -> Result<u64, Error> {
-        let mut sc = context.snapshot2_context().lock().expect("lock");
-        let (program, _) =
-            sc.load_data(&location.data_piece_id, location.offset, location.length)?;
         let metadata = parse_elf::<u64>(&program, machine.machine.version())?;
         let bytes = match args {
             Some((vm_id, argc, argv)) => {
@@ -818,6 +841,7 @@ where
             }
             None => machine.load_program_with_metadata(&program, &metadata, vec![].into_iter())?,
         };
+        let mut sc = context.snapshot2_context().lock().expect("lock");
         sc.mark_program(
             &mut machine.machine,
             &metadata,

script/src/syscalls/exec_v2.rs

@@ -1,46 +1,27 @@
 use crate::syscalls::{
-    Place, Source, EXEC, INDEX_OUT_OF_BOUND, SLICE_OUT_OF_BOUND, SOURCE_ENTRY_MASK,
-    SOURCE_GROUP_FLAG,
+    Place, Source, EXEC, INDEX_OUT_OF_BOUND, SOURCE_ENTRY_MASK, SOURCE_GROUP_FLAG,
 };
-use crate::types::{DataLocation, DataPieceId, ExecV2Args, Message, TxData, VmId};
-use ckb_traits::{CellDataProvider, ExtensionProvider, HeaderProvider};
+use crate::types::{DataLocation, DataPieceId, ExecV2Args, Message, VmId};
 use ckb_vm::{
     registers::{A0, A1, A2, A3, A4, A5, A7},
-    snapshot2::Snapshot2Context,
     Error as VMError, Register, SupportMachine, Syscalls,
 };
 use std::sync::{Arc, Mutex};
 
-pub struct ExecV2<DL>
-where
-    DL: CellDataProvider + HeaderProvider + ExtensionProvider + Send + Sync + Clone + 'static,
-{
+pub struct ExecV2 {
     id: VmId,
     message_box: Arc<Mutex<Vec<Message>>>,
-    snapshot2_context: Arc<Mutex<Snapshot2Context<DataPieceId, TxData<DL>>>>,
 }
 
-impl<DL> ExecV2<DL>
-where
-    DL: CellDataProvider + HeaderProvider + ExtensionProvider + Send + Sync + Clone + 'static,
-{
-    pub fn new(
-        id: VmId,
-        message_box: Arc<Mutex<Vec<Message>>>,
-        snapshot2_context: Arc<Mutex<Snapshot2Context<DataPieceId, TxData<DL>>>>,
-    ) -> ExecV2<DL> {
-        ExecV2 {
-            id,
-            message_box,
-            snapshot2_context,
-        }
+impl ExecV2 {
+    pub fn new(id: VmId, message_box: Arc<Mutex<Vec<Message>>>) -> ExecV2 {
+        ExecV2 { id, message_box }
     }
 }
 
-impl<Mac, DL> Syscalls<Mac> for ExecV2<DL>
+impl<Mac> Syscalls<Mac> for ExecV2
 where
     Mac: SupportMachine,
-    DL: CellDataProvider + HeaderProvider + ExtensionProvider + Send + Sync + Clone + 'static,
 {
     fn initialize(&mut self, _machine: &mut Mac) -> Result<(), VMError> {
         Ok(())
@@ -74,34 +55,6 @@ where
         let offset = bounds >> 32;
         let length = bounds as u32 as u64;
 
-        // We are fetching the actual cell here for some in-place validation
-        let mut sc = self
-            .snapshot2_context
-            .lock()
-            .map_err(|e| VMError::Unexpected(e.to_string()))?;
-        let (_, full_length) = match sc.load_data(&data_piece_id, 0, 0) {
-            Ok(val) => val,
-            Err(VMError::SnapshotDataLoadError) => {
-                // This comes from TxData results in an out of bound error, to
-                // mimic current behavior, we would return INDEX_OUT_OF_BOUND error.
-                machine.set_register(A0, Mac::REG::from_u8(INDEX_OUT_OF_BOUND));
-                return Ok(true);
-            }
-            Err(e) => return Err(e),
-        };
-        if offset >= full_length {
-            machine.set_register(A0, Mac::REG::from_u8(SLICE_OUT_OF_BOUND));
-            return Ok(true);
-        }
-        if length > 0 {
-            // Both offset and length are <= u32::MAX, so offset.checked_add(length) will be always a Some.
-            let end = offset.checked_add(length).ok_or(VMError::MemOutOfBound)?;
-            if end > full_length {
-                machine.set_register(A0, Mac::REG::from_u8(SLICE_OUT_OF_BOUND));
-                return Ok(true);
-            }
-        }
-
         let argc = machine.registers()[A4].to_u64();
         let argv = machine.registers()[A5].to_u64();
         self.message_box

script/src/verify.rs

@@ -183,11 +183,8 @@ where
     }
 
     /// Build syscall: exec. When script version >= V2, this exec implementation is used.
-    pub fn build_exec_v2(
-        &self,
-        snapshot2_context: Arc<Mutex<Snapshot2Context<DataPieceId, TxData<DL>>>>,
-    ) -> ExecV2<DL> {
-        ExecV2::new(self.vm_id, Arc::clone(&self.message_box), snapshot2_context)
+    pub fn build_exec_v2(&self) -> ExecV2 {
+        ExecV2::new(self.vm_id, Arc::clone(&self.message_box))
     }
 
     /// Build syscall: load_tx
@@ -329,7 +326,7 @@ where
             syscalls.append(&mut vec![
                 Box::new(self.build_vm_version()),
                 if script_version >= ScriptVersion::V2 {
-                    Box::new(self.build_exec_v2(Arc::clone(&snapshot2_context)))
+                    Box::new(self.build_exec_v2())
                 } else {
                     Box::new(self.build_exec(
                         Arc::clone(&script_group_input_indices),

script/src/verify/tests/ckb_latest/features_since_v2021.rs

@@ -490,8 +490,17 @@ fn check_exec_big_offset_length() {
 
     let verifier = TransactionScriptsVerifierWithEnv::new();
     let result = verifier.verify_without_limit(script_version, &rtx);
-    if script_version >= ScriptVersion::V1 {
-        assert!(result.unwrap_err().to_string().contains("error code 3"));
+    match script_version {
+        ScriptVersion::V0 => {}
+        ScriptVersion::V1 => {
+            assert!(result.unwrap_err().to_string().contains("error code 3"));
+        }
+        _ => {
+            assert!(result
+                .unwrap_err()
+                .to_string()
+                .contains("VM Internal Error: ElfParseError"));
+        }
     }
 }
 
@@ -1763,7 +1772,11 @@ fn exec_slice() {
     test_exec(0b0000, 1, 2, 1, from, res);
 
     let from = ExecFrom::OutOfSlice(length + 1);
-    let res = Err("error code 3".to_string());
+    let res = if script_version >= ScriptVersion::V2 {
+        Ok(2)
+    } else {
+        Err("error code 3".to_string())
+    };
     test_exec(0b0000, 1, 2, 1, from, res);
 
     let from = ExecFrom::OutOfSlice(((length - 1) << 32) | 1);