From f37f83fd12cd14f08950e69ca2ca7c45ecb169f6 Mon Sep 17 00:00:00 2001 From: sakridge Date: Sat, 2 May 2020 08:07:52 -0700 Subject: [PATCH] Fuzzer test and fixes (#9853) --- runtime/src/accounts.rs | 34 +++--- runtime/src/bank.rs | 227 +++++++++++++++++++++++++++++++++++----- sdk/src/transaction.rs | 5 + 3 files changed, 227 insertions(+), 39 deletions(-) diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 4c65e4e3fe..0b95a73280 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -143,12 +143,6 @@ impl Accounts { if tx.signatures.is_empty() && fee != 0 { Err(TransactionError::MissingSignatureForFee) } else { - // Check for unique account keys - if Self::has_duplicates(&message.account_keys) { - error_counters.account_loaded_twice += 1; - return Err(TransactionError::AccountLoadedTwice); - } - // There is no way to predict what program will execute without an error // If a fee can pay for execution then the program will be scheduled let mut payer_index = None; @@ -535,10 +529,12 @@ impl Accounts { } fn unlock_account(&self, tx: &Transaction, result: &Result<()>, locks: &mut HashSet) { - let (writable_keys, readonly_keys) = &tx.message().get_account_keys_by_lock_type(); match result { Err(TransactionError::AccountInUse) => (), + Err(TransactionError::SanitizeFailure) => (), + Err(TransactionError::AccountLoadedTwice) => (), _ => { + let (writable_keys, readonly_keys) = &tx.message().get_account_keys_by_lock_type(); for k in writable_keys { locks.remove(k); } @@ -572,13 +568,26 @@ impl Accounts { txs: &[Transaction], txs_iteration_order: Option<&[usize]>, ) -> Vec> { - let keys: Vec<_> = OrderedIterator::new(txs, txs_iteration_order) - .map(|tx| tx.message().get_account_keys_by_lock_type()) + use solana_sdk::sanitize::Sanitize; + let keys: Vec> = OrderedIterator::new(txs, txs_iteration_order) + .map(|tx| { + tx.sanitize() + .map_err(|_| TransactionError::SanitizeFailure)?; + + if Self::has_duplicates(&tx.message.account_keys) { + return Err(TransactionError::AccountLoadedTwice); + } + + Ok(tx.message().get_account_keys_by_lock_type()) + }) .collect(); let mut account_locks = &mut self.account_locks.lock().unwrap(); keys.into_iter() - .map(|(writable_keys, readonly_keys)| { - self.lock_account(&mut account_locks, writable_keys, readonly_keys) + .map(|result| match result { + Ok((writable_keys, readonly_keys)) => { + self.lock_account(&mut account_locks, writable_keys, readonly_keys) + } + Err(e) => Err(e), }) .collect() } @@ -1344,12 +1353,11 @@ mod tests { ); let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); - assert_eq!(error_counters.account_loaded_twice, 1); assert_eq!(loaded_accounts.len(), 1); assert_eq!( loaded_accounts[0], ( - Err(TransactionError::AccountLoadedTwice), + Err(TransactionError::InvalidAccountForFee), Some(HashAgeKind::Extant) ) ); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 7a20104020..833b1284a6 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -46,7 +46,6 @@ use solana_sdk::{ inflation::Inflation, native_loader, nonce, pubkey::Pubkey, - sanitize::Sanitize, signature::{Keypair, Signature}, slot_hashes::SlotHashes, slot_history::SlotHistory, @@ -1071,25 +1070,6 @@ impl Bank { &self.rent_collector, ) } - fn check_refs( - &self, - txs: &[Transaction], - iteration_order: Option<&[usize]>, - lock_results: &[Result<()>], - error_counters: &mut ErrorCounters, - ) -> Vec> { - OrderedIterator::new(txs, iteration_order) - .zip(lock_results) - .map(|(tx, lock_res)| { - if lock_res.is_ok() && tx.sanitize().is_err() { - error_counters.invalid_account_index += 1; - Err(TransactionError::InvalidAccountIndex) - } else { - lock_res.clone() - } - }) - .collect() - } fn check_age( &self, txs: &[Transaction], @@ -1185,11 +1165,10 @@ impl Bank { max_age: usize, mut error_counters: &mut ErrorCounters, ) -> Vec { - let refs_results = self.check_refs(txs, iteration_order, lock_results, &mut error_counters); let age_results = self.check_age( txs, iteration_order, - refs_results, + lock_results.to_vec(), max_age, &mut error_counters, ); @@ -3502,6 +3481,7 @@ mod tests { #[test] fn test_account_not_found() { + solana_logger::setup(); let (genesis_config, mint_keypair) = create_genesis_config(0); let bank = Bank::new(&genesis_config); let keypair = Keypair::new(); @@ -4007,14 +3987,14 @@ mod tests { tx_invalid_program_index.message.instructions[0].program_id_index = 42; assert_eq!( bank.process_transaction(&tx_invalid_program_index), - Err(TransactionError::InvalidAccountIndex) + Err(TransactionError::SanitizeFailure) ); let mut tx_invalid_account_index = tx.clone(); tx_invalid_account_index.message.instructions[0].accounts[0] = 42; assert_eq!( bank.process_transaction(&tx_invalid_account_index), - Err(TransactionError::InvalidAccountIndex) + Err(TransactionError::SanitizeFailure) ); } @@ -4605,7 +4585,7 @@ mod tests { assert_eq!( bank.process_transaction(&tx), - Err(TransactionError::InvalidAccountIndex) + Err(TransactionError::SanitizeFailure) ); assert_eq!(bank.get_balance(&key.pubkey()), 0); } @@ -5883,6 +5863,39 @@ mod tests { assert_eq!(bank.capitalization(), pre_capitalization - burn_amount); } + #[test] + fn test_duplicate_account_key() { + solana_logger::setup(); + let (genesis_config, mint_keypair) = create_genesis_config(500); + let mut bank = Bank::new(&genesis_config); + + let from_pubkey = Pubkey::new_rand(); + let to_pubkey = Pubkey::new_rand(); + + let account_metas = vec![ + AccountMeta::new(from_pubkey, false), + AccountMeta::new(to_pubkey, false), + ]; + + bank.add_static_program( + "mock_vote", + solana_vote_program::id(), + mock_ok_vote_processor, + ); + + let instruction = Instruction::new(solana_vote_program::id(), &10, account_metas); + let mut tx = Transaction::new_signed_with_payer( + &[instruction], + Some(&mint_keypair.pubkey()), + &[&mint_keypair], + bank.last_blockhash(), + ); + tx.message.account_keys.push(from_pubkey); + + let result = bank.process_transaction(&tx); + assert_eq!(result, Err(TransactionError::AccountLoadedTwice)); + } + #[test] fn test_program_id_as_payer() { solana_logger::setup(); @@ -5928,7 +5941,7 @@ mod tests { tx.message.instructions[0].accounts.push(3); let result = bank.process_transaction(&tx); - assert_eq!(result, Err(TransactionError::InvalidAccountIndex)); + assert_eq!(result, Err(TransactionError::SanitizeFailure)); } fn mock_ok_vote_processor( @@ -5974,4 +5987,166 @@ mod tests { let result = bank.process_transaction(&tx); assert_eq!(result, Ok(())); } + + #[test] + fn test_fuzz_instructions() { + solana_logger::setup(); + use rand::{thread_rng, Rng}; + let (genesis_config, _mint_keypair) = create_genesis_config(1_000_000_000); + let mut bank = Bank::new(&genesis_config); + + let max_programs = 5; + let program_keys: Vec<_> = (0..max_programs) + .into_iter() + .enumerate() + .map(|i| { + let key = Pubkey::new_rand(); + let name = format!("program{:?}", i); + bank.add_static_program(&name, key, mock_ok_vote_processor); + (key, name.as_bytes().to_vec()) + }) + .collect(); + let max_keys = 100; + let keys: Vec<_> = (0..max_keys) + .into_iter() + .enumerate() + .map(|_| { + let key = Pubkey::new_rand(); + let balance = if thread_rng().gen_ratio(9, 10) { + let lamports = if thread_rng().gen_ratio(1, 5) { + thread_rng().gen_range(0, 10) + } else { + thread_rng().gen_range(20, 100) + }; + let space = thread_rng().gen_range(0, 10); + let owner = Pubkey::default(); + let account = Account::new(lamports, space, &owner); + bank.store_account(&key, &account); + lamports + } else { + 0 + }; + (key, balance) + }) + .collect(); + let mut results = HashMap::new(); + for _ in 0..2_000 { + let num_keys = if thread_rng().gen_ratio(1, 5) { + thread_rng().gen_range(0, max_keys) + } else { + thread_rng().gen_range(1, 4) + }; + let num_instructions = thread_rng().gen_range(0, max_keys - num_keys); + + let mut account_keys: Vec<_> = if thread_rng().gen_ratio(1, 5) { + (0..num_keys) + .into_iter() + .map(|_| { + let idx = thread_rng().gen_range(0, keys.len()); + keys[idx].0 + }) + .collect() + } else { + use std::collections::HashSet; + let mut inserted = HashSet::new(); + (0..num_keys) + .into_iter() + .map(|_| { + let mut idx; + loop { + idx = thread_rng().gen_range(0, keys.len()); + if !inserted.contains(&idx) { + break; + } + } + inserted.insert(idx); + keys[idx].0 + }) + .collect() + }; + + let instructions: Vec<_> = if num_keys > 0 { + (0..num_instructions) + .into_iter() + .map(|_| { + let num_accounts_to_pass = thread_rng().gen_range(0, num_keys); + let account_indexes = (0..num_accounts_to_pass) + .into_iter() + .map(|_| thread_rng().gen_range(0, num_keys)) + .collect(); + let program_index: u8 = thread_rng().gen_range(0, num_keys) as u8; + if thread_rng().gen_ratio(4, 5) { + let programs_index = thread_rng().gen_range(0, program_keys.len()); + account_keys[program_index as usize] = program_keys[programs_index].0; + } + CompiledInstruction::new(program_index, &10, account_indexes) + }) + .collect() + } else { + vec![] + }; + + let account_keys_len = std::cmp::max(account_keys.len(), 2); + let num_signatures = if thread_rng().gen_ratio(1, 5) { + thread_rng().gen_range(0, account_keys_len + 10) + } else { + thread_rng().gen_range(1, account_keys_len) + }; + + let num_required_signatures = if thread_rng().gen_ratio(1, 5) { + thread_rng().gen_range(0, account_keys_len + 10) as u8 + } else { + thread_rng().gen_range(1, std::cmp::max(2, num_signatures)) as u8 + }; + let num_readonly_signed_accounts = if thread_rng().gen_ratio(1, 5) { + thread_rng().gen_range(0, account_keys_len) as u8 + } else { + let max = if num_required_signatures > 1 { + num_required_signatures - 1 + } else { + 1 + }; + thread_rng().gen_range(0, max) as u8 + }; + + let num_readonly_unsigned_accounts = if thread_rng().gen_ratio(1, 5) + || (num_required_signatures as usize) >= account_keys_len + { + thread_rng().gen_range(0, account_keys_len) as u8 + } else { + thread_rng().gen_range(0, account_keys_len - num_required_signatures as usize) as u8 + }; + + let header = MessageHeader { + num_required_signatures, + num_readonly_signed_accounts, + num_readonly_unsigned_accounts, + }; + let message = Message { + header, + account_keys, + recent_blockhash: bank.last_blockhash(), + instructions, + }; + + let tx = Transaction { + signatures: vec![Signature::default(); num_signatures], + message, + }; + + let result = bank.process_transaction(&tx); + for (key, balance) in &keys { + assert_eq!(bank.get_balance(key), *balance); + } + for (key, name) in &program_keys { + let account = bank.get_account(key).unwrap(); + assert!(account.executable); + assert_eq!(account.data, *name); + } + info!("result: {:?}", result); + let result_key = format!("{:?}", result); + *results.entry(result_key).or_insert(0) += 1; + } + info!("results: {:?}", results); + } } diff --git a/sdk/src/transaction.rs b/sdk/src/transaction.rs index 4b3ab36621..ff3f6168c6 100644 --- a/sdk/src/transaction.rs +++ b/sdk/src/transaction.rs @@ -64,6 +64,11 @@ pub enum TransactionError { /// This program may not be used for executing instructions InvalidProgramForExecution, + + /// Transaction failed to sanitize accounts offsets correctly + /// implies that account locks are not taken for this TX, and should + /// not be unlocked. + SanitizeFailure, } pub type Result = result::Result;