From 5136613afd4ab96c04b6cc872b73523b0a6981a9 Mon Sep 17 00:00:00 2001 From: mohanson Date: Tue, 21 Jan 2025 11:25:40 +0800 Subject: [PATCH] Remove out of bounds check in exec_v2 --- script/src/scheduler.rs | 38 +++++++++--- script/src/syscalls/exec_v2.rs | 61 +++---------------- script/src/verify.rs | 9 +-- .../tests/ckb_latest/features_since_v2021.rs | 19 +++++- 4 files changed, 57 insertions(+), 70 deletions(-) diff --git a/script/src/scheduler.rs b/script/src/scheduler.rs index 2c007f4107..29d7f64042 100644 --- a/script/src/scheduler.rs +++ b/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,6 +357,25 @@ 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)?; @@ -363,6 +383,7 @@ where &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
, machine: &mut Machine, location: &DataLocation, + program: Bytes, args: Option<(u64, u64, u64)>, ) -> Result { - 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::(&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, diff --git a/script/src/syscalls/exec_v2.rs b/script/src/syscalls/exec_v2.rs index 03f63fff44..e237ceb027 100644 --- a/script/src/syscalls/exec_v2.rs +++ b/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
-where - DL: CellDataProvider + HeaderProvider + ExtensionProvider + Send + Sync + Clone + 'static, -{ +pub struct ExecV2 { id: VmId, message_box: Arc>>, - snapshot2_context: Arc>>>, } -impl
ExecV2
-where - DL: CellDataProvider + HeaderProvider + ExtensionProvider + Send + Sync + Clone + 'static, -{ - pub fn new( - id: VmId, - message_box: Arc>>, - snapshot2_context: Arc>>>, - ) -> ExecV2
{ - ExecV2 { - id, - message_box, - snapshot2_context, - } +impl ExecV2 { + pub fn new(id: VmId, message_box: Arc>>) -> ExecV2 { + ExecV2 { id, message_box } } } -impl Syscalls for ExecV2
+impl Syscalls 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 diff --git a/script/src/verify.rs b/script/src/verify.rs index 70eb0e7fea..a9315e3a79 100644 --- a/script/src/verify.rs +++ b/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>>>, - ) -> ExecV2
{ - 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), diff --git a/script/src/verify/tests/ckb_latest/features_since_v2021.rs b/script/src/verify/tests/ckb_latest/features_since_v2021.rs index 927ebc5fd9..026adeb459 100644 --- a/script/src/verify/tests/ckb_latest/features_since_v2021.rs +++ b/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);