From 71aee4fcaff618d2a5fa6378533f2e515aaf2c8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Mon, 26 Sep 2022 10:47:16 +0200 Subject: [PATCH] Feature: Explicitly limit `TransactionContext::instruction_trace_capacity` (#27938) * Renames instruction_stack_capacity => instruction_stack_capacity. * Replaces number_of_instructions_at_transaction_level by instruction_trace_capacity. * Adds MaxInstructionTraceLengthExceeded. * Adjusts TransactionContext::new() parameter. * Adds feature gate limit_max_instruction_trace_length. * Adds test_max_instruction_trace_length(). --- .../developing/programming-model/runtime.md | 1 + program-runtime/src/compute_budget.rs | 3 ++ program-runtime/src/invoke_context.rs | 31 ++++++++++++++----- programs/bpf_loader/src/lib.rs | 18 ++++++++--- programs/bpf_loader/src/syscalls/mod.rs | 6 ++-- runtime/src/bank.rs | 20 ++++++++---- runtime/src/nonce_keyed_account.rs | 2 +- sdk/program/src/instruction.rs | 4 +++ sdk/program/src/program_error.rs | 14 +++++++++ sdk/src/feature_set.rs | 5 +++ sdk/src/transaction_context.rs | 26 +++++++++++----- storage-proto/proto/transaction_by_addr.proto | 1 + storage-proto/src/convert.rs | 4 +++ 13 files changed, 106 insertions(+), 29 deletions(-) diff --git a/docs/src/developing/programming-model/runtime.md b/docs/src/developing/programming-model/runtime.md index ac8284b723d92e..53146fcf32298b 100644 --- a/docs/src/developing/programming-model/runtime.md +++ b/docs/src/developing/programming-model/runtime.md @@ -85,6 +85,7 @@ log_u64_units: 100, create_program address units: 1500, invoke_units: 1000, max_invoke_depth: 4, +max_instruction_trace_length: 64, max_call_depth: 64, stack_frame_size: 4096, log_pubkey_units: 100, diff --git a/program-runtime/src/compute_budget.rs b/program-runtime/src/compute_budget.rs index e2b9ed38c0473f..9b5db88384de45 100644 --- a/program-runtime/src/compute_budget.rs +++ b/program-runtime/src/compute_budget.rs @@ -37,6 +37,8 @@ pub struct ComputeBudget { pub invoke_units: u64, /// Maximum cross-program invocation depth allowed pub max_invoke_depth: usize, + /// Maximum cross-program invocation and instructions per transaction + pub max_instruction_trace_length: usize, /// Base number of compute units consumed to call SHA256 pub sha256_base_cost: u64, /// Incremental number of units consumed by SHA256 (based on bytes) @@ -100,6 +102,7 @@ impl ComputeBudget { create_program_address_units: 1500, invoke_units: 1000, max_invoke_depth: 4, + max_instruction_trace_length: 64, sha256_base_cost: 85, sha256_byte_cost: 1, sha256_max_slices: 20_000, diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index c50f4576f5c7bd..7a3da76cfa42bf 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -1022,11 +1022,12 @@ pub fn with_mock_invoke_context R>( }]; let preparation = prepare_mock_invoke_context(transaction_accounts, instruction_accounts, &program_indices); + let compute_budget = ComputeBudget::default(); let mut transaction_context = TransactionContext::new( preparation.transaction_accounts, Some(Rent::default()), - ComputeBudget::default().max_invoke_depth.saturating_add(1), - 1, + compute_budget.max_invoke_depth.saturating_add(1), + compute_budget.max_instruction_trace_length, ); transaction_context.enable_cap_accounts_data_allocations_per_transaction(); let mut invoke_context = InvokeContext::new_mock(&mut transaction_context, &[]); @@ -1057,11 +1058,12 @@ pub fn mock_process_instruction( preparation .transaction_accounts .push((*loader_id, processor_account)); + let compute_budget = ComputeBudget::default(); let mut transaction_context = TransactionContext::new( preparation.transaction_accounts, Some(Rent::default()), - ComputeBudget::default().max_invoke_depth.saturating_add(1), - 1, + compute_budget.max_invoke_depth.saturating_add(1), + compute_budget.max_instruction_trace_length, ); transaction_context.enable_cap_accounts_data_allocations_per_transaction(); let mut invoke_context = InvokeContext::new_mock(&mut transaction_context, &[]); @@ -1284,7 +1286,7 @@ mod tests { accounts, Some(Rent::default()), ComputeBudget::default().max_invoke_depth, - 1, + MAX_DEPTH, ); let mut invoke_context = InvokeContext::new_mock(&mut transaction_context, &[]); @@ -1309,6 +1311,21 @@ mod tests { assert!(depth_reached < MAX_DEPTH); } + #[test] + fn test_max_instruction_trace_length() { + const MAX_INSTRUCTIONS: usize = 8; + let mut transaction_context = + TransactionContext::new(Vec::new(), Some(Rent::default()), 1, MAX_INSTRUCTIONS); + for _ in 0..MAX_INSTRUCTIONS { + transaction_context.push().unwrap(); + transaction_context.pop().unwrap(); + } + assert_eq!( + transaction_context.push(), + Err(InstructionError::MaxInstructionTraceLengthExceeded) + ); + } + #[test] fn test_process_instruction() { let callee_program_id = solana_sdk::pubkey::new_rand(); @@ -1345,7 +1362,7 @@ mod tests { }) .collect::>(); let mut transaction_context = - TransactionContext::new(accounts, Some(Rent::default()), 2, 9); + TransactionContext::new(accounts, Some(Rent::default()), 2, 18); let mut invoke_context = InvokeContext::new_mock(&mut transaction_context, builtin_programs); @@ -1436,7 +1453,7 @@ mod tests { let accounts = vec![(solana_sdk::pubkey::new_rand(), AccountSharedData::default())]; let mut transaction_context = - TransactionContext::new(accounts, Some(Rent::default()), 1, 3); + TransactionContext::new(accounts, Some(Rent::default()), 1, 1); let mut invoke_context = InvokeContext::new_mock(&mut transaction_context, &[]); invoke_context.compute_budget = ComputeBudget::new(compute_budget::DEFAULT_INSTRUCTION_COMPUTE_UNIT_LIMIT as u64); diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index ab6cf86de8ec71..00f60f9099ddd8 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -44,12 +44,14 @@ use { 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_ix, error_on_syscall_bpf_function_hash_collisions, - reject_callx_r10, + limit_max_instruction_trace_length, reject_callx_r10, }, instruction::{AccountMeta, InstructionError}, loader_instruction::LoaderInstruction, loader_upgradeable_instruction::UpgradeableLoaderInstruction, - program_error::MAX_ACCOUNTS_DATA_ALLOCATIONS_EXCEEDED, + program_error::{ + MAX_ACCOUNTS_DATA_ALLOCATIONS_EXCEEDED, MAX_INSTRUCTION_TRACE_LENGTH_EXCEEDED, + }, program_utils::limited_deserialize, pubkey::Pubkey, saturating_add_assign, @@ -1362,14 +1364,20 @@ impl Executor for BpfExecutor { } match result { Ok(status) if status != SUCCESS => { - let error: InstructionError = if status + let error: InstructionError = if (status == MAX_ACCOUNTS_DATA_ALLOCATIONS_EXCEEDED && !invoke_context .feature_set - .is_active(&cap_accounts_data_allocations_per_transaction::id()) + .is_active(&cap_accounts_data_allocations_per_transaction::id())) + || (status == MAX_INSTRUCTION_TRACE_LENGTH_EXCEEDED + && !invoke_context + .feature_set + .is_active(&limit_max_instruction_trace_length::id())) { // Until the cap_accounts_data_allocations_per_transaction feature is - // enabled, map the MAX_ACCOUNTS_DATA_ALLOCATIONS_EXCEEDED error to InvalidError + // enabled, map the `MAX_ACCOUNTS_DATA_ALLOCATIONS_EXCEEDED` error to `InvalidError`. + // Until the limit_max_instruction_trace_length feature is + // enabled, map the `MAX_INSTRUCTION_TRACE_LENGTH_EXCEEDED` error to `InvalidError`. InstructionError::InvalidError } else { status.into() diff --git a/programs/bpf_loader/src/syscalls/mod.rs b/programs/bpf_loader/src/syscalls/mod.rs index 3d8bc275e9ac90..52a82b99af588d 100644 --- a/programs/bpf_loader/src/syscalls/mod.rs +++ b/programs/bpf_loader/src/syscalls/mod.rs @@ -3870,8 +3870,10 @@ mod tests { ) }) .collect::>(); - let mut transaction_context = TransactionContext::new(transaction_accounts, None, 4, 1); - for (index_in_trace, stack_height) in [1, 2, 3, 2, 2, 3, 4, 3].into_iter().enumerate() { + let instruction_trace = [1, 2, 3, 2, 2, 3, 4, 3]; + let mut transaction_context = + TransactionContext::new(transaction_accounts, None, 4, instruction_trace.len()); + for (index_in_trace, stack_height) in instruction_trace.into_iter().enumerate() { while stack_height <= transaction_context.get_instruction_context_stack_height() { transaction_context.pop().unwrap(); } diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index f22ca8c3164602..943866cf64daed 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -285,7 +285,7 @@ impl RentDebits { } pub type BankStatusCache = StatusCache>; -#[frozen_abi(digest = "7FSSacrCi7vf2QZFm3Ui9JqTii4U6h1XWYD3LKSuVwV8")] +#[frozen_abi(digest = "A7T7XohiSoo8FGoCPTsaXAYYugXTkoYnBjQAdBgYHH85")] pub type BankSlotDelta = SlotDelta>; // Eager rent collection repeats in cyclic manner. @@ -4363,7 +4363,14 @@ impl Bank { None }, compute_budget.max_invoke_depth.saturating_add(1), - tx.message().instructions().len(), + if self + .feature_set + .is_active(&feature_set::limit_max_instruction_trace_length::id()) + { + compute_budget.max_instruction_trace_length + } else { + std::usize::MAX + }, ); if self .feature_set @@ -18774,7 +18781,6 @@ pub(crate) mod tests { sol_to_lamports(1.), bank.last_blockhash(), ); - let number_of_instructions_at_transaction_level = tx.message().instructions.len(); let num_accounts = tx.message().account_keys.len(); let sanitized_tx = SanitizedTransaction::try_from_legacy_transaction(tx).unwrap(); let mut error_counters = TransactionErrorMetrics::default(); @@ -18797,7 +18803,7 @@ pub(crate) mod tests { loaded_txs[0].0.as_ref().unwrap().accounts.clone(), Some(Rent::default()), compute_budget.max_invoke_depth.saturating_add(1), - number_of_instructions_at_transaction_level, + compute_budget.max_instruction_trace_length, ); assert_eq!( @@ -18945,8 +18951,10 @@ pub(crate) mod tests { #[test] fn test_inner_instructions_list_from_instruction_trace() { - let mut transaction_context = TransactionContext::new(vec![], None, 3, 3); - for (index_in_trace, stack_height) in [1, 2, 1, 1, 2, 3, 2].into_iter().enumerate() { + let instruction_trace = [1, 2, 1, 1, 2, 3, 2]; + let mut transaction_context = + TransactionContext::new(vec![], None, 3, instruction_trace.len()); + for (index_in_trace, stack_height) in instruction_trace.into_iter().enumerate() { while stack_height <= transaction_context.get_instruction_context_stack_height() { transaction_context.pop().unwrap(); } diff --git a/runtime/src/nonce_keyed_account.rs b/runtime/src/nonce_keyed_account.rs index 1bf21a0ea903da..d24cb113c388fa 100644 --- a/runtime/src/nonce_keyed_account.rs +++ b/runtime/src/nonce_keyed_account.rs @@ -334,7 +334,7 @@ mod test { }, ]; let mut transaction_context = - TransactionContext::new(accounts, Some(Rent::default()), 1, 2); + TransactionContext::new(accounts, Some(Rent::default()), 1, 1); let mut $invoke_context = InvokeContext::new_mock(&mut transaction_context, &[]); }; } diff --git a/sdk/program/src/instruction.rs b/sdk/program/src/instruction.rs index 7f67f5427dded6..959dd810d05503 100644 --- a/sdk/program/src/instruction.rs +++ b/sdk/program/src/instruction.rs @@ -256,6 +256,10 @@ pub enum InstructionError { /// Max accounts exceeded #[error("Max accounts exceeded")] MaxAccountsExceeded, + + /// Max instruction trace length exceeded + #[error("Max instruction trace length exceeded")] + MaxInstructionTraceLengthExceeded, // Note: For any new error added here an equivalent ProgramError and its // conversions must also be added } diff --git a/sdk/program/src/program_error.rs b/sdk/program/src/program_error.rs index c12d42eda57a32..c8fa9f071e9321 100644 --- a/sdk/program/src/program_error.rs +++ b/sdk/program/src/program_error.rs @@ -55,6 +55,8 @@ pub enum ProgramError { MaxAccountsDataAllocationsExceeded, #[error("Account data reallocation was invalid")] InvalidRealloc, + #[error("Instruction trace length exceeded the maximum allowed per transaction")] + MaxInstructionTraceLengthExceeded, } pub trait PrintProgramError { @@ -97,6 +99,9 @@ impl PrintProgramError for ProgramError { msg!("Error: MaxAccountsDataAllocationsExceeded") } Self::InvalidRealloc => msg!("Error: InvalidRealloc"), + Self::MaxInstructionTraceLengthExceeded => { + msg!("Error: MaxInstructionTraceLengthExceeded") + } } } } @@ -129,6 +134,7 @@ pub const UNSUPPORTED_SYSVAR: u64 = to_builtin!(17); pub const ILLEGAL_OWNER: u64 = to_builtin!(18); pub const MAX_ACCOUNTS_DATA_ALLOCATIONS_EXCEEDED: u64 = to_builtin!(19); pub const INVALID_ACCOUNT_DATA_REALLOC: u64 = to_builtin!(20); +pub const MAX_INSTRUCTION_TRACE_LENGTH_EXCEEDED: u64 = to_builtin!(21); // Warning: Any new program errors added here must also be: // - Added to the below conversions // - Added as an equivalent to InstructionError @@ -159,6 +165,9 @@ impl From for u64 { MAX_ACCOUNTS_DATA_ALLOCATIONS_EXCEEDED } ProgramError::InvalidRealloc => INVALID_ACCOUNT_DATA_REALLOC, + ProgramError::MaxInstructionTraceLengthExceeded => { + MAX_INSTRUCTION_TRACE_LENGTH_EXCEEDED + } ProgramError::Custom(error) => { if error == 0 { CUSTOM_ZERO @@ -193,6 +202,7 @@ impl From for ProgramError { ILLEGAL_OWNER => Self::IllegalOwner, MAX_ACCOUNTS_DATA_ALLOCATIONS_EXCEEDED => Self::MaxAccountsDataAllocationsExceeded, INVALID_ACCOUNT_DATA_REALLOC => Self::InvalidRealloc, + MAX_INSTRUCTION_TRACE_LENGTH_EXCEEDED => Self::MaxInstructionTraceLengthExceeded, _ => Self::Custom(error as u32), } } @@ -225,6 +235,9 @@ impl TryFrom for ProgramError { Ok(Self::MaxAccountsDataAllocationsExceeded) } Self::Error::InvalidRealloc => Ok(Self::InvalidRealloc), + Self::Error::MaxInstructionTraceLengthExceeded => { + Ok(Self::MaxInstructionTraceLengthExceeded) + } _ => Err(error), } } @@ -257,6 +270,7 @@ where ILLEGAL_OWNER => Self::IllegalOwner, MAX_ACCOUNTS_DATA_ALLOCATIONS_EXCEEDED => Self::MaxAccountsDataAllocationsExceeded, INVALID_ACCOUNT_DATA_REALLOC => Self::InvalidRealloc, + MAX_INSTRUCTION_TRACE_LENGTH_EXCEEDED => Self::MaxInstructionTraceLengthExceeded, _ => { // A valid custom error has no bits set in the upper 32 if error >> BUILTIN_BIT_SHIFT == 0 { diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index e1291686e4f299..d3b2202b5f70bf 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -526,6 +526,10 @@ pub mod increase_tx_account_lock_limit { solana_sdk::declare_id!("9LZdXeKGeBV6hRLdxS1rHbHoEUsKqesCC2ZAPTPKJAbK"); } +pub mod limit_max_instruction_trace_length { + solana_sdk::declare_id!("GQALDaC48fEhZGWRj9iL5Q889emJKcj3aCvHF7VCbbF4"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -652,6 +656,7 @@ lazy_static! { (epoch_accounts_hash::id(), "enable epoch accounts hash calculation #27539"), (remove_deprecated_request_unit_ix::id(), "remove support for RequestUnitsDeprecated instruction #27500"), (increase_tx_account_lock_limit::id(), "increase tx account lock limit to 128 #27241"), + (limit_max_instruction_trace_length::id(), "limit max instruction trace length"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter() diff --git a/sdk/src/transaction_context.rs b/sdk/src/transaction_context.rs index 1dd000a36b8eef..e27bf27ffa89ae 100644 --- a/sdk/src/transaction_context.rs +++ b/sdk/src/transaction_context.rs @@ -113,7 +113,8 @@ pub struct TransactionContext { accounts: Pin]>>, #[cfg(not(target_os = "solana"))] account_touched_flags: RefCell>>, - instruction_context_capacity: usize, + instruction_stack_capacity: usize, + instruction_trace_capacity: usize, instruction_stack: Vec, instruction_trace: Vec, return_data: TransactionReturnData, @@ -130,8 +131,8 @@ impl TransactionContext { pub fn new( transaction_accounts: Vec, rent: Option, - instruction_context_capacity: usize, - _number_of_instructions_at_transaction_level: usize, + instruction_stack_capacity: usize, + instruction_trace_capacity: usize, ) -> Self { let (account_keys, accounts): (Vec, Vec>) = transaction_accounts @@ -143,8 +144,9 @@ impl TransactionContext { account_keys: Pin::new(account_keys.into_boxed_slice()), accounts: Pin::new(accounts.into_boxed_slice()), account_touched_flags: RefCell::new(Pin::new(account_touched_flags.into_boxed_slice())), - instruction_context_capacity, - instruction_stack: Vec::with_capacity(instruction_context_capacity), + instruction_stack_capacity, + instruction_trace_capacity, + instruction_stack: Vec::with_capacity(instruction_stack_capacity), instruction_trace: vec![InstructionContext::default()], return_data: TransactionReturnData::default(), accounts_resize_delta: RefCell::new(0), @@ -213,6 +215,11 @@ impl TransactionContext { .map(|index| index as IndexOfAccount) } + /// Gets the max length of the InstructionContext trace + pub fn get_instruction_trace_capacity(&self) -> usize { + self.instruction_trace_capacity + } + /// Returns the instruction trace length. /// /// Not counting the last empty InstructionContext which is always pre-reserved for the next instruction. @@ -246,8 +253,8 @@ impl TransactionContext { } /// Gets the max height of the InstructionContext stack - pub fn get_instruction_context_capacity(&self) -> usize { - self.instruction_context_capacity + pub fn get_instruction_stack_capacity(&self) -> usize { + self.instruction_stack_capacity } /// Gets instruction stack height, top-level instructions are height @@ -307,8 +314,11 @@ impl TransactionContext { callee_instruction_accounts_lamport_sum; } let index_in_trace = self.get_instruction_trace_length(); + if index_in_trace >= self.instruction_trace_capacity { + return Err(InstructionError::MaxInstructionTraceLengthExceeded); + } self.instruction_trace.push(InstructionContext::default()); - if nesting_level >= self.instruction_context_capacity { + if nesting_level >= self.instruction_stack_capacity { return Err(InstructionError::CallDepth); } self.instruction_stack.push(index_in_trace); diff --git a/storage-proto/proto/transaction_by_addr.proto b/storage-proto/proto/transaction_by_addr.proto index 9dccc0b4edb357..6a1869fc2c517e 100644 --- a/storage-proto/proto/transaction_by_addr.proto +++ b/storage-proto/proto/transaction_by_addr.proto @@ -122,6 +122,7 @@ enum InstructionErrorType { ILLEGAL_OWNER = 49; MAX_ACCOUNTS_DATA_ALLOCATIONS_EXCEEDED = 50; MAX_ACCOUNTS_EXCEEDED = 51; + MAX_INSTRUCTION_TRACE_LENGTH_EXCEEDED = 52; } message UnixTimestamp { diff --git a/storage-proto/src/convert.rs b/storage-proto/src/convert.rs index d7a9bcdd3819d1..4e2d6ef3df2767 100644 --- a/storage-proto/src/convert.rs +++ b/storage-proto/src/convert.rs @@ -712,6 +712,7 @@ impl TryFrom for TransactionError { 49 => InstructionError::IllegalOwner, 50 => InstructionError::MaxAccountsDataAllocationsExceeded, 51 => InstructionError::MaxAccountsExceeded, + 52 => InstructionError::MaxInstructionTraceLengthExceeded, _ => return Err("Invalid InstructionError"), }; @@ -1031,6 +1032,9 @@ impl From for tx_by_addr::TransactionError { InstructionError::MaxAccountsExceeded => { tx_by_addr::InstructionErrorType::MaxAccountsExceeded } + InstructionError::MaxInstructionTraceLengthExceeded => { + tx_by_addr::InstructionErrorType::MaxInstructionTraceLengthExceeded + } } as i32, custom: match instruction_error { InstructionError::Custom(custom) => {