From 2cd9dc99b660fd555e1975f06aceba8da6a59480 Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Tue, 28 Sep 2021 10:59:08 -0500 Subject: [PATCH] Restore ability for programs to upgrade themselves (#20265) * Make helper associated fn * Add feature definition * Add handling to preserve program-id write lock when upgradeable loader is present; restore bpf upgrade-self test * Use single feature --- programs/bpf/tests/programs.rs | 96 ++++++++++++++++++++++++++-- runtime/src/accounts.rs | 9 +-- sdk/program/src/message/legacy.rs | 12 +++- sdk/program/src/message/mapped.rs | 15 ++++- sdk/program/src/message/sanitized.rs | 8 +++ sdk/src/feature_set.rs | 2 +- 6 files changed, 126 insertions(+), 16 deletions(-) diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index c99dfacff..2be7c2114 100644 --- a/programs/bpf/tests/programs.rs +++ b/programs/bpf/tests/programs.rs @@ -1867,7 +1867,7 @@ fn test_program_bpf_upgrade_and_invoke_in_same_tx() { "solana_bpf_rust_panic", ); - // Attempt to invoke, then upgrade the program in same tx + // Invoke, then upgrade the program, and then invoke again in same tx let message = Message::new( &[ invoke_instruction.clone(), @@ -1886,12 +1886,10 @@ fn test_program_bpf_upgrade_and_invoke_in_same_tx() { message.clone(), bank.last_blockhash(), ); - // program_id is automatically demoted to readonly, preventing the upgrade, which requires - // writeability let (result, _) = process_transaction_and_record_inner(&bank, tx); assert_eq!( result.unwrap_err(), - TransactionError::InstructionError(1, InstructionError::InvalidArgument) + TransactionError::InstructionError(2, InstructionError::ProgramFailedToComplete) ); } @@ -2187,6 +2185,96 @@ fn test_program_bpf_upgrade_via_cpi() { assert_ne!(programdata, original_programdata); } +#[cfg(feature = "bpf_rust")] +#[test] +fn test_program_bpf_upgrade_self_via_cpi() { + solana_logger::setup(); + + let GenesisConfigInfo { + genesis_config, + mint_keypair, + .. + } = create_genesis_config(50); + let mut bank = Bank::new_for_tests(&genesis_config); + let (name, id, entrypoint) = solana_bpf_loader_program!(); + bank.add_builtin(&name, id, entrypoint); + let (name, id, entrypoint) = solana_bpf_loader_upgradeable_program!(); + bank.add_builtin(&name, id, entrypoint); + let bank = Arc::new(bank); + let bank_client = BankClient::new_shared(&bank); + let noop_program_id = load_bpf_program( + &bank_client, + &bpf_loader::id(), + &mint_keypair, + "solana_bpf_rust_noop", + ); + + // Deploy upgradeable program + let buffer_keypair = Keypair::new(); + let program_keypair = Keypair::new(); + let program_id = program_keypair.pubkey(); + let authority_keypair = Keypair::new(); + load_upgradeable_bpf_program( + &bank_client, + &mint_keypair, + &buffer_keypair, + &program_keypair, + &authority_keypair, + "solana_bpf_rust_invoke_and_return", + ); + + let mut invoke_instruction = Instruction::new_with_bytes( + program_id, + &[0], + vec![ + AccountMeta::new_readonly(noop_program_id, false), + AccountMeta::new_readonly(noop_program_id, false), + AccountMeta::new_readonly(clock::id(), false), + ], + ); + + // Call the upgraded program + invoke_instruction.data[0] += 1; + let result = + bank_client.send_and_confirm_instruction(&mint_keypair, invoke_instruction.clone()); + assert!(result.is_ok()); + + // Prepare for upgrade + let buffer_keypair = Keypair::new(); + load_upgradeable_buffer( + &bank_client, + &mint_keypair, + &buffer_keypair, + &authority_keypair, + "solana_bpf_rust_panic", + ); + + // Invoke, then upgrade the program, and then invoke again in same tx + let message = Message::new( + &[ + invoke_instruction.clone(), + bpf_loader_upgradeable::upgrade( + &program_id, + &buffer_keypair.pubkey(), + &authority_keypair.pubkey(), + &mint_keypair.pubkey(), + ), + invoke_instruction, + ], + Some(&mint_keypair.pubkey()), + ); + let tx = Transaction::new( + &[&mint_keypair, &authority_keypair], + message.clone(), + bank.last_blockhash(), + ); + let (result, _) = process_transaction_and_record_inner(&bank, tx); + assert_eq!( + result.unwrap_err(), + TransactionError::InstructionError(2, InstructionError::ProgramFailedToComplete) + ); +} + #[cfg(feature = "bpf_rust")] #[test] fn test_program_bpf_set_upgrade_authority_via_cpi() { diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 97323bf0f..f32e769f0 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -241,7 +241,6 @@ 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) { @@ -280,7 +279,7 @@ impl Accounts { 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 + && !message.is_upgradeable_loader_present() { error_counters.invalid_writable_account += 1; return Err(TransactionError::InvalidWritableAccount); @@ -1133,12 +1132,6 @@ 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, diff --git a/sdk/program/src/message/legacy.rs b/sdk/program/src/message/legacy.rs index 21742b837..bbbb49d2d 100644 --- a/sdk/program/src/message/legacy.rs +++ b/sdk/program/src/message/legacy.rs @@ -354,6 +354,9 @@ impl Message { } pub fn is_writable(&self, i: usize, demote_program_write_locks: bool) -> bool { + let demote_program_id = demote_program_write_locks + && self.is_key_called_as_program(i) + && !self.is_upgradeable_loader_present(); (i < (self.header.num_required_signatures - self.header.num_readonly_signed_accounts) as usize || (i >= self.header.num_required_signatures as usize @@ -363,7 +366,7 @@ impl Message { let key = self.account_keys[i]; sysvar::is_sysvar_id(&key) || BUILTIN_PROGRAMS_KEYS.contains(&key) } - && !(demote_program_write_locks && self.is_key_called_as_program(i)) + && !demote_program_id } pub fn is_signer(&self, i: usize) -> bool { @@ -503,6 +506,13 @@ impl Message { } false } + + /// Returns true if any account is the bpf upgradeable loader + pub fn is_upgradeable_loader_present(&self) -> bool { + self.account_keys + .iter() + .any(|&key| key == bpf_loader_upgradeable::id()) + } } #[cfg(test)] diff --git a/sdk/program/src/message/mapped.rs b/sdk/program/src/message/mapped.rs index 0f30e3523..7c599fb6d 100644 --- a/sdk/program/src/message/mapped.rs +++ b/sdk/program/src/message/mapped.rs @@ -1,5 +1,6 @@ use { crate::{ + bpf_loader_upgradeable, message::{legacy::BUILTIN_PROGRAMS_KEYS, v0}, pubkey::Pubkey, sysvar, @@ -99,8 +100,12 @@ impl MappedMessage { pub fn is_writable(&self, key_index: usize, demote_program_write_locks: bool) -> bool { if self.is_writable_index(key_index) { if let Some(key) = self.get_account_key(key_index) { - return !(sysvar::is_sysvar_id(key) || BUILTIN_PROGRAMS_KEYS.contains(key) - || (demote_program_write_locks && self.is_key_called_as_program(key_index))); + let demote_program_id = demote_program_write_locks + && self.is_key_called_as_program(key_index) + && !self.is_upgradeable_loader_present(); + return !(sysvar::is_sysvar_id(key) + || BUILTIN_PROGRAMS_KEYS.contains(key) + || demote_program_id); } } false @@ -116,6 +121,12 @@ impl MappedMessage { false } } + + /// Returns true if any account is the bpf upgradeable loader + pub fn is_upgradeable_loader_present(&self) -> bool { + self.account_keys_iter() + .any(|&key| key == bpf_loader_upgradeable::id()) + } } #[cfg(test)] diff --git a/sdk/program/src/message/sanitized.rs b/sdk/program/src/message/sanitized.rs index 3b8d3d797..d771f2ca0 100644 --- a/sdk/program/src/message/sanitized.rs +++ b/sdk/program/src/message/sanitized.rs @@ -308,6 +308,14 @@ impl SanitizedMessage { .saturating_add(num_secp256k1_signatures), ) } + + /// Inspect all message keys for the bpf upgradeable loader + pub fn is_upgradeable_loader_present(&self) -> bool { + match self { + Self::Legacy(message) => message.is_upgradeable_loader_present(), + Self::V0(message) => message.is_upgradeable_loader_present(), + } + } } #[cfg(test)] diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 44db6fc86..502d0b472 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -264,7 +264,7 @@ lazy_static! { (instructions_sysvar_owned_by_sysvar::id(), "fix owner for instructions sysvar"), (close_upgradeable_program_accounts::id(), "enable closing upgradeable program accounts"), (stake_program_advance_activating_credits_observed::id(), "Enable advancing credits observed for activation epoch #19309"), - (demote_program_write_locks::id(), "demote program write locks to readonly #19593"), + (demote_program_write_locks::id(), "demote program write locks to readonly, except when upgradeable loader present #19593 #20265"), (ed25519_program_enabled::id(), "enable builtin ed25519 signature verify program"), (allow_native_ids::id(), "allow native program ids in program derived addresses"), (check_seed_length::id(), "Check program address seed lengths"),