use PROGRAM_OWNER + program data to decide whether an account is exec…

…utable during transaction loading and processing

program-runtime/src/invoke_context.rs

@@ -962,8 +962,8 @@ mod tests {
         let owned_account = AccountSharedData::new(42, 1, &callee_program_id);
         let not_owned_account = AccountSharedData::new(84, 1, &solana_sdk::pubkey::new_rand());
         let readonly_account = AccountSharedData::new(168, 1, &solana_sdk::pubkey::new_rand());
-        let loader_account = AccountSharedData::new(0, 0, &native_loader::id());
-        let mut program_account = AccountSharedData::new(1, 0, &native_loader::id());
+        let loader_account = AccountSharedData::new(0, 1, &native_loader::id());
+        let mut program_account = AccountSharedData::new(1, 1, &native_loader::id());
         program_account.set_executable(true);
         let transaction_accounts = vec![
             (solana_sdk::pubkey::new_rand(), owned_account),
@@ -990,7 +990,7 @@ mod tests {
         let mut programs_loaded_for_tx_batch = LoadedProgramsForTxBatch::default();
         programs_loaded_for_tx_batch.replenish(
             callee_program_id,
-            Arc::new(LoadedProgram::new_builtin(0, 0, MockBuiltin::vm)),
+            Arc::new(LoadedProgram::new_builtin(0, 1, MockBuiltin::vm)),
         );
         invoke_context.programs_loaded_for_tx_batch = &programs_loaded_for_tx_batch;
 

programs/bpf_loader/src/lib.rs

@@ -2641,36 +2641,13 @@ mod tests {
             &elf_orig,
             &elf_new,
         );
-        transaction_accounts
-            .get_mut(1)
-            .unwrap()
-            .1
-            .set_executable(false);
+        transaction_accounts.get_mut(1).unwrap().1.set_data(vec![]); // set the account data empty to make it not executable
         process_instruction(
             transaction_accounts,
             instruction_accounts,
             Err(InstructionError::AccountNotExecutable),
         );
 
-        // Case: Program account now owned by loader
-        let (mut transaction_accounts, instruction_accounts) = get_accounts(
-            &buffer_address,
-            &upgrade_authority_address,
-            &upgrade_authority_address,
-            &elf_orig,
-            &elf_new,
-        );
-        transaction_accounts
-            .get_mut(1)
-            .unwrap()
-            .1
-            .set_owner(Pubkey::new_unique());
-        process_instruction(
-            transaction_accounts,
-            instruction_accounts,
-            Err(InstructionError::IncorrectProgramId),
-        );
-
         // Case: Program account not writable
         let (transaction_accounts, mut instruction_accounts) = get_accounts(
             &buffer_address,
@@ -2686,26 +2663,6 @@ mod tests {
             Err(InstructionError::InvalidArgument),
         );
 
-        // Case: Program account not initialized
-        let (mut transaction_accounts, instruction_accounts) = get_accounts(
-            &buffer_address,
-            &upgrade_authority_address,
-            &upgrade_authority_address,
-            &elf_orig,
-            &elf_new,
-        );
-        transaction_accounts
-            .get_mut(1)
-            .unwrap()
-            .1
-            .set_state(&UpgradeableLoaderState::Uninitialized)
-            .unwrap();
-        process_instruction(
-            transaction_accounts,
-            instruction_accounts,
-            Err(InstructionError::InvalidAccountData),
-        );
-
         // Case: Program ProgramData account mismatch
         let (mut transaction_accounts, mut instruction_accounts) = get_accounts(
             &buffer_address,

runtime/src/accounts/mod.rs

@@ -24,7 +24,10 @@ use {
         loaded_programs::LoadedProgramsForTxBatch,
     },
     solana_sdk::{
-        account::{Account, AccountSharedData, ReadableAccount, WritableAccount},
+        account::{
+            is_builtin, is_executable, mock_executable_meta, Account, AccountSharedData,
+            ReadableAccount, WritableAccount,
+        },
         account_utils::StateMut,
         bpf_loader_upgradeable::{self, UpgradeableLoaderState},
         feature_set::{
@@ -149,6 +152,22 @@ fn load_transaction_accounts(
     loaded_programs: &LoadedProgramsForTxBatch,
     should_collect_rent: bool,
 ) -> Result<LoadedTransaction> {
+    macro_rules! log_exec_diff {
+        ($key: expr, $account: expr,  $name: expr) => {
+            let exec2 = is_builtin(&$account) || is_executable(&$account);
+            if exec2 != $account.executable() {
+                log::error!(
+                    "exec_flag diffs {} {} {:?} {} {}",
+                    $name,
+                    $key,
+                    &$account,
+                    exec2,
+                    &$account.executable()
+                );
+            }
+        };
+    }
+
     let in_reward_interval = reward_interval == RewardInterval::InsideInterval;
 
     // NOTE: this check will never fail because `tx` is sanitized
@@ -288,7 +307,9 @@ fn load_transaction_accounts(
                             return Err(TransactionError::InvalidWritableAccount);
                         }
 
-                        if account.executable() {
+                        log_exec_diff!(key, account, "bpf_upgradable_program");
+
+                        if is_builtin(&account) || is_executable(&account) {
                             // The upgradeable loader requires the derived ProgramData account
                             if let Ok(UpgradeableLoaderState::Program {
                                 programdata_address,
@@ -306,9 +327,14 @@ fn load_transaction_accounts(
                                 return Err(TransactionError::InvalidProgramForExecution);
                             }
                         }
-                    } else if account.executable() && message.is_writable(i) {
-                        error_counters.invalid_writable_account += 1;
-                        return Err(TransactionError::InvalidWritableAccount);
+                    } else {
+                        log_exec_diff!(key, account, "bpf_program");
+                        if (is_builtin(&account) || is_executable(&account))
+                            && message.is_writable(i)
+                        {
+                            error_counters.invalid_writable_account += 1;
+                            return Err(TransactionError::InvalidWritableAccount);
+                        }
                     }
                 }
 
@@ -355,15 +381,19 @@ fn load_transaction_accounts(
             let (program_id, program_account) = accounts
                 .get(program_index)
                 .ok_or(TransactionError::ProgramAccountNotFound)?;
-            let account_found = accounts_found.get(program_index).unwrap_or(&true);
             if native_loader::check_id(program_id) {
                 return Ok(account_indices);
             }
+
+            let account_found = accounts_found.get(program_index).unwrap_or(&true);
             if !account_found {
                 error_counters.account_not_found += 1;
                 return Err(TransactionError::ProgramAccountNotFound);
             }
-            if !program_account.executable() {
+
+            log_exec_diff!(program_id, program_account, "buildin");
+
+            if !(is_builtin(program_account) || is_executable(program_account)) {
                 error_counters.invalid_program_for_execution += 1;
                 return Err(TransactionError::InvalidProgramForExecution);
             }
@@ -384,8 +414,10 @@ fn load_transaction_accounts(
                 if let Some((owner_account, _)) =
                     accounts_db.load_with_fixed_root(ancestors, owner_id)
                 {
+                    log_exec_diff!(owner_id, owner_account, "buildin");
+
                     if !native_loader::check_id(owner_account.owner())
-                        || !owner_account.executable()
+                        || !(is_builtin(&owner_account) || is_executable(&owner_account))
                     {
                         error_counters.invalid_program_for_execution += 1;
                         return Err(TransactionError::InvalidProgramForExecution);
@@ -460,6 +492,7 @@ fn account_shared_data_from_program(
         .ok_or(TransactionError::AccountNotFound)?;
     program_account.set_owner(**program_owner);
     program_account.set_executable(true);
+    program_account.set_data_from_slice(&mock_executable_meta(program_owner));
     Ok(program_account)
 }
 
@@ -981,7 +1014,7 @@ mod tests {
         let account = AccountSharedData::new(1, 0, &Pubkey::default());
         accounts.push((key0, account));
 
-        let account = AccountSharedData::new(40, 1, &native_loader::id());
+        let account = AccountSharedData::new(40, 0, &native_loader::id());
         accounts.push((key1, account));
 
         let instructions = vec![CompiledInstruction::new(1, &(), vec![0])];

runtime/src/bank.rs

@@ -123,9 +123,9 @@ use {
     },
     solana_sdk::{
         account::{
-            create_account_shared_data_with_fields as create_account, from_account, Account,
-            AccountSharedData, InheritableAccountFields, ReadableAccount, WritableAccount,
-            PROGRAM_OWNERS,
+            create_account_shared_data_with_fields as create_account, from_account,
+            mock_executable_meta, Account, AccountSharedData, InheritableAccountFields,
+            ReadableAccount, WritableAccount, PROGRAM_OWNERS,
         },
         account_utils::StateMut,
         bpf_loader_upgradeable::{self, UpgradeableLoaderState},
@@ -3988,7 +3988,6 @@ impl Bank {
     fn add_precompiled_account_with_owner(&self, program_id: &Pubkey, owner: Pubkey) {
         if let Some(account) = self.get_account_with_fixed_root(program_id) {
             if account.executable() {
-                // The account is already executable, that's all we need
                 return;
             } else {
                 // malicious account is pre-occupying at program_id
@@ -4004,10 +4003,13 @@ impl Bank {
 
         // Add a bogus executable account, which will be loaded and ignored.
         let (lamports, rent_epoch) = self.inherit_specially_retained_account_fields(&None);
+
+        // The account is already executable, that's all we need
+        let account_data = mock_executable_meta(&owner);
         let account = AccountSharedData::from(Account {
             lamports,
             owner,
-            data: vec![],
+            data: account_data,
             executable: true,
             rent_epoch,
         });

sdk/src/account.rs

@@ -7,7 +7,7 @@ use {
         bpf_loader, bpf_loader_deprecated, bpf_loader_upgradeable,
         clock::{Epoch, INITIAL_RENT_EPOCH},
         lamports::LamportsError,
-        loader_v4,
+        loader_v4, native_loader,
         pubkey::Pubkey,
     },
     serde::{
@@ -764,6 +764,62 @@ pub const PROGRAM_OWNERS: &[Pubkey] = &[
     loader_v4::id(),
 ];
 
+pub fn mock_executable_meta(owner: &Pubkey) -> Vec<u8> {
+    if bpf_loader_upgradeable::check_id(owner) {
+        // For upgradable program account, only
+        // `UpgradeableLoaderState::Program` variant (i.e. discriminant = 2) is
+        // *executable*.
+        return vec![2];
+    }
+
+    if loader_v4::check_id(owner) {
+        // LoaderV4Status (byte_offset = 40)
+        let mut v = vec![0; 41];
+        v[40] = 1;
+        return v;
+    }
+
+    vec![1]
+}
+
+/// Return true if the account program is executable.
+pub fn is_executable(account: &AccountSharedData) -> bool {
+    // empty account is not executable.
+    if account.data().is_empty() {
+        return false;
+    }
+
+    // bpf_loader still relies on `executable` on the program account. When the
+    // program account is finalized, the loader will mark `executable` flag on
+    // the account. Once we disable the deployment of these two loaders, we can
+    // remove the following dependency on `executable`.
+    if bpf_loader::check_id(account.owner()) || bpf_loader_deprecated::check_id(account.owner()) {
+        return account.executable();
+    }
+
+    if bpf_loader_upgradeable::check_id(account.owner()) {
+        // For upgradable program account, only
+        // `UpgradeableLoaderState::Program` variant (i.e. discriminant = 2) is
+        // *executable*.
+        return account.data()[0] == 2;
+    }
+
+    if loader_v4::check_id(account.owner()) {
+        // LoaderV4Status (byte_offset = 40)
+        // return account.data()[40] != 0;
+        return false; // TODO: return false for now
+    }
+
+    false
+}
+
+/// Return true if the account program is a builtin program. Note that for
+/// builtin program, even when its account data is empty, it is still be
+/// executable, such as vote program etc.
+pub fn is_builtin(account: &AccountSharedData) -> bool {
+    native_loader::check_id(account.owner()) && !account.data().is_empty()
+}
+
 #[cfg(test)]
 pub mod tests {
     use super::*;

sdk/src/transaction_context.rs

@@ -17,7 +17,7 @@ use {
 };
 use {
     crate::{
-        account::{AccountSharedData, ReadableAccount},
+        account::{is_builtin, is_executable, AccountSharedData, ReadableAccount},
         instruction::InstructionError,
         pubkey::Pubkey,
     },
@@ -999,7 +999,14 @@ impl<'a> BorrowedAccount<'a> {
     /// Returns whether this account is executable (transaction wide)
     #[inline]
     pub fn is_executable(&self) -> bool {
-        self.account.executable()
+        let executable = is_builtin(&self.account) || is_executable(&self.account);
+        if executable != self.account.executable() {
+            log::error!(
+                "exec_flag diffs borrowed_account {:?}",
+                (self, executable, self.account.executable())
+            );
+        }
+        is_builtin(&self.account) || is_executable(&self.account)
     }
 
     /// Configures whether this account is executable (transaction wide)
@@ -1022,11 +1029,11 @@ impl<'a> BorrowedAccount<'a> {
             return Err(InstructionError::ExecutableModified);
         }
         // one can not clear the executable flag
-        if self.is_executable() && !is_executable {
+        if self.account.executable() && !is_executable {
             return Err(InstructionError::ExecutableModified);
         }
         // don't touch the account if the executable flag does not change
-        if self.is_executable() == is_executable {
+        if self.account.executable() == is_executable {
             return Ok(());
         }
         self.touch()?;