From 4979b95de44fa6fb73fcf28685a76ca58fb18cf0 Mon Sep 17 00:00:00 2001 From: Joe C Date: Sat, 9 Nov 2024 13:06:26 +0400 Subject: [PATCH] Program: add custom error codes for readonly writes (#21) * program: add custom error codes for executable, ro writes * check in new compute unit benchmarks * drop executable code * check in new compute unit benchmark --- Cargo.lock | 3 ++ program/Cargo.toml | 3 ++ program/benches/compute_units.md | 41 ++++++++++++++++++++++++++ program/src/entrypoint.rs | 13 +++++++-- program/src/error.rs | 37 ++++++++++++++++++++++++ program/src/lib.rs | 1 + program/src/processor.rs | 25 +++++++++++++++- program/tests/functional.rs | 49 +++++++++++++++++++++++++------- 8 files changed, 158 insertions(+), 14 deletions(-) create mode 100644 program/src/error.rs diff --git a/Cargo.lock b/Cargo.lock index 0af8e60..02b3e5d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3842,9 +3842,12 @@ dependencies = [ "bincode", "mollusk-svm", "mollusk-svm-bencher", + "num-derive 0.4.2", + "num-traits", "serde", "solana-program", "solana-sdk", + "thiserror", ] [[package]] diff --git a/program/Cargo.toml b/program/Cargo.toml index 1c63f7b..4677492 100644 --- a/program/Cargo.toml +++ b/program/Cargo.toml @@ -17,8 +17,11 @@ test-sbf = [] [dependencies] bincode = "1.3.3" +num-derive = "0.4" +num-traits = "0.2" serde = { version = "1.0.193", features = ["derive"] } solana-program = "2.0.1" +thiserror = "1.0.61" [dev-dependencies] mollusk-svm = { version = "0.0.5", features = ["fuzz"] } diff --git a/program/benches/compute_units.md b/program/benches/compute_units.md index a8abcab..4c96e57 100644 --- a/program/benches/compute_units.md +++ b/program/benches/compute_units.md @@ -1,3 +1,44 @@ +#### Compute Units: 2024-11-08 12:36:57.438693 UTC + +| Name | CUs | Delta | +|------|------|-------| +| config_small_init_0_keys | 616 | +4 | +| config_small_init_1_keys | 1247 | +4 | +| config_small_init_5_keys | 2866 | +4 | +| config_small_init_10_keys | 4936 | +4 | +| config_small_init_25_keys | 11778 | +4 | +| config_small_init_37_keys | 16781 | +4 | +| config_small_store_0_keys | 616 | +4 | +| config_small_store_1_keys | 1501 | +4 | +| config_small_store_5_keys | 4036 | +4 | +| config_small_store_10_keys | 7251 | +4 | +| config_small_store_25_keys | 17528 | +4 | +| config_small_store_37_keys | 25279 | +4 | +| config_medium_init_0_keys | 607 | +4 | +| config_medium_init_1_keys | 1194 | +4 | +| config_medium_init_5_keys | 2866 | +4 | +| config_medium_init_10_keys | 4936 | +4 | +| config_medium_init_25_keys | 11778 | +4 | +| config_medium_init_37_keys | 16781 | +4 | +| config_medium_store_0_keys | 607 | +4 | +| config_medium_store_1_keys | 1448 | +4 | +| config_medium_store_5_keys | 4036 | +4 | +| config_medium_store_10_keys | 7251 | +4 | +| config_medium_store_25_keys | 17528 | +4 | +| config_medium_store_37_keys | 25279 | +4 | +| config_large_init_0_keys | 728 | +4 | +| config_large_init_1_keys | 1315 | +4 | +| config_large_init_5_keys | 2987 | +4 | +| config_large_init_10_keys | 5058 | +4 | +| config_large_init_25_keys | 11902 | +4 | +| config_large_init_37_keys | 16906 | +4 | +| config_large_store_0_keys | 728 | +4 | +| config_large_store_1_keys | 1569 | +4 | +| config_large_store_5_keys | 4157 | +4 | +| config_large_store_10_keys | 7373 | +4 | +| config_large_store_25_keys | 17652 | +4 | +| config_large_store_37_keys | 25404 | +4 | + #### Compute Units: 2024-11-04 12:41:50.422792 UTC | Name | CUs | Delta | diff --git a/program/src/entrypoint.rs b/program/src/entrypoint.rs index 231128a..bf2af37 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::ConfigError, 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..20b9c7c --- /dev/null +++ b/program/src/error.rs @@ -0,0 +1,37 @@ +//! 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 ConfigError { + /// Instruction modified data of a read-only account. + #[error("Instruction modified data of a read-only account")] + ReadonlyDataModified, +} + +impl PrintProgramError for ConfigError { + fn print(&self) { + msg!(&self.to_string()); + } +} + +impl From for ProgramError { + fn from(e: ConfigError) -> Self { + ProgramError::Custom(e as u32) + } +} + +impl DecodeError for ConfigError { + fn type_of() -> &'static str { + "ConfigError" + } +} diff --git a/program/src/lib.rs b/program/src/lib.rs index 03e1fd2..6a29970 100644 --- a/program/src/lib.rs +++ b/program/src/lib.rs @@ -2,6 +2,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 809eef4..46a0ba3 100644 --- a/program/src/processor.rs +++ b/program/src/processor.rs @@ -1,7 +1,7 @@ //! Config program processor. use { - crate::state::ConfigKeys, + crate::{error::ConfigError, state::ConfigKeys}, solana_program::{ account_info::AccountInfo, entrypoint::ProgramResult, msg, program_error::ProgramError, pubkey::Pubkey, @@ -160,6 +160,29 @@ pub fn process(program_id: &Pubkey, accounts: &[AccountInfo], input: &[u8]) -> P return Err(ProgramError::InvalidInstructionData); } + // [Core BPF]: + // When a builtin program attempts to write to an executable or read-only + // account, it will be immediately rejected by the `TransactionContext`. + // However, BPF programs do not query the `TransactionContext` for the + // ability to perform a write. Instead, they perform writes at-will, and + // the loader will inspect the serialized account memory region for any + // account update violations _after_ the VM has completed execution. + // + // The loader's inspection will catch any unauthorized modifications, + // however, when the exact same data is written to the account, thus + // rendering the serialized account state unchanged, the program succeeds. + // + // In order to maximize backwards compatibility between the BPF version and + // its original builtin, we add this check from `TransactionContext` to the + // program directly, to throw even when the data being written is the same + // same as what's currently in the account. + // + // Since the account can never be executable and also owned by the config + // program, we'll just focus on readonly. + if !config_account.is_writable { + return Err(ConfigError::ReadonlyDataModified.into()); + } + config_account.try_borrow_mut_data()?[..input.len()].copy_from_slice(input); Ok(()) diff --git a/program/tests/functional.rs b/program/tests/functional.rs index 594a28f..bcb54ee 100644 --- a/program/tests/functional.rs +++ b/program/tests/functional.rs @@ -5,6 +5,7 @@ use { mollusk_svm::{result::Check, Mollusk}, serde::{Deserialize, Serialize}, solana_config_program::{ + error::ConfigError, instruction as config_instruction, state::{ConfigKeys, ConfigState}, }, @@ -83,7 +84,7 @@ fn test_process_create_ok() { &[(config, config_account)], &[ Check::success(), - Check::compute_units(612), + Check::compute_units(616), Check::account(&config) .data( &bincode::serialize(&(ConfigKeys { keys: vec![] }, MyConfig::default())) @@ -111,7 +112,7 @@ fn test_process_store_ok() { &[(config, config_account)], &[ Check::success(), - Check::compute_units(612), + Check::compute_units(616), Check::account(&config) .data(&bincode::serialize(&(ConfigKeys { keys }, my_config)).unwrap()) .build(), @@ -186,7 +187,7 @@ fn test_process_store_with_additional_signers() { ], &[ Check::success(), - Check::compute_units(3_267), + Check::compute_units(3_271), Check::account(&config) .data(&bincode::serialize(&(ConfigKeys { keys }, my_config)).unwrap()) .build(), @@ -334,7 +335,7 @@ fn test_config_updates() { (signer0, AccountSharedData::default()), (signer1, AccountSharedData::default()), ], - &[Check::success(), Check::compute_units(3_267)], + &[Check::success(), Check::compute_units(3_271)], ); // Use this for next invoke. @@ -352,7 +353,7 @@ fn test_config_updates() { ], &[ Check::success(), - Check::compute_units(3_268), + Check::compute_units(3_272), Check::account(&config) .data(&bincode::serialize(&(ConfigKeys { keys }, new_config)).unwrap()) .build(), @@ -465,7 +466,7 @@ fn test_config_update_contains_duplicates_fails() { (signer0, AccountSharedData::default()), (signer1, AccountSharedData::default()), ], - &[Check::success(), Check::compute_units(3_267)], + &[Check::success(), Check::compute_units(3_271)], ); // Attempt update with duplicate signer inputs. @@ -509,7 +510,7 @@ fn test_config_updates_requiring_config() { ], &[ Check::success(), - Check::compute_units(3_363), + Check::compute_units(3_367), Check::account(&config) .data(&bincode::serialize(&(ConfigKeys { keys: keys.clone() }, my_config)).unwrap()) .build(), @@ -530,7 +531,7 @@ fn test_config_updates_requiring_config() { ], &[ Check::success(), - Check::compute_units(3_363), + Check::compute_units(3_367), Check::account(&config) .data(&bincode::serialize(&(ConfigKeys { keys }, new_config)).unwrap()) .build(), @@ -624,7 +625,7 @@ fn test_maximum_keys_input() { let result = mollusk.process_and_validate_instruction( &instruction, &[(config, config_account)], - &[Check::success(), Check::compute_units(25_275)], + &[Check::success(), Check::compute_units(25_279)], ); // Use this for next invoke. @@ -637,7 +638,7 @@ fn test_maximum_keys_input() { let result = mollusk.process_and_validate_instruction( &instruction, &[(config, updated_config_account)], - &[Check::success(), Check::compute_units(25_275)], + &[Check::success(), Check::compute_units(25_279)], ); // Use this for next invoke. @@ -748,3 +749,31 @@ fn test_safe_deserialize_from_state() { &[Check::err(ProgramError::InvalidAccountData)], ); } + +// Backwards compatibility test case. +#[test] +fn test_write_same_data_to_readonly() { + let mollusk = setup(); + + let config = Pubkey::new_unique(); + let keys = vec![]; + + // Creates a config account with `MyConfig::default()`. + let config_account = create_config_account(&mollusk, keys.clone()); + + // Pass the exact same data (`MyConfig::default()`) to the instruction, + // which we'll attempt to write into the account. + let mut instruction = + config_instruction::store(&config, true, keys.clone(), &MyConfig::default()); + + // Make the config account read-only. + instruction.accounts[0].is_writable = false; + + mollusk.process_and_validate_instruction( + &instruction, + &[(config, config_account)], + &[Check::err(ProgramError::Custom( + ConfigError::ReadonlyDataModified as u32, + ))], + ); +}