Runtime: Core BPF Migration: Add checks for executable program account

[buffalojoec]

Problem

When the Core BPF migration module was being added to the runtime, the executable flag for accounts was being deprecated, so checks for this flag were not added to the mechanisms for migrating and upgrading Core BPF programs. See #309.

These changes were reverted, and the Core BPF migration code must be aware of the executable flag on accounts.

Additionally, even though the program can still be invoked via instruction, the program account must be set as executable when performing a migration, to ensure all executable flag checks are successful on the new program.

Summary of Changes

Two small changes:

• Add a check for executable when loading an existing Core BPF program in preparation for an upgrade.
• Manually set the program account to executable: true when performing a migration from builtin to BPF.

[mergify]

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

[CriesofCarrots]

Can the CPI test be a separate PR, please?
The changes to active runtime code look good.

[buffalojoec]

Can the CPI test be a separate PR, please? The changes to active runtime code look good.

Sure thing. Assuming you don't want the CPI test as part of the backport, or do you? And for backporting in general, is it preferred to make my case for backport here, or in the backport PR itself?

[CriesofCarrots]

Assuming you don't want the CPI test as part of the backport, or do you?

I'm leaning toward yes backport, since the feature being tested is in that branch. Zero-risk backport, since it doesn't touch live code.

is it preferred to make my case for backport here, or in the backport PR itself?

It's slightly easier for the backport reviewers if the argument is on the backport PR, but not a huge deal since we're most likely looking at the original PR as well anyway.

[buffalojoec]

I'm leaning toward yes backport, since the feature being tested is in that branch. Zero-risk backport, since it doesn't touch live code.

Sounds good. So I'll split the CPI test out, backport this one, and then later backport that one (once approved).

[buffalojoec]

CPI test was removed. I'll open that PR once this one goes in. Thanks!

[2501babe]

looks great! just to confirm, new_target_program_account() sets up an executable program account and set_up_test_core_bpf_program() sets up an executable programdata account, right? just making sure the bit is set on both types

[buffalojoec]

looks great! just to confirm, new_target_program_account() sets up an executable program account

Yep!

set_up_test_core_bpf_program() sets up an executable programdata account, right? just making sure the bit is set on both types

It's only setting the program account to executable: true. The program data account by convention isn't marked executable.

[2501babe]

thats strange. i asked because i thought i saw both accounts were marked executable when i was testing something yesterday. but now i used solana program deploy to put a random program on a solana-test-validator to check (1.19.0 installed from cli tools) and... neither are marked executable? not sure if its just something wrong with my setup

[buffalojoec]

thats strange. i asked because i thought i saw both accounts were marked executable when i was testing something yesterday. but now i used solana program deploy to put a random program on a solana-test-validator to check (1.19.0 installed from cli tools) and... neither are marked executable? not sure if its just something wrong with my setup

Hm, that is strange. The CPI test I added fails if the program account isn't marked executable, but works when it is. In both cases the programdata account is not executable.

There's a few places in the codebase where we explicitly mark the programdata account as executable: false, and SIMD 0162 even describes the logic behind this for loader v3, being a workaround for the original intended use for executable.

agave/program-test/src/programs.rs

Line 65 in c5e8d16

pub(crate) fn bpf_loader_upgradeable_program_accounts(

agave/programs/bpf_loader/src/lib.rs

Lines 668 to 699 in c5e8d16

	// Update the ProgramData account and record the program bits
	{
	let mut programdata =
	instruction_context.try_borrow_instruction_account(transaction_context, 1)?;
	programdata.set_state(&UpgradeableLoaderState::ProgramData {
	slot: clock.slot,
	upgrade_authority_address: authority_key,
	})?;
	let dst_slice = programdata
	.get_data_mut()?
	.get_mut(
	programdata_data_offset
	..programdata_data_offset.saturating_add(buffer_data_len),
	)
	.ok_or(InstructionError::AccountDataTooSmall)?;
	let mut buffer =
	instruction_context.try_borrow_instruction_account(transaction_context, 3)?;
	let src_slice = buffer
	.get_data()
	.get(buffer_data_offset..)
	.ok_or(InstructionError::AccountDataTooSmall)?;
	dst_slice.copy_from_slice(src_slice);
	buffer.set_data_length(UpgradeableLoaderState::size_of_buffer(0))?;
	}
	// Update the Program account
	let mut program =
	instruction_context.try_borrow_instruction_account(transaction_context, 2)?;
	program.set_state(&UpgradeableLoaderState::Program {
	programdata_address: programdata_key,
	})?;
	program.set_executable(true)?;

[2501babe]

i figured it out. both accounts (program and programdata) get marked executable on my local solana-test-validator when i use the --bpf-program flag, and neither get marked executable when i use solana program deploy. i was running on a 1.19 version of the cli tools, which the labs website serves as its edge release, which based on history looks like a point in time when the executable flag had been removed, but not yet un-removed. since the un-removal was backported to 2.0 i think everything is fine