diff --git a/program-runtime/src/instruction_processor.rs b/program-runtime/src/instruction_processor.rs index 0e511b82ab..c2a0eef872 100644 --- a/program-runtime/src/instruction_processor.rs +++ b/program-runtime/src/instruction_processor.rs @@ -5,10 +5,10 @@ use solana_sdk::{ account_utils::StateMut, bpf_loader_upgradeable::{self, UpgradeableLoaderState}, feature_set::{demote_program_write_locks, fix_write_privs}, - ic_logger_msg, ic_msg, - instruction::{CompiledInstruction, Instruction, InstructionError}, + ic_msg, + instruction::{Instruction, InstructionError}, message::Message, - process_instruction::{Executor, InvokeContext, Logger, ProcessInstructionWithContext}, + process_instruction::{Executor, InvokeContext, ProcessInstructionWithContext}, pubkey::Pubkey, rent::Rent, system_program, @@ -516,20 +516,16 @@ impl InstructionProcessor { caller_write_privileges.push(caller_keyed_accounts[*index].is_writable()); } }; - let accounts = message - .account_keys - .iter() - .map(|account_key| { - invoke_context - .get_account(account_key) - .ok_or(InstructionError::MissingAccount) - .map(|(_account_index, account)| (*account_key, account)) - }) - .collect::, InstructionError>>()?; - let account_sizes = accounts - .iter() - .map(|(_key, account)| account.borrow().data().len()) - .collect::>(); + let mut account_indices = Vec::with_capacity(message.account_keys.len()); + let mut accounts = Vec::with_capacity(message.account_keys.len()); + for account_key in message.account_keys.iter() { + let (account_index, account) = invoke_context + .get_account(account_key) + .ok_or(InstructionError::MissingAccount)?; + let account_length = account.borrow().data().len(); + account_indices.push(account_index); + accounts.push((account, account_length)); + } // Record the instruction invoke_context.record_instruction(&instruction); @@ -538,13 +534,13 @@ impl InstructionProcessor { InstructionProcessor::process_cross_program_instruction( &message, &program_indices, - &accounts, + &account_indices, &caller_write_privileges, *invoke_context, )?; // Verify the called program has not misbehaved - for ((_key, account), prev_size) in accounts.iter().zip(account_sizes.iter()) { + for (account, prev_size) in accounts.iter() { if *prev_size != account.borrow().data().len() && *prev_size != 0 { // Only support for `CreateAccount` at this time. // Need a way to limit total realloc size across multiple CPI calls @@ -564,7 +560,7 @@ impl InstructionProcessor { pub fn process_cross_program_instruction( message: &Message, program_indices: &[usize], - accounts: &[(Pubkey, Rc>)], + account_indices: &[usize], caller_write_privileges: &[bool], invoke_context: &mut dyn InvokeContext, ) -> Result<(), InstructionError> { @@ -577,13 +573,19 @@ impl InstructionProcessor { let program_id = instruction.program_id(&message.account_keys); // Verify the calling program hasn't misbehaved - invoke_context.verify_and_update(instruction, accounts, caller_write_privileges)?; + invoke_context.verify_and_update(instruction, account_indices, caller_write_privileges)?; // clear the return data invoke_context.set_return_data(None); // Invoke callee - invoke_context.push(program_id, message, instruction, program_indices, accounts)?; + invoke_context.push( + program_id, + message, + instruction, + program_indices, + account_indices, + )?; let mut instruction_processor = InstructionProcessor::default(); for (program_id, process_instruction) in invoke_context.get_programs().iter() { @@ -602,67 +604,14 @@ impl InstructionProcessor { let write_privileges: Vec = (0..message.account_keys.len()) .map(|i| message.is_writable(i, demote_program_write_locks)) .collect(); - result = invoke_context.verify_and_update(instruction, accounts, &write_privileges); + result = + invoke_context.verify_and_update(instruction, account_indices, &write_privileges); } // Restore previous state invoke_context.pop(); result } - - /// Verify the results of a cross-program instruction - #[allow(clippy::too_many_arguments)] - pub fn verify_and_update( - instruction: &CompiledInstruction, - pre_accounts: &mut [PreAccount], - accounts: &[(Pubkey, Rc>)], - program_id: &Pubkey, - rent: &Rent, - write_privileges: &[bool], - timings: &mut ExecuteDetailsTimings, - logger: Rc>, - ) -> Result<(), InstructionError> { - // Verify the per-account instruction results - let (mut pre_sum, mut post_sum) = (0_u128, 0_u128); - let mut work = |_unique_index: usize, account_index: usize| { - if account_index < write_privileges.len() && account_index < accounts.len() { - let (key, account) = &accounts[account_index]; - let is_writable = write_privileges[account_index]; - // Find the matching PreAccount - for pre_account in pre_accounts.iter_mut() { - if key == pre_account.key() { - { - // Verify account has no outstanding references - let _ = account - .try_borrow_mut() - .map_err(|_| InstructionError::AccountBorrowOutstanding)?; - } - let account = account.borrow(); - pre_account - .verify(program_id, is_writable, rent, &account, timings, false) - .map_err(|err| { - ic_logger_msg!(logger, "failed to verify account {}: {}", key, err); - err - })?; - pre_sum += u128::from(pre_account.lamports()); - post_sum += u128::from(account.lamports()); - if is_writable && !pre_account.executable() { - pre_account.update(&account); - } - return Ok(()); - } - } - } - Err(InstructionError::MissingAccount) - }; - instruction.visit_each_account(&mut work)?; - - // Verify that the total sum of all the lamports did not change - if pre_sum != post_sum { - return Err(InstructionError::UnbalancedInstruction); - } - Ok(()) - } } #[cfg(test)] diff --git a/program-test/src/lib.rs b/program-test/src/lib.rs index 92642745b7..69b97f5ecf 100644 --- a/program-test/src/lib.rs +++ b/program-test/src/lib.rs @@ -275,30 +275,34 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs { stable_log::program_invoke(&logger, &program_id, invoke_context.invoke_depth()); // Convert AccountInfos into Accounts - fn ai_to_a(ai: &AccountInfo) -> AccountSharedData { - AccountSharedData::from(Account { - lamports: ai.lamports(), - data: ai.try_borrow_data().unwrap().to_vec(), - owner: *ai.owner, - executable: ai.executable, - rent_epoch: ai.rent_epoch, - }) - } - let mut accounts = vec![]; - 'outer: for key in &message.account_keys { - for account_info in account_infos { - if account_info.unsigned_key() == key { - accounts.push((*key, Rc::new(RefCell::new(ai_to_a(account_info))))); - continue 'outer; - } + let mut account_indices = Vec::with_capacity(message.account_keys.len()); + let mut accounts = Vec::with_capacity(message.account_keys.len()); + for (i, account_key) in message.account_keys.iter().enumerate() { + let ((account_index, account), account_info) = invoke_context + .get_account(account_key) + .zip( + account_infos + .iter() + .find(|account_info| account_info.unsigned_key() == account_key), + ) + .ok_or(InstructionError::MissingAccount) + .unwrap(); + { + let mut account = account.borrow_mut(); + account.copy_into_owner_from_slice(account_info.owner.as_ref()); + account.set_data_from_slice(&account_info.try_borrow_data().unwrap()); + account.set_lamports(account_info.lamports()); + account.set_executable(account_info.executable); + account.set_rent_epoch(account_info.rent_epoch); } - panic!("Account {} wasn't found in account_infos", key); + let account_info = if message.is_writable(i, demote_program_write_locks) { + Some(account_info) + } else { + None + }; + account_indices.push(account_index); + accounts.push((account, account_info)); } - assert_eq!( - accounts.len(), - message.account_keys.len(), - "Missing or not enough accounts passed to invoke" - ); let (program_account_index, _program_account) = invoke_context.get_account(&program_id).unwrap(); let program_indices = vec![program_account_index]; @@ -330,43 +334,37 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs { InstructionProcessor::process_cross_program_instruction( &message, &program_indices, - &accounts, + &account_indices, &caller_privileges, invoke_context, ) .map_err(|err| ProgramError::try_from(err).unwrap_or_else(|err| panic!("{}", err)))?; // Copy writeable account modifications back into the caller's AccountInfos - for (i, (pubkey, account)) in accounts.iter().enumerate().take(message.account_keys.len()) { - if !message.is_writable(i, demote_program_write_locks) { - continue; - } - for account_info in account_infos { - if account_info.unsigned_key() == pubkey { - **account_info.try_borrow_mut_lamports().unwrap() = account.borrow().lamports(); - - let mut data = account_info.try_borrow_mut_data()?; - let account_borrow = account.borrow(); - let new_data = account_borrow.data(); - if account_info.owner != account.borrow().owner() { - // TODO Figure out a better way to allow the System Program to set the account owner - #[allow(clippy::transmute_ptr_to_ptr)] - #[allow(mutable_transmutes)] - let account_info_mut = - unsafe { transmute::<&Pubkey, &mut Pubkey>(account_info.owner) }; - *account_info_mut = *account.borrow().owner(); - } - if data.len() != new_data.len() { - // TODO: Figure out how to allow the System Program to resize the account data - panic!( - "Account data resizing not supported yet: {} -> {}. \ - Consider making this test conditional on `#[cfg(feature = \"test-bpf\")]`", - data.len(), - new_data.len() - ); - } - data.clone_from_slice(new_data); + for (account, account_info) in accounts.iter() { + if let Some(account_info) = account_info { + **account_info.try_borrow_mut_lamports().unwrap() = account.borrow().lamports(); + let mut data = account_info.try_borrow_mut_data()?; + let account_borrow = account.borrow(); + let new_data = account_borrow.data(); + if account_info.owner != account.borrow().owner() { + // TODO Figure out a better way to allow the System Program to set the account owner + #[allow(clippy::transmute_ptr_to_ptr)] + #[allow(mutable_transmutes)] + let account_info_mut = + unsafe { transmute::<&Pubkey, &mut Pubkey>(account_info.owner) }; + *account_info_mut = *account.borrow().owner(); } + if data.len() != new_data.len() { + // TODO: Figure out how to allow the System Program to resize the account data + panic!( + "Account data resizing not supported yet: {} -> {}. \ + Consider making this test conditional on `#[cfg(feature = \"test-bpf\")]`", + data.len(), + new_data.len() + ); + } + data.clone_from_slice(new_data); } } diff --git a/programs/bpf_loader/src/syscalls.rs b/programs/bpf_loader/src/syscalls.rs index cf03ea9fcc..d118942e85 100644 --- a/programs/bpf_loader/src/syscalls.rs +++ b/programs/bpf_loader/src/syscalls.rs @@ -1513,8 +1513,11 @@ struct AccountReferences<'a> { rent_epoch: u64, } type TranslatedAccounts<'a> = ( - Vec<(Pubkey, Rc>)>, - Vec>>, + Vec, + Vec<( + Rc>, + Option>, + )>, ); /// Implemented by language specific data structure translators @@ -2052,16 +2055,16 @@ where { let demote_program_write_locks = invoke_context.is_feature_active(&demote_program_write_locks::id()); + let mut account_indices = Vec::with_capacity(message.account_keys.len()); let mut accounts = Vec::with_capacity(message.account_keys.len()); - let mut refs = Vec::with_capacity(message.account_keys.len()); for (i, account_key) in message.account_keys.iter().enumerate() { - if let Some((_account_index, account)) = invoke_context.get_account(account_key) { + if let Some((account_index, account)) = invoke_context.get_account(account_key) { if i == message.instructions[0].program_id_index as usize || account.borrow().executable() { // Use the known account - accounts.push((*account_key, account)); - refs.push(None); + account_indices.push(account_index); + accounts.push((account, None)); continue; } else if let Some(account_ref_index) = account_info_keys.iter().position(|key| *key == account_key) @@ -2075,12 +2078,13 @@ where account.set_executable(account_ref.executable); account.set_rent_epoch(account_ref.rent_epoch); } - accounts.push((*account_key, account)); - refs.push(if message.is_writable(i, demote_program_write_locks) { + let account_ref = if message.is_writable(i, demote_program_write_locks) { Some(account_ref) } else { None - }); + }; + account_indices.push(account_index); + accounts.push((account, account_ref)); continue; } } @@ -2092,7 +2096,7 @@ where return Err(SyscallError::InstructionError(InstructionError::MissingAccount).into()); } - Ok((accounts, refs)) + Ok((account_indices, accounts)) } fn check_instruction_size( @@ -2176,7 +2180,7 @@ fn call<'a>( &instruction.data, invoke_context.is_feature_active(&close_upgradeable_program_accounts::id()), )?; - let (accounts, account_refs) = syscall.translate_accounts( + let (account_indices, mut accounts) = syscall.translate_accounts( &message, account_infos_addr, account_infos_len, @@ -2191,16 +2195,16 @@ fn call<'a>( InstructionProcessor::process_cross_program_instruction( &message, &program_indices, - &accounts, + &account_indices, &caller_write_privileges, *invoke_context, ) .map_err(SyscallError::InstructionError)?; // Copy results back to caller - for ((_key, account), account_ref) in accounts.iter().zip(account_refs) { + for (account, account_ref) in accounts.iter_mut() { let account = account.borrow(); - if let Some(mut account_ref) = account_ref { + if let Some(account_ref) = account_ref { *account_ref.lamports = account.lamports(); *account_ref.owner = *account.owner(); if account_ref.data.len() != account.data().len() { diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index a2c9459759..5216087fbc 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -119,7 +119,14 @@ impl<'a> ThisInvokeContext<'a> { fee_calculator, return_data: None, }; - invoke_context.push(program_id, message, instruction, program_indices, accounts)?; + let account_indices = (0..accounts.len()).collect::>(); + invoke_context.push( + program_id, + message, + instruction, + program_indices, + &account_indices, + )?; Ok(invoke_context) } } @@ -130,7 +137,7 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { message: &Message, instruction: &CompiledInstruction, program_indices: &[usize], - instruction_accounts: &[(Pubkey, Rc>)], + account_indices: &[usize], ) -> Result<(), InstructionError> { if self.invoke_stack.len() > self.compute_budget.max_invoke_depth { return Err(InstructionError::CallDepth); @@ -161,44 +168,17 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { &self.accounts[*account_index].1 as &RefCell, ) }) - .chain(instruction.accounts.iter().map(|index| { - let index = *index as usize; + .chain(instruction.accounts.iter().map(|index_in_instruction| { + let index_in_instruction = *index_in_instruction as usize; + let account_index = account_indices[index_in_instruction]; ( - message.is_signer(index), - message.is_writable(index, demote_program_write_locks), - &instruction_accounts[index].0, - &instruction_accounts[index].1 as &RefCell, + message.is_signer(index_in_instruction), + message.is_writable(index_in_instruction, demote_program_write_locks), + &self.accounts[account_index].0, + &self.accounts[account_index].1 as &RefCell, ) })) .collect::>(); - - // Alias the keys and account references in the provided keyed_accounts - // with the ones already existing in self, so that the lifetime 'a matches. - fn transmute_lifetime<'a, 'b, T: Sized>(value: &'a T) -> &'b T { - unsafe { std::mem::transmute(value) } - } - let keyed_accounts = keyed_accounts - .iter() - .map(|(is_signer, is_writable, search_key, account)| { - self.accounts - .iter() - .position(|(key, _account)| key == *search_key) - .map(|index| { - // TODO - // Currently we are constructing new accounts on the stack - // before calling MessageProcessor::process_cross_program_instruction - // Ideally we would recycle the existing accounts here. - ( - *is_signer, - *is_writable, - &self.accounts[index].0, - // &self.accounts[index] as &RefCell - transmute_lifetime(*account), - ) - }) - }) - .collect::>>() - .ok_or(InstructionError::InvalidArgument)?; self.invoke_stack.push(InvokeContextStackFrame::new( *key, create_keyed_accounts_unified(keyed_accounts.as_slice()), @@ -214,24 +194,63 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { fn verify_and_update( &mut self, instruction: &CompiledInstruction, - accounts: &[(Pubkey, Rc>)], + account_indices: &[usize], write_privileges: &[bool], ) -> Result<(), InstructionError> { let stack_frame = self .invoke_stack .last() .ok_or(InstructionError::CallDepth)?; + let program_id = &stack_frame.key; + let rent = &self.rent; let logger = self.get_logger(); - InstructionProcessor::verify_and_update( - instruction, - &mut self.pre_accounts, - accounts, - &stack_frame.key, - &self.rent, - write_privileges, - &mut self.timings, - logger, - ) + let accounts = &self.accounts; + let pre_accounts = &mut self.pre_accounts; + let timings = &mut self.timings; + + // Verify the per-account instruction results + let (mut pre_sum, mut post_sum) = (0_u128, 0_u128); + let mut work = |_unique_index: usize, index_in_instruction: usize| { + if index_in_instruction < write_privileges.len() + && index_in_instruction < account_indices.len() + { + let account_index = account_indices[index_in_instruction]; + let (key, account) = &accounts[account_index]; + let is_writable = write_privileges[index_in_instruction]; + // Find the matching PreAccount + for pre_account in pre_accounts.iter_mut() { + if key == pre_account.key() { + { + // Verify account has no outstanding references + let _ = account + .try_borrow_mut() + .map_err(|_| InstructionError::AccountBorrowOutstanding)?; + } + let account = account.borrow(); + pre_account + .verify(program_id, is_writable, rent, &account, timings, false) + .map_err(|err| { + ic_logger_msg!(logger, "failed to verify account {}: {}", key, err); + err + })?; + pre_sum += u128::from(pre_account.lamports()); + post_sum += u128::from(account.lamports()); + if is_writable && !pre_account.executable() { + pre_account.update(&account); + } + return Ok(()); + } + } + } + Err(InstructionError::MissingAccount) + }; + instruction.visit_each_account(&mut work)?; + + // Verify that the total sum of all the lamports did not change + if pre_sum != post_sum { + return Err(InstructionError::UnbalancedInstruction); + } + Ok(()) } fn get_caller(&self) -> Result<&Pubkey, InstructionError> { self.invoke_stack @@ -671,6 +690,7 @@ mod tests { )); metas.push(AccountMeta::new(*program_id, false)); } + let account_indices = (0..accounts.len()).collect::>(); let message = Message::new( &[Instruction::new_with_bytes(invoke_stack[0], &[0], metas)], @@ -709,7 +729,7 @@ mod tests { &message, &message.instructions[0], &[], - &accounts, + &account_indices, ) { break; @@ -734,24 +754,19 @@ mod tests { )], None, ); + let write_privileges: Vec = (0..message.account_keys.len()) + .map(|i| message.is_writable(i, /*demote_program_write_locks=*/ true)) + .collect(); // modify account owned by the program accounts[owned_index].1.borrow_mut().data_as_mut_slice()[0] = (MAX_DEPTH + owned_index) as u8; - let mut these_accounts = accounts[not_owned_index..owned_index + 1].to_vec(); - these_accounts.push(( - message.account_keys[2], - Rc::new(RefCell::new(AccountSharedData::new( - 1, - 1, - &solana_sdk::pubkey::Pubkey::default(), - ))), - )); - let write_privileges: Vec = (0..message.account_keys.len()) - .map(|i| message.is_writable(i, /*demote_program_write_locks=*/ true)) - .collect(); invoke_context - .verify_and_update(&message.instructions[0], &these_accounts, &write_privileges) + .verify_and_update( + &message.instructions[0], + &account_indices[not_owned_index..owned_index + 1], + &write_privileges, + ) .unwrap(); assert_eq!( invoke_context.pre_accounts[owned_index].data()[0], @@ -765,7 +780,7 @@ mod tests { assert_eq!( invoke_context.verify_and_update( &message.instructions[0], - &accounts[not_owned_index..owned_index + 1], + &account_indices[not_owned_index..owned_index + 1], &write_privileges, ), Err(InstructionError::ExternalAccountDataModified) @@ -1217,6 +1232,7 @@ mod tests { ), (callee_program_id, Rc::new(RefCell::new(program_account))), ]; + let account_indices = [0, 1, 2]; let program_indices = vec![3]; let programs: Vec<(_, ProcessInstructionWithContext)> = @@ -1274,7 +1290,7 @@ mod tests { InstructionProcessor::process_cross_program_instruction( &message, &program_indices, - &accounts, + &account_indices, &caller_write_privileges, &mut invoke_context, ), @@ -1288,7 +1304,7 @@ mod tests { InstructionProcessor::process_cross_program_instruction( &message, &program_indices, - &accounts, + &account_indices, &caller_write_privileges, &mut invoke_context, ), @@ -1348,7 +1364,7 @@ mod tests { InstructionProcessor::process_cross_program_instruction( &message, &program_indices, - &accounts, + &account_indices, &caller_write_privileges, &mut invoke_context, ), diff --git a/sdk/src/process_instruction.rs b/sdk/src/process_instruction.rs index d2711e529a..bc6645aa3c 100644 --- a/sdk/src/process_instruction.rs +++ b/sdk/src/process_instruction.rs @@ -51,19 +51,15 @@ impl<'a> InvokeContextStackFrame<'a> { /// Invocation context passed to loaders pub trait InvokeContext { /// Push a stack frame onto the invocation stack - /// - /// Used in MessageProcessor::process_cross_program_instruction fn push( &mut self, key: &Pubkey, message: &Message, instruction: &CompiledInstruction, program_indices: &[usize], - instruction_accounts: &[(Pubkey, Rc>)], + account_indices: &[usize], ) -> Result<(), InstructionError>; /// Pop a stack frame from the invocation stack - /// - /// Used in MessageProcessor::process_cross_program_instruction fn pop(&mut self); /// Current depth of the invocation stake fn invoke_depth(&self) -> usize; @@ -71,7 +67,7 @@ pub trait InvokeContext { fn verify_and_update( &mut self, instruction: &CompiledInstruction, - accounts: &[(Pubkey, Rc>)], + account_indices: &[usize], write_privileges: &[bool], ) -> Result<(), InstructionError>; /// Get the program ID of the currently executing program @@ -478,7 +474,7 @@ impl<'a> InvokeContext for MockInvokeContext<'a> { _message: &Message, _instruction: &CompiledInstruction, _program_indices: &[usize], - _instruction_accounts: &[(Pubkey, Rc>)], + _account_indices: &[usize], ) -> Result<(), InstructionError> { self.invoke_stack.push(InvokeContextStackFrame::new( *_key, @@ -495,7 +491,7 @@ impl<'a> InvokeContext for MockInvokeContext<'a> { fn verify_and_update( &mut self, _instruction: &CompiledInstruction, - _accounts: &[(Pubkey, Rc>)], + _account_indices: &[usize], _write_pivileges: &[bool], ) -> Result<(), InstructionError> { Ok(())