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
solana-program · Nov 9, 2024 · 4979b95 · 4979b95
1 parent e2024b2
commit 4979b95
Show file tree

Hide file tree

Showing 8 changed files with 158 additions and 14 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git 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
@@ -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
@@ -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::<ConfigError>();
+        return Err(error);
+    }
+    Ok(())
 }
diff --git a/program/src/error.rs 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<E>(&self) {
+        msg!(&self.to_string());
+    }
+}
+
+impl From<ConfigError> for ProgramError {
+    fn from(e: ConfigError) -> Self {
+        ProgramError::Custom(e as u32)
+    }
+}
+
+impl<T> DecodeError<T> for ConfigError {
+    fn type_of() -> &'static str {
+        "ConfigError"
+    }
+}
diff --git 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
@@ -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
@@ -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,
+        ))],
+    );
+}