Skip to content

Commit 0088838

Browse files
buffalojoecbuffalojoec
authored
program: safe deserialize config keys (#16)
* program: safe deserialize config keys * adjust CUs in tests * check in new compute unit benchmark * use constants
1 parent a7fef8e commit 0088838

File tree

3 files changed

+152
-21
lines changed

3 files changed

+152
-21
lines changed

‎program/benches/compute_units.md‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,44 @@
1+
#### Compute Units: 2024-10-31 17:22:37.667457 UTC
2+
3+
| Name | CUs | Delta |
4+
|------|------|-------|
5+
| config_small_init_0_keys | 584 | +3 |
6+
| config_small_init_1_keys | 1215 | +11 |
7+
| config_small_init_5_keys | 2834 | +35 |
8+
| config_small_init_10_keys | 4904 | +65 |
9+
| config_small_init_25_keys | 11746 | +155 |
10+
| config_small_init_37_keys | 16749 | +227 |
11+
| config_small_store_0_keys | 584 | +3 |
12+
| config_small_store_1_keys | 1469 | +11 |
13+
| config_small_store_5_keys | 4004 | +35 |
14+
| config_small_store_10_keys | 7219 | +65 |
15+
| config_small_store_25_keys | 17496 | +155 |
16+
| config_small_store_37_keys | 25247 | +227 |
17+
| config_medium_init_0_keys | 575 | +3 |
18+
| config_medium_init_1_keys | 1162 | +11 |
19+
| config_medium_init_5_keys | 2834 | +35 |
20+
| config_medium_init_10_keys | 4904 | +65 |
21+
| config_medium_init_25_keys | 11746 | +155 |
22+
| config_medium_init_37_keys | 16749 | +227 |
23+
| config_medium_store_0_keys | 575 | +3 |
24+
| config_medium_store_1_keys | 1416 | +11 |
25+
| config_medium_store_5_keys | 4004 | +35 |
26+
| config_medium_store_10_keys | 7219 | +65 |
27+
| config_medium_store_25_keys | 17496 | +155 |
28+
| config_medium_store_37_keys | 25247 | +227 |
29+
| config_large_init_0_keys | 696 | +3 |
30+
| config_large_init_1_keys | 1283 | +11 |
31+
| config_large_init_5_keys | 2955 | +35 |
32+
| config_large_init_10_keys | 5026 | +65 |
33+
| config_large_init_25_keys | 11870 | +155 |
34+
| config_large_init_37_keys | 16874 | +227 |
35+
| config_large_store_0_keys | 696 | +3 |
36+
| config_large_store_1_keys | 1537 | +11 |
37+
| config_large_store_5_keys | 4125 | +35 |
38+
| config_large_store_10_keys | 7341 | +65 |
39+
| config_large_store_25_keys | 17620 | +155 |
40+
| config_large_store_37_keys | 25372 | +227 |
41+
142
#### Compute Units: 2024-10-23 12:46:27.264402 UTC
243

344
| Name | CUs | Delta |

‎program/src/processor.rs‎

Lines changed: 39 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -12,21 +12,49 @@ use {
1212
std::collections::BTreeSet,
1313
};
1414

15-
// [Core BPF]: Locally-implemented
16-
// `solana_sdk::program_utils::limited_deserialize`.
17-
fn limited_deserialize<T>(input: &[u8]) -> Result<T, ProgramError>
18-
where
19-
T: serde::de::DeserializeOwned,
20-
{
21-
solana_program::program_utils::limited_deserialize(
22-
input, 1232, // [Core BPF]: See `solana_sdk::packet::PACKET_DATA_SIZE`
23-
)
24-
.map_err(|_| ProgramError::InvalidInstructionData)
15+
// Maximum input buffer length that can be deserialized.
16+
// See `solana_sdk::packet::PACKET_DATA_SIZE`.
17+
const MAX_INPUT_LEN: usize = 1232;
18+
// Maximum vector length for a `ConfigKeys` struct's `keys` list.
19+
// See comments below for `safe_deserialize_config_keys`.
20+
//
21+
// Take the maximum input length and subtract (up to) 3 bytes for the
22+
// `ShortU16`, then divide that by the size of a `(Pubkey, bool)` entry.
23+
const MAX_VECTOR_LEN: usize = (MAX_INPUT_LEN - 3) / (32 + 1);
24+
25+
// [Core BPF]: The original Config builtin leverages the
26+
// `solana_sdk::program_utils::limited_deserialize` method to cap the length of
27+
// the input buffer at `MAX_INPUT_LEN` (1232). As a result, any input buffer
28+
// larger than `MAX_INPUT_LEN` will abort deserialization and return
29+
// `InstructionError::InvalidInstructionData`.
30+
//
31+
// Howevever, since `ConfigKeys` contains a vector of `(Pubkey, bool)`, the
32+
// `limited_deserialize` method will still read the vector's length and attempt
33+
// to allocate a vector of the designated size. For extremely large length
34+
// values, this can cause the initial allocation of a large vector to exhuast
35+
// the BPF program's heap before deserialization can proceed.
36+
//
37+
// To mitigate this memory issue, the BPF version of the Config program has
38+
// been designed to "peek" the length value, and ensure it cannot allocate a
39+
// vector that would otherwise violate the input buffer length restriction.
40+
// Since a `ShortU16` value for `MAX_VECTOR_LEN` fits in just one byte, we can
41+
// simply peek the first byte before attempting deserialization.
42+
fn safe_deserialize_config_keys(input: &[u8]) -> Result<ConfigKeys, ProgramError> {
43+
match input.first() {
44+
Some(first_byte) if *first_byte as usize <= MAX_VECTOR_LEN => {
45+
solana_program::program_utils::limited_deserialize::<ConfigKeys>(
46+
input,
47+
MAX_INPUT_LEN as u64,
48+
)
49+
.map_err(|_| ProgramError::InvalidInstructionData)
50+
}
51+
_ => Err(ProgramError::InvalidInstructionData),
52+
}
2553
}
2654

2755
/// Config program processor.
2856
pub fn process(program_id: &Pubkey, accounts: &[AccountInfo], input: &[u8]) -> ProgramResult {
29-
let key_list: ConfigKeys = limited_deserialize(input)?;
57+
let key_list = safe_deserialize_config_keys(input)?;
3058

3159
let mut accounts_iter = accounts.iter();
3260
let config_account = next_account_info(&mut accounts_iter)?;

‎program/tests/functional.rs‎

Lines changed: 72 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ fn test_process_create_ok() {
8383
&[(config, config_account)],
8484
&[
8585
Check::success(),
86-
Check::compute_units(581),
86+
Check::compute_units(584),
8787
Check::account(&config)
8888
.data(
8989
&bincode::serialize(&(ConfigKeys { keys: vec![] }, MyConfig::default()))
@@ -111,7 +111,7 @@ fn test_process_store_ok() {
111111
&[(config, config_account)],
112112
&[
113113
Check::success(),
114-
Check::compute_units(581),
114+
Check::compute_units(584),
115115
Check::account(&config)
116116
.data(&bincode::serialize(&(ConfigKeys { keys }, my_config)).unwrap())
117117
.build(),
@@ -186,7 +186,7 @@ fn test_process_store_with_additional_signers() {
186186
],
187187
&[
188188
Check::success(),
189-
Check::compute_units(3_228),
189+
Check::compute_units(3_253),
190190
Check::account(&config)
191191
.data(&bincode::serialize(&(ConfigKeys { keys }, my_config)).unwrap())
192192
.build(),
@@ -285,7 +285,7 @@ fn test_config_updates() {
285285
(signer0, AccountSharedData::default()),
286286
(signer1, AccountSharedData::default()),
287287
],
288-
&[Check::success(), Check::compute_units(3_228)],
288+
&[Check::success(), Check::compute_units(3_253)],
289289
);
290290

291291
// Use this for next invoke.
@@ -303,7 +303,7 @@ fn test_config_updates() {
303303
],
304304
&[
305305
Check::success(),
306-
Check::compute_units(3_229),
306+
Check::compute_units(3_254),
307307
Check::account(&config)
308308
.data(&bincode::serialize(&(ConfigKeys { keys }, new_config)).unwrap())
309309
.build(),
@@ -399,7 +399,7 @@ fn test_config_update_contains_duplicates_fails() {
399399
(signer0, AccountSharedData::default()),
400400
(signer1, AccountSharedData::default()),
401401
],
402-
&[Check::success(), Check::compute_units(3_228)],
402+
&[Check::success(), Check::compute_units(3_253)],
403403
);
404404

405405
// Attempt update with duplicate signer inputs.
@@ -443,7 +443,7 @@ fn test_config_updates_requiring_config() {
443443
],
444444
&[
445445
Check::success(),
446-
Check::compute_units(3_324),
446+
Check::compute_units(3_352),
447447
Check::account(&config)
448448
.data(&bincode::serialize(&(ConfigKeys { keys: keys.clone() }, my_config)).unwrap())
449449
.build(),
@@ -464,7 +464,7 @@ fn test_config_updates_requiring_config() {
464464
],
465465
&[
466466
Check::success(),
467-
Check::compute_units(3_324),
467+
Check::compute_units(3_352),
468468
Check::account(&config)
469469
.data(&bincode::serialize(&(ConfigKeys { keys }, new_config)).unwrap())
470470
.build(),
@@ -558,7 +558,7 @@ fn test_maximum_keys_input() {
558558
let result = mollusk.process_and_validate_instruction(
559559
&instruction,
560560
&[(config, config_account)],
561-
&[Check::success(), Check::compute_units(25_020)],
561+
&[Check::success(), Check::compute_units(25_247)],
562562
);
563563

564564
// Use this for next invoke.
@@ -571,7 +571,7 @@ fn test_maximum_keys_input() {
571571
let result = mollusk.process_and_validate_instruction(
572572
&instruction,
573573
&[(config, updated_config_account)],
574-
&[Check::success(), Check::compute_units(25_020)],
574+
&[Check::success(), Check::compute_units(25_247)],
575575
);
576576

577577
// Use this for next invoke.
@@ -587,3 +587,65 @@ fn test_maximum_keys_input() {
587587
&[Check::err(ProgramError::InvalidInstructionData)],
588588
);
589589
}
590+
591+
#[test]
592+
fn test_safe_deserialize() {
593+
let mollusk = setup();
594+
595+
// Accounts don't matter for this test.
596+
597+
// First try to spoof the program with just `ShortU16` length values.
598+
let build_instruction =
599+
|data: &[u8]| Instruction::new_with_bytes(solana_config_program::id(), data, vec![]);
600+
601+
mollusk.process_and_validate_instruction(
602+
// Empty buffer. Not a valid `ShortU16`.
603+
&build_instruction(&[]),
604+
&[],
605+
&[Check::err(ProgramError::InvalidInstructionData)],
606+
);
607+
608+
mollusk.process_and_validate_instruction(
609+
// `ShortU16` value of 38. One byte too large.
610+
&build_instruction(&[0x26]),
611+
&[],
612+
&[Check::err(ProgramError::InvalidInstructionData)],
613+
);
614+
615+
mollusk.process_and_validate_instruction(
616+
// `ShortU16` value of 37. OK for vector size, but no keys following.
617+
&build_instruction(&[0x25]),
618+
&[],
619+
&[Check::err(ProgramError::InvalidInstructionData)],
620+
);
621+
622+
// Now try with some actual `ConfigKeys` inputs.
623+
let mut keys = Vec::new();
624+
let serialized_config_keys = |keys: &[(Pubkey, bool)]| {
625+
let config_keys = ConfigKeys {
626+
keys: keys.to_vec(),
627+
};
628+
bincode::serialize(&config_keys).unwrap()
629+
};
630+
631+
// First build out to an acceptable size of 37.
632+
(0..37).for_each(|i| keys.push((Pubkey::new_unique(), i % 2 == 0)));
633+
634+
mollusk.process_and_validate_instruction(
635+
// `ShortU16` value of 37. OK.
636+
&build_instruction(&serialized_config_keys(&keys)),
637+
&[],
638+
// Falls through to account keys failure.
639+
&[Check::err(ProgramError::NotEnoughAccountKeys)],
640+
);
641+
642+
// Add one more key, pushing the size to 38.
643+
keys.push((Pubkey::new_unique(), true));
644+
645+
mollusk.process_and_validate_instruction(
646+
// `ShortU16` value of 38. Err.
647+
&build_instruction(&serialized_config_keys(&keys)),
648+
&[],
649+
&[Check::err(ProgramError::InvalidInstructionData)],
650+
);
651+
}

0 commit comments

Comments
 (0)