From 757e46c3c7d93f40c6d733b0ca07996f3582cd8e Mon Sep 17 00:00:00 2001 From: Brooks Prumo Date: Mon, 29 Aug 2022 14:30:48 -0400 Subject: [PATCH] Set cap for new allocations per transaction (#27385) --- program-runtime/src/invoke_context.rs | 2 + programs/bpf_loader/src/lib.rs | 13 ++-- runtime/src/bank.rs | 64 ++++++++++++++++++- sdk/program/src/instruction.rs | 6 +- sdk/program/src/program_error.rs | 22 ++++--- sdk/program/src/system_instruction.rs | 7 ++ sdk/src/feature_set.rs | 5 ++ sdk/src/transaction_context.rs | 29 ++++++++- storage-proto/proto/transaction_by_addr.proto | 2 +- storage-proto/src/convert.rs | 6 +- 10 files changed, 131 insertions(+), 25 deletions(-) diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index ca3582c7a..c3c2852f8 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -1028,6 +1028,7 @@ pub fn with_mock_invoke_context R>( ComputeBudget::default().max_invoke_depth.saturating_add(1), 1, ); + transaction_context.enable_cap_accounts_data_allocations_per_transaction(); let mut invoke_context = InvokeContext::new_mock(&mut transaction_context, &[]); invoke_context .push(&preparation.instruction_accounts, &program_indices, &[]) @@ -1059,6 +1060,7 @@ pub fn mock_process_instruction( ComputeBudget::default().max_invoke_depth.saturating_add(1), 1, ); + transaction_context.enable_cap_accounts_data_allocations_per_transaction(); let mut invoke_context = InvokeContext::new_mock(&mut transaction_context, &[]); if let Some(sysvar_cache) = sysvar_cache_override { invoke_context.sysvar_cache = Cow::Borrowed(sysvar_cache); diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index ee629bc91..a01ee3ad6 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -41,7 +41,7 @@ use { bpf_loader_upgradeable::{self, UpgradeableLoaderState}, entrypoint::{HEAP_LENGTH, SUCCESS}, feature_set::{ - cap_accounts_data_len, cap_bpf_program_instruction_accounts, + cap_accounts_data_allocations_per_transaction, cap_bpf_program_instruction_accounts, disable_deploy_of_alloc_free_syscall, disable_deprecated_loader, enable_bpf_loader_extend_program_data_ix, error_on_syscall_bpf_function_hash_collisions, reject_callx_r10, @@ -49,7 +49,7 @@ use { instruction::{AccountMeta, InstructionError}, loader_instruction::LoaderInstruction, loader_upgradeable_instruction::UpgradeableLoaderInstruction, - program_error::MAX_ACCOUNTS_DATA_SIZE_EXCEEDED, + program_error::MAX_ACCOUNTS_DATA_ALLOCATIONS_EXCEEDED, program_utils::limited_deserialize, pubkey::Pubkey, saturating_add_assign, @@ -1339,13 +1339,14 @@ impl Executor for BpfExecutor { } match result { Ok(status) if status != SUCCESS => { - let error: InstructionError = if status == MAX_ACCOUNTS_DATA_SIZE_EXCEEDED + let error: InstructionError = if status + == MAX_ACCOUNTS_DATA_ALLOCATIONS_EXCEEDED && !invoke_context .feature_set - .is_active(&cap_accounts_data_len::id()) + .is_active(&cap_accounts_data_allocations_per_transaction::id()) { - // Until the cap_accounts_data_len feature is enabled, map the - // MAX_ACCOUNTS_DATA_SIZE_EXCEEDED error to InvalidError + // Until the cap_accounts_data_allocations_per_transaction feature is + // enabled, map the MAX_ACCOUNTS_DATA_ALLOCATIONS_EXCEEDED error to InvalidError InstructionError::InvalidError } else { status.into() diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index aa967d7da..b5390af19 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -284,7 +284,7 @@ impl RentDebits { } pub type BankStatusCache = StatusCache>; -#[frozen_abi(digest = "HEJXoycXvGT2pwMuKcUKzzbeemnqbfrUC4jHZx1ncaWv")] +#[frozen_abi(digest = "7FSSacrCi7vf2QZFm3Ui9JqTii4U6h1XWYD3LKSuVwV8")] pub type BankSlotDelta = SlotDelta>; // Eager rent collection repeats in cyclic manner. @@ -4352,6 +4352,12 @@ impl Bank { compute_budget.max_invoke_depth.saturating_add(1), tx.message().instructions().len(), ); + if self + .feature_set + .is_active(&feature_set::cap_accounts_data_allocations_per_transaction::id()) + { + transaction_context.enable_cap_accounts_data_allocations_per_transaction(); + } let pre_account_state_info = self.get_transaction_account_state_info(&transaction_context, tx.message()); @@ -8038,7 +8044,10 @@ pub(crate) mod tests { instruction as stake_instruction, state::{Authorized, Delegation, Lockup, Stake}, }, - system_instruction::{self, SystemError, MAX_PERMITTED_DATA_LENGTH}, + system_instruction::{ + self, SystemError, MAX_PERMITTED_ACCOUNTS_DATA_ALLOCATIONS_PER_TRANSACTION, + MAX_PERMITTED_DATA_LENGTH, + }, system_program, timing::duration_as_s, transaction::MAX_TX_ACCOUNT_LOCKS, @@ -19482,4 +19491,55 @@ pub(crate) mod tests { ); } } + + /// Ensures that if a transaction exceeds the maximum allowed accounts data allocation size: + /// 1. The transaction fails + /// 2. The bank's accounts_data_size is unmodified + #[test] + fn test_cap_accounts_data_allocations_per_transaction() { + use solana_sdk::signature::KeypairInsecureClone; + const NUM_MAX_SIZE_ALLOCATIONS_PER_TRANSACTION: usize = + MAX_PERMITTED_ACCOUNTS_DATA_ALLOCATIONS_PER_TRANSACTION as usize + / MAX_PERMITTED_DATA_LENGTH as usize; + + let (genesis_config, mint_keypair) = create_genesis_config(1_000_000 * LAMPORTS_PER_SOL); + let mut bank = Bank::new_for_tests(&genesis_config); + bank.activate_feature( + &feature_set::enable_early_verification_of_account_modifications::id(), + ); + bank.activate_feature(&feature_set::cap_accounts_data_allocations_per_transaction::id()); + + let mut instructions = Vec::new(); + let mut keypairs = vec![mint_keypair.clone()]; + for _ in 0..=NUM_MAX_SIZE_ALLOCATIONS_PER_TRANSACTION { + let keypair = Keypair::new(); + let instruction = system_instruction::create_account( + &mint_keypair.pubkey(), + &keypair.pubkey(), + bank.rent_collector() + .rent + .minimum_balance(MAX_PERMITTED_DATA_LENGTH as usize), + MAX_PERMITTED_DATA_LENGTH, + &solana_sdk::system_program::id(), + ); + keypairs.push(keypair); + instructions.push(instruction); + } + let message = Message::new(&instructions, Some(&mint_keypair.pubkey())); + let signers: Vec<_> = keypairs.iter().collect(); + let transaction = Transaction::new(&signers, message, bank.last_blockhash()); + + let accounts_data_size_before = bank.load_accounts_data_size(); + let result = bank.process_transaction(&transaction); + let accounts_data_size_after = bank.load_accounts_data_size(); + + assert_eq!(accounts_data_size_before, accounts_data_size_after); + assert_eq!( + result, + Err(TransactionError::InstructionError( + NUM_MAX_SIZE_ALLOCATIONS_PER_TRANSACTION as u8, + solana_sdk::instruction::InstructionError::MaxAccountsDataAllocationsExceeded, + )), + ); + } } diff --git a/sdk/program/src/instruction.rs b/sdk/program/src/instruction.rs index 3a6ff0e0b..8f53646ed 100644 --- a/sdk/program/src/instruction.rs +++ b/sdk/program/src/instruction.rs @@ -249,9 +249,9 @@ pub enum InstructionError { #[error("Provided owner is not allowed")] IllegalOwner, - /// Account data allocation exceeded the maximum accounts data size limit - #[error("Account data allocation exceeded the maximum accounts data size limit")] - MaxAccountsDataSizeExceeded, + /// Accounts data allocations exceeded the maximum allowed per transaction + #[error("Accounts data allocations exceeded the maximum allowed per transaction")] + MaxAccountsDataAllocationsExceeded, /// Max accounts exceeded #[error("Max accounts exceeded")] diff --git a/sdk/program/src/program_error.rs b/sdk/program/src/program_error.rs index 37cb58f60..c12d42eda 100644 --- a/sdk/program/src/program_error.rs +++ b/sdk/program/src/program_error.rs @@ -51,8 +51,8 @@ pub enum ProgramError { UnsupportedSysvar, #[error("Provided owner is not allowed")] IllegalOwner, - #[error("Account data allocation exceeded the maximum accounts data size limit")] - MaxAccountsDataSizeExceeded, + #[error("Accounts data allocations exceeded the maximum allowed per transaction")] + MaxAccountsDataAllocationsExceeded, #[error("Account data reallocation was invalid")] InvalidRealloc, } @@ -93,7 +93,9 @@ impl PrintProgramError for ProgramError { Self::AccountNotRentExempt => msg!("Error: AccountNotRentExempt"), Self::UnsupportedSysvar => msg!("Error: UnsupportedSysvar"), Self::IllegalOwner => msg!("Error: IllegalOwner"), - Self::MaxAccountsDataSizeExceeded => msg!("Error: MaxAccountsDataSizeExceeded"), + Self::MaxAccountsDataAllocationsExceeded => { + msg!("Error: MaxAccountsDataAllocationsExceeded") + } Self::InvalidRealloc => msg!("Error: InvalidRealloc"), } } @@ -125,7 +127,7 @@ pub const BORSH_IO_ERROR: u64 = to_builtin!(15); pub const ACCOUNT_NOT_RENT_EXEMPT: u64 = to_builtin!(16); pub const UNSUPPORTED_SYSVAR: u64 = to_builtin!(17); pub const ILLEGAL_OWNER: u64 = to_builtin!(18); -pub const MAX_ACCOUNTS_DATA_SIZE_EXCEEDED: u64 = to_builtin!(19); +pub const MAX_ACCOUNTS_DATA_ALLOCATIONS_EXCEEDED: u64 = to_builtin!(19); pub const INVALID_ACCOUNT_DATA_REALLOC: u64 = to_builtin!(20); // Warning: Any new program errors added here must also be: // - Added to the below conversions @@ -153,7 +155,9 @@ impl From for u64 { ProgramError::AccountNotRentExempt => ACCOUNT_NOT_RENT_EXEMPT, ProgramError::UnsupportedSysvar => UNSUPPORTED_SYSVAR, ProgramError::IllegalOwner => ILLEGAL_OWNER, - ProgramError::MaxAccountsDataSizeExceeded => MAX_ACCOUNTS_DATA_SIZE_EXCEEDED, + ProgramError::MaxAccountsDataAllocationsExceeded => { + MAX_ACCOUNTS_DATA_ALLOCATIONS_EXCEEDED + } ProgramError::InvalidRealloc => INVALID_ACCOUNT_DATA_REALLOC, ProgramError::Custom(error) => { if error == 0 { @@ -187,7 +191,7 @@ impl From for ProgramError { ACCOUNT_NOT_RENT_EXEMPT => Self::AccountNotRentExempt, UNSUPPORTED_SYSVAR => Self::UnsupportedSysvar, ILLEGAL_OWNER => Self::IllegalOwner, - MAX_ACCOUNTS_DATA_SIZE_EXCEEDED => Self::MaxAccountsDataSizeExceeded, + MAX_ACCOUNTS_DATA_ALLOCATIONS_EXCEEDED => Self::MaxAccountsDataAllocationsExceeded, INVALID_ACCOUNT_DATA_REALLOC => Self::InvalidRealloc, _ => Self::Custom(error as u32), } @@ -217,7 +221,9 @@ impl TryFrom for ProgramError { Self::Error::AccountNotRentExempt => Ok(Self::AccountNotRentExempt), Self::Error::UnsupportedSysvar => Ok(Self::UnsupportedSysvar), Self::Error::IllegalOwner => Ok(Self::IllegalOwner), - Self::Error::MaxAccountsDataSizeExceeded => Ok(Self::MaxAccountsDataSizeExceeded), + Self::Error::MaxAccountsDataAllocationsExceeded => { + Ok(Self::MaxAccountsDataAllocationsExceeded) + } Self::Error::InvalidRealloc => Ok(Self::InvalidRealloc), _ => Err(error), } @@ -249,7 +255,7 @@ where ACCOUNT_NOT_RENT_EXEMPT => Self::AccountNotRentExempt, UNSUPPORTED_SYSVAR => Self::UnsupportedSysvar, ILLEGAL_OWNER => Self::IllegalOwner, - MAX_ACCOUNTS_DATA_SIZE_EXCEEDED => Self::MaxAccountsDataSizeExceeded, + MAX_ACCOUNTS_DATA_ALLOCATIONS_EXCEEDED => Self::MaxAccountsDataAllocationsExceeded, INVALID_ACCOUNT_DATA_REALLOC => Self::InvalidRealloc, _ => { // A valid custom error has no bits set in the upper 32 diff --git a/sdk/program/src/system_instruction.rs b/sdk/program/src/system_instruction.rs index 9ddc2e2cf..a4c4b10cf 100644 --- a/sdk/program/src/system_instruction.rs +++ b/sdk/program/src/system_instruction.rs @@ -142,6 +142,13 @@ pub fn instruction_to_nonce_error( /// Maximum permitted size of data: 10 MiB pub const MAX_PERMITTED_DATA_LENGTH: u64 = 10 * 1024 * 1024; +/// Maximum permitted size of new allocations per transaction, in bytes +/// +/// The value was chosen such at least one max sized account could be created, +/// plus some additional resize allocations. +pub const MAX_PERMITTED_ACCOUNTS_DATA_ALLOCATIONS_PER_TRANSACTION: i64 = + MAX_PERMITTED_DATA_LENGTH as i64 * 2; + // SBF program entrypoint assumes that the max account data length // will fit inside a u32. If this constant no longer fits in a u32, // the entrypoint deserialization code in the SDK must be updated. diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 790e615b2..fc0573dbd 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -510,6 +510,10 @@ pub mod vote_state_update_root_fix { solana_sdk::declare_id!("G74BkWBzmsByZ1kxHy44H3wjwp5hp7JbrGRuDpco22tY"); } +pub mod cap_accounts_data_allocations_per_transaction { + solana_sdk::declare_id!("9gxu85LYRAcZL38We8MYJ4A9AwgBBPtVBAqebMcT1241"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -632,6 +636,7 @@ lazy_static! { (relax_authority_signer_check_for_lookup_table_creation::id(), "relax authority signer check for lookup table creation #27205"), (stop_sibling_instruction_search_at_parent::id(), "stop the search in get_processed_sibling_instruction when the parent instruction is reached #27289"), (vote_state_update_root_fix::id(), "fix root in vote state updates #27361"), + (cap_accounts_data_allocations_per_transaction::id(), "cap accounts data allocations per transaction #27375"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter() diff --git a/sdk/src/transaction_context.rs b/sdk/src/transaction_context.rs index 0c944787c..276f319c6 100644 --- a/sdk/src/transaction_context.rs +++ b/sdk/src/transaction_context.rs @@ -6,7 +6,9 @@ use { instruction::InstructionError, pubkey::Pubkey, rent::Rent, - system_instruction::MAX_PERMITTED_DATA_LENGTH, + system_instruction::{ + MAX_PERMITTED_ACCOUNTS_DATA_ALLOCATIONS_PER_TRANSACTION, MAX_PERMITTED_DATA_LENGTH, + }, }, std::{ cell::{RefCell, RefMut}, @@ -52,6 +54,7 @@ pub struct TransactionContext { return_data: TransactionReturnData, accounts_resize_delta: RefCell, rent: Option, + is_cap_accounts_data_allocations_per_transaction_enabled: bool, } impl TransactionContext { @@ -78,6 +81,7 @@ impl TransactionContext { return_data: TransactionReturnData::default(), accounts_resize_delta: RefCell::new(0), rent, + is_cap_accounts_data_allocations_per_transaction_enabled: false, } } @@ -312,6 +316,11 @@ impl TransactionContext { .map_err(|_| InstructionError::GenericError) .map(|value_ref| *value_ref) } + + /// Enables enforcing a maximum accounts data allocation size per transaction + pub fn enable_cap_accounts_data_allocations_per_transaction(&mut self) { + self.is_cap_accounts_data_allocations_per_transaction_enabled = true; + } } /// Return data at the end of a transaction @@ -859,14 +868,30 @@ impl<'a> BorrowedAccount<'a> { { return Ok(()); } + let old_length = self.get_data().len(); // Only the owner can change the length of the data - if new_length != self.get_data().len() && !self.is_owned_by_current_program() { + if new_length != old_length && !self.is_owned_by_current_program() { return Err(InstructionError::AccountDataSizeChanged); } // The new length can not exceed the maximum permitted length if new_length > MAX_PERMITTED_DATA_LENGTH as usize { return Err(InstructionError::InvalidRealloc); } + if self + .transaction_context + .is_cap_accounts_data_allocations_per_transaction_enabled + { + // The resize can not exceed the per-transaction maximum + let length_delta = (new_length as i64).saturating_sub(old_length as i64); + if self + .transaction_context + .accounts_resize_delta()? + .saturating_add(length_delta) + > MAX_PERMITTED_ACCOUNTS_DATA_ALLOCATIONS_PER_TRANSACTION + { + return Err(InstructionError::MaxAccountsDataAllocationsExceeded); + } + } Ok(()) } diff --git a/storage-proto/proto/transaction_by_addr.proto b/storage-proto/proto/transaction_by_addr.proto index 4c95b1829..9dccc0b4e 100644 --- a/storage-proto/proto/transaction_by_addr.proto +++ b/storage-proto/proto/transaction_by_addr.proto @@ -120,7 +120,7 @@ enum InstructionErrorType { ARITHMETIC_OVERFLOW = 47; UNSUPPORTED_SYSVAR = 48; ILLEGAL_OWNER = 49; - MAX_ACCOUNTS_DATA_SIZE_EXCEEDED = 50; + MAX_ACCOUNTS_DATA_ALLOCATIONS_EXCEEDED = 50; MAX_ACCOUNTS_EXCEEDED = 51; } diff --git a/storage-proto/src/convert.rs b/storage-proto/src/convert.rs index ee342be6a..399a3eb99 100644 --- a/storage-proto/src/convert.rs +++ b/storage-proto/src/convert.rs @@ -710,7 +710,7 @@ impl TryFrom for TransactionError { 47 => InstructionError::ArithmeticOverflow, 48 => InstructionError::UnsupportedSysvar, 49 => InstructionError::IllegalOwner, - 50 => InstructionError::MaxAccountsDataSizeExceeded, + 50 => InstructionError::MaxAccountsDataAllocationsExceeded, 51 => InstructionError::MaxAccountsExceeded, _ => return Err("Invalid InstructionError"), }; @@ -1025,8 +1025,8 @@ impl From for tx_by_addr::TransactionError { InstructionError::IllegalOwner => { tx_by_addr::InstructionErrorType::IllegalOwner } - InstructionError::MaxAccountsDataSizeExceeded => { - tx_by_addr::InstructionErrorType::MaxAccountsDataSizeExceeded + InstructionError::MaxAccountsDataAllocationsExceeded => { + tx_by_addr::InstructionErrorType::MaxAccountsDataAllocationsExceeded } InstructionError::MaxAccountsExceeded => { tx_by_addr::InstructionErrorType::MaxAccountsExceeded