From e3f253d559604d574f039bfaca57286b3074c5ee Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Fri, 28 Jul 2023 18:34:27 +0700 Subject: [PATCH] introduce SerializedAccountMetadata (#32644) * bpf_loader: move computing original account lengths inside serialize_paramters_(aligned|unaligned) This is in preparation of returning more than just the original length * bpf_loader: deserialize*: take original lens as an iterator instead of a slice This is in preparation of extracting account lenghts from a larger context * bpf_loader: introduce SerializedAccountMetadata Instead of passing original_account_lengths around as Vec, introduce an explicit type that includes the length and soon more. --- program-runtime/src/invoke_context.rs | 7 +- programs/bpf_loader/src/lib.rs | 20 +-- programs/bpf_loader/src/serialization.rs | 179 +++++++++++++---------- programs/bpf_loader/src/syscalls/cpi.rs | 15 +- 4 files changed, 130 insertions(+), 91 deletions(-) diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index c09fc1cb37..808891b7f8 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -146,10 +146,15 @@ impl BpfAllocator { pub struct SyscallContext { pub allocator: BpfAllocator, - pub orig_account_lengths: Vec, + pub accounts_metadata: Vec, pub trace_log: Vec<[u64; 12]>, } +#[derive(Debug, Clone)] +pub struct SerializedAccountMetadata { + pub original_data_len: usize, +} + pub struct InvokeContext<'a> { pub transaction_context: &'a mut TransactionContext, rent: Rent, diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 625ae34842..c8afee7204 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -8,7 +8,7 @@ use { solana_measure::measure::Measure, solana_program_runtime::{ ic_logger_msg, ic_msg, - invoke_context::{BpfAllocator, InvokeContext, SyscallContext}, + invoke_context::{BpfAllocator, InvokeContext, SerializedAccountMetadata, SyscallContext}, loaded_programs::{ LoadProgramMetrics, LoadedProgram, LoadedProgramType, DELAY_VISIBILITY_SLOT_OFFSET, }, @@ -204,7 +204,7 @@ pub fn calculate_heap_cost(heap_size: u64, heap_cost: u64, enable_rounding_fix: pub fn create_vm<'a, 'b>( program: &'a Executable>, regions: Vec, - orig_account_lengths: Vec, + accounts_metadata: Vec, invoke_context: &'a mut InvokeContext<'b>, stack: &mut AlignedMemory, heap: &mut AlignedMemory, @@ -238,7 +238,7 @@ pub fn create_vm<'a, 'b>( )?; invoke_context.set_syscall_context(SyscallContext { allocator: BpfAllocator::new(heap_size as u64), - orig_account_lengths, + accounts_metadata, trace_log: Vec::new(), })?; Ok(EbpfVm::new( @@ -253,7 +253,7 @@ pub fn create_vm<'a, 'b>( /// Create the SBF virtual machine #[macro_export] macro_rules! create_vm { - ($vm:ident, $program:expr, $regions:expr, $orig_account_lengths:expr, $invoke_context:expr $(,)?) => { + ($vm:ident, $program:expr, $regions:expr, $accounts_metadata:expr, $invoke_context:expr $(,)?) => { let invoke_context = &*$invoke_context; let stack_size = $program.get_config().stack_size(); let heap_size = invoke_context @@ -282,7 +282,7 @@ macro_rules! create_vm { let vm = $crate::create_vm( $program, $regions, - $orig_account_lengths, + $accounts_metadata, $invoke_context, &mut stack, &mut heap, @@ -295,7 +295,7 @@ macro_rules! create_vm { #[macro_export] macro_rules! mock_create_vm { - ($vm:ident, $additional_regions:expr, $orig_account_lengths:expr, $invoke_context:expr $(,)?) => { + ($vm:ident, $additional_regions:expr, $accounts_metadata:expr, $invoke_context:expr $(,)?) => { let loader = std::sync::Arc::new(BuiltinProgram::new_loader( solana_rbpf::vm::Config::default(), )); @@ -315,7 +315,7 @@ macro_rules! mock_create_vm { $vm, &verified_executable, $additional_regions, - $orig_account_lengths, + $accounts_metadata, $invoke_context, ); }; @@ -1500,7 +1500,7 @@ fn execute<'a, 'b: 'a>( .is_active(&bpf_account_data_direct_mapping::id()); let mut serialize_time = Measure::start("serialize"); - let (parameter_bytes, regions, account_lengths) = serialization::serialize_parameters( + let (parameter_bytes, regions, accounts_metadata) = serialization::serialize_parameters( invoke_context.transaction_context, instruction_context, invoke_context @@ -1523,7 +1523,7 @@ fn execute<'a, 'b: 'a>( let mut execute_time; let execution_result = { let compute_meter_prev = invoke_context.get_remaining(); - create_vm!(vm, executable, regions, account_lengths, invoke_context,); + create_vm!(vm, executable, regions, accounts_metadata, invoke_context,); let mut vm = match vm { Ok(info) => info, Err(e) => { @@ -1604,7 +1604,7 @@ fn execute<'a, 'b: 'a>( .get_current_instruction_context()?, copy_account_data, parameter_bytes, - &invoke_context.get_syscall_context()?.orig_account_lengths, + &invoke_context.get_syscall_context()?.accounts_metadata, ) } diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index 40dcc0ad97..768ba79211 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -2,6 +2,7 @@ use { byteorder::{ByteOrder, LittleEndian}, + solana_program_runtime::invoke_context::SerializedAccountMetadata, solana_rbpf::{ aligned_memory::{AlignedMemory, Pod}, ebpf::{HOST_ALIGN, MM_INPUT_START}, @@ -175,54 +176,63 @@ pub fn serialize_parameters( instruction_context: &InstructionContext, should_cap_ix_accounts: bool, copy_account_data: bool, -) -> Result<(AlignedMemory, Vec, Vec), InstructionError> { +) -> Result< + ( + AlignedMemory, + Vec, + Vec, + ), + InstructionError, +> { let num_ix_accounts = instruction_context.get_number_of_instruction_accounts(); if should_cap_ix_accounts && num_ix_accounts > MAX_INSTRUCTION_ACCOUNTS as IndexOfAccount { return Err(InstructionError::MaxAccountsExceeded); } - let is_loader_deprecated = *instruction_context - .try_borrow_last_program_account(transaction_context)? - .get_owner() - == bpf_loader_deprecated::id(); - let num_accounts = instruction_context.get_number_of_instruction_accounts() as usize; - let mut accounts = Vec::with_capacity(num_accounts); - let mut account_lengths: Vec = Vec::with_capacity(num_accounts); - for instruction_account_index in 0..instruction_context.get_number_of_instruction_accounts() { - if let Some(index) = - instruction_context.is_instruction_account_duplicate(instruction_account_index)? - { - accounts.push(SerializeAccount::Duplicate(index)); - // unwrap here is safe: if an account is a duplicate, we must have - // seen the original already - account_lengths.push(*account_lengths.get(index as usize).unwrap()); - } else { - let account = instruction_context - .try_borrow_instruction_account(transaction_context, instruction_account_index)?; - account_lengths.push(account.get_data().len()); - accounts.push(SerializeAccount::Account( - instruction_account_index, - account, - )); - }; - } + let (program_id, is_loader_deprecated) = { + let program_account = + instruction_context.try_borrow_last_program_account(transaction_context)?; + ( + *program_account.get_key(), + *program_account.get_owner() == bpf_loader_deprecated::id(), + ) + }; + + let accounts = (0..instruction_context.get_number_of_instruction_accounts()) + .map(|instruction_account_index| { + if let Some(index) = instruction_context + .is_instruction_account_duplicate(instruction_account_index) + .unwrap() + { + SerializeAccount::Duplicate(index) + } else { + let account = instruction_context + .try_borrow_instruction_account(transaction_context, instruction_account_index) + .unwrap(); + SerializeAccount::Account(instruction_account_index, account) + } + }) + // fun fact: jemalloc is good at caching tiny allocations like this one, + // so collecting here is actually faster than passing the iterator + // around, since the iterator does the work to produce its items each + // time it's iterated on. + .collect::>(); if is_loader_deprecated { serialize_parameters_unaligned( - transaction_context, - instruction_context, accounts, + instruction_context.get_instruction_data(), + &program_id, copy_account_data, ) } else { serialize_parameters_aligned( - transaction_context, - instruction_context, accounts, + instruction_context.get_instruction_data(), + &program_id, copy_account_data, ) } - .map(|(buffer, regions)| (buffer, regions, account_lengths)) } pub fn deserialize_parameters( @@ -230,12 +240,13 @@ pub fn deserialize_parameters( instruction_context: &InstructionContext, copy_account_data: bool, buffer: &[u8], - account_lengths: &[usize], + accounts_metadata: &[SerializedAccountMetadata], ) -> Result<(), InstructionError> { let is_loader_deprecated = *instruction_context .try_borrow_last_program_account(transaction_context)? .get_owner() == bpf_loader_deprecated::id(); + let account_lengths = accounts_metadata.iter().map(|a| a.original_data_len); if is_loader_deprecated { deserialize_parameters_unaligned( transaction_context, @@ -256,11 +267,18 @@ pub fn deserialize_parameters( } fn serialize_parameters_unaligned( - transaction_context: &TransactionContext, - instruction_context: &InstructionContext, accounts: Vec, + instruction_data: &[u8], + program_id: &Pubkey, copy_account_data: bool, -) -> Result<(AlignedMemory, Vec), InstructionError> { +) -> Result< + ( + AlignedMemory, + Vec, + Vec, + ), + InstructionError, +> { // Calculate size in order to alloc once let mut size = size_of::(); for account in &accounts { @@ -283,16 +301,23 @@ fn serialize_parameters_unaligned( } } size += size_of::() // instruction data len - + instruction_context.get_instruction_data().len() // instruction data + + instruction_data.len() // instruction data + size_of::(); // program id let mut s = Serializer::new(size, MM_INPUT_START, false, copy_account_data); + let mut accounts_metadata: Vec = Vec::with_capacity(accounts.len()); s.write::((accounts.len() as u64).to_le()); for account in accounts { match account { - SerializeAccount::Duplicate(position) => s.write(position as u8), + SerializeAccount::Duplicate(position) => { + accounts_metadata.push(accounts_metadata.get(position as usize).unwrap().clone()); + s.write(position as u8); + } SerializeAccount::Account(_, mut account) => { + accounts_metadata.push(SerializedAccountMetadata { + original_data_len: account.get_data().len(), + }); s.write::(NON_DUP_MARKER); s.write::(account.is_signer() as u8); s.write::(account.is_writable() as u8); @@ -306,28 +331,25 @@ fn serialize_parameters_unaligned( } }; } - s.write::((instruction_context.get_instruction_data().len() as u64).to_le()); - s.write_all(instruction_context.get_instruction_data()); - s.write_all( - instruction_context - .try_borrow_last_program_account(transaction_context)? - .get_key() - .as_ref(), - ); + s.write::((instruction_data.len() as u64).to_le()); + s.write_all(instruction_data); + s.write_all(program_id.as_ref()); - Ok(s.finish()) + let (mem, regions) = s.finish(); + Ok((mem, regions, accounts_metadata)) } -pub fn deserialize_parameters_unaligned( +pub fn deserialize_parameters_unaligned>( transaction_context: &TransactionContext, instruction_context: &InstructionContext, copy_account_data: bool, buffer: &[u8], - account_lengths: &[usize], + account_lengths: I, ) -> Result<(), InstructionError> { let mut start = size_of::(); // number of accounts - for (instruction_account_index, pre_len) in - (0..instruction_context.get_number_of_instruction_accounts()).zip(account_lengths.iter()) + for (instruction_account_index, pre_len) in (0..instruction_context + .get_number_of_instruction_accounts()) + .zip(account_lengths.into_iter()) { let duplicate = instruction_context.is_instruction_account_duplicate(instruction_account_index)?; @@ -372,11 +394,19 @@ pub fn deserialize_parameters_unaligned( } fn serialize_parameters_aligned( - transaction_context: &TransactionContext, - instruction_context: &InstructionContext, accounts: Vec, + instruction_data: &[u8], + program_id: &Pubkey, copy_account_data: bool, -) -> Result<(AlignedMemory, Vec), InstructionError> { +) -> Result< + ( + AlignedMemory, + Vec, + Vec, + ), + InstructionError, +> { + let mut account_lengths = Vec::with_capacity(accounts.len()); // Calculate size in order to alloc once let mut size = size_of::(); for account in &accounts { @@ -404,7 +434,7 @@ fn serialize_parameters_aligned( } } size += size_of::() // data len - + instruction_context.get_instruction_data().len() + + instruction_data.len() + size_of::(); // program id; let mut s = Serializer::new(size, MM_INPUT_START, true, copy_account_data); @@ -414,6 +444,9 @@ fn serialize_parameters_aligned( for account in accounts { match account { SerializeAccount::Account(_, mut borrowed_account) => { + account_lengths.push(SerializedAccountMetadata { + original_data_len: borrowed_account.get_data().len(), + }); s.write::(NON_DUP_MARKER); s.write::(borrowed_account.is_signer() as u8); s.write::(borrowed_account.is_writable() as u8); @@ -427,33 +460,31 @@ fn serialize_parameters_aligned( s.write::((borrowed_account.get_rent_epoch()).to_le()); } SerializeAccount::Duplicate(position) => { + account_lengths.push(account_lengths.get(position as usize).unwrap().clone()); s.write::(position as u8); s.write_all(&[0u8, 0, 0, 0, 0, 0, 0]); } }; } - s.write::((instruction_context.get_instruction_data().len() as u64).to_le()); - s.write_all(instruction_context.get_instruction_data()); - s.write_all( - instruction_context - .try_borrow_last_program_account(transaction_context)? - .get_key() - .as_ref(), - ); + s.write::((instruction_data.len() as u64).to_le()); + s.write_all(instruction_data); + s.write_all(program_id.as_ref()); - Ok(s.finish()) + let (mem, regions) = s.finish(); + Ok((mem, regions, account_lengths)) } -pub fn deserialize_parameters_aligned( +pub fn deserialize_parameters_aligned>( transaction_context: &TransactionContext, instruction_context: &InstructionContext, copy_account_data: bool, buffer: &[u8], - account_lengths: &[usize], + account_lengths: I, ) -> Result<(), InstructionError> { let mut start = size_of::(); // number of accounts - for (instruction_account_index, pre_len) in - (0..instruction_context.get_number_of_instruction_accounts()).zip(account_lengths.iter()) + for (instruction_account_index, pre_len) in (0..instruction_context + .get_number_of_instruction_accounts()) + .zip(account_lengths.into_iter()) { let duplicate = instruction_context.is_instruction_account_duplicate(instruction_account_index)?; @@ -487,13 +518,13 @@ pub fn deserialize_parameters_aligned( .ok_or(InstructionError::InvalidArgument)?, ) as usize; start += size_of::(); // data length - if post_len.saturating_sub(*pre_len) > MAX_PERMITTED_DATA_INCREASE + if post_len.saturating_sub(pre_len) > MAX_PERMITTED_DATA_INCREASE || post_len > MAX_PERMITTED_DATA_LENGTH as usize { return Err(InstructionError::InvalidRealloc); } // The redundant check helps to avoid the expensive data comparison if we can - let alignment_offset = (*pre_len as *const u8).align_offset(BPF_ALIGN_OF_U128); + let alignment_offset = (pre_len as *const u8).align_offset(BPF_ALIGN_OF_U128); if copy_account_data { let data = buffer .get(start..start + post_len) @@ -506,7 +537,7 @@ pub fn deserialize_parameters_aligned( Err(err) if borrowed_account.get_data() != data => return Err(err), _ => {} } - start += *pre_len; // data + start += pre_len; // data } else { // See Serializer::write_account() as to why we have this // padding before the realloc region here. @@ -520,11 +551,11 @@ pub fn deserialize_parameters_aligned( { Ok(()) => { borrowed_account.set_data_length(post_len)?; - let allocated_bytes = post_len.saturating_sub(*pre_len); + let allocated_bytes = post_len.saturating_sub(pre_len); if allocated_bytes > 0 { borrowed_account .get_data_mut()? - .get_mut(*pre_len..pre_len.saturating_add(allocated_bytes)) + .get_mut(pre_len..pre_len.saturating_add(allocated_bytes)) .ok_or(InstructionError::InvalidArgument)? .copy_from_slice( data.get(0..allocated_bytes) @@ -842,7 +873,7 @@ mod tests { .unwrap(); // check serialize_parameters_aligned - let (mut serialized, regions, account_lengths) = serialize_parameters( + let (mut serialized, regions, accounts_metadata) = serialize_parameters( invoke_context.transaction_context, instruction_context, true, @@ -907,7 +938,7 @@ mod tests { instruction_context, copy_account_data, serialized.as_slice(), - &account_lengths, + &accounts_metadata, ) .unwrap(); for (index_in_transaction, (_key, original_account)) in diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 0e3cc945c4..eea321ebd8 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -728,10 +728,10 @@ where // unwrapping here is fine: we're in a syscall and the method below fails // only outside syscalls - let orig_data_lens = &invoke_context + let accounts_metadata = &invoke_context .get_syscall_context() .unwrap() - .orig_account_lengths; + .accounts_metadata; let direct_mapping = invoke_context .feature_set @@ -763,7 +763,7 @@ where } else if let Some(caller_account_index) = account_info_keys.iter().position(|key| *key == account_key) { - let original_data_len = *orig_data_lens + let original_data_len = accounts_metadata .get(instruction_account.index_in_caller as usize) .ok_or_else(|| { ic_msg!( @@ -772,7 +772,8 @@ where account_key ); Box::new(InstructionError::MissingAccount) - })?; + })? + .original_data_len; // build the CallerAccount corresponding to this account. let caller_account = @@ -1266,7 +1267,9 @@ mod tests { use { super::*, crate::mock_create_vm, - solana_program_runtime::with_mock_invoke_context, + solana_program_runtime::{ + invoke_context::SerializedAccountMetadata, with_mock_invoke_context, + }, solana_rbpf::{ ebpf::MM_INPUT_START, elf::SBPFVersion, memory_region::MemoryRegion, vm::Config, }, @@ -2155,7 +2158,7 @@ mod tests { mock_create_vm!( _vm, Vec::new(), - vec![original_data_len], + vec![SerializedAccountMetadata { original_data_len }], &mut invoke_context );