Skip to content

Commit

Permalink
Ensure mapping of callee is updated with direct mapping
Browse files Browse the repository at this point in the history
Consider this scenario:

 - Program increases length of an account
 - Program start CPI and adds this account as a read-only account
 - In fn update_callee_account() we resize account, which may change
   the pointer
 - Once CPI finishes, the program continues and may read/write from
   the account. The mapping must be up-to-date else we use stale
   pointers.

Note that we always call callee_account.set_data_length(), which
may change the pointer. In testing I found that resizing a vector
from 10240 down to 127 sometimes changes its pointer. So, always
update the pointer.

NEEDS TESTS
  • Loading branch information
seanyoung committed Apr 30, 2024
1 parent 7fcd8c0 commit 9223edf
Showing 1 changed file with 14 additions and 5 deletions.
19 changes: 14 additions & 5 deletions programs/bpf_loader/src/syscalls/cpi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -927,7 +927,7 @@ where
// account (caller_account). We need to update the corresponding
// BorrowedAccount (callee_account) so the callee can see the
// changes.
update_callee_account(
let update_caller = update_callee_account(
invoke_context,
memory_mapping,
is_loader_deprecated,
Expand All @@ -936,7 +936,7 @@ where
direct_mapping,
)?;

let caller_account = if instruction_account.is_writable {
let caller_account = if instruction_account.is_writable || update_caller {
Some(caller_account)
} else {
None
Expand Down Expand Up @@ -1170,14 +1170,19 @@ fn cpi_common<S: SyscallInvokeSigned>(
//
// This method updates callee_account so the CPI callee can see the caller's
// changes.
//
// When true is returned, the caller account must be updated after CPI. This
// is only set for direct mapping when the pointer may have changed.
fn update_callee_account(
invoke_context: &InvokeContext,
memory_mapping: &MemoryMapping,
is_loader_deprecated: bool,
caller_account: &CallerAccount,
mut callee_account: BorrowedAccount<'_>,
direct_mapping: bool,
) -> Result<(), Error> {
) -> Result<bool, Error> {
let mut must_update_caller = false;

if callee_account.get_lamports() != *caller_account.lamports {
callee_account.set_lamports(*caller_account.lamports)?;
}
Expand All @@ -1195,7 +1200,11 @@ fn update_callee_account(
if is_loader_deprecated && realloc_bytes_used > 0 {
return Err(InstructionError::InvalidRealloc.into());
}
callee_account.set_data_length(post_len)?;
if prev_len != post_len {
callee_account.set_data_length(post_len)?;
// pointer to data may have changed, so caller must be updated
must_update_caller = true;
}
if realloc_bytes_used > 0 {
let serialized_data = translate_slice::<u8>(
memory_mapping,
Expand Down Expand Up @@ -1236,7 +1245,7 @@ fn update_callee_account(
callee_account.set_owner(caller_account.owner.as_ref())?;
}

Ok(())
Ok(must_update_caller)
}

fn update_caller_account_perms(
Expand Down

0 comments on commit 9223edf

Please sign in to comment.