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 3 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
Copy link
Contributor

@joncinque joncinque Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the extra check that expensive? Why are all the CU numbers so much higher? Or is it a new toolchain?

Copy link
Contributor Author

@buffalojoec buffalojoec Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why CUs changed so much, or why the change isn't constant, but I've built this branch with CLI v2.0.2 as declared in the workspace manifest. I cannot guarantee I've always built with v2.0.2 though...

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
44 changes: 33 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,43 @@ 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)
// [Core BPF]: The original Config builtin leverages the
// `solana_sdk::program_utils::limited_deserialize` method to cap the length of
// the input buffer at 1232 (`solana_sdk::packet::PACKET_DATA_SIZE`). As a
// result, any input buffer larger than 1232 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 of
// 1232.
//
// Taking the maximum input length of 1232 and subtracting (up to) 3 bytes for
// the `ShortU16`, then dividing that by the size of a `(Pubkey, bool)` entry
// (33), we get a maximum vector size of 37. A `ShortU16` value for 37 fits in
// just one byte (`[0x25]`), so this function can simply check the first
// provided byte.
fn safe_deserialize_config_keys(input: &[u8]) -> Result<ConfigKeys, ProgramError> {
match input.first() {
Some(first_byte) if *first_byte <= 0x25 => {
solana_program::program_utils::limited_deserialize::<ConfigKeys>(
input, 1232, // [Core BPF]: See `solana_sdk::packet::PACKET_DATA_SIZE`
)
.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)],
);
}