diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index cc337f48683572..31979d4937eb63 100644 --- a/programs/bpf/tests/programs.rs +++ b/programs/bpf/tests/programs.rs @@ -1865,9 +1865,9 @@ fn test_program_bpf_invoke_upgradeable_via_cpi() { invoke_and_return, &[0], vec![ - AccountMeta::new(program_id, false), - AccountMeta::new(program_id, false), - AccountMeta::new(clock::id(), false), + AccountMeta::new_readonly(program_id, false), + AccountMeta::new_readonly(program_id, false), + AccountMeta::new_readonly(clock::id(), false), ], ); @@ -2054,9 +2054,9 @@ fn test_program_bpf_upgrade_via_cpi() { invoke_and_return, &[0], vec![ - AccountMeta::new(program_id, false), - AccountMeta::new(program_id, false), - AccountMeta::new(clock::id(), false), + AccountMeta::new_readonly(program_id, false), + AccountMeta::new_readonly(program_id, false), + AccountMeta::new_readonly(clock::id(), false), ], ); diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 9033bfc5238e64..71e1999959d4bd 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -241,6 +241,7 @@ impl Accounts { let rent_for_sysvars = feature_set.is_active(&feature_set::rent_for_sysvars::id()); let demote_program_write_locks = feature_set.is_active(&feature_set::demote_program_write_locks::id()); + let is_upgradeable_loader_present = is_upgradeable_loader_present(message); for (i, key) in message.account_keys_iter().enumerate() { let account = if message.is_non_loader_key(i) { @@ -249,9 +250,6 @@ impl Accounts { } if solana_sdk::sysvar::instructions::check_id(key) { - if message.is_writable(i, demote_program_write_locks) { - return Err(TransactionError::InvalidAccountIndex); - } Self::construct_instructions_account( message, feature_set @@ -276,27 +274,42 @@ impl Accounts { }) .unwrap_or_default(); - if account.executable() && bpf_loader_upgradeable::check_id(account.owner()) - { - // The upgradeable loader requires the derived ProgramData account - if let Ok(UpgradeableLoaderState::Program { - programdata_address, - }) = account.state() + if bpf_loader_upgradeable::check_id(account.owner()) { + if demote_program_write_locks + && message.is_writable(i, demote_program_write_locks) + && !is_upgradeable_loader_present { - if let Some(account) = self - .accounts_db - .load_with_fixed_root(ancestors, &programdata_address) - .map(|(account, _)| account) + error_counters.invalid_writable_account += 1; + return Err(TransactionError::InvalidWritableAccount); + } + + if account.executable() { + // The upgradeable loader requires the derived ProgramData account + if let Ok(UpgradeableLoaderState::Program { + programdata_address, + }) = account.state() { - account_deps.push((programdata_address, account)); + if let Some(account) = self + .accounts_db + .load_with_fixed_root(ancestors, &programdata_address) + .map(|(account, _)| account) + { + account_deps.push((programdata_address, account)); + } else { + error_counters.account_not_found += 1; + return Err(TransactionError::ProgramAccountNotFound); + } } else { - error_counters.account_not_found += 1; - return Err(TransactionError::ProgramAccountNotFound); + error_counters.invalid_program_for_execution += 1; + return Err(TransactionError::InvalidProgramForExecution); } - } else { - error_counters.invalid_program_for_execution += 1; - return Err(TransactionError::InvalidProgramForExecution); } + } else if account.executable() + && demote_program_write_locks + && message.is_writable(i, demote_program_write_locks) + { + error_counters.invalid_writable_account += 1; + return Err(TransactionError::InvalidWritableAccount); } tx_rent += rent; @@ -1104,6 +1117,12 @@ pub fn prepare_if_nonce_account( false } +fn is_upgradeable_loader_present(message: &SanitizedMessage) -> bool { + message + .account_keys_iter() + .any(|&key| key == bpf_loader_upgradeable::id()) +} + pub fn create_test_accounts( accounts: &Accounts, pubkeys: &mut Vec, @@ -1680,6 +1699,247 @@ mod tests { assert_eq!(loaded, vec![]); } + #[test] + fn test_load_accounts_executable_with_write_lock() { + let mut accounts: Vec<(Pubkey, AccountSharedData)> = Vec::new(); + let mut error_counters = ErrorCounters::default(); + + let keypair = Keypair::new(); + let key0 = keypair.pubkey(); + let key1 = Pubkey::new(&[5u8; 32]); + let key2 = Pubkey::new(&[6u8; 32]); + + let mut account = AccountSharedData::new(1, 0, &Pubkey::default()); + account.set_rent_epoch(1); + accounts.push((key0, account)); + + let mut account = AccountSharedData::new(40, 1, &native_loader::id()); + account.set_executable(true); + account.set_rent_epoch(1); + accounts.push((key1, account)); + + let mut account = AccountSharedData::new(40, 1, &native_loader::id()); + account.set_executable(true); + account.set_rent_epoch(1); + accounts.push((key2, account)); + + let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; + let mut message = Message::new_with_compiled_instructions( + 1, + 0, + 1, // only one executable marked as readonly + vec![key0, key1, key2], + Hash::default(), + instructions, + ); + let tx = Transaction::new(&[&keypair], message.clone(), Hash::default()); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + + assert_eq!(error_counters.invalid_writable_account, 1); + assert_eq!(loaded_accounts.len(), 1); + assert_eq!( + loaded_accounts[0], + (Err(TransactionError::InvalidWritableAccount), None) + ); + + // Mark executables as readonly + message.account_keys = vec![key0, key1, key2]; // revert key change + message.header.num_readonly_unsigned_accounts = 2; // mark both executables as readonly + let tx = Transaction::new(&[&keypair], message, Hash::default()); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + + assert_eq!(error_counters.invalid_writable_account, 1); + assert_eq!(loaded_accounts.len(), 1); + let result = loaded_accounts[0].0.as_ref().unwrap(); + assert_eq!(result.accounts[..2], accounts[..2]); + assert_eq!(result.loaders[0], vec![accounts[2].clone()]); + } + + #[test] + fn test_load_accounts_upgradeable_with_write_lock() { + let mut accounts: Vec<(Pubkey, AccountSharedData)> = Vec::new(); + let mut error_counters = ErrorCounters::default(); + + let keypair = Keypair::new(); + let key0 = keypair.pubkey(); + let key1 = Pubkey::new(&[5u8; 32]); + let key2 = Pubkey::new(&[6u8; 32]); + let programdata_key1 = Pubkey::new(&[7u8; 32]); + let programdata_key2 = Pubkey::new(&[8u8; 32]); + + let mut account = AccountSharedData::new(1, 0, &Pubkey::default()); + account.set_rent_epoch(1); + accounts.push((key0, account)); + + let program_data = UpgradeableLoaderState::ProgramData { + slot: 42, + upgrade_authority_address: None, + }; + + let program = UpgradeableLoaderState::Program { + programdata_address: programdata_key1, + }; + let mut account = + AccountSharedData::new_data(40, &program, &bpf_loader_upgradeable::id()).unwrap(); + account.set_executable(true); + account.set_rent_epoch(1); + accounts.push((key1, account)); + let mut account = + AccountSharedData::new_data(40, &program_data, &bpf_loader_upgradeable::id()).unwrap(); + account.set_rent_epoch(1); + accounts.push((programdata_key1, account)); + + let program = UpgradeableLoaderState::Program { + programdata_address: programdata_key2, + }; + let mut account = + AccountSharedData::new_data(40, &program, &bpf_loader_upgradeable::id()).unwrap(); + account.set_executable(true); + account.set_rent_epoch(1); + accounts.push((key2, account)); + let mut account = + AccountSharedData::new_data(40, &program_data, &bpf_loader_upgradeable::id()).unwrap(); + account.set_rent_epoch(1); + accounts.push((programdata_key2, account)); + + let mut account = AccountSharedData::new(40, 1, &native_loader::id()); // create mock bpf_loader_upgradeable + account.set_executable(true); + account.set_rent_epoch(1); + accounts.push((bpf_loader_upgradeable::id(), account)); + + let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; + let mut message = Message::new_with_compiled_instructions( + 1, + 0, + 1, // only one executable marked as readonly + vec![key0, key1, key2], + Hash::default(), + instructions, + ); + let tx = Transaction::new(&[&keypair], message.clone(), Hash::default()); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + + assert_eq!(error_counters.invalid_writable_account, 1); + assert_eq!(loaded_accounts.len(), 1); + assert_eq!( + loaded_accounts[0], + (Err(TransactionError::InvalidWritableAccount), None) + ); + + // Solution 1: include bpf_loader_upgradeable account + message.account_keys = vec![key0, key1, bpf_loader_upgradeable::id()]; + let tx = Transaction::new(&[&keypair], message.clone(), Hash::default()); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + + assert_eq!(error_counters.invalid_writable_account, 1); + assert_eq!(loaded_accounts.len(), 1); + let result = loaded_accounts[0].0.as_ref().unwrap(); + assert_eq!(result.accounts[..2], accounts[..2]); + assert_eq!(result.loaders[0], vec![accounts[5].clone()]); + + // Solution 2: mark programdata as readonly + message.account_keys = vec![key0, key1, key2]; // revert key change + message.header.num_readonly_unsigned_accounts = 2; // mark both executables as readonly + let tx = Transaction::new(&[&keypair], message, Hash::default()); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + + assert_eq!(error_counters.invalid_writable_account, 1); + assert_eq!(loaded_accounts.len(), 1); + let result = loaded_accounts[0].0.as_ref().unwrap(); + assert_eq!(result.accounts[..2], accounts[..2]); + assert_eq!( + result.loaders[0], + vec![ + accounts[5].clone(), + accounts[3].clone(), + accounts[4].clone() + ] + ); + } + + #[test] + fn test_load_accounts_programdata_with_write_lock() { + let mut accounts: Vec<(Pubkey, AccountSharedData)> = Vec::new(); + let mut error_counters = ErrorCounters::default(); + + let keypair = Keypair::new(); + let key0 = keypair.pubkey(); + let key1 = Pubkey::new(&[5u8; 32]); + let key2 = Pubkey::new(&[6u8; 32]); + + let mut account = AccountSharedData::new(1, 0, &Pubkey::default()); + account.set_rent_epoch(1); + accounts.push((key0, account)); + + let program_data = UpgradeableLoaderState::ProgramData { + slot: 42, + upgrade_authority_address: None, + }; + let mut account = + AccountSharedData::new_data(40, &program_data, &bpf_loader_upgradeable::id()).unwrap(); + account.set_rent_epoch(1); + accounts.push((key1, account)); + + let mut account = AccountSharedData::new(40, 1, &native_loader::id()); + account.set_executable(true); + account.set_rent_epoch(1); + accounts.push((key2, account)); + + let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; + let mut message = Message::new_with_compiled_instructions( + 1, + 0, + 1, // only the program marked as readonly + vec![key0, key1, key2], + Hash::default(), + instructions, + ); + let tx = Transaction::new(&[&keypair], message.clone(), Hash::default()); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + + assert_eq!(error_counters.invalid_writable_account, 1); + assert_eq!(loaded_accounts.len(), 1); + assert_eq!( + loaded_accounts[0], + (Err(TransactionError::InvalidWritableAccount), None) + ); + + // Solution 1: include bpf_loader_upgradeable account + let mut account = AccountSharedData::new(40, 1, &native_loader::id()); // create mock bpf_loader_upgradeable + account.set_executable(true); + account.set_rent_epoch(1); + let accounts_with_upgradeable_loader = vec![ + accounts[0].clone(), + accounts[1].clone(), + (bpf_loader_upgradeable::id(), account), + ]; + message.account_keys = vec![key0, key1, bpf_loader_upgradeable::id()]; + let tx = Transaction::new(&[&keypair], message.clone(), Hash::default()); + let loaded_accounts = + load_accounts(tx, &accounts_with_upgradeable_loader, &mut error_counters); + + assert_eq!(error_counters.invalid_writable_account, 1); + assert_eq!(loaded_accounts.len(), 1); + let result = loaded_accounts[0].0.as_ref().unwrap(); + assert_eq!(result.accounts[..2], accounts_with_upgradeable_loader[..2]); + assert_eq!( + result.loaders[0], + vec![accounts_with_upgradeable_loader[2].clone()] + ); + + // Solution 2: mark programdata as readonly + message.account_keys = vec![key0, key1, key2]; // revert key change + message.header.num_readonly_unsigned_accounts = 2; // extend readonly set to include programdata + let tx = Transaction::new(&[&keypair], message, Hash::default()); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + + assert_eq!(error_counters.invalid_writable_account, 1); + assert_eq!(loaded_accounts.len(), 1); + let result = loaded_accounts[0].0.as_ref().unwrap(); + assert_eq!(result.accounts[..2], accounts[..2]); + assert_eq!(result.loaders[0], vec![accounts[2].clone()]); + } + #[test] fn test_accounts_account_not_found() { let accounts = Accounts::new_with_config_for_tests( diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index cfde6f540712ea..480b50d7cb73d0 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -193,6 +193,7 @@ pub struct ErrorCounters { pub invalid_account_index: usize, pub invalid_program_for_execution: usize, pub not_allowed_during_cluster_maintenance: usize, + pub invalid_writable_account: usize, } #[derive(Default, Debug)] diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 7e01ceb8bcf945..5979f3604c14d9 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -192,7 +192,7 @@ impl ExecuteTimings { } type BankStatusCache = StatusCache>; -#[frozen_abi(digest = "GT81Hdwrh73i55ScQvFqmzeHfUL42yxuavZods8VyzGc")] +#[frozen_abi(digest = "5Br3PNyyX1L7XoS4jYLt5JTeMXowLSsu7v9LhokC8vnq")] pub type BankSlotDelta = SlotDelta>; type TransactionAccountRefCells = Vec<(Pubkey, Rc>)>; type TransactionLoaderRefCells = Vec>)>>; @@ -3122,6 +3122,12 @@ impl Bank { error_counters.not_allowed_during_cluster_maintenance ); } + if 0 != error_counters.invalid_writable_account { + inc_new_counter_info!( + "bank-process_transactions-error-invalid_writable_account", + error_counters.invalid_writable_account + ); + } } /// Converts Accounts into RefCell, this involves moving diff --git a/sdk/src/transaction/mod.rs b/sdk/src/transaction/mod.rs index 14e4217f68b6b7..a0354c751040b9 100644 --- a/sdk/src/transaction/mod.rs +++ b/sdk/src/transaction/mod.rs @@ -119,6 +119,10 @@ pub enum TransactionError { /// Transaction version is unsupported #[error("Transaction version is unsupported")] UnsupportedVersion, + + /// Transaction loads a writable account that cannot be written + #[error("Transaction loads a writable account that cannot be written")] + InvalidWritableAccount, } pub type Result = result::Result; diff --git a/storage-proto/proto/transaction_by_addr.proto b/storage-proto/proto/transaction_by_addr.proto index b8d93ccb325b01..0278d81d7c84c8 100644 --- a/storage-proto/proto/transaction_by_addr.proto +++ b/storage-proto/proto/transaction_by_addr.proto @@ -43,6 +43,7 @@ enum TransactionErrorType { ACCOUNT_BORROW_OUTSTANDING_TX = 16; WOULD_EXCEED_MAX_BLOCK_COST_LIMIT = 17; UNSUPPORTED_VERSION = 18; + INVALID_WRITABLE_ACCOUNT = 19; } message InstructionError { diff --git a/storage-proto/src/convert.rs b/storage-proto/src/convert.rs index c4361c12e5cc97..b0def567f445db 100644 --- a/storage-proto/src/convert.rs +++ b/storage-proto/src/convert.rs @@ -550,6 +550,7 @@ impl TryFrom for TransactionError { 16 => TransactionError::AccountBorrowOutstanding, 17 => TransactionError::WouldExceedMaxBlockCostLimit, 18 => TransactionError::UnsupportedVersion, + 19 => TransactionError::InvalidWritableAccount, _ => return Err("Invalid TransactionError"), }) } @@ -614,6 +615,9 @@ impl From for tx_by_addr::TransactionError { TransactionError::UnsupportedVersion => { tx_by_addr::TransactionErrorType::UnsupportedVersion } + TransactionError::InvalidWritableAccount => { + tx_by_addr::TransactionErrorType::InvalidWritableAccount + } } as i32, instruction_error: match transaction_error { TransactionError::InstructionError(index, ref instruction_error) => {