gaspersic/solana
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Only copy whats needed to verify an instruction after processing (#6669)

Browse Source
This commit is contained in:
Jack May 2019-11-05 10:57:32 -08:00 committed by GitHub
parent 08973f9f05
commit b9d8e3e55a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 187 additions and 103 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
runtime/benches/message_processor.rs
View File

@ -21,23 +21,34 @@ fn bench_verify_instruction_data(bencher: &mut Bencher) {
let owner = Pubkey::new_rand(); let owner = Pubkey::new_rand();
let non_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); let post = Account::new(0, BUFSIZE, &owner);
assert_eq!(verify_instruction(true, &owner, &pre, &post), Ok(())); assert_eq!(verify_instruction(&owner, &pre, &post), Ok(()));
bencher.iter(|| pre.data == post.data);
let summary = bencher.bench(|_bencher| {}).unwrap();
info!("data compare {} ns/iter", summary.median);
// this one should be faster // this one should be faster
bencher.iter(|| { bencher.iter(|| {
verify_instruction(true, &owner, &pre, &post).unwrap(); verify_instruction(&owner, &pre, &post).unwrap();
}); });
let summary = bencher.bench(|_bencher| {}).unwrap(); let summary = bencher.bench(|_bencher| {}).unwrap();
info!("data no change by owner: {} ns/iter", summary.median); 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(|| { bencher.iter(|| {
verify_instruction(true, &non_owner, &pre, &post).unwrap(); verify_instruction(&non_owner, &pre, &post).unwrap();
}); });
let summary = bencher.bench(|_bencher| {}).unwrap(); let summary = bencher.bench(|_bencher| {}).unwrap();
info!("data no change by non owner: {} ns/iter", summary.median); info!("data no change by non owner: {} ns/iter", summary.median);

263
runtime/src/message_processor.rs
View File

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