diff --git a/runtime/benches/message_processor.rs b/runtime/benches/message_processor.rs index 212501ac16..017ed41ff0 100644 --- a/runtime/benches/message_processor.rs +++ b/runtime/benches/message_processor.rs @@ -21,23 +21,34 @@ fn bench_verify_instruction_data(bencher: &mut Bencher) { let owner = Pubkey::new_rand(); let non_owner = Pubkey::new_rand(); - let pre = Account::new(0, BUFSIZE, &owner); + let pre = PreInstructionAccount::new( + &Account::new(0, BUFSIZE, &owner), + true, + need_account_data_checked(&owner, &owner, true), + ); let post = Account::new(0, BUFSIZE, &owner); - assert_eq!(verify_instruction(true, &owner, &pre, &post), Ok(())); - - bencher.iter(|| pre.data == post.data); - let summary = bencher.bench(|_bencher| {}).unwrap(); - info!("data compare {} ns/iter", summary.median); + assert_eq!(verify_instruction(&owner, &pre, &post), Ok(())); // this one should be faster bencher.iter(|| { - verify_instruction(true, &owner, &pre, &post).unwrap(); + verify_instruction(&owner, &pre, &post).unwrap(); }); let summary = bencher.bench(|_bencher| {}).unwrap(); info!("data no change by owner: {} ns/iter", summary.median); + let pre = PreInstructionAccount::new( + &Account::new(0, BUFSIZE, &owner), + true, + need_account_data_checked(&owner, &non_owner, true), + ); + match pre.data { + Some(ref data) => bencher.iter(|| *data == post.data), + None => panic!("No data!"), + } + let summary = bencher.bench(|_bencher| {}).unwrap(); + info!("data compare {} ns/iter", summary.median); bencher.iter(|| { - verify_instruction(true, &non_owner, &pre, &post).unwrap(); + verify_instruction(&non_owner, &pre, &post).unwrap(); }); let summary = bencher.bench(|_bencher| {}).unwrap(); info!("data no change by non owner: {} ns/iter", summary.median); diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index a26f15aaf9..ebed004983 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -2,6 +2,7 @@ use crate::native_loader; use crate::system_instruction_processor; use serde::{Deserialize, Serialize}; use solana_sdk::account::{create_keyed_readonly_accounts, Account, KeyedAccount}; +use solana_sdk::clock::Epoch; use solana_sdk::instruction::{CompiledInstruction, InstructionError}; use solana_sdk::instruction_processor_utils; use solana_sdk::loader_instruction::LoaderInstruction; @@ -56,10 +57,43 @@ fn get_subset_unchecked_mut<'a, T>( .collect()) } +// The relevant state of an account before an Instruction executes, used +// to verify account integrity after the Instruction completes +pub struct PreInstructionAccount { + pub is_writable: bool, + pub lamports: u64, + pub data_len: usize, + pub data: Option>, + pub owner: Pubkey, + pub executable: bool, + pub rent_epoch: Epoch, +} +impl PreInstructionAccount { + pub fn new(account: &Account, is_writable: bool, copy_data: bool) -> Self { + Self { + is_writable, + lamports: account.lamports, + data_len: account.data.len(), + data: if copy_data { + Some(account.data.clone()) + } else { + None + }, + owner: account.owner, + executable: account.executable, + rent_epoch: account.rent_epoch, + } + } +} +pub fn need_account_data_checked(program_id: &Pubkey, owner: &Pubkey, is_writable: bool) -> bool { + // For accounts not assigned to the program, the data may not change. + program_id != owner + // Read-only account data may not change. + || !is_writable +} pub fn verify_instruction( - is_writable: bool, program_id: &Pubkey, - pre: &Account, + pre: &PreInstructionAccount, post: &Account, ) -> Result<(), InstructionError> { // Verify the transaction @@ -68,7 +102,7 @@ pub fn verify_instruction( // only if the account is writable and // only if the data is zero-initialized or empty if pre.owner != post.owner - && (!is_writable // line coverage used to get branch coverage + && (!pre.is_writable // line coverage used to get branch coverage || *program_id != pre.owner // line coverage used to get branch coverage || !is_zeroed(&post.data)) { @@ -83,7 +117,7 @@ pub fn verify_instruction( } // The balance of read-only accounts may not change. - if !is_writable // line coverage used to get branch coverage + if !pre.is_writable // line coverage used to get branch coverage && pre.lamports != post.lamports { return Err(InstructionError::ReadonlyLamportChange); @@ -91,49 +125,29 @@ pub fn verify_instruction( // Only the system program can change the size of the data // and only if the system program owns the account - if pre.data.len() != post.data.len() + if pre.data_len != post.data.len() && (!system_program::check_id(program_id) // line coverage used to get branch coverage || !system_program::check_id(&pre.owner)) { return Err(InstructionError::AccountDataSizeChanged); } - enum DataChanged { - Unchecked, - Checked(bool), - }; - - // Verify data, remember answer because comparing - // a megabyte costs us multiple microseconds... - let mut data_changed = DataChanged::Unchecked; - let mut data_changed = || -> bool { - match data_changed { - DataChanged::Unchecked => { - let changed = pre.data != post.data; - data_changed = DataChanged::Checked(changed); - changed + if need_account_data_checked(&pre.owner, program_id, pre.is_writable) { + match &pre.data { + Some(data) if *data == post.data => (), + _ => { + if !pre.is_writable { + return Err(InstructionError::ReadonlyDataModified); + } else { + return Err(InstructionError::ExternalAccountDataModified); + } } - DataChanged::Checked(changed) => changed, } - }; - - // For accounts not assigned to the program, the data may not change. - if *program_id != pre.owner // line coverage used to get branch coverage - && data_changed() - { - return Err(InstructionError::ExternalAccountDataModified); - } - - // Read-only account data may not change. - if !is_writable // line coverage used to get branch coverage - && data_changed() - { - return Err(InstructionError::ReadonlyDataModified); } // executable is one-way (false->true) and only the account owner may set it. if pre.executable != post.executable - && (!is_writable // line coverage used to get branch coverage + && (!pre.is_writable // line coverage used to get branch coverage || pre.executable // line coverage used to get branch coverage || *program_id != pre.owner) { @@ -270,6 +284,10 @@ impl MessageProcessor { ) } + fn sum_account_lamports(accounts: &mut [&mut Account]) -> u128 { + accounts.iter().map(|a| u128::from(a.lamports)).sum() + } + /// Execute an instruction /// This method calls the instruction's program entrypoint method and verifies that the result of /// the call does not violate the bank's accounting rules. @@ -281,40 +299,32 @@ impl MessageProcessor { executable_accounts: &mut [(Pubkey, Account)], program_accounts: &mut [&mut Account], ) -> Result<(), InstructionError> { - let program_id = instruction.program_id(&message.account_keys); assert_eq!(instruction.accounts.len(), program_accounts.len()); - // TODO: the runtime should be checking read/write access to memory - // we are trusting the hard-coded programs not to clobber - let pre_total: u128 = program_accounts - .iter() - .map(|a| u128::from(a.lamports)) - .sum(); - #[allow(clippy::map_clone)] + let program_id = instruction.program_id(&message.account_keys); + // Copy only what we need to verify after instruction processing let pre_accounts: Vec<_> = program_accounts .iter_mut() - .map(|account| account.clone()) // cloned() doesn't work on & & + .enumerate() + .map(|(i, account)| { + let is_writable = message.is_writable(instruction.accounts[i] as usize); + PreInstructionAccount::new( + account, + is_writable, + need_account_data_checked(&account.owner, program_id, is_writable), + ) + }) .collect(); + // Sum total lamports before instruction processing + let pre_total = Self::sum_account_lamports(program_accounts); self.process_instruction(message, instruction, executable_accounts, program_accounts)?; + // Verify the instruction - for (pre_account, (post_account, is_writable)) in - pre_accounts - .iter() - .zip(program_accounts.iter().enumerate().map(|(i, account)| { - ( - account, - message.is_writable(instruction.accounts[i] as usize), - ) - })) - { - verify_instruction(is_writable, &program_id, pre_account, post_account)?; + for (pre_account, post_account) in pre_accounts.iter().zip(program_accounts.iter()) { + verify_instruction(&program_id, pre_account, post_account)?; } // The total sum of all the lamports in all the accounts cannot change. - let post_total: u128 = program_accounts - .iter() - .map(|a| u128::from(a.lamports)) - .sum(); - + let post_total = Self::sum_account_lamports(program_accounts); if pre_total != post_total { return Err(InstructionError::UnbalancedInstruction); } @@ -432,9 +442,12 @@ mod tests { is_writable: bool, ) -> Result<(), InstructionError> { verify_instruction( - is_writable, &ix, - &Account::new(0, 0, pre), + &PreInstructionAccount::new( + &Account::new(0, 0, pre), + is_writable, + need_account_data_checked(pre, ix, is_writable), + ), &Account::new(0, 0, post), ) } @@ -487,9 +500,12 @@ mod tests { assert_eq!( verify_instruction( - true, &mallory_program_id, - &Account::new_data(0, &[42], &mallory_program_id,).unwrap(), + &PreInstructionAccount::new( + &Account::new_data(0, &[42], &mallory_program_id).unwrap(), + true, + need_account_data_checked(&mallory_program_id, &mallory_program_id, true), + ), &Account::new_data(0, &[0], &alice_program_id,).unwrap(), ), Ok(()), @@ -497,9 +513,12 @@ mod tests { ); assert_eq!( verify_instruction( - true, &mallory_program_id, - &Account::new_data(0, &[42], &mallory_program_id,).unwrap(), + &PreInstructionAccount::new( + &Account::new_data(0, &[42], &mallory_program_id).unwrap(), + true, + need_account_data_checked(&mallory_program_id, &mallory_program_id, true), + ), &Account::new_data(0, &[42], &alice_program_id,).unwrap(), ), Err(InstructionError::ModifiedProgramId), @@ -515,18 +534,22 @@ mod tests { pre_executable: bool, post_executable: bool| -> Result<(), InstructionError> { - let pre = Account { - owner, - executable: pre_executable, - ..Account::default() - }; + let pre = PreInstructionAccount::new( + &Account { + owner, + executable: pre_executable, + ..Account::default() + }, + is_writable, + need_account_data_checked(&owner, &program_id, is_writable), + ); let post = Account { owner, executable: post_executable, ..Account::default() }; - verify_instruction(is_writable, &program_id, &pre, &post) + verify_instruction(&program_id, &pre, &post) }; let mallory_program_id = Pubkey::new_rand(); @@ -563,9 +586,12 @@ mod tests { fn test_verify_instruction_change_data_len() { assert_eq!( verify_instruction( - true, &system_program::id(), - &Account::new_data(0, &[0], &system_program::id()).unwrap(), + &PreInstructionAccount::new( + &Account::new_data(0, &[0], &system_program::id()).unwrap(), + true, + need_account_data_checked(&system_program::id(), &system_program::id(), true), + ), &Account::new_data(0, &[0, 0], &system_program::id()).unwrap(), ), Ok(()), @@ -575,9 +601,12 @@ mod tests { assert_eq!( verify_instruction( - true, &system_program::id(), - &Account::new_data(0, &[0], &alice_program_id).unwrap(), + &PreInstructionAccount::new( + &Account::new_data(0, &[0], &alice_program_id).unwrap(), + true, + need_account_data_checked(&alice_program_id, &system_program::id(), true), + ), &Account::new_data(0, &[0, 0], &alice_program_id).unwrap(), ), Err(InstructionError::AccountDataSizeChanged), @@ -591,9 +620,13 @@ mod tests { let change_data = |program_id: &Pubkey, is_writable: bool| -> Result<(), InstructionError> { - let pre = Account::new_data(0, &[0], &alice_program_id).unwrap(); + let pre = PreInstructionAccount::new( + &Account::new_data(0, &[0], &alice_program_id).unwrap(), + is_writable, + need_account_data_checked(&alice_program_id, &program_id, is_writable), + ); let post = Account::new_data(0, &[42], &alice_program_id).unwrap(); - verify_instruction(is_writable, &program_id, &pre, &post) + verify_instruction(&program_id, &pre, &post) }; let mallory_program_id = Pubkey::new_rand(); @@ -619,18 +652,22 @@ mod tests { #[test] fn test_verify_instruction_rent_epoch() { let alice_program_id = Pubkey::new_rand(); - let pre = Account::new(0, 0, &alice_program_id); + let pre = PreInstructionAccount::new( + &Account::new(0, 0, &alice_program_id), + false, + need_account_data_checked(&alice_program_id, &system_program::id(), false), + ); let mut post = Account::new(0, 0, &alice_program_id); assert_eq!( - verify_instruction(false, &system_program::id(), &pre, &post), + verify_instruction(&system_program::id(), &pre, &post), Ok(()), "nothing changed!" ); post.rent_epoch += 1; assert_eq!( - verify_instruction(false, &system_program::id(), &pre, &post), + verify_instruction(&system_program::id(), &pre, &post), Err(InstructionError::RentEpochModified), "no one touches rent_epoch" ); @@ -640,12 +677,16 @@ mod tests { fn test_verify_instruction_deduct_lamports_and_reassign_account() { let alice_program_id = Pubkey::new_rand(); let bob_program_id = Pubkey::new_rand(); - let pre = Account::new_data(42, &[42], &alice_program_id).unwrap(); + let pre = PreInstructionAccount::new( + &Account::new_data(42, &[42], &alice_program_id).unwrap(), + true, + need_account_data_checked(&alice_program_id, &alice_program_id, true), + ); let post = Account::new_data(1, &[0], &bob_program_id).unwrap(); // positive test of this capability assert_eq!( - verify_instruction(true, &alice_program_id, &pre, &post), + verify_instruction(&alice_program_id, &pre, &post), Ok(()), "alice should be able to deduct lamports and give the account to bob if the data is zeroed", ); @@ -654,32 +695,51 @@ mod tests { #[test] fn test_verify_instruction_change_lamports() { let alice_program_id = Pubkey::new_rand(); - let pre = Account::new(42, 0, &alice_program_id); + let pre = PreInstructionAccount::new( + &Account::new(42, 0, &alice_program_id), + false, + need_account_data_checked(&alice_program_id, &system_program::id(), false), + ); let post = Account::new(0, 0, &alice_program_id); assert_eq!( - verify_instruction(false, &system_program::id(), &pre, &post), + verify_instruction(&system_program::id(), &pre, &post), Err(InstructionError::ExternalAccountLamportSpend), "debit should fail, even if system program" ); + + let pre = PreInstructionAccount::new( + &Account::new(42, 0, &alice_program_id), + false, + need_account_data_checked(&alice_program_id, &alice_program_id, false), + ); + assert_eq!( - verify_instruction(false, &alice_program_id, &pre, &post,), + verify_instruction(&alice_program_id, &pre, &post,), Err(InstructionError::ReadonlyLamportChange), "debit should fail, even if owning program" ); - let pre = Account::new(42, 0, &alice_program_id); + let pre = PreInstructionAccount::new( + &Account::new(42, 0, &alice_program_id), + true, + need_account_data_checked(&alice_program_id, &system_program::id(), true), + ); let post = Account::new(0, 0, &system_program::id()); assert_eq!( - verify_instruction(true, &system_program::id(), &pre, &post), + verify_instruction(&system_program::id(), &pre, &post), Err(InstructionError::ModifiedProgramId), "system program can't debit the account unless it was the pre.owner" ); - let pre = Account::new(42, 0, &system_program::id()); + let pre = PreInstructionAccount::new( + &Account::new(42, 0, &system_program::id()), + true, + need_account_data_checked(&system_program::id(), &system_program::id(), true), + ); let post = Account::new(0, 0, &alice_program_id); assert_eq!( - verify_instruction(true, &system_program::id(), &pre, &post), + verify_instruction(&system_program::id(), &pre, &post), Ok(()), "system can spend (and change owner)" ); @@ -688,21 +748,34 @@ mod tests { #[test] fn test_verify_instruction_data_size_changed() { let alice_program_id = Pubkey::new_rand(); - let pre = Account::new_data(42, &[0], &alice_program_id).unwrap(); + let pre = PreInstructionAccount::new( + &Account::new_data(42, &[0], &alice_program_id).unwrap(), + true, + need_account_data_checked(&alice_program_id, &system_program::id(), true), + ); let post = Account::new_data(42, &[0, 0], &alice_program_id).unwrap(); assert_eq!( - verify_instruction(true, &system_program::id(), &pre, &post), + verify_instruction(&system_program::id(), &pre, &post), Err(InstructionError::AccountDataSizeChanged), "system program should not be able to change another program's account data size" ); + let pre = PreInstructionAccount::new( + &Account::new_data(42, &[0], &alice_program_id).unwrap(), + true, + need_account_data_checked(&alice_program_id, &alice_program_id, true), + ); assert_eq!( - verify_instruction(true, &alice_program_id, &pre, &post), + verify_instruction(&alice_program_id, &pre, &post), Err(InstructionError::AccountDataSizeChanged), "non-system programs cannot change their data size" ); - let pre = Account::new_data(42, &[0], &system_program::id()).unwrap(); + let pre = PreInstructionAccount::new( + &Account::new_data(42, &[0], &system_program::id()).unwrap(), + true, + need_account_data_checked(&system_program::id(), &system_program::id(), true), + ); assert_eq!( - verify_instruction(true, &system_program::id(), &pre, &post), + verify_instruction(&system_program::id(), &pre, &post), Ok(()), "system program should be able to change acount data size" );