program: safe deserialize config keys from state (#18)

* program: safe deserialize config keys from state

* check in new compute unit benchmark

Author: buffalojoec

program/benches/compute_units.md

@@ -1,3 +1,44 @@
+#### Compute Units: 2024-11-04 12:41:50.422792 UTC
+
+| Name | CUs | Delta |
+|------|------|-------|
+| config_small_init_0_keys | 612 | +32 |
+| config_small_init_1_keys | 1243 | +32 |
+| config_small_init_5_keys | 2862 | +32 |
+| config_small_init_10_keys | 4932 | +32 |
+| config_small_init_25_keys | 11774 | +32 |
+| config_small_init_37_keys | 16777 | +32 |
+| config_small_store_0_keys | 612 | +32 |
+| config_small_store_1_keys | 1497 | +32 |
+| config_small_store_5_keys | 4032 | +32 |
+| config_small_store_10_keys | 7247 | +32 |
+| config_small_store_25_keys | 17524 | +32 |
+| config_small_store_37_keys | 25275 | +32 |
+| config_medium_init_0_keys | 603 | +32 |
+| config_medium_init_1_keys | 1190 | +32 |
+| config_medium_init_5_keys | 2862 | +32 |
+| config_medium_init_10_keys | 4932 | +32 |
+| config_medium_init_25_keys | 11774 | +32 |
+| config_medium_init_37_keys | 16777 | +32 |
+| config_medium_store_0_keys | 603 | +32 |
+| config_medium_store_1_keys | 1444 | +32 |
+| config_medium_store_5_keys | 4032 | +32 |
+| config_medium_store_10_keys | 7247 | +32 |
+| config_medium_store_25_keys | 17524 | +32 |
+| config_medium_store_37_keys | 25275 | +32 |
+| config_large_init_0_keys | 724 | +32 |
+| config_large_init_1_keys | 1311 | +32 |
+| config_large_init_5_keys | 2983 | +32 |
+| config_large_init_10_keys | 5054 | +32 |
+| config_large_init_25_keys | 11898 | +32 |
+| config_large_init_37_keys | 16902 | +32 |
+| config_large_store_0_keys | 724 | +32 |
+| config_large_store_1_keys | 1565 | +32 |
+| config_large_store_5_keys | 4153 | +32 |
+| config_large_store_10_keys | 7369 | +32 |
+| config_large_store_25_keys | 17648 | +32 |
+| config_large_store_37_keys | 25400 | +32 |
+
 #### Compute Units: 2024-11-01 15:31:11.543055 UTC
 
 | Name | CUs | Delta |

program/src/processor.rs

@@ -49,6 +49,33 @@ fn safe_deserialize_config_keys(input: &[u8]) -> Result<ConfigKeys, ProgramError
     }
 }
 
+// [Core BPF]: Similar to `safe_deserialize_config_keys`, this helper serves to
+// avoid over-allocations of memory, especially when the trailing data is
+// invalid.
+//
+// Consider a case where an account is malformed, and the `ShortU16` vector
+// length actually stores a value larger than the buffer itself. The original
+// Config program builtin can handle the initial memory allocation, eventually
+// returning `ProgramError::InvalidAccountData` when bincode throws an EOF
+// error. However, the BPF version will panic on OOM before it can successfully
+// return `ProgramError::InvalidAccountData`.
+//
+// This helper's purpose is solely to catch malformed `ConfigKeys` data before
+// a memory allocation panic can occur, to ensure maximum backwards
+// compatibility with the original builtin.
+fn safe_deserialize_config_keys_from_state(input: &[u8]) -> Result<ConfigKeys, ProgramError> {
+    let (vector_len, offset) = solana_program::short_vec::decode_shortu16_len(input)
+        .map_err(|_| ProgramError::InvalidAccountData)?;
+    if input[offset..].len() / (32 + 1) < vector_len {
+        Err(ProgramError::InvalidAccountData)
+    } else {
+        bincode::deserialize(input).map_err(|err| {
+            msg!("Unable to deserialize config account: {}", err);
+            ProgramError::InvalidAccountData
+        })
+    }
+}
+
 /// Config program processor.
 pub fn process(program_id: &Pubkey, accounts: &[AccountInfo], input: &[u8]) -> ProgramResult {
     let key_list = safe_deserialize_config_keys(input)?;
@@ -60,11 +87,7 @@ pub fn process(program_id: &Pubkey, accounts: &[AccountInfo], input: &[u8]) -> P
         return Err(ProgramError::InvalidAccountOwner);
     }
 
-    let current_data: ConfigKeys = bincode::deserialize(&config_account.try_borrow_data()?)
-        .map_err(|err| {
-            msg!("Unable to deserialize config account: {}", err);
-            ProgramError::InvalidAccountData
-        })?;
+    let current_data = safe_deserialize_config_keys_from_state(&config_account.try_borrow_data()?)?;
 
     let current_signer_keys: Vec<Pubkey> = current_data
         .keys

program/tests/functional.rs

@@ -83,7 +83,7 @@ fn test_process_create_ok() {
         &[(config, config_account)],
         &[
             Check::success(),
-            Check::compute_units(580),
+            Check::compute_units(612),
             Check::account(&config)
                 .data(
                     &bincode::serialize(&(ConfigKeys { keys: vec![] }, MyConfig::default()))
@@ -111,7 +111,7 @@ fn test_process_store_ok() {
         &[(config, config_account)],
         &[
             Check::success(),
-            Check::compute_units(580),
+            Check::compute_units(612),
             Check::account(&config)
                 .data(&bincode::serialize(&(ConfigKeys { keys }, my_config)).unwrap())
                 .build(),
@@ -186,7 +186,7 @@ fn test_process_store_with_additional_signers() {
         ],
         &[
             Check::success(),
-            Check::compute_units(3_234),
+            Check::compute_units(3_267),
             Check::account(&config)
                 .data(&bincode::serialize(&(ConfigKeys { keys }, my_config)).unwrap())
                 .build(),
@@ -334,7 +334,7 @@ fn test_config_updates() {
             (signer0, AccountSharedData::default()),
             (signer1, AccountSharedData::default()),
         ],
-        &[Check::success(), Check::compute_units(3_234)],
+        &[Check::success(), Check::compute_units(3_267)],
     );
 
     // Use this for next invoke.
@@ -352,7 +352,7 @@ fn test_config_updates() {
         ],
         &[
             Check::success(),
-            Check::compute_units(3_235),
+            Check::compute_units(3_268),
             Check::account(&config)
                 .data(&bincode::serialize(&(ConfigKeys { keys }, new_config)).unwrap())
                 .build(),
@@ -465,7 +465,7 @@ fn test_config_update_contains_duplicates_fails() {
             (signer0, AccountSharedData::default()),
             (signer1, AccountSharedData::default()),
         ],
-        &[Check::success(), Check::compute_units(3_234)],
+        &[Check::success(), Check::compute_units(3_267)],
     );
 
     // Attempt update with duplicate signer inputs.
@@ -509,7 +509,7 @@ fn test_config_updates_requiring_config() {
         ],
         &[
             Check::success(),
-            Check::compute_units(3_330),
+            Check::compute_units(3_363),
             Check::account(&config)
                 .data(&bincode::serialize(&(ConfigKeys { keys: keys.clone() }, my_config)).unwrap())
                 .build(),
@@ -530,7 +530,7 @@ fn test_config_updates_requiring_config() {
         ],
         &[
             Check::success(),
-            Check::compute_units(3_330),
+            Check::compute_units(3_363),
             Check::account(&config)
                 .data(&bincode::serialize(&(ConfigKeys { keys }, new_config)).unwrap())
                 .build(),
@@ -624,7 +624,7 @@ fn test_maximum_keys_input() {
     let result = mollusk.process_and_validate_instruction(
         &instruction,
         &[(config, config_account)],
-        &[Check::success(), Check::compute_units(25_243)],
+        &[Check::success(), Check::compute_units(25_275)],
     );
 
     // Use this for next invoke.
@@ -637,7 +637,7 @@ fn test_maximum_keys_input() {
     let result = mollusk.process_and_validate_instruction(
         &instruction,
         &[(config, updated_config_account)],
-        &[Check::success(), Check::compute_units(25_243)],
+        &[Check::success(), Check::compute_units(25_275)],
     );
 
     // Use this for next invoke.
@@ -715,3 +715,36 @@ fn test_safe_deserialize() {
         &[Check::err(ProgramError::InvalidInstructionData)],
     );
 }
+
+#[test]
+fn test_safe_deserialize_from_state() {
+    let mollusk = setup();
+
+    let config = Pubkey::new_unique();
+
+    let keys = vec![(config, false), (config, true)];
+    let my_config = MyConfig::new(42);
+
+    // Input doesn't matter for this test.
+    let instruction = config_instruction::store(&config, false, keys.clone(), &my_config);
+
+    // Store the keys in the config account, but give it the wrong length.
+    let config_account = {
+        let space = bincode::serialized_size(&ConfigKeys { keys: keys.clone() }).unwrap() as usize;
+        let lamports = mollusk.sysvars.rent.minimum_balance(space);
+
+        let mut data = vec![0; space];
+        bincode::serialize_into(&mut data, &ConfigKeys { keys }).unwrap();
+        data[0] = 255; // length of 255.
+
+        let mut account = AccountSharedData::new(lamports, space, &solana_config_program::id());
+        account.set_data_from_slice(&data);
+        account
+    };
+
+    mollusk.process_and_validate_instruction(
+        &instruction,
+        &[(config, config_account)],
+        &[Check::err(ProgramError::InvalidAccountData)],
+    );
+}