From 8e59e379ae987feb4b5c62a8627f2926d435b316 Mon Sep 17 00:00:00 2001 From: Joe Caulfield Date: Fri, 8 Nov 2024 09:28:20 +0000 Subject: [PATCH] program: add custom error codes for executable, ro writes --- Cargo.lock | 3 + program/Cargo.toml | 3 + program/src/entrypoint.rs | 13 ++++- program/src/error.rs | 40 +++++++++++++ program/src/lib.rs | 1 + program/src/processor.rs | 46 +++++++++++++++ program/tests/extend_lookup_table_ix.rs | 75 +++++++++++++++++++++++++ 7 files changed, 178 insertions(+), 3 deletions(-) create mode 100644 program/src/error.rs diff --git a/Cargo.lock b/Cargo.lock index b22229c..e39b066 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2719,12 +2719,15 @@ dependencies = [ "bytemuck", "mollusk-svm", "mollusk-svm-bencher", + "num-derive 0.4.2", + "num-traits", "serde", "solana-frozen-abi 2.0.1", "solana-frozen-abi-macro 2.0.1", "solana-program 2.0.1", "solana-sdk", "test-case", + "thiserror", ] [[package]] diff --git a/program/Cargo.toml b/program/Cargo.toml index 81c43c2..d934e55 100644 --- a/program/Cargo.toml +++ b/program/Cargo.toml @@ -19,10 +19,13 @@ test-sbf = [] [dependencies] bincode = "1.3.3" bytemuck = "1.14.1" +num-derive = "0.4" +num-traits = "0.2" serde = { version = "1.0.193", features = ["derive"] } solana-frozen-abi = { version = "2.0.1", optional = true } solana-frozen-abi-macro = { version = "2.0.1", optional = true } solana-program = "2.0.1" +thiserror = "1.0.61" [dev-dependencies] mollusk-svm = { version = "0.0.5", features = ["fuzz"] } diff --git a/program/src/entrypoint.rs b/program/src/entrypoint.rs index f79f7f5..f9f16f8 100644 --- a/program/src/entrypoint.rs +++ b/program/src/entrypoint.rs @@ -1,8 +1,11 @@ //! Program entrypoint use { - crate::processor, - solana_program::{account_info::AccountInfo, entrypoint::ProgramResult, pubkey::Pubkey}, + crate::{error::AddressLookupTableError, processor}, + solana_program::{ + account_info::AccountInfo, entrypoint::ProgramResult, program_error::PrintProgramError, + pubkey::Pubkey, + }, }; solana_program::entrypoint!(process_instruction); @@ -11,5 +14,9 @@ fn process_instruction( accounts: &[AccountInfo], instruction_data: &[u8], ) -> ProgramResult { - processor::process(program_id, accounts, instruction_data) + if let Err(error) = processor::process(program_id, accounts, instruction_data) { + error.print::(); + return Err(error); + } + Ok(()) } diff --git a/program/src/error.rs b/program/src/error.rs new file mode 100644 index 0000000..c7626d5 --- /dev/null +++ b/program/src/error.rs @@ -0,0 +1,40 @@ +//! Program error types. + +use { + num_derive::FromPrimitive, + solana_program::{ + decode_error::DecodeError, + msg, + program_error::{PrintProgramError, ProgramError}, + }, + thiserror::Error, +}; + +/// Errors that can be returned by the Config program. +#[derive(Error, Clone, Debug, Eq, PartialEq, FromPrimitive)] +pub enum AddressLookupTableError { + /// Instruction changed executable account's data. + #[error("Instruction changed executable account's data")] + ExecutableDataModified = 10, // Avoid collisions with SystemError from CPI. + /// Instruction modified data of a read-only account. + #[error("Instruction modified data of a read-only account")] + ReadonlyDataModified, +} + +impl PrintProgramError for AddressLookupTableError { + fn print(&self) { + msg!(&self.to_string()); + } +} + +impl From for ProgramError { + fn from(e: AddressLookupTableError) -> Self { + ProgramError::Custom(e as u32) + } +} + +impl DecodeError for AddressLookupTableError { + fn type_of() -> &'static str { + "AddressLookupTableError" + } +} diff --git a/program/src/lib.rs b/program/src/lib.rs index 6bb092a..bae8b5f 100644 --- a/program/src/lib.rs +++ b/program/src/lib.rs @@ -3,6 +3,7 @@ #[cfg(all(target_os = "solana", feature = "bpf-entrypoint"))] mod entrypoint; +pub mod error; pub mod instruction; pub mod processor; pub mod state; diff --git a/program/src/processor.rs b/program/src/processor.rs index 0ef8d2c..38dbfe2 100644 --- a/program/src/processor.rs +++ b/program/src/processor.rs @@ -3,6 +3,7 @@ use { crate::{ check_id, + error::AddressLookupTableError, instruction::AddressLookupTableInstruction, state::{ AddressLookupTable, ProgramState, LOOKUP_TABLE_MAX_ADDRESSES, LOOKUP_TABLE_META_SIZE, @@ -145,6 +146,8 @@ fn process_create_lookup_table( )?; } + // [Core BPF]: No need to check `is_executable` or `is_writable` here, + // since they will be checked by System. invoke_signed( &system_instruction::allocate(lookup_table_info.key, lookup_table_data_len as u64), &[lookup_table_info.clone()], @@ -205,6 +208,10 @@ fn process_freeze_lookup_table(program_id: &Pubkey, accounts: &[AccountInfo]) -> lookup_table.meta }; + // [Core BPF]: No need to check `is_executable` or `is_writable` here, + // since only non-frozen lookup tables can be frozen (never a same-data + // write). + lookup_table_meta.authority = None; AddressLookupTable::overwrite_meta_data( &mut lookup_table_info.try_borrow_mut_data()?[..], @@ -297,6 +304,36 @@ fn process_extend_lookup_table( ) }; + // [Core BPF]: + // When a builtin program attempts to write to an executable or read-only + // account, it will be immediately rejected by the `TransactionContext`. + // For more information, see https://github.com/solana-program/config/pull/21. + // + // However, in the case of the Address Lookup Table program's + // `ExtendLookupTable` instruction, since the processor rejects any + // zero-length "new keys" vectors, and will gladly append the same keys + // again to the table, the issue here is slightly different than the linked + // PR. + // + // The builtin version of the Address Lookup Table program will throw + // when it attempts to overwrite the metadata, while the BPF version will + // continue. In the case where an executable or read-only lookup table + // account is provided, and some other requirement below is violated + // (ie. no payer or system program accounts provided, payer is not a + // signer, payer has insufficent balance, etc.), the BPF version will throw + // based on one of those violations, rather than throwing immediately when + // it encounters the executable or read-only lookup table account. + // + // As a result, this placement of these mocked out `InstructionError` + // variants ensures maximum backwards compatibility with the builtin + // version. + if lookup_table_info.executable { + return Err(AddressLookupTableError::ExecutableDataModified.into()); + } + if !lookup_table_info.is_writable { + return Err(AddressLookupTableError::ReadonlyDataModified.into()); + } + AddressLookupTable::overwrite_meta_data( &mut lookup_table_info.try_borrow_mut_data()?[..], lookup_table_meta, @@ -374,6 +411,11 @@ fn process_deactivate_lookup_table(program_id: &Pubkey, accounts: &[AccountInfo] }; let clock = ::get()?; + + // [Core BPF]: No need to check `is_executable` or `is_writable` here, + // since only non-deactivated lookup tables can be deactivated (never a + // same-data write). + lookup_table_meta.deactivation_slot = clock.slot; AddressLookupTable::overwrite_meta_data( @@ -444,6 +486,10 @@ fn process_close_lookup_table(program_id: &Pubkey, accounts: &[AccountInfo]) -> }?; } + // [Core BPF]: No need to check `is_executable` or `is_writable` here, + // since only non-closed lookup tables can be deactivated. Deserialization + // would fail earlier in the processor. + let new_recipient_lamports = lookup_table_info .lamports() .checked_add(recipient_info.lamports()) diff --git a/program/tests/extend_lookup_table_ix.rs b/program/tests/extend_lookup_table_ix.rs index 9cf642c..6733af9 100644 --- a/program/tests/extend_lookup_table_ix.rs +++ b/program/tests/extend_lookup_table_ix.rs @@ -10,6 +10,7 @@ use { Mollusk, }, solana_address_lookup_table_program::{ + error::AddressLookupTableError, instruction::extend_lookup_table, state::{AddressLookupTable, LookupTableMeta}, }, @@ -355,3 +356,77 @@ fn test_extend_prepaid_lookup_table_without_payer() { }, ); } + +// Backwards compatibility test case. +#[test] +fn test_extend_executable() { + let mollusk = setup(); + + let payer = Pubkey::new_unique(); + let authority = Pubkey::new_unique(); + + let initialized_table = new_address_lookup_table(Some(authority), 0); + + let lookup_table_address = Pubkey::new_unique(); + let mut lookup_table_account = lookup_table_account(initialized_table); + + // Make the lookup table account executable. + lookup_table_account.set_executable(true); + + let new_addresses = vec![Pubkey::new_unique()]; + let instruction = + extend_lookup_table(lookup_table_address, authority, Some(payer), new_addresses); + + mollusk.process_and_validate_instruction( + &instruction, + &[ + (lookup_table_address, lookup_table_account), + (authority, AccountSharedData::default()), + ( + payer, + AccountSharedData::new(100_000_000, 0, &system_program::id()), + ), + keyed_account_for_system_program(), + ], + &[Check::err(ProgramError::Custom( + AddressLookupTableError::ExecutableDataModified as u32, + ))], + ); +} + +// Backwards compatibility test case. +#[test] +fn test_extend_readonly() { + let mollusk = setup(); + + let payer = Pubkey::new_unique(); + let authority = Pubkey::new_unique(); + + let initialized_table = new_address_lookup_table(Some(authority), 0); + + let lookup_table_address = Pubkey::new_unique(); + let lookup_table_account = lookup_table_account(initialized_table); + + let new_addresses = vec![Pubkey::new_unique()]; + let mut instruction = + extend_lookup_table(lookup_table_address, authority, Some(payer), new_addresses); + + // Make the lookup table account read-only. + instruction.accounts[0].is_writable = false; + + mollusk.process_and_validate_instruction( + &instruction, + &[ + (lookup_table_address, lookup_table_account), + (authority, AccountSharedData::default()), + ( + payer, + AccountSharedData::new(100_000_000, 0, &system_program::id()), + ), + keyed_account_for_system_program(), + ], + &[Check::err(ProgramError::Custom( + AddressLookupTableError::ReadonlyDataModified as u32, + ))], + ); +}