Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature - Remove accounts executable flag checks #2182

Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Adds the feature gate logic.
Lichtso committed Nov 4, 2024
commit e10d303f95118905294a8c1396064a5101f24d5a
10 changes: 8 additions & 2 deletions program-runtime/src/invoke_context.rs
Original file line number Diff line number Diff line change
@@ -8,7 +8,9 @@ use {
sysvar_cache::SysvarCache,
},
solana_compute_budget::compute_budget::ComputeBudget,
solana_feature_set::{move_precompile_verification_to_svm, FeatureSet},
solana_feature_set::{
move_precompile_verification_to_svm, remove_accounts_executable_flag_checks, FeatureSet,
},
solana_log_collector::{ic_msg, LogCollector},
solana_measure::measure::Measure,
solana_rbpf::{
@@ -435,7 +437,11 @@ impl<'a> InvokeContext<'a> {
})?;
let borrowed_program_account = instruction_context
.try_borrow_instruction_account(self.transaction_context, program_account_index)?;
if !borrowed_program_account.is_executable() {
if !self
.get_feature_set()
.is_active(&remove_accounts_executable_flag_checks::id())
&& !borrowed_program_account.is_executable()
{
ic_msg!(self, "Account {} is not executable", callee_program_id);
return Err(InstructionError::AccountNotExecutable);
}
75 changes: 62 additions & 13 deletions programs/bpf_loader/src/lib.rs
Original file line number Diff line number Diff line change
@@ -8,6 +8,7 @@ use {
solana_compute_budget::compute_budget::MAX_INSTRUCTION_STACK_DEPTH,
solana_feature_set::{
bpf_account_data_direct_mapping, enable_bpf_loader_set_authority_checked_ix,
remove_accounts_executable_flag_checks,
},
solana_log_collector::{ic_logger_msg, ic_msg, LogCollector},
solana_measure::measure::Measure,
@@ -423,14 +424,27 @@ pub fn process_instruction_inner(
Err(InstructionError::UnsupportedProgramId)
} else {
ic_logger_msg!(log_collector, "Invalid BPF loader id");
Err(InstructionError::IncorrectProgramId)
Err(
if invoke_context
.get_feature_set()
.is_active(&remove_accounts_executable_flag_checks::id())
{
InstructionError::UnsupportedProgramId
} else {
InstructionError::IncorrectProgramId
},
)
}
.map(|_| 0)
.map_err(|error| Box::new(error) as Box<dyn std::error::Error>);
}

// Program Invocation
if !program_account.is_executable() {
if !invoke_context
.get_feature_set()
.is_active(&remove_accounts_executable_flag_checks::id())
&& !program_account.is_executable()
{
ic_logger_msg!(log_collector, "Program is not executable");
return Err(Box::new(InstructionError::IncorrectProgramId));
}
@@ -441,7 +455,14 @@ pub fn process_instruction_inner(
.find(program_account.get_key())
.ok_or_else(|| {
ic_logger_msg!(log_collector, "Program is not cached");
InstructionError::InvalidAccountData
if invoke_context
.get_feature_set()
.is_active(&remove_accounts_executable_flag_checks::id())
{
InstructionError::UnsupportedProgramId
} else {
InstructionError::InvalidAccountData
}
})?;
drop(program_account);
get_or_create_executor_time.stop();
@@ -456,10 +477,28 @@ pub fn process_instruction_inner(
| ProgramCacheEntryType::Closed
| ProgramCacheEntryType::DelayVisibility => {
ic_logger_msg!(log_collector, "Program is not deployed");
Err(Box::new(InstructionError::InvalidAccountData) as Box<dyn std::error::Error>)
let instruction_error = if invoke_context
.get_feature_set()
.is_active(&remove_accounts_executable_flag_checks::id())
{
InstructionError::UnsupportedProgramId
} else {
InstructionError::InvalidAccountData
};
Err(Box::new(instruction_error) as Box<dyn std::error::Error>)
}
ProgramCacheEntryType::Loaded(executable) => execute(executable, invoke_context),
_ => Err(Box::new(InstructionError::IncorrectProgramId) as Box<dyn std::error::Error>),
_ => {
let instruction_error = if invoke_context
.get_feature_set()
.is_active(&remove_accounts_executable_flag_checks::id())
{
InstructionError::UnsupportedProgramId
} else {
InstructionError::IncorrectProgramId
};
Err(Box::new(instruction_error) as Box<dyn std::error::Error>)
}
}
.map(|_| 0)
}
@@ -718,7 +757,11 @@ fn process_loader_upgradeable_instruction(

let program =
instruction_context.try_borrow_instruction_account(transaction_context, 1)?;
if !program.is_executable() {
if !invoke_context
.get_feature_set()
.is_active(&remove_accounts_executable_flag_checks::id())
&& !program.is_executable()
{
ic_logger_msg!(log_collector, "Program account not executable");
return Err(InstructionError::AccountNotExecutable);
}
@@ -1457,13 +1500,19 @@ pub fn execute<'a, 'b: 'a>(
instruction_account_index as IndexOfAccount,
)?;

error = EbpfError::SyscallError(Box::new(if account.is_executable() {
InstructionError::ExecutableDataModified
} else if account.is_writable() {
InstructionError::ExternalAccountDataModified
} else {
InstructionError::ReadonlyDataModified
}));
error = EbpfError::SyscallError(Box::new(
if !invoke_context
.get_feature_set()
.is_active(&remove_accounts_executable_flag_checks::id())
&& account.is_executable()
{
InstructionError::ExecutableDataModified
} else if account.is_writable() {
InstructionError::ExternalAccountDataModified
} else {
InstructionError::ReadonlyDataModified
},
));
}
}
}
26 changes: 22 additions & 4 deletions sdk/src/transaction_context.rs
Original file line number Diff line number Diff line change
@@ -140,6 +140,8 @@ pub struct TransactionContext {
return_data: TransactionReturnData,
accounts_resize_delta: RefCell<i64>,
#[cfg(not(target_os = "solana"))]
remove_accounts_executable_flag_checks: bool,
#[cfg(not(target_os = "solana"))]
rent: Rent,
/// Useful for debugging to filter by or to look it up on the explorer
#[cfg(all(not(target_os = "solana"), feature = "full", debug_assertions))]
@@ -168,12 +170,18 @@ impl TransactionContext {
instruction_trace: vec![InstructionContext::default()],
return_data: TransactionReturnData::default(),
accounts_resize_delta: RefCell::new(0),
remove_accounts_executable_flag_checks: true,
rent,
#[cfg(all(not(target_os = "solana"), feature = "full", debug_assertions))]
signature: Signature::default(),
}
}

#[cfg(not(target_os = "solana"))]
pub fn set_remove_accounts_executable_flag_checks(&mut self, enabled: bool) {
self.remove_accounts_executable_flag_checks = enabled;
}

/// Used in mock_process_instruction
#[cfg(not(target_os = "solana"))]
pub fn deconstruct_without_keys(self) -> Result<Vec<AccountSharedData>, InstructionError> {
@@ -746,7 +754,7 @@ impl<'a> BorrowedAccount<'a> {
return Err(InstructionError::ModifiedProgramId);
}
// and only if the account is not executable
if self.is_executable() {
if self.is_executable_internal() {
return Err(InstructionError::ModifiedProgramId);
}
// and only if the data is zero-initialized or empty
@@ -780,7 +788,7 @@ impl<'a> BorrowedAccount<'a> {
return Err(InstructionError::ReadonlyLamportChange);
}
// The balance of executable accounts may not change
if self.is_executable() {
if self.is_executable_internal() {
return Err(InstructionError::ExecutableLamportChange);
}
// don't touch the account if the lamports do not change
@@ -996,6 +1004,16 @@ impl<'a> BorrowedAccount<'a> {
self.account.executable()
}

/// Feature gating to remove `is_executable` flag related checks
#[cfg(not(target_os = "solana"))]
#[inline]
fn is_executable_internal(&self) -> bool {
!self
.transaction_context
.remove_accounts_executable_flag_checks
&& self.account.executable()
}

/// Configures whether this account is executable (transaction wide)
#[cfg(not(target_os = "solana"))]
pub fn set_executable(&mut self, is_executable: bool) -> Result<(), InstructionError> {
@@ -1016,7 +1034,7 @@ impl<'a> BorrowedAccount<'a> {
return Err(InstructionError::ExecutableModified);
}
// one can not clear the executable flag
if self.is_executable() && !is_executable {
if self.is_executable_internal() && !is_executable {
return Err(InstructionError::ExecutableModified);
}
// don't touch the account if the executable flag does not change
@@ -1073,7 +1091,7 @@ impl<'a> BorrowedAccount<'a> {
#[cfg(not(target_os = "solana"))]
pub fn can_data_be_changed(&self) -> Result<(), InstructionError> {
// Only non-executable accounts data can be changed
if self.is_executable() {
if self.is_executable_internal() {
return Err(InstructionError::ExecutableDataModified);
}
// and only if the account is writable
8 changes: 6 additions & 2 deletions svm/src/account_loader.rs
Original file line number Diff line number Diff line change
@@ -356,7 +356,9 @@ fn load_transaction_accounts<CB: TransactionProcessingCallback>(
return Err(TransactionError::ProgramAccountNotFound);
}

if !program_account.executable() {
if !feature_set.is_active(&feature_set::remove_accounts_executable_flag_checks::id())
&& !program_account.executable()
{
error_metrics.invalid_program_for_execution += 1;
return Err(TransactionError::InvalidProgramForExecution);
}
@@ -373,7 +375,9 @@ fn load_transaction_accounts<CB: TransactionProcessingCallback>(
{
if let Some(owner_account) = callbacks.get_account_shared_data(owner_id) {
if !native_loader::check_id(owner_account.owner())
|| !owner_account.executable()
|| (!feature_set
.is_active(&feature_set::remove_accounts_executable_flag_checks::id())
&& !owner_account.executable())
{
error_metrics.invalid_program_for_execution += 1;
return Err(TransactionError::InvalidProgramForExecution);
8 changes: 7 additions & 1 deletion svm/src/transaction_processor.rs
Original file line number Diff line number Diff line change
@@ -25,7 +25,8 @@ use {
},
solana_compute_budget::compute_budget::ComputeBudget,
solana_feature_set::{
enable_transaction_loading_failure_fees, remove_rounding_in_fee_calculation, FeatureSet,
enable_transaction_loading_failure_fees, remove_accounts_executable_flag_checks,
remove_rounding_in_fee_calculation, FeatureSet,
},
solana_log_collector::LogCollector,
solana_measure::{measure::Measure, measure_us},
@@ -780,6 +781,11 @@ impl<FG: ForkGraph> TransactionBatchProcessor<FG> {
compute_budget.max_instruction_stack_depth,
compute_budget.max_instruction_trace_length,
);
transaction_context.set_remove_accounts_executable_flag_checks(
environment
.feature_set
.is_active(&remove_accounts_executable_flag_checks::id()),
);
#[cfg(debug_assertions)]
transaction_context.set_signature(tx.signature());