Skip to content

Commit 5785317

Browse files
LichtsoLichtso
authored
Fix - Account shrinks in CPI but grows in deserialize_parameters_aligned() (#9142)
* Adds INVOKE_REALLOC_SHRINK_THEN_CPI_THEN_REALLOC_EXTEND to test_program_sbf_realloc_invoke(). * Zero the slice between post_len and the current account len when stricter_abi_and_runtime_constraints && !account_data_direct_mapping in deserialize_parameters_aligned(). * Revert "Zero the slice between post_len and the current account len when stricter_abi_and_runtime_constraints && !account_data_direct_mapping in deserialize_parameters_aligned()." This reverts commit 3045db332cd1de077f058c66e2a137ac7afc150d. * Zero the slice between prev_len and post_len in update_callee_account() when stricter_abi_and_runtime_constraints && !account_data_direct_mapping. * Rekeys stricter_abi_and_runtime_constraints and account_data_direct_mapping.
1 parent 0d5dfb0 commit 5785317

File tree

5 files changed

+134
-4
lines changed

5 files changed

+134
-4
lines changed

‎feature-set/src/lib.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -769,11 +769,11 @@ pub mod apply_cost_tracker_during_replay {
769769
}
770770

771771
pub mod stricter_abi_and_runtime_constraints {
772-
solana_pubkey::declare_id!("sD3uVpaavUXQRvDXrMFCQ2CqLqnbz5mK8ttWNXbtD3r");
772+
solana_pubkey::declare_id!("Eoh7e1sDqtyPtuiWAhBNSJinvtJWTTDgeUMRi3RF8zWS");
773773
}
774774

775775
pub mod account_data_direct_mapping {
776-
solana_pubkey::declare_id!("DFN8MyKpQqFW31qczcahgnnxcAHQc6P94wtTEX5EP1RA");
776+
solana_pubkey::declare_id!("6f2qai82RU7Dutj1WJfRzLJKYA36QWvTa89CR1imgj7N");
777777
}
778778

779779
pub mod add_set_tx_loaded_accounts_data_size_instruction {

‎program-runtime/src/cpi.rs‎

Lines changed: 57 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -864,6 +864,7 @@ pub fn cpi_common<S: SyscallInvokeSigned>(
864864
let callee_account = instruction_context
865865
.try_borrow_instruction_account(translated_account.index_in_caller)?;
866866
let update_caller = update_callee_account(
867+
memory_mapping,
867868
check_aligned,
868869
&translated_account.caller_account,
869870
callee_account,
@@ -1097,6 +1098,7 @@ where
10971098
// BorrowedAccount (callee_account) so the callee can see the
10981099
// changes.
10991100
update_callee_account(
1101+
memory_mapping,
11001102
check_aligned,
11011103
&caller_account,
11021104
callee_account,
@@ -1141,6 +1143,7 @@ fn consume_compute_meter(invoke_context: &InvokeContext, amount: u64) -> Result<
11411143
// When true is returned, the caller account must be updated after CPI. This
11421144
// is only set for stricter_abi_and_runtime_constraints when the pointer may have changed.
11431145
fn update_callee_account(
1146+
memory_mapping: &MemoryMapping,
11441147
check_aligned: bool,
11451148
caller_account: &CallerAccount,
11461149
mut callee_account: BorrowedInstructionAccount<'_, '_>,
@@ -1168,6 +1171,21 @@ fn update_callee_account(
11681171
if post_len > address_space_reserved_for_account {
11691172
return Err(InstructionError::InvalidRealloc.into());
11701173
}
1174+
if !account_data_direct_mapping && post_len < prev_len {
1175+
// If the account has been shrunk, we're going to zero the unused memory
1176+
// *that was previously used*.
1177+
let serialized_data = CallerAccount::get_serialized_data(
1178+
memory_mapping,
1179+
caller_account.vm_data_addr,
1180+
prev_len as u64,
1181+
stricter_abi_and_runtime_constraints,
1182+
account_data_direct_mapping,
1183+
)?;
1184+
serialized_data
1185+
.get_mut(post_len..)
1186+
.ok_or_else(|| Box::new(InstructionError::AccountDataTooSmall))?
1187+
.fill(0);
1188+
}
11711189
callee_account.set_data_length(post_len)?;
11721190
// pointer to data may have changed, so caller must be updated
11731191
must_update_caller = true;
@@ -2192,7 +2210,16 @@ mod tests {
21922210

21932211
let mut mock_caller_account =
21942212
MockCallerAccount::new(1234, *account.owner(), account.data(), false);
2195-
2213+
let config = Config {
2214+
aligned_memory_mapping: false,
2215+
..Config::default()
2216+
};
2217+
let memory_mapping = MemoryMapping::new(
2218+
mock_caller_account.regions.clone(),
2219+
&config,
2220+
SBPFVersion::V3,
2221+
)
2222+
.unwrap();
21962223
let caller_account = mock_caller_account.caller_account();
21972224

21982225
borrow_instruction_account!(callee_account, invoke_context, 0);
@@ -2201,6 +2228,7 @@ mod tests {
22012228
*caller_account.owner = Pubkey::new_unique();
22022229

22032230
update_callee_account(
2231+
&memory_mapping,
22042232
true, // check_aligned
22052233
&caller_account,
22062234
callee_account,
@@ -2236,13 +2264,23 @@ mod tests {
22362264

22372265
let mut mock_caller_account =
22382266
MockCallerAccount::new(1234, *account.owner(), account.data(), false);
2239-
2267+
let config = Config {
2268+
aligned_memory_mapping: false,
2269+
..Config::default()
2270+
};
2271+
let memory_mapping = MemoryMapping::new(
2272+
mock_caller_account.regions.clone(),
2273+
&config,
2274+
SBPFVersion::V3,
2275+
)
2276+
.unwrap();
22402277
let mut caller_account = mock_caller_account.caller_account();
22412278
borrow_instruction_account!(callee_account, invoke_context, 0);
22422279

22432280
// stricter_abi_and_runtime_constraints does not copy data in update_callee_account()
22442281
caller_account.serialized_data[0] = b'b';
22452282
update_callee_account(
2283+
&memory_mapping,
22462284
true, // check_aligned
22472285
&caller_account,
22482286
callee_account,
@@ -2259,6 +2297,7 @@ mod tests {
22592297
caller_account.serialized_data = &mut data;
22602298
assert_eq!(
22612299
update_callee_account(
2300+
&memory_mapping,
22622301
true, // check_aligned
22632302
&caller_account,
22642303
callee_account,
@@ -2276,6 +2315,7 @@ mod tests {
22762315
borrow_instruction_account!(callee_account, invoke_context, 0);
22772316
assert_eq!(
22782317
update_callee_account(
2318+
&memory_mapping,
22792319
true, // check_aligned
22802320
&caller_account,
22812321
callee_account,
@@ -2294,6 +2334,7 @@ mod tests {
22942334
caller_account.owner = &mut owner;
22952335
borrow_instruction_account!(callee_account, invoke_context, 0);
22962336
update_callee_account(
2337+
&memory_mapping,
22972338
true, // check_aligned
22982339
&caller_account,
22992340
callee_account,
@@ -2307,6 +2348,7 @@ mod tests {
23072348
// growing beyond address_space_reserved_for_account
23082349
*caller_account.ref_to_len_in_vm = (7 + MAX_PERMITTED_DATA_INCREASE) as u64;
23092350
let result = update_callee_account(
2351+
&memory_mapping,
23102352
true, // check_aligned
23112353
&caller_account,
23122354
callee_account,
@@ -2345,13 +2387,24 @@ mod tests {
23452387

23462388
let mut mock_caller_account =
23472389
MockCallerAccount::new(1234, *account.owner(), account.data(), false);
2390+
let config = Config {
2391+
aligned_memory_mapping: false,
2392+
..Config::default()
2393+
};
2394+
let memory_mapping = MemoryMapping::new(
2395+
mock_caller_account.regions.clone(),
2396+
&config,
2397+
SBPFVersion::V3,
2398+
)
2399+
.unwrap();
23482400
let mut caller_account = mock_caller_account.caller_account();
23492401
borrow_instruction_account!(callee_account, invoke_context, 0);
23502402

23512403
// stricter_abi_and_runtime_constraints does not copy data in update_callee_account()
23522404
caller_account.serialized_data[0] = b'b';
23532405
assert_matches!(
23542406
update_callee_account(
2407+
&memory_mapping,
23552408
true, // check_aligned
23562409
&caller_account,
23572410
callee_account,
@@ -2368,6 +2421,7 @@ mod tests {
23682421
borrow_instruction_account!(callee_account, invoke_context, 0);
23692422
assert_matches!(
23702423
update_callee_account(
2424+
&memory_mapping,
23712425
true, // check_aligned
23722426
&caller_account,
23732427
callee_account,
@@ -2384,6 +2438,7 @@ mod tests {
23842438
borrow_instruction_account!(callee_account, invoke_context, 0);
23852439
assert_matches!(
23862440
update_callee_account(
2441+
&memory_mapping,
23872442
true, // check_aligned
23882443
&caller_account,
23892444
callee_account,

‎programs/sbf/rust/realloc_invoke/src/lib.rs‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,46 @@ fn process_instruction(
9494
account.resize(new_len.saturating_add(extend_len))?;
9595
assert_eq!(new_len.saturating_add(extend_len), account.data_len());
9696
}
97+
INVOKE_REALLOC_SHRINK_THEN_CPI_THEN_REALLOC_EXTEND => {
98+
let (bytes, remaining_data) =
99+
instruction_data[2..].split_at(std::mem::size_of::<usize>());
100+
let cpi_len = usize::from_le_bytes(bytes.try_into().unwrap());
101+
msg!("realloc to {} byte(s)", cpi_len);
102+
account.resize(cpi_len)?;
103+
msg!("invoke");
104+
let realloc_to_ix = {
105+
let mut instruction_data = vec![INVOKE_REALLOC_TO, 1];
106+
instruction_data.extend_from_slice(&cpi_len.to_le_bytes());
107+
108+
Instruction::new_with_bytes(
109+
*invoke_program_id,
110+
&instruction_data,
111+
vec![
112+
AccountMeta::new(*account.key, false),
113+
AccountMeta::new_readonly(*invoke_program_id, false),
114+
],
115+
)
116+
};
117+
invoke(&realloc_to_ix, accounts)?;
118+
assert_eq!(cpi_len, account.data_len());
119+
let (bytes, _) = remaining_data.split_at(std::mem::size_of::<usize>());
120+
let final_len = usize::from_le_bytes(bytes.try_into().unwrap());
121+
msg!("realloc to {} byte(s)", final_len.saturating_add(1));
122+
unsafe {
123+
*account
124+
.try_borrow_mut_data()?
125+
.as_mut_ptr()
126+
.add(final_len.saturating_add(1)) = 0xAA;
127+
}
128+
msg!("realloc to {} byte(s)", final_len);
129+
// We want to see the program runtime handling the zeroing of the extension.
130+
// Thus we manually overwrite the length instead of calling account.resize()
131+
// which would zero extend inside the program.
132+
unsafe {
133+
*(account.try_borrow_mut_data()?.as_mut_ptr().offset(-8) as *mut u64) =
134+
final_len as u64;
135+
}
136+
}
97137
INVOKE_REALLOC_MAX_TWICE => {
98138
msg!("invoke realloc max twice");
99139
invoke(

‎programs/sbf/rust/realloc_invoke_dep/src/lib.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,3 +17,4 @@ pub const INVOKE_REALLOC_MAX_TWICE: u8 = 13;
1717
pub const INVOKE_REALLOC_MAX_INVOKE_MAX: u8 = 14;
1818
pub const INVOKE_INVOKE_MAX_TWICE: u8 = 15;
1919
pub const INVOKE_REALLOC_TO_THEN_LOCAL_REALLOC_EXTEND: u8 = 16;
20+
pub const INVOKE_REALLOC_SHRINK_THEN_CPI_THEN_REALLOC_EXTEND: u8 = 17;

‎programs/sbf/tests/programs.rs‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3607,6 +3607,40 @@ fn test_program_sbf_realloc_invoke() {
36073607
}
36083608
}
36093609

3610+
// Realloc shrink, then CPI, then realloc extend
3611+
let mut invoke_account = AccountSharedData::new(100_000_000, 10, &realloc_invoke_program_id);
3612+
invoke_account.set_data(vec![0, 1, 2, 3, 4, 5, 6, 7, 8, 9]);
3613+
bank.store_account(&invoke_pubkey, &invoke_account);
3614+
let mut instruction_data = vec![];
3615+
instruction_data.extend_from_slice(&[INVOKE_REALLOC_SHRINK_THEN_CPI_THEN_REALLOC_EXTEND, 1]);
3616+
instruction_data.extend_from_slice(&5_u64.to_le_bytes());
3617+
instruction_data.extend_from_slice(&10_u64.to_le_bytes());
3618+
let result = bank_client.send_and_confirm_message(
3619+
signer,
3620+
Message::new(
3621+
&[
3622+
Instruction::new_with_bytes(
3623+
realloc_invoke_program_id,
3624+
&instruction_data,
3625+
vec![
3626+
AccountMeta::new(invoke_pubkey, false),
3627+
AccountMeta::new_readonly(realloc_invoke_program_id, false),
3628+
],
3629+
),
3630+
ComputeBudgetInstruction::set_loaded_accounts_data_size_limit(
3631+
LOADED_ACCOUNTS_DATA_SIZE_LIMIT_FOR_TEST,
3632+
),
3633+
],
3634+
Some(&mint_pubkey),
3635+
),
3636+
);
3637+
assert!(result.is_ok());
3638+
let data = bank_client
3639+
.get_account_data(&invoke_pubkey)
3640+
.unwrap()
3641+
.unwrap();
3642+
assert_eq!(data, &[0, 1, 2, 3, 4, 0, 0, 0, 0, 0]);
3643+
36103644
// Realloc invoke max twice
36113645
let invoke_account = AccountSharedData::new(42, 0, &realloc_program_id);
36123646
bank.store_account(&invoke_pubkey, &invoke_account);

0 commit comments

Comments
 (0)