Safer invoke context (#22898)

* Safer invoke context

* feedback and rebase with master
This commit is contained in:
Jack May 2022-02-03 02:34:51 -08:00 committed by GitHub
parent bd1850df25
commit cc94a93b56
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 150 additions and 65 deletions

View File

@ -241,10 +241,15 @@ impl<'a> InvokeContext<'a> {
let mut sysvar_cache = SysvarCache::default();
sysvar_cache.fill_missing_entries(|pubkey| {
(0..transaction_context.get_number_of_accounts()).find_map(|index| {
if transaction_context.get_key_of_account_at_index(index) == pubkey {
if transaction_context
.get_key_of_account_at_index(index)
.unwrap()
== pubkey
{
Some(
transaction_context
.get_account_at_index(index)
.unwrap()
.borrow()
.clone(),
)
@ -283,10 +288,13 @@ impl<'a> InvokeContext<'a> {
return Err(InstructionError::CallDepth);
}
let program_id = program_indices.last().map(|account_index| {
self.transaction_context
.get_key_of_account_at_index(*account_index)
});
let program_id = program_indices
.last()
.map(|account_index| {
self.transaction_context
.get_key_of_account_at_index(*account_index)
})
.transpose()?;
if program_id.is_none()
&& self
.feature_set
@ -328,12 +336,13 @@ impl<'a> InvokeContext<'a> {
{
let account = self
.transaction_context
.get_account_at_index(instruction_account.index_in_transaction)
.get_account_at_index(instruction_account.index_in_transaction)?
.borrow()
.clone();
self.pre_accounts.push(PreAccount::new(
self.transaction_context
.get_key_of_account_at_index(instruction_account.index_in_transaction),
self.transaction_context.get_key_of_account_at_index(
instruction_account.index_in_transaction,
)?,
account,
));
return Ok(());
@ -372,26 +381,26 @@ impl<'a> InvokeContext<'a> {
let keyed_accounts = program_indices
.iter()
.map(|account_index| {
(
Ok((
false,
false,
self.transaction_context
.get_key_of_account_at_index(*account_index),
.get_key_of_account_at_index(*account_index)?,
self.transaction_context
.get_account_at_index(*account_index),
)
.get_account_at_index(*account_index)?,
))
})
.chain(instruction_accounts.iter().map(|instruction_account| {
(
Ok((
instruction_account.is_signer,
instruction_account.is_writable,
self.transaction_context
.get_key_of_account_at_index(instruction_account.index_in_transaction),
.get_key_of_account_at_index(instruction_account.index_in_transaction)?,
self.transaction_context
.get_account_at_index(instruction_account.index_in_transaction),
)
.get_account_at_index(instruction_account.index_in_transaction)?,
))
}))
.collect::<Vec<_>>();
.collect::<Result<Vec<_>, InstructionError>>()?;
// Unsafe will be removed together with the keyed_accounts
self.invoke_stack.push(StackFrame::new(
@ -433,7 +442,7 @@ impl<'a> InvokeContext<'a> {
// Verify all executable accounts have zero outstanding refs
for account_index in program_indices.iter() {
self.transaction_context
.get_account_at_index(*account_index)
.get_account_at_index(*account_index)?
.try_borrow_mut()
.map_err(|_| InstructionError::AccountBorrowOutstanding)?;
}
@ -446,7 +455,7 @@ impl<'a> InvokeContext<'a> {
// Verify account has no outstanding references
let _ = self
.transaction_context
.get_account_at_index(instruction_account.index_in_transaction)
.get_account_at_index(instruction_account.index_in_transaction)?
.try_borrow_mut()
.map_err(|_| InstructionError::AccountBorrowOutstanding)?;
}
@ -454,7 +463,7 @@ impl<'a> InvokeContext<'a> {
pre_account_index = pre_account_index.saturating_add(1);
let account = self
.transaction_context
.get_account_at_index(instruction_account.index_in_transaction)
.get_account_at_index(instruction_account.index_in_transaction)?
.borrow();
pre_account
.verify(
@ -521,9 +530,9 @@ impl<'a> InvokeContext<'a> {
< transaction_context.get_number_of_accounts()
{
let key = transaction_context
.get_key_of_account_at_index(instruction_account.index_in_transaction);
.get_key_of_account_at_index(instruction_account.index_in_transaction)?;
let account = transaction_context
.get_account_at_index(instruction_account.index_in_transaction);
.get_account_at_index(instruction_account.index_in_transaction)?;
let is_writable = if before_instruction_context_push {
instruction_context
.try_borrow_account(
@ -607,7 +616,7 @@ impl<'a> InvokeContext<'a> {
for instruction_account in instruction_accounts.iter() {
let account_length = self
.transaction_context
.get_account_at_index(instruction_account.index_in_transaction)
.get_account_at_index(instruction_account.index_in_transaction)?
.borrow()
.data()
.len();
@ -630,7 +639,7 @@ impl<'a> InvokeContext<'a> {
&& prev_size
!= self
.transaction_context
.get_account_at_index(account_index)
.get_account_at_index(account_index)?
.borrow()
.data()
.len()
@ -798,8 +807,12 @@ impl<'a> InvokeContext<'a> {
*compute_units_consumed = 0;
let program_id = program_indices
.last()
.map(|index| *self.transaction_context.get_key_of_account_at_index(*index))
.unwrap_or_else(native_loader::id);
.map(|index| {
self.transaction_context
.get_key_of_account_at_index(*index)
.map(|pubkey| *pubkey)
})
.unwrap_or_else(|| Ok(native_loader::id()))?;
let stack_height = self
.transaction_context
@ -997,7 +1010,10 @@ impl<'a> InvokeContext<'a> {
}
// Get pubkey of account at index
pub fn get_key_of_account_at_index(&self, index_in_transaction: usize) -> &Pubkey {
pub fn get_key_of_account_at_index(
&self,
index_in_transaction: usize,
) -> Result<&Pubkey, InstructionError> {
self.transaction_context
.get_key_of_account_at_index(index_in_transaction)
}
@ -1364,6 +1380,7 @@ mod tests {
invoke_context
.transaction_context
.get_account_at_index(owned_index)
.unwrap()
.borrow_mut()
.data_as_mut_slice()[0] = (MAX_DEPTH + owned_index) as u8;
invoke_context
@ -1378,11 +1395,13 @@ mod tests {
let data = invoke_context
.transaction_context
.get_account_at_index(not_owned_index)
.unwrap()
.borrow_mut()
.data()[0];
invoke_context
.transaction_context
.get_account_at_index(not_owned_index)
.unwrap()
.borrow_mut()
.data_as_mut_slice()[0] = (MAX_DEPTH + not_owned_index) as u8;
assert_eq!(
@ -1393,6 +1412,7 @@ mod tests {
invoke_context
.transaction_context
.get_account_at_index(not_owned_index)
.unwrap()
.borrow_mut()
.data_as_mut_slice()[0] = data;
@ -1468,6 +1488,7 @@ mod tests {
invoke_context
.transaction_context
.get_account_at_index(1)
.unwrap()
.borrow_mut()
.data_as_mut_slice()[0] = 1;
assert_eq!(
@ -1477,6 +1498,7 @@ mod tests {
invoke_context
.transaction_context
.get_account_at_index(1)
.unwrap()
.borrow_mut()
.data_as_mut_slice()[0] = 0;
@ -1484,6 +1506,7 @@ mod tests {
invoke_context
.transaction_context
.get_account_at_index(2)
.unwrap()
.borrow_mut()
.data_as_mut_slice()[0] = 1;
assert_eq!(
@ -1493,6 +1516,7 @@ mod tests {
invoke_context
.transaction_context
.get_account_at_index(2)
.unwrap()
.borrow_mut()
.data_as_mut_slice()[0] = 0;

View File

@ -271,7 +271,8 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {
for instruction_account in instruction_accounts.iter() {
let account_key = invoke_context
.transaction_context
.get_key_of_account_at_index(instruction_account.index_in_transaction);
.get_key_of_account_at_index(instruction_account.index_in_transaction)
.unwrap();
let account_info_index = account_infos
.iter()
.position(|account_info| account_info.unsigned_key() == account_key)
@ -281,6 +282,7 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {
let mut account = invoke_context
.transaction_context
.get_account_at_index(instruction_account.index_in_transaction)
.unwrap()
.borrow_mut();
account.copy_into_owner_from_slice(account_info.owner.as_ref());
account.set_data_from_slice(&account_info.try_borrow_data().unwrap());
@ -309,6 +311,7 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {
let account = invoke_context
.transaction_context
.get_account_at_index(index_in_transaction)
.unwrap()
.borrow_mut();
let account_info = &account_infos[account_info_index];
**account_info.try_borrow_mut_lamports().unwrap() = account.lamports();

View File

@ -493,6 +493,7 @@ mod tests {
let account = invoke_context
.transaction_context
.get_account_at_index(index_in_transaction)
.unwrap()
.borrow();
assert_eq!(account.lamports(), account_info.lamports());
assert_eq!(account.data(), &account_info.data.borrow()[..]);
@ -518,6 +519,7 @@ mod tests {
let mut account = invoke_context
.transaction_context
.get_account_at_index(index_in_transaction)
.unwrap()
.borrow_mut();
account.set_lamports(0);
account.set_data(vec![0; 0]);
@ -536,6 +538,7 @@ mod tests {
let account = invoke_context
.transaction_context
.get_account_at_index(index_in_transaction)
.unwrap()
.borrow();
assert_eq!(&*account, original_account);
}
@ -567,6 +570,7 @@ mod tests {
let account = invoke_context
.transaction_context
.get_account_at_index(index_in_transaction)
.unwrap()
.borrow();
assert_eq!(account.lamports(), account_info.lamports());
assert_eq!(account.data(), &account_info.data.borrow()[..]);
@ -579,6 +583,7 @@ mod tests {
let mut account = invoke_context
.transaction_context
.get_account_at_index(index_in_transaction)
.unwrap()
.borrow_mut();
account.set_lamports(0);
account.set_data(vec![0; 0]);
@ -596,6 +601,7 @@ mod tests {
let account = invoke_context
.transaction_context
.get_account_at_index(index_in_transaction)
.unwrap()
.borrow();
assert_eq!(&*account, original_account);
}

View File

@ -2546,10 +2546,12 @@ where
for instruction_account in instruction_accounts.iter() {
let account = invoke_context
.transaction_context
.get_account_at_index(instruction_account.index_in_transaction);
.get_account_at_index(instruction_account.index_in_transaction)
.map_err(SyscallError::InstructionError)?;
let account_key = invoke_context
.transaction_context
.get_key_of_account_at_index(instruction_account.index_in_transaction);
.get_key_of_account_at_index(instruction_account.index_in_transaction)
.map_err(SyscallError::InstructionError)?;
if account.borrow().executable() {
// Use the known account
accounts.push((instruction_account.index_in_transaction, None));
@ -2725,6 +2727,7 @@ fn call<'a, 'b: 'a>(
let callee_account = invoke_context
.transaction_context
.get_account_at_index(*callee_account_index)
.map_err(SyscallError::InstructionError)?
.borrow();
*caller_account.lamports = callee_account.lamports();
*caller_account.owner = *callee_account.owner();
@ -3103,16 +3106,20 @@ impl<'a, 'b> SyscallObject<BpfError> for SyscallGetProcessedSiblingInstruction<'
*program_id =
instruction_context.get_program_id(invoke_context.transaction_context);
data.clone_from_slice(instruction_context.get_instruction_data());
let account_metas = instruction_context
.get_instruction_accounts_metas()
.iter()
.map(|meta| AccountMeta {
pubkey: *invoke_context
.get_key_of_account_at_index(meta.index_in_transaction),
is_signer: meta.is_signer,
is_writable: meta.is_writable,
})
.collect::<Vec<_>>();
let account_metas = question_mark!(
instruction_context
.get_instruction_accounts_metas()
.iter()
.map(|meta| Ok(AccountMeta {
pubkey: *invoke_context
.get_key_of_account_at_index(meta.index_in_transaction)
.map_err(SyscallError::InstructionError)?,
is_signer: meta.is_signer,
is_writable: meta.is_writable,
}))
.collect::<Result<Vec<_>, EbpfError<BpfError>>>(),
result
);
accounts.clone_from_slice(account_metas.as_slice());
}
*data_len = instruction_context.get_instruction_data().len();

View File

@ -60,7 +60,9 @@ pub(crate) fn check_rent_state(
debug!(
"Account {:?} not rent exempt, state {:?}",
transaction_context.get_key_of_account_at_index(index),
transaction_context.get_account_at_index(index).borrow(),
transaction_context
.get_account_at_index(index)
.map(|account| account.borrow()),
);
return Err(TransactionError::InvalidRentPayingAccount);
}

View File

@ -22,16 +22,25 @@ impl Bank {
(0..message.account_keys_len())
.map(|i| {
let rent_state = if message.is_writable(i) {
let account = transaction_context.get_account_at_index(i).borrow();
let state = if let Ok(account) = transaction_context.get_account_at_index(i) {
let account = account.borrow();
// Native programs appear to be RentPaying because they carry low lamport
// balances; however they will never be loaded as writable
debug_assert!(!native_loader::check_id(account.owner()));
// Native programs appear to be RentPaying because they carry low lamport
// balances; however they will never be loaded as writable
debug_assert!(!native_loader::check_id(account.owner()));
Some(RentState::from_account(
&account,
&self.rent_collector().rent,
))
Some(RentState::from_account(
&account,
&self.rent_collector().rent,
))
} else {
None
};
debug_assert!(
state.is_some(),
"message and transaction context out of sync, fatal"
);
state
} else {
None
};

View File

@ -103,6 +103,7 @@ impl MessageProcessor {
let mut mut_account_ref = invoke_context
.transaction_context
.get_account_at_index(account_index)
.map_err(|_| TransactionError::InvalidAccountIndex)?
.borrow_mut();
instructions::store_current_index(
mut_account_ref.data_as_mut_slice(),
@ -243,8 +244,14 @@ mod tests {
let program_indices = vec![vec![2]];
let executors = Rc::new(RefCell::new(Executors::default()));
let account_metas = vec![
AccountMeta::new(*transaction_context.get_key_of_account_at_index(0), true),
AccountMeta::new_readonly(*transaction_context.get_key_of_account_at_index(1), false),
AccountMeta::new(
*transaction_context.get_key_of_account_at_index(0).unwrap(),
true,
),
AccountMeta::new_readonly(
*transaction_context.get_key_of_account_at_index(1).unwrap(),
false,
),
];
let message = SanitizedMessage::Legacy(Message::new(
@ -253,7 +260,7 @@ mod tests {
&MockSystemInstruction::Correct,
account_metas.clone(),
)],
Some(transaction_context.get_key_of_account_at_index(0)),
Some(transaction_context.get_key_of_account_at_index(0).unwrap()),
));
let sysvar_cache = SysvarCache::default();
let result = MessageProcessor::process_message(
@ -276,6 +283,7 @@ mod tests {
assert_eq!(
transaction_context
.get_account_at_index(0)
.unwrap()
.borrow()
.lamports(),
100
@ -283,6 +291,7 @@ mod tests {
assert_eq!(
transaction_context
.get_account_at_index(1)
.unwrap()
.borrow()
.lamports(),
0
@ -294,7 +303,7 @@ mod tests {
&MockSystemInstruction::TransferLamports { lamports: 50 },
account_metas.clone(),
)],
Some(transaction_context.get_key_of_account_at_index(0)),
Some(transaction_context.get_key_of_account_at_index(0).unwrap()),
));
let result = MessageProcessor::process_message(
builtin_programs,
@ -326,7 +335,7 @@ mod tests {
&MockSystemInstruction::ChangeData { data: 50 },
account_metas,
)],
Some(transaction_context.get_key_of_account_at_index(0)),
Some(transaction_context.get_key_of_account_at_index(0).unwrap()),
));
let result = MessageProcessor::process_message(
builtin_programs,
@ -439,9 +448,18 @@ mod tests {
let program_indices = vec![vec![2]];
let executors = Rc::new(RefCell::new(Executors::default()));
let account_metas = vec![
AccountMeta::new(*transaction_context.get_key_of_account_at_index(0), true),
AccountMeta::new(*transaction_context.get_key_of_account_at_index(1), false),
AccountMeta::new(*transaction_context.get_key_of_account_at_index(0), false),
AccountMeta::new(
*transaction_context.get_key_of_account_at_index(0).unwrap(),
true,
),
AccountMeta::new(
*transaction_context.get_key_of_account_at_index(1).unwrap(),
false,
),
AccountMeta::new(
*transaction_context.get_key_of_account_at_index(0).unwrap(),
false,
),
];
// Try to borrow mut the same account
@ -451,7 +469,7 @@ mod tests {
&MockSystemInstruction::BorrowFail,
account_metas.clone(),
)],
Some(transaction_context.get_key_of_account_at_index(0)),
Some(transaction_context.get_key_of_account_at_index(0).unwrap()),
));
let sysvar_cache = SysvarCache::default();
let result = MessageProcessor::process_message(
@ -485,7 +503,7 @@ mod tests {
&MockSystemInstruction::MultiBorrowMut,
account_metas.clone(),
)],
Some(transaction_context.get_key_of_account_at_index(0)),
Some(transaction_context.get_key_of_account_at_index(0).unwrap()),
));
let result = MessageProcessor::process_message(
builtin_programs,
@ -515,7 +533,7 @@ mod tests {
},
account_metas,
)],
Some(transaction_context.get_key_of_account_at_index(0)),
Some(transaction_context.get_key_of_account_at_index(0).unwrap()),
));
let result = MessageProcessor::process_message(
builtin_programs,
@ -537,6 +555,7 @@ mod tests {
assert_eq!(
transaction_context
.get_account_at_index(0)
.unwrap()
.borrow()
.lamports(),
80
@ -544,12 +563,17 @@ mod tests {
assert_eq!(
transaction_context
.get_account_at_index(1)
.unwrap()
.borrow()
.lamports(),
20
);
assert_eq!(
transaction_context.get_account_at_index(0).borrow().data(),
transaction_context
.get_account_at_index(0)
.unwrap()
.borrow()
.data(),
&vec![42]
);
}

View File

@ -96,13 +96,23 @@ impl TransactionContext {
}
/// Searches for an account by its key
pub fn get_key_of_account_at_index(&self, index_in_transaction: usize) -> &Pubkey {
&self.account_keys[index_in_transaction]
pub fn get_key_of_account_at_index(
&self,
index_in_transaction: usize,
) -> Result<&Pubkey, InstructionError> {
self.account_keys
.get(index_in_transaction)
.ok_or(InstructionError::NotEnoughAccountKeys)
}
/// Searches for an account by its key
pub fn get_account_at_index(&self, index_in_transaction: usize) -> &RefCell<AccountSharedData> {
&self.accounts[index_in_transaction]
pub fn get_account_at_index(
&self,
index_in_transaction: usize,
) -> Result<&RefCell<AccountSharedData>, InstructionError> {
self.accounts
.get(index_in_transaction)
.ok_or(InstructionError::NotEnoughAccountKeys)
}
/// Searches for an account by its key