From 4de5fff3cac148cfcf2bfcb5356040e6698b9a7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Wed, 29 Sep 2021 19:11:06 +0200 Subject: [PATCH] Simplifies return_data accessors in InvokeContext. (#20290) --- program-runtime/src/instruction_processor.rs | 2 +- programs/bpf/tests/programs.rs | 2 +- programs/bpf_loader/src/lib.rs | 10 +-- programs/bpf_loader/src/syscalls.rs | 87 ++++++++------------ runtime/src/message_processor.rs | 14 ++-- sdk/src/process_instruction.rs | 17 ++-- 6 files changed, 58 insertions(+), 74 deletions(-) diff --git a/program-runtime/src/instruction_processor.rs b/program-runtime/src/instruction_processor.rs index fcbf4c483..7dc8f075e 100644 --- a/program-runtime/src/instruction_processor.rs +++ b/program-runtime/src/instruction_processor.rs @@ -602,7 +602,7 @@ impl InstructionProcessor { invoke_context.verify_and_update(instruction, account_indices, caller_write_privileges)?; // clear the return data - invoke_context.set_return_data(None); + invoke_context.set_return_data(Vec::new())?; // Invoke callee invoke_context.push( diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index 53b55bf07..177e50611 100644 --- a/programs/bpf/tests/programs.rs +++ b/programs/bpf/tests/programs.rs @@ -230,7 +230,7 @@ fn run_program( for i in 0..2 { let mut parameter_bytes = parameter_bytes.clone(); { - invoke_context.set_return_data(None); + invoke_context.set_return_data(Vec::new()).unwrap(); let mut vm = create_vm( &loader_id, diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 843cfd1ce..b7ff80aeb 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -945,9 +945,9 @@ impl Executor for BpfExecutor { trace!("BPF Program Instruction Trace:\n{}", trace_string); } drop(vm); - let return_data = invoke_context.get_return_data(); - if let Some((program_id, return_data)) = return_data { - stable_log::program_return(&logger, program_id, return_data); + let (program_id, return_data) = invoke_context.get_return_data(); + if !return_data.is_empty() { + stable_log::program_return(&logger, &program_id, return_data); } match result { Ok(status) => { @@ -960,7 +960,7 @@ impl Executor for BpfExecutor { } else { status.into() }; - stable_log::program_failure(&logger, program_id, &error); + stable_log::program_failure(&logger, &program_id, &error); return Err(error); } } @@ -974,7 +974,7 @@ impl Executor for BpfExecutor { InstructionError::ProgramFailedToComplete } }; - stable_log::program_failure(&logger, program_id, &error); + stable_log::program_failure(&logger, &program_id, &error); return Err(error); } } diff --git a/programs/bpf_loader/src/syscalls.rs b/programs/bpf_loader/src/syscalls.rs index 3cd423cec..8c7e9ebbc 100644 --- a/programs/bpf_loader/src/syscalls.rs +++ b/programs/bpf_loader/src/syscalls.rs @@ -44,7 +44,6 @@ use solana_sdk::{ use std::{ alloc::Layout, cell::{Ref, RefCell, RefMut}, - cmp::min, mem::{align_of, size_of}, rc::Rc, slice::from_raw_parts_mut, @@ -2324,23 +2323,21 @@ impl<'a> SyscallObject for SyscallSetReturnData<'a> { return; } - if len == 0 { - invoke_context.set_return_data(None); + let return_data = if len == 0 { + Vec::new() } else { - let return_data = question_mark!( + question_mark!( translate_slice::(memory_mapping, addr, len, self.loader_id), result - ); - - let program_id = *question_mark!( - invoke_context - .get_caller() - .map_err(SyscallError::InstructionError), - result - ); - - invoke_context.set_return_data(Some((program_id, return_data.to_vec()))); - } + ) + .to_vec() + }; + question_mark!( + invoke_context + .set_return_data(return_data) + .map_err(SyscallError::InstructionError), + result + ); *result = Ok(0); } @@ -2354,7 +2351,7 @@ impl<'a> SyscallObject for SyscallGetReturnData<'a> { fn call( &mut self, return_data_addr: u64, - len: u64, + mut length: u64, program_id_addr: u64, _arg4: u64, _arg5: u64, @@ -2377,47 +2374,33 @@ impl<'a> SyscallObject for SyscallGetReturnData<'a> { result ); - if let Some((program_id, return_data)) = invoke_context.get_return_data() { - if len != 0 { - let length = min(return_data.len() as u64, len); + let (program_id, return_data) = invoke_context.get_return_data(); + length = length.min(return_data.len() as u64); + if length != 0 { + question_mark!( + invoke_context + .get_compute_meter() + .consume((length + size_of::() as u64) / budget.cpi_bytes_per_unit), + result + ); - question_mark!( - invoke_context - .get_compute_meter() - .consume((length + size_of::() as u64) / budget.cpi_bytes_per_unit), - result - ); + let return_data_result = question_mark!( + translate_slice_mut::(memory_mapping, return_data_addr, length, self.loader_id,), + result + ); - let return_data_result = question_mark!( - translate_slice_mut::( - memory_mapping, - return_data_addr, - length, - self.loader_id, - ), - result - ); + return_data_result.copy_from_slice(&return_data[..length as usize]); - return_data_result.copy_from_slice(&return_data[..length as usize]); + let program_id_result = question_mark!( + translate_slice_mut::(memory_mapping, program_id_addr, 1, self.loader_id,), + result + ); - let program_id_result = question_mark!( - translate_slice_mut::( - memory_mapping, - program_id_addr, - 1, - self.loader_id, - ), - result - ); - - program_id_result[0] = *program_id; - } - - // Return the actual length, rather the length returned - *result = Ok(return_data.len() as u64); - } else { - *result = Ok(0); + program_id_result[0] = program_id; } + + // Return the actual length, rather the length returned + *result = Ok(return_data.len() as u64); } } diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index e48cda806..b5a5a38e1 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -69,8 +69,7 @@ pub struct ThisInvokeContext<'a> { sysvars: RefCell>>)>>, blockhash: &'a Hash, fee_calculator: &'a FeeCalculator, - // return data and program_id that set it - return_data: Option<(Pubkey, Vec)>, + return_data: (Pubkey, Vec), } impl<'a> ThisInvokeContext<'a> { #[allow(clippy::too_many_arguments)] @@ -109,7 +108,7 @@ impl<'a> ThisInvokeContext<'a> { sysvars: RefCell::new(vec![]), blockhash, fee_calculator, - return_data: None, + return_data: (Pubkey::default(), Vec::new()), } } } @@ -428,11 +427,12 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { fn get_fee_calculator(&self) -> &FeeCalculator { self.fee_calculator } - fn set_return_data(&mut self, return_data: Option<(Pubkey, Vec)>) { - self.return_data = return_data; + fn set_return_data(&mut self, data: Vec) -> Result<(), InstructionError> { + self.return_data = (*self.get_caller()?, data); + Ok(()) } - fn get_return_data(&self) -> &Option<(Pubkey, Vec)> { - &self.return_data + fn get_return_data(&self) -> (Pubkey, &[u8]) { + (self.return_data.0, &self.return_data.1) } } pub struct ThisLogger { diff --git a/sdk/src/process_instruction.rs b/sdk/src/process_instruction.rs index c711a6596..d407cb0e4 100644 --- a/sdk/src/process_instruction.rs +++ b/sdk/src/process_instruction.rs @@ -124,9 +124,9 @@ pub trait InvokeContext { /// Get this invocation's `FeeCalculator` fn get_fee_calculator(&self) -> &FeeCalculator; /// Set the return data - fn set_return_data(&mut self, return_data: Option<(Pubkey, Vec)>); + fn set_return_data(&mut self, data: Vec) -> Result<(), InstructionError>; /// Get the return data - fn get_return_data(&self) -> &Option<(Pubkey, Vec)>; + fn get_return_data(&self) -> (Pubkey, &[u8]); } /// Convenience macro to log a message with an `Rc>` @@ -447,7 +447,7 @@ pub struct MockInvokeContext<'a> { pub disabled_features: HashSet, pub blockhash: Hash, pub fee_calculator: FeeCalculator, - pub return_data: Option<(Pubkey, Vec)>, + pub return_data: (Pubkey, Vec), } impl<'a> MockInvokeContext<'a> { @@ -467,7 +467,7 @@ impl<'a> MockInvokeContext<'a> { disabled_features: HashSet::default(), blockhash: Hash::default(), fee_calculator: FeeCalculator::default(), - return_data: None, + return_data: (Pubkey::default(), Vec::new()), }; invoke_context .invoke_stack @@ -605,10 +605,11 @@ impl<'a> InvokeContext for MockInvokeContext<'a> { fn get_fee_calculator(&self) -> &FeeCalculator { &self.fee_calculator } - fn set_return_data(&mut self, return_data: Option<(Pubkey, Vec)>) { - self.return_data = return_data; + fn set_return_data(&mut self, data: Vec) -> Result<(), InstructionError> { + self.return_data = (*self.get_caller()?, data); + Ok(()) } - fn get_return_data(&self) -> &Option<(Pubkey, Vec)> { - &self.return_data + fn get_return_data(&self) -> (Pubkey, &[u8]) { + (self.return_data.0, &self.return_data.1) } }