Skip to content

Commit

Permalink
Refactor: Make program_id always last in program chain (solana-labs#2…
Browse files Browse the repository at this point in the history 
…0598)

* Replaces program_id field in InvokeContextStackFrame by index.

* Swaps order of program account and programdata account.

* Removes program_id parameter from InvokeContext::push().
  • Loading branch information
@Lichtso @dankelleher
Lichtso authored and dankelleher committed Nov 24, 2021
1 parent 8dc0dac commit 5d2fc6c
Show file tree
Hide file tree
Showing 5 changed files with 110 additions and 137 deletions.
13 changes: 3 additions & 10 deletions program-runtime/src/instruction_processor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@ impl InstructionProcessor {
);
return Err(InstructionError::AccountNotExecutable);
}
let mut program_indices = vec![program_account_index];
let mut program_indices = vec![];
if program_account.borrow().owner() == &bpf_loader_upgradeable::id() {
if let UpgradeableLoaderState::Program {
programdata_address,
Expand All @@ -512,6 +512,7 @@ impl InstructionProcessor {
return Err(InstructionError::MissingAccount);
}
}
program_indices.push(program_account_index);

Ok((message, caller_write_privileges, program_indices))
}
Expand Down Expand Up @@ -593,22 +594,14 @@ impl InstructionProcessor {
.get(0)
.ok_or(InstructionError::GenericError)?;

let program_id = instruction.program_id(&message.account_keys);

// Verify the calling program hasn't misbehaved
invoke_context.verify_and_update(instruction, account_indices, caller_write_privileges)?;

// clear the return data
invoke_context.set_return_data(Vec::new())?;

// Invoke callee
invoke_context.push(
program_id,
message,
instruction,
program_indices,
Some(account_indices),
)?;
invoke_context.push(message, instruction, program_indices, Some(account_indices))?;

let mut instruction_processor = InstructionProcessor::default();
for (program_id, process_instruction) in invoke_context.get_programs().iter() {
Expand Down
116 changes: 55 additions & 61 deletions programs/bpf_loader/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,72 +199,71 @@ fn process_instruction_common(
use_jit: bool,
) -> Result<(), InstructionError> {
let logger = invoke_context.get_logger();
let keyed_accounts = invoke_context.get_keyed_accounts()?;
let program_id = invoke_context.get_caller()?;

let keyed_accounts = invoke_context.get_keyed_accounts()?;
let first_account = keyed_account_at_index(keyed_accounts, first_instruction_account)?;
if first_account.executable()? {
let second_account = keyed_account_at_index(keyed_accounts, first_instruction_account + 1);
let (program, next_first_instruction_account) = if first_account.unsigned_key() == program_id {
(first_account, first_instruction_account)
} else if second_account
.as_ref()
.map(|keyed_account| keyed_account.unsigned_key() == program_id)
.unwrap_or(false)
{
(second_account?, first_instruction_account + 1)
} else {
if first_account.executable()? {
ic_logger_msg!(logger, "BPF loader is executable");
return Err(InstructionError::IncorrectProgramId);
}
(first_account, first_instruction_account)
};

if program.executable()? {
debug_assert_eq!(
first_instruction_account,
1 - (invoke_context.invoke_depth() > 1) as usize,
);

let program_id = invoke_context.get_caller()?;
if first_account.unsigned_key() != program_id {
ic_logger_msg!(logger, "Program id mismatch");
if !check_loader_id(&program.owner()?) {
ic_logger_msg!(logger, "Executable account not owned by the BPF loader");
return Err(InstructionError::IncorrectProgramId);
}

let (first_instruction_account, program_data_offset) =
if bpf_loader_upgradeable::check_id(&first_account.owner()?) {
if let UpgradeableLoaderState::Program {
programdata_address,
} = first_account.state()?
{
let programdata =
keyed_account_at_index(keyed_accounts, first_instruction_account + 1)?;
if programdata_address != *programdata.unsigned_key() {
ic_logger_msg!(
logger,
"Wrong ProgramData account for this Program account"
);
return Err(InstructionError::InvalidArgument);
}
if !matches!(
programdata.state()?,
UpgradeableLoaderState::ProgramData {
slot: _,
upgrade_authority_address: _,
}
) {
ic_logger_msg!(logger, "Program has been closed");
return Err(InstructionError::InvalidAccountData);
let program_data_offset = if bpf_loader_upgradeable::check_id(&program.owner()?) {
if let UpgradeableLoaderState::Program {
programdata_address,
} = program.state()?
{
if programdata_address != *first_account.unsigned_key() {
ic_logger_msg!(logger, "Wrong ProgramData account for this Program account");
return Err(InstructionError::InvalidArgument);
}
if !matches!(
first_account.state()?,
UpgradeableLoaderState::ProgramData {
slot: _,
upgrade_authority_address: _,
}
(
first_instruction_account + 1,
UpgradeableLoaderState::programdata_data_offset()?,
)
} else {
ic_logger_msg!(logger, "Invalid Program account");
) {
ic_logger_msg!(logger, "Program has been closed");
return Err(InstructionError::InvalidAccountData);
}
UpgradeableLoaderState::programdata_data_offset()?
} else {
(first_instruction_account, 0)
};

let keyed_accounts = invoke_context.get_keyed_accounts()?;
let program = keyed_account_at_index(keyed_accounts, first_instruction_account)?;
let loader_id = &program.owner()?;

if !check_loader_id(loader_id) {
ic_logger_msg!(logger, "Executable account not owned by the BPF loader");
return Err(InstructionError::IncorrectProgramId);
}
ic_logger_msg!(logger, "Invalid Program account");
return Err(InstructionError::InvalidAccountData);
}
} else {
0
};

let executor = match invoke_context.get_executor(program_id) {
Some(executor) => executor,
None => {
let executor = create_executor(
first_instruction_account,
next_first_instruction_account,
program_data_offset,
invoke_context,
use_jit,
Expand All @@ -275,37 +274,32 @@ fn process_instruction_common(
}
};
executor.execute(
first_instruction_account,
next_first_instruction_account,
instruction_data,
invoke_context,
use_jit,
)?
)
} else {
debug_assert_eq!(first_instruction_account, 1);

let program_id = invoke_context.get_caller()?;
if !check_loader_id(program_id) {
ic_logger_msg!(logger, "Invalid BPF loader id");
return Err(InstructionError::IncorrectProgramId);
}

if bpf_loader_upgradeable::check_id(program_id) {
process_loader_upgradeable_instruction(
first_instruction_account,
instruction_data,
invoke_context,
use_jit,
)?;
} else {
)
} else if bpf_loader::check_id(program_id) || bpf_loader_deprecated::check_id(program_id) {
process_loader_instruction(
first_instruction_account,
instruction_data,
invoke_context,
use_jit,
)?;
)
} else {
ic_logger_msg!(logger, "Invalid BPF loader id");
Err(InstructionError::IncorrectProgramId)
}
}
Ok(())
}

fn process_loader_upgradeable_instruction(
Expand Down Expand Up @@ -3374,8 +3368,8 @@ mod tests {

// Try to invoke closed account
let keyed_accounts: Vec<KeyedAccountTuple> = vec![
(false, false, &program_address, &program_account),
(false, false, &programdata_address, &programdata_account),
(false, false, &program_address, &program_account),
];
assert_eq!(
Err(InstructionError::InvalidAccountData),
Expand Down
7 changes: 3 additions & 4 deletions runtime/src/accounts.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ impl Accounts {

// Add loader to chain
let program_owner = *program.owner();

account_indices.insert(0, program_account_index);
if bpf_loader_upgradeable::check_id(&program_owner) {
// The upgradeable loader requires the derived ProgramData account
if let Ok(UpgradeableLoaderState::Program {
Expand Down Expand Up @@ -457,7 +457,6 @@ impl Accounts {
}
}

account_indices.insert(0, program_account_index);
program_id = program_owner;
}
Ok(account_indices)
Expand Down Expand Up @@ -1923,8 +1922,8 @@ mod tests {
let result = loaded_accounts[0].0.as_ref().unwrap();
assert_eq!(result.accounts[..2], accounts[..2]);
assert_eq!(result.accounts[result.program_indices[0][0]], accounts[5]);
assert_eq!(result.accounts[result.program_indices[0][1]], accounts[3]);
assert_eq!(result.accounts[result.program_indices[0][2]], accounts[4]);
assert_eq!(result.accounts[result.program_indices[0][1]], accounts[4]);
assert_eq!(result.accounts[result.program_indices[0][2]], accounts[3]);
}

#[test]
Expand Down
Loading

0 comments on commit 5d2fc6c

Please sign in to comment.