Skip to content

Commit

Permalink
Ensure mapping of callee is updated with direct mapping (#1093)
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.

(cherry picked from commit cd7f34e)

# Conflicts:
#	programs/sbf/rust/invoke/src/lib.rs
  • Loading branch information
seanyoung authored and mergify[bot] committed Jun 18, 2024
1 parent 65604c6 commit ce5f3f1
Show file tree
Hide file tree
Showing 4 changed files with 1,646 additions and 6 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
1 change: 1 addition & 0 deletions programs/sbf/rust/invoke/src/instructions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ pub const TEST_CPI_INVALID_LAMPORTS_POINTER: u8 = 36;
pub const TEST_CPI_INVALID_DATA_POINTER: u8 = 37;
pub const TEST_CPI_CHANGE_ACCOUNT_DATA_MEMORY_ALLOCATION: u8 = 38;
pub const TEST_WRITE_ACCOUNT: u8 = 39;
pub const TEST_CALLEE_ACCOUNT_UPDATES: u8 = 40;

pub const MINT_INDEX: usize = 0;
pub const ARGUMENT_INDEX: usize = 1;
Expand Down
Loading

0 comments on commit ce5f3f1

Please sign in to comment.