diff --git a/program/benches/compute_units.md b/program/benches/compute_units.md index 5ffd045..b95870b 100644 --- a/program/benches/compute_units.md +++ b/program/benches/compute_units.md @@ -1,3 +1,44 @@ +#### Compute Units: 2024-10-31 17:22:37.667457 UTC + +| Name | CUs | Delta | +|------|------|-------| +| config_small_init_0_keys | 584 | +3 | +| config_small_init_1_keys | 1215 | +11 | +| config_small_init_5_keys | 2834 | +35 | +| config_small_init_10_keys | 4904 | +65 | +| config_small_init_25_keys | 11746 | +155 | +| config_small_init_37_keys | 16749 | +227 | +| config_small_store_0_keys | 584 | +3 | +| config_small_store_1_keys | 1469 | +11 | +| config_small_store_5_keys | 4004 | +35 | +| config_small_store_10_keys | 7219 | +65 | +| config_small_store_25_keys | 17496 | +155 | +| config_small_store_37_keys | 25247 | +227 | +| config_medium_init_0_keys | 575 | +3 | +| config_medium_init_1_keys | 1162 | +11 | +| config_medium_init_5_keys | 2834 | +35 | +| config_medium_init_10_keys | 4904 | +65 | +| config_medium_init_25_keys | 11746 | +155 | +| config_medium_init_37_keys | 16749 | +227 | +| config_medium_store_0_keys | 575 | +3 | +| config_medium_store_1_keys | 1416 | +11 | +| config_medium_store_5_keys | 4004 | +35 | +| config_medium_store_10_keys | 7219 | +65 | +| config_medium_store_25_keys | 17496 | +155 | +| config_medium_store_37_keys | 25247 | +227 | +| config_large_init_0_keys | 696 | +3 | +| config_large_init_1_keys | 1283 | +11 | +| config_large_init_5_keys | 2955 | +35 | +| config_large_init_10_keys | 5026 | +65 | +| config_large_init_25_keys | 11870 | +155 | +| config_large_init_37_keys | 16874 | +227 | +| config_large_store_0_keys | 696 | +3 | +| config_large_store_1_keys | 1537 | +11 | +| config_large_store_5_keys | 4125 | +35 | +| config_large_store_10_keys | 7341 | +65 | +| config_large_store_25_keys | 17620 | +155 | +| config_large_store_37_keys | 25372 | +227 | + #### Compute Units: 2024-10-23 12:46:27.264402 UTC | Name | CUs | Delta | diff --git a/program/src/processor.rs b/program/src/processor.rs index 72c79b3..c25c31a 100644 --- a/program/src/processor.rs +++ b/program/src/processor.rs @@ -12,21 +12,49 @@ use { std::collections::BTreeSet, }; -// [Core BPF]: Locally-implemented -// `solana_sdk::program_utils::limited_deserialize`. -fn limited_deserialize(input: &[u8]) -> Result -where - T: serde::de::DeserializeOwned, -{ - solana_program::program_utils::limited_deserialize( - input, 1232, // [Core BPF]: See `solana_sdk::packet::PACKET_DATA_SIZE` - ) - .map_err(|_| ProgramError::InvalidInstructionData) +// Maximum input buffer length that can be deserialized. +// See `solana_sdk::packet::PACKET_DATA_SIZE`. +const MAX_INPUT_LEN: usize = 1232; +// Maximum vector length for a `ConfigKeys` struct's `keys` list. +// See comments below for `safe_deserialize_config_keys`. +// +// Take the maximum input length and subtract (up to) 3 bytes for the +// `ShortU16`, then divide that by the size of a `(Pubkey, bool)` entry. +const MAX_VECTOR_LEN: usize = (MAX_INPUT_LEN - 3) / (32 + 1); + +// [Core BPF]: The original Config builtin leverages the +// `solana_sdk::program_utils::limited_deserialize` method to cap the length of +// the input buffer at `MAX_INPUT_LEN` (1232). As a result, any input buffer +// larger than `MAX_INPUT_LEN` will abort deserialization and return +// `InstructionError::InvalidInstructionData`. +// +// Howevever, since `ConfigKeys` contains a vector of `(Pubkey, bool)`, the +// `limited_deserialize` method will still read the vector's length and attempt +// to allocate a vector of the designated size. For extremely large length +// values, this can cause the initial allocation of a large vector to exhuast +// the BPF program's heap before deserialization can proceed. +// +// To mitigate this memory issue, the BPF version of the Config program has +// been designed to "peek" the length value, and ensure it cannot allocate a +// vector that would otherwise violate the input buffer length restriction. +// Since a `ShortU16` value for `MAX_VECTOR_LEN` fits in just one byte, we can +// simply peek the first byte before attempting deserialization. +fn safe_deserialize_config_keys(input: &[u8]) -> Result { + match input.first() { + Some(first_byte) if *first_byte as usize <= MAX_VECTOR_LEN => { + solana_program::program_utils::limited_deserialize::( + input, + MAX_INPUT_LEN as u64, + ) + .map_err(|_| ProgramError::InvalidInstructionData) + } + _ => Err(ProgramError::InvalidInstructionData), + } } /// Config program processor. pub fn process(program_id: &Pubkey, accounts: &[AccountInfo], input: &[u8]) -> ProgramResult { - let key_list: ConfigKeys = limited_deserialize(input)?; + let key_list = safe_deserialize_config_keys(input)?; let mut accounts_iter = accounts.iter(); let config_account = next_account_info(&mut accounts_iter)?; diff --git a/program/tests/functional.rs b/program/tests/functional.rs index 2cd5d85..89c89a4 100644 --- a/program/tests/functional.rs +++ b/program/tests/functional.rs @@ -83,7 +83,7 @@ fn test_process_create_ok() { &[(config, config_account)], &[ Check::success(), - Check::compute_units(581), + Check::compute_units(584), 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(581), + Check::compute_units(584), 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_228), + Check::compute_units(3_253), Check::account(&config) .data(&bincode::serialize(&(ConfigKeys { keys }, my_config)).unwrap()) .build(), @@ -285,7 +285,7 @@ fn test_config_updates() { (signer0, AccountSharedData::default()), (signer1, AccountSharedData::default()), ], - &[Check::success(), Check::compute_units(3_228)], + &[Check::success(), Check::compute_units(3_253)], ); // Use this for next invoke. @@ -303,7 +303,7 @@ fn test_config_updates() { ], &[ Check::success(), - Check::compute_units(3_229), + Check::compute_units(3_254), Check::account(&config) .data(&bincode::serialize(&(ConfigKeys { keys }, new_config)).unwrap()) .build(), @@ -399,7 +399,7 @@ fn test_config_update_contains_duplicates_fails() { (signer0, AccountSharedData::default()), (signer1, AccountSharedData::default()), ], - &[Check::success(), Check::compute_units(3_228)], + &[Check::success(), Check::compute_units(3_253)], ); // Attempt update with duplicate signer inputs. @@ -443,7 +443,7 @@ fn test_config_updates_requiring_config() { ], &[ Check::success(), - Check::compute_units(3_324), + Check::compute_units(3_352), Check::account(&config) .data(&bincode::serialize(&(ConfigKeys { keys: keys.clone() }, my_config)).unwrap()) .build(), @@ -464,7 +464,7 @@ fn test_config_updates_requiring_config() { ], &[ Check::success(), - Check::compute_units(3_324), + Check::compute_units(3_352), Check::account(&config) .data(&bincode::serialize(&(ConfigKeys { keys }, new_config)).unwrap()) .build(), @@ -558,7 +558,7 @@ fn test_maximum_keys_input() { let result = mollusk.process_and_validate_instruction( &instruction, &[(config, config_account)], - &[Check::success(), Check::compute_units(25_020)], + &[Check::success(), Check::compute_units(25_247)], ); // Use this for next invoke. @@ -571,7 +571,7 @@ fn test_maximum_keys_input() { let result = mollusk.process_and_validate_instruction( &instruction, &[(config, updated_config_account)], - &[Check::success(), Check::compute_units(25_020)], + &[Check::success(), Check::compute_units(25_247)], ); // Use this for next invoke. @@ -587,3 +587,65 @@ fn test_maximum_keys_input() { &[Check::err(ProgramError::InvalidInstructionData)], ); } + +#[test] +fn test_safe_deserialize() { + let mollusk = setup(); + + // Accounts don't matter for this test. + + // First try to spoof the program with just `ShortU16` length values. + let build_instruction = + |data: &[u8]| Instruction::new_with_bytes(solana_config_program::id(), data, vec![]); + + mollusk.process_and_validate_instruction( + // Empty buffer. Not a valid `ShortU16`. + &build_instruction(&[]), + &[], + &[Check::err(ProgramError::InvalidInstructionData)], + ); + + mollusk.process_and_validate_instruction( + // `ShortU16` value of 38. One byte too large. + &build_instruction(&[0x26]), + &[], + &[Check::err(ProgramError::InvalidInstructionData)], + ); + + mollusk.process_and_validate_instruction( + // `ShortU16` value of 37. OK for vector size, but no keys following. + &build_instruction(&[0x25]), + &[], + &[Check::err(ProgramError::InvalidInstructionData)], + ); + + // Now try with some actual `ConfigKeys` inputs. + let mut keys = Vec::new(); + let serialized_config_keys = |keys: &[(Pubkey, bool)]| { + let config_keys = ConfigKeys { + keys: keys.to_vec(), + }; + bincode::serialize(&config_keys).unwrap() + }; + + // First build out to an acceptable size of 37. + (0..37).for_each(|i| keys.push((Pubkey::new_unique(), i % 2 == 0))); + + mollusk.process_and_validate_instruction( + // `ShortU16` value of 37. OK. + &build_instruction(&serialized_config_keys(&keys)), + &[], + // Falls through to account keys failure. + &[Check::err(ProgramError::NotEnoughAccountKeys)], + ); + + // Add one more key, pushing the size to 38. + keys.push((Pubkey::new_unique(), true)); + + mollusk.process_and_validate_instruction( + // `ShortU16` value of 38. Err. + &build_instruction(&serialized_config_keys(&keys)), + &[], + &[Check::err(ProgramError::InvalidInstructionData)], + ); +}