Skip to content

program: safe deserialize config keys #16

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 1, 2024
+152 −21
Merged

program: safe deserialize config keys #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d015f7b
program: safe deserialize config keys
buffalojoec Oct 31, 2024
7a30947
adjust CUs in tests
buffalojoec Oct 31, 2024
5b000cf
check in new compute unit benchmark
buffalojoec Oct 31, 2024
3ed2ba6
use constants
buffalojoec Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions program/benches/compute_units.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 |
Expand Down
50 changes: 39 additions & 11 deletions program/src/processor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,21 +12,49 @@ use {
std::collections::BTreeSet,
};

// [Core BPF]: Locally-implemented
// `solana_sdk::program_utils::limited_deserialize`.
fn limited_deserialize<T>(input: &[u8]) -> Result<T, ProgramError>
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<ConfigKeys, ProgramError> {
match input.first() {
Some(first_byte) if *first_byte as usize <= MAX_VECTOR_LEN => {
solana_program::program_utils::limited_deserialize::<ConfigKeys>(
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)?;
Expand Down
82 changes: 72 additions & 10 deletions program/tests/functional.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()))
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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.
Expand All @@ -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(),
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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(),
Expand All @@ -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(),
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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)],
);
}