Feature - Remove accounts executable flag checks (#2182)

* Adds the feature.

* Adds the feature gate logic.

* Adjusts tests.

* Adds deprecation attributes.

* Set is_executable flag upon initialization in loader-v4.

(cherry picked from commit 3bf9c2e)

program-runtime/src/invoke_context.rs

@@ -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,12 @@ 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() {
+        #[allow(deprecated)]
+        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);
         }

programs/bpf_loader/src/lib.rs

@@ -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,28 @@ 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() {
+    #[allow(deprecated)]
+    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 +456,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 +478,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 +758,12 @@ fn process_loader_upgradeable_instruction(
 
             let program =
                 instruction_context.try_borrow_instruction_account(transaction_context, 1)?;
-            if !program.is_executable() {
+            #[allow(deprecated)]
+            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 +1502,20 @@ 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(
+                                #[allow(deprecated)]
+                                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
+                                },
+                            ));
                         }
                     }
                 }
@@ -1636,7 +1688,7 @@ mod tests {
     fn test_bpf_loader_invoke_main() {
         let loader_id = bpf_loader::id();
         let program_id = Pubkey::new_unique();
-        let mut program_account =
+        let program_account =
             load_program_account_from_elf(&loader_id, "test_elfs/out/noop_aligned.so");
         let parameter_id = Pubkey::new_unique();
         let parameter_account = AccountSharedData::new(1, 0, &loader_id);
@@ -1686,7 +1738,7 @@ mod tests {
             &[],
             vec![
                 (program_id, program_account.clone()),
-                (parameter_id, parameter_account),
+                (parameter_id, parameter_account.clone()),
             ],
             vec![parameter_meta.clone(), parameter_meta],
             Ok(()),
@@ -1697,7 +1749,7 @@ mod tests {
             &loader_id,
             vec![0],
             &[],
-            vec![(program_id, program_account.clone())],
+            vec![(program_id, program_account)],
             Vec::new(),
             Err(InstructionError::ProgramFailedToComplete),
             Entrypoint::vm,
@@ -1709,14 +1761,29 @@ mod tests {
         );
 
         // Case: Account not a program
-        program_account.set_executable(false);
+        mock_process_instruction(
+            &loader_id,
+            vec![0],
+            &[],
+            vec![(program_id, parameter_account.clone())],
+            Vec::new(),
+            Err(InstructionError::IncorrectProgramId),
+            Entrypoint::vm,
+            |invoke_context| {
+                let mut feature_set = invoke_context.get_feature_set().clone();
+                feature_set.deactivate(&remove_accounts_executable_flag_checks::id());
+                invoke_context.mock_set_feature_set(Arc::new(feature_set));
+                test_utils::load_all_invoked_programs(invoke_context);
+            },
+            |_invoke_context| {},
+        );
         process_instruction(
             &loader_id,
             &[0],
             &[],
-            vec![(program_id, program_account)],
+            vec![(program_id, parameter_account)],
             Vec::new(),
-            Err(InstructionError::IncorrectProgramId),
+            Err(InstructionError::UnsupportedProgramId),
         );
     }
 
@@ -2379,22 +2446,35 @@ mod tests {
         );
 
         // Case: Program account not executable
-        let (mut transaction_accounts, instruction_accounts) = get_accounts(
+        let (transaction_accounts, mut instruction_accounts) = get_accounts(
             &buffer_address,
             &upgrade_authority_address,
             &upgrade_authority_address,
             &elf_orig,
             &elf_new,
         );
-        transaction_accounts
-            .get_mut(1)
-            .unwrap()
-            .1
-            .set_executable(false);
-        process_instruction(
-            transaction_accounts,
-            instruction_accounts,
+        *instruction_accounts.get_mut(1).unwrap() = instruction_accounts.get(2).unwrap().clone();
+        let instruction_data = bincode::serialize(&UpgradeableLoaderInstruction::Upgrade).unwrap();
+        mock_process_instruction(
+            &bpf_loader_upgradeable::id(),
+            Vec::new(),
+            &instruction_data,
+            transaction_accounts.clone(),
+            instruction_accounts.clone(),
             Err(InstructionError::AccountNotExecutable),
+            Entrypoint::vm,
+            |invoke_context| {
+                let mut feature_set = invoke_context.get_feature_set().clone();
+                feature_set.deactivate(&remove_accounts_executable_flag_checks::id());
+                invoke_context.mock_set_feature_set(Arc::new(feature_set));
+                test_utils::load_all_invoked_programs(invoke_context);
+            },
+            |_invoke_context| {},
+        );
+        process_instruction(
+            transaction_accounts.clone(),
+            instruction_accounts.clone(),
+            Err(InstructionError::InvalidAccountData),
         );
 
         // Case: Program account now owned by loader
@@ -3593,7 +3673,7 @@ mod tests {
                 (program_address, program_account.clone()),
             ],
             Vec::new(),
-            Err(InstructionError::InvalidAccountData),
+            Err(InstructionError::UnsupportedProgramId),
         );
 
         // Case: Reopen should fail

programs/bpf_loader/src/serialization.rs

@@ -339,6 +339,7 @@ fn serialize_parameters_unaligned(
                 s.write::<u64>((account.get_data().len() as u64).to_le());
                 let vm_data_addr = s.write_account(&mut account)?;
                 let vm_owner_addr = s.write_all(account.get_owner().as_ref());
+                #[allow(deprecated)]
                 s.write::<u8>(account.is_executable() as u8);
                 s.write::<u64>((account.get_rent_epoch()).to_le());
                 accounts_metadata.push(SerializedAccountMetadata {
@@ -467,6 +468,7 @@ fn serialize_parameters_aligned(
                 s.write::<u8>(NON_DUP_MARKER);
                 s.write::<u8>(borrowed_account.is_signer() as u8);
                 s.write::<u8>(borrowed_account.is_writable() as u8);
+                #[allow(deprecated)]
                 s.write::<u8>(borrowed_account.is_executable() as u8);
                 s.write_all(&[0u8, 0, 0, 0]);
                 let vm_key_addr = s.write_all(borrowed_account.get_key().as_ref());

programs/bpf_loader/src/syscalls/cpi.rs

@@ -890,6 +890,7 @@ where
             .transaction_context
             .get_key_of_account_at_index(instruction_account.index_in_transaction)?;
 
+        #[allow(deprecated)]
         if callee_account.is_executable() {
             // Use the known account
             consume_compute_meter(

programs/loader-v4/src/lib.rs

@@ -198,6 +198,7 @@ pub fn process_instruction_truncate(
             LoaderV4State::program_data_offset().saturating_add(new_size as usize),
         )?;
         if is_initialization {
+            program.set_executable(true)?;
             let state = get_state_mut(program.get_data_mut()?)?;
             state.slot = 0;
             state.status = LoaderV4Status::Retracted;

programs/sbf/tests/programs.rs

@@ -918,15 +918,15 @@ fn test_program_sbf_invoke_sanity() {
 
         do_invoke_failure_test_local(
             TEST_PPROGRAM_NOT_OWNED_BY_LOADER,
-            TransactionError::InstructionError(0, InstructionError::AccountNotExecutable),
-            &[],
+            TransactionError::InstructionError(0, InstructionError::UnsupportedProgramId),
+            &[argument_keypair.pubkey()],
             None,
         );
 
         do_invoke_failure_test_local(
             TEST_PPROGRAM_NOT_EXECUTABLE,
-            TransactionError::InstructionError(0, InstructionError::AccountNotExecutable),
-            &[],
+            TransactionError::InstructionError(0, InstructionError::UnsupportedProgramId),
+            &[unexecutable_program_keypair.pubkey()],
             None,
         );
 
@@ -1895,7 +1895,7 @@ fn test_program_sbf_invoke_in_same_tx_as_deployment() {
             let (result, _, _) = process_transaction_and_record_inner(&bank, tx);
             assert_eq!(
                 result.unwrap_err(),
-                TransactionError::InstructionError(2, InstructionError::InvalidAccountData),
+                TransactionError::InstructionError(2, InstructionError::UnsupportedProgramId),
             );
         }
     }
@@ -2006,7 +2006,7 @@ fn test_program_sbf_invoke_in_same_tx_as_redeployment() {
         let (result, _, _) = process_transaction_and_record_inner(&bank, tx);
         assert_eq!(
             result.unwrap_err(),
-            TransactionError::InstructionError(1, InstructionError::InvalidAccountData),
+            TransactionError::InstructionError(1, InstructionError::UnsupportedProgramId),
         );
     }
 }
@@ -2101,7 +2101,7 @@ fn test_program_sbf_invoke_in_same_tx_as_undeployment() {
         let (result, _, _) = process_transaction_and_record_inner(&bank, tx);
         assert_eq!(
             result.unwrap_err(),
-            TransactionError::InstructionError(1, InstructionError::InvalidAccountData),
+            TransactionError::InstructionError(1, InstructionError::UnsupportedProgramId),
         );
     }
 }
@@ -4650,7 +4650,7 @@ fn test_deny_executable_write() {
         let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction);
         assert_eq!(
             result.unwrap_err().unwrap(),
-            TransactionError::InstructionError(0, InstructionError::ExecutableDataModified)
+            TransactionError::InstructionError(0, InstructionError::ReadonlyDataModified)
         );
     }
 }

runtime/src/bank/tests.rs

@@ -7229,7 +7229,10 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
     );
     assert_eq!(
         bank.process_transaction(&transaction),
-        Err(TransactionError::InvalidProgramForExecution),
+        Err(TransactionError::InstructionError(
+            0,
+            InstructionError::UnsupportedProgramId
+        )),
     );
     {
         let program_cache = bank.transaction_processor.program_cache.read().unwrap();
@@ -7250,7 +7253,10 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
     let transaction = Transaction::new(&[&binding], message, bank.last_blockhash());
     assert_eq!(
         bank.process_transaction(&transaction),
-        Err(TransactionError::InvalidProgramForExecution),
+        Err(TransactionError::InstructionError(
+            0,
+            InstructionError::UnsupportedProgramId,
+        )),
     );
     {
         let program_cache = bank.transaction_processor.program_cache.read().unwrap();

sdk/feature-set/src/lib.rs

@@ -881,6 +881,10 @@ pub mod migrate_stake_program_to_core_bpf {
     solana_pubkey::declare_id!("6M4oQ6eXneVhtLoiAr4yRYQY43eVLjrKbiDZDJc892yk");
 }
 
+pub mod remove_accounts_executable_flag_checks {
+    solana_pubkey::declare_id!("FfgtauHUWKeXTzjXkua9Px4tNGBFHKZ9WaigM5VbbzFx");
+}
+
 lazy_static! {
     /// Map of feature identifiers to user-visible description
     pub static ref FEATURE_NAMES: HashMap<Pubkey, &'static str> = [
@@ -1096,6 +1100,7 @@ lazy_static! {
         (disable_account_loader_special_case::id(), "Disable account loader special case #3513"),
         (enable_secp256r1_precompile::id(), "Enable secp256r1 precompile SIMD-0075"),
         (migrate_stake_program_to_core_bpf::id(), "Migrate Stake program to Core BPF SIMD-0196 #3655"),
+        (remove_accounts_executable_flag_checks::id(), "Remove checks of accounts is_executable flag SIMD-0162"),
         /*************** ADD NEW FEATURES HERE ***************/
     ]
     .iter()