From 855ae79598b6912c20d1256f15e930edf374da73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Fri, 21 May 2021 22:34:07 +0200 Subject: [PATCH] Fix InvokeContext::push() account_deps (#17350) * Reverts aliasing of account_deps with the previous invocation stack frame in InvokeContext::push(). * Adds explicit assert of programdata account content in test_program_bpf_upgrade_via_cpi. --- programs/bpf/tests/programs.rs | 27 +++++++++++++++++++++++---- programs/bpf_loader/src/lib.rs | 2 -- runtime/src/message_processor.rs | 30 +++++++++++------------------- 3 files changed, 34 insertions(+), 25 deletions(-) diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index 9bbfabd88..94599191e 100644 --- a/programs/bpf/tests/programs.rs +++ b/programs/bpf/tests/programs.rs @@ -9,11 +9,10 @@ use solana_account_decoder::parse_bpf_loader::{ parse_bpf_upgradeable_loader, BpfUpgradeableLoaderAccountType, }; use solana_bpf_loader_program::{ - BpfError, create_vm, serialization::{deserialize_parameters, serialize_parameters}, syscalls::register_syscalls, - ThisInstructionMeter, + BpfError, ThisInstructionMeter, }; use solana_cli_output::display::println_transaction; use solana_rbpf::vm::{Config, Executable, Tracer}; @@ -28,6 +27,7 @@ use solana_runtime::{ }; use solana_sdk::{ account::{AccountSharedData, ReadableAccount}, + account_utils::StateMut, bpf_loader, bpf_loader_deprecated, bpf_loader_upgradeable, client::SyncClient, clock::MAX_PROCESSING_AGE, @@ -212,7 +212,8 @@ fn run_program( enable_instruction_meter: true, enable_instruction_tracing: true, }; - let mut executable = >::from_elf(&data, None, config).unwrap(); + let mut executable = + >::from_elf(&data, None, config).unwrap(); executable.set_syscall_registry(register_syscalls(&mut invoke_context).unwrap()); executable.jit_compile().unwrap(); @@ -1987,6 +1988,17 @@ fn test_program_bpf_upgrade_via_cpi() { &authority_keypair, "solana_bpf_rust_upgradeable", ); + let program_account = bank_client.get_account(&program_id).unwrap().unwrap(); + let programdata_address = match program_account.state() { + Ok(bpf_loader_upgradeable::UpgradeableLoaderState::Program { + programdata_address, + }) => programdata_address, + _ => unreachable!(), + }; + let original_programdata = bank_client + .get_account_data(&programdata_address) + .unwrap() + .unwrap(); let mut instruction = Instruction::new_with_bytes( invoke_and_return, @@ -1999,7 +2011,7 @@ fn test_program_bpf_upgrade_via_cpi() { ], ); - // Call the upgraded program + // Call the upgradable program instruction.data[0] += 1; let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction.clone()); assert_eq!( @@ -2046,6 +2058,13 @@ fn test_program_bpf_upgrade_via_cpi() { result.unwrap_err().unwrap(), TransactionError::InstructionError(0, InstructionError::Custom(43)) ); + + // Validate that the programdata was actually overwritten + let programdata = bank_client + .get_account_data(&programdata_address) + .unwrap() + .unwrap(); + assert_ne!(programdata, original_programdata); } #[cfg(feature = "bpf_rust")] diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 864440ebe..0a94e9229 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -310,8 +310,6 @@ fn process_loader_upgradeable_instruction( let buffer = keyed_account_at_index(keyed_accounts, 3)?; let rent = from_keyed_account::(keyed_account_at_index(keyed_accounts, 4)?)?; let clock = from_keyed_account::(keyed_account_at_index(keyed_accounts, 5)?)?; - // TODO [KeyedAccounts to InvokeContext refactoring] - // let _system = keyed_account_at_index(keyed_accounts, 6)?; let authority = keyed_account_at_index(keyed_accounts, 7)?; let upgrade_authority_address = Some(*authority.unsigned_key()); let upgrade_authority_signer = authority.signer_key().is_none(); diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index 037eb0b79..7fa60e5fb 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -354,27 +354,19 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { .chain(self.message.account_keys.iter()) .position(|key| key == *search_key) .map(|mut index| { - if index < self.account_deps.len() { - ( - *is_signer, - *is_writable, - &self.account_deps[index].0, - &self.account_deps[index].1 as &RefCell, - ) + // TODO + // Currently we are constructing new accounts on the stack + // before calling MessageProcessor::process_cross_program_instruction + // Ideally we would recycle the existing accounts here. + let key = if index < self.account_deps.len() { + &self.account_deps[index].0 + // &self.account_deps[index].1 as &RefCell, } else { index = index.saturating_sub(self.account_deps.len()); - ( - *is_signer, - *is_writable, - &self.message.account_keys[index], - // TODO - // Currently we are constructing new accounts on the stack - // before calling MessageProcessor::process_cross_program_instruction - // Ideally we would recycle the existing accounts here like this: - // &self.accounts[index] as &RefCell, - transmute_lifetime(*account), - ) - } + &self.message.account_keys[index] + // &self.accounts[index] as &RefCell, + }; + (*is_signer, *is_writable, key, transmute_lifetime(*account)) }) }) .collect::>>()