Cap the number of accounts passed to a bpf program ix (#26630)

* Cap the number of accounts passed to a bpf program ix

* update bank abi hash

* fix ci failures

programs/bpf/benches/bpf_loader.rs

@@ -230,6 +230,7 @@ fn bench_create_vm(bencher: &mut Bencher) {
                 .transaction_context
                 .get_current_instruction_context()
                 .unwrap(),
+            true, // should_cap_ix_accounts
         )
         .unwrap();
 
@@ -277,6 +278,7 @@ fn bench_instruction_count_tuner(_bencher: &mut Bencher) {
                 .transaction_context
                 .get_current_instruction_context()
                 .unwrap(),
+            true, // should_cap_ix_accounts
         )
         .unwrap();
 

programs/bpf/tests/programs.rs

@@ -213,6 +213,7 @@ fn run_program(name: &str) -> u64 {
                 .transaction_context
                 .get_current_instruction_context()
                 .unwrap(),
+            true, // should_cap_ix_accounts
         )
         .unwrap();
 

programs/bpf_loader/src/lib.rs

@@ -41,9 +41,10 @@ use {
         bpf_loader_upgradeable::{self, UpgradeableLoaderState},
         entrypoint::{HEAP_LENGTH, SUCCESS},
         feature_set::{
-            cap_accounts_data_len, disable_bpf_deprecated_load_instructions,
-            disable_bpf_unresolved_symbols_at_runtime, disable_deploy_of_alloc_free_syscall,
-            disable_deprecated_loader, enable_bpf_loader_extend_program_data_ix,
+            cap_accounts_data_len, cap_bpf_program_instruction_accounts,
+            disable_bpf_deprecated_load_instructions, disable_bpf_unresolved_symbols_at_runtime,
+            disable_deploy_of_alloc_free_syscall, disable_deprecated_loader,
+            enable_bpf_loader_extend_program_data_ix,
             error_on_syscall_bpf_function_hash_collisions, reject_callx_r10,
         },
         instruction::{AccountMeta, InstructionError},
@@ -1285,8 +1286,13 @@ impl Executor for BpfExecutor {
         let program_id = *instruction_context.get_last_program_key(transaction_context)?;
 
         let mut serialize_time = Measure::start("serialize");
-        let (mut parameter_bytes, account_lengths) =
-            serialize_parameters(invoke_context.transaction_context, instruction_context)?;
+        let (mut parameter_bytes, account_lengths) = serialize_parameters(
+            invoke_context.transaction_context,
+            instruction_context,
+            invoke_context
+                .feature_set
+                .is_active(&cap_bpf_program_instruction_accounts::ID),
+        )?;
         serialize_time.stop();
 
         let mut create_vm_time = Measure::start("create_vm");

programs/bpf_loader/src/serialization.rs

@@ -5,7 +5,7 @@ use {
     solana_rbpf::{aligned_memory::AlignedMemory, ebpf::HOST_ALIGN},
     solana_sdk::{
         bpf_loader_deprecated,
-        entrypoint::{BPF_ALIGN_OF_U128, MAX_PERMITTED_DATA_INCREASE},
+        entrypoint::{BPF_ALIGN_OF_U128, MAX_PERMITTED_DATA_INCREASE, NON_DUP_MARKER},
         instruction::InstructionError,
         pubkey::Pubkey,
         system_instruction::MAX_PERMITTED_DATA_LENGTH,
@@ -14,10 +14,20 @@ use {
     std::{io::prelude::*, mem::size_of},
 };
 
+/// Maximum number of instruction accounts that can be serialized into the
+/// BPF VM.
+const MAX_INSTRUCTION_ACCOUNTS: u8 = NON_DUP_MARKER;
+
 pub fn serialize_parameters(
     transaction_context: &TransactionContext,
     instruction_context: &InstructionContext,
+    should_cap_ix_accounts: bool,
 ) -> Result<(AlignedMemory, Vec<usize>), InstructionError> {
+    let num_ix_accounts = instruction_context.get_number_of_instruction_accounts();
+    if should_cap_ix_accounts && num_ix_accounts > usize::from(MAX_INSTRUCTION_ACCOUNTS) {
+        return Err(InstructionError::MaxAccountsExceeded);
+    }
+
     let is_loader_deprecated = *instruction_context
         .try_borrow_last_program_account(transaction_context)?
         .get_owner()
@@ -109,7 +119,7 @@ pub fn serialize_parameters_unaligned(
         } else {
             let borrowed_account = instruction_context
                 .try_borrow_instruction_account(transaction_context, instruction_account_index)?;
-            v.write_u8(std::u8::MAX)
+            v.write_u8(NON_DUP_MARKER)
                 .map_err(|_| InstructionError::InvalidArgument)?;
             v.write_u8(borrowed_account.is_signer() as u8)
                 .map_err(|_| InstructionError::InvalidArgument)?;
@@ -245,7 +255,7 @@ pub fn serialize_parameters_aligned(
         } else {
             let borrowed_account = instruction_context
                 .try_borrow_instruction_account(transaction_context, instruction_account_index)?;
-            v.write_u8(std::u8::MAX)
+            v.write_u8(NON_DUP_MARKER)
                 .map_err(|_| InstructionError::InvalidArgument)?;
             v.write_u8(borrowed_account.is_signer() as u8)
                 .map_err(|_| InstructionError::InvalidArgument)?;
@@ -381,6 +391,161 @@ mod tests {
         },
     };
 
+    #[test]
+    fn test_serialize_parameters_with_many_accounts() {
+        struct TestCase {
+            num_ix_accounts: usize,
+            append_dup_account: bool,
+            should_cap_ix_accounts: bool,
+            expected_err: Option<InstructionError>,
+            name: &'static str,
+        }
+
+        for TestCase {
+            num_ix_accounts,
+            append_dup_account,
+            should_cap_ix_accounts,
+            expected_err,
+            name,
+        } in [
+            TestCase {
+                name: "serialize max accounts without cap",
+                num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS),
+                should_cap_ix_accounts: false,
+                append_dup_account: false,
+                expected_err: None,
+            },
+            TestCase {
+                name: "serialize max accounts and append dup without cap",
+                num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS),
+                should_cap_ix_accounts: false,
+                append_dup_account: true,
+                expected_err: None,
+            },
+            TestCase {
+                name: "serialize max accounts with cap",
+                num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS),
+                should_cap_ix_accounts: true,
+                append_dup_account: false,
+                expected_err: None,
+            },
+            TestCase {
+                name: "serialize too many accounts with cap",
+                num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS) + 1,
+                should_cap_ix_accounts: true,
+                append_dup_account: false,
+                expected_err: Some(InstructionError::MaxAccountsExceeded),
+            },
+            TestCase {
+                name: "serialize too many accounts and append dup with cap",
+                num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS),
+                should_cap_ix_accounts: true,
+                append_dup_account: true,
+                expected_err: Some(InstructionError::MaxAccountsExceeded),
+            },
+            // This test case breaks parameter deserialization and can be cleaned up
+            // when should_cap_ix_accounts is enabled.
+            //
+            // TestCase {
+            //     name: "serialize too many accounts and append dup without cap",
+            //     num_ix_accounts: usize::from(MAX_INSTRUCTION_ACCOUNTS) + 1,
+            //     should_cap_ix_accounts: false,
+            //     append_dup_account: true,
+            //     expected_err: None,
+            // },
+        ] {
+            let program_id = solana_sdk::pubkey::new_rand();
+            let mut transaction_accounts = vec![(
+                program_id,
+                AccountSharedData::from(Account {
+                    lamports: 0,
+                    data: vec![],
+                    owner: bpf_loader::id(),
+                    executable: true,
+                    rent_epoch: 0,
+                }),
+            )];
+
+            let instruction_account_keys: Vec<Pubkey> =
+                (0..num_ix_accounts).map(|_| Pubkey::new_unique()).collect();
+
+            for key in &instruction_account_keys {
+                transaction_accounts.push((
+                    *key,
+                    AccountSharedData::from(Account {
+                        lamports: 0,
+                        data: vec![],
+                        owner: program_id,
+                        executable: false,
+                        rent_epoch: 0,
+                    }),
+                ));
+            }
+
+            let mut instruction_account_metas: Vec<_> = instruction_account_keys
+                .iter()
+                .map(|key| AccountMeta::new_readonly(*key, false))
+                .collect();
+            if append_dup_account {
+                instruction_account_metas.push(instruction_account_metas.last().cloned().unwrap());
+            }
+
+            let program_indices = [0];
+            let instruction_accounts = prepare_mock_invoke_context(
+                transaction_accounts.clone(),
+                instruction_account_metas,
+                &program_indices,
+            )
+            .instruction_accounts;
+
+            let transaction_context =
+                TransactionContext::new(transaction_accounts, Some(Rent::default()), 1, 1);
+            let instruction_data = vec![];
+            let instruction_context = InstructionContext::new(
+                0,
+                0,
+                &program_indices,
+                &instruction_accounts,
+                &instruction_data,
+            );
+
+            let serialization_result = serialize_parameters(
+                &transaction_context,
+                &instruction_context,
+                should_cap_ix_accounts,
+            );
+            assert_eq!(
+                serialization_result.as_ref().err(),
+                expected_err.as_ref(),
+                "{} test case failed",
+                name
+            );
+            if expected_err.is_some() {
+                continue;
+            }
+
+            let (mut serialized, _account_lengths) = serialization_result.unwrap();
+            let (de_program_id, de_accounts, de_instruction_data) =
+                unsafe { deserialize(serialized.as_slice_mut().first_mut().unwrap() as *mut u8) };
+            assert_eq!(de_program_id, &program_id);
+            assert_eq!(de_instruction_data, &instruction_data);
+            for (index, account_info) in de_accounts.into_iter().enumerate() {
+                let ix_account = &instruction_accounts.get(index).unwrap();
+                assert_eq!(
+                    account_info.key,
+                    transaction_context
+                        .get_key_of_account_at_index(ix_account.index_in_transaction)
+                        .unwrap()
+                );
+                assert_eq!(account_info.owner, &program_id);
+                assert!(!account_info.executable);
+                assert!(account_info.data_is_empty());
+                assert!(!account_info.is_writable);
+                assert!(!account_info.is_signer);
+            }
+        }
+    }
+
     #[test]
     fn test_serialize_parameters() {
         let program_id = solana_sdk::pubkey::new_rand();
@@ -496,8 +661,12 @@ mod tests {
 
         // check serialize_parameters_aligned
 
-        let (mut serialized, account_lengths) =
-            serialize_parameters(invoke_context.transaction_context, instruction_context).unwrap();
+        let (mut serialized, account_lengths) = serialize_parameters(
+            invoke_context.transaction_context,
+            instruction_context,
+            true,
+        )
+        .unwrap();
 
         let (de_program_id, de_accounts, de_instruction_data) =
             unsafe { deserialize(serialized.as_slice_mut().first_mut().unwrap() as *mut u8) };
@@ -568,8 +737,12 @@ mod tests {
             .borrow_mut()
             .set_owner(bpf_loader_deprecated::id());
 
-        let (mut serialized, account_lengths) =
-            serialize_parameters(invoke_context.transaction_context, instruction_context).unwrap();
+        let (mut serialized, account_lengths) = serialize_parameters(
+            invoke_context.transaction_context,
+            instruction_context,
+            true,
+        )
+        .unwrap();
 
         let (de_program_id, de_accounts, de_instruction_data) = unsafe {
             deserialize_unaligned(serialized.as_slice_mut().first_mut().unwrap() as *mut u8)
@@ -630,7 +803,7 @@ mod tests {
         for _ in 0..num_accounts {
             let dup_info = *(input.add(offset) as *const u8);
             offset += size_of::<u8>();
-            if dup_info == std::u8::MAX {
+            if dup_info == NON_DUP_MARKER {
                 #[allow(clippy::cast_ptr_alignment)]
                 let is_signer = *(input.add(offset) as *const u8) != 0;
                 offset += size_of::<u8>();

rbpf-cli/src/main.rs

@@ -236,6 +236,7 @@ native machine code before execting it in the virtual machine.",
             .transaction_context
             .get_current_instruction_context()
             .unwrap(),
+        true, // should_cap_ix_accounts
     )
     .unwrap();
     let compute_meter = invoke_context.get_compute_meter();

runtime/src/bank.rs

@@ -263,7 +263,7 @@ impl RentDebits {
 }
 
 pub type BankStatusCache = StatusCache<Result<()>>;
-#[frozen_abi(digest = "2YZk2K45HmmAafmxPJnYVXyQ7uA7WuBrRkpwrCawdK31")]
+#[frozen_abi(digest = "HEJXoycXvGT2pwMuKcUKzzbeemnqbfrUC4jHZx1ncaWv")]
 pub type BankSlotDelta = SlotDelta<Result<()>>;
 
 // Eager rent collection repeats in cyclic manner.

sdk/program/src/entrypoint.rs

@@ -36,6 +36,9 @@ pub const HEAP_START_ADDRESS: u64 = 0x300000000;
 /// Length of the heap memory region used for program heap.
 pub const HEAP_LENGTH: usize = 32 * 1024;
 
+/// Value used to indicate that a serialized account is not a duplicate
+pub const NON_DUP_MARKER: u8 = u8::MAX;
+
 /// Declare the program entry point and set up global handlers.
 ///
 /// This macro emits the common boilerplate necessary to begin program
@@ -283,7 +286,7 @@ pub unsafe fn deserialize<'a>(input: *mut u8) -> (&'a Pubkey, Vec<AccountInfo<'a
     for _ in 0..num_accounts {
         let dup_info = *(input.add(offset) as *const u8);
         offset += size_of::<u8>();
-        if dup_info == std::u8::MAX {
+        if dup_info == NON_DUP_MARKER {
             #[allow(clippy::cast_ptr_alignment)]
             let is_signer = *(input.add(offset) as *const u8) != 0;
             offset += size_of::<u8>();

sdk/program/src/instruction.rs

@@ -253,9 +253,9 @@ pub enum InstructionError {
     #[error("Account data allocation exceeded the maximum accounts data size limit")]
     MaxAccountsDataSizeExceeded,
 
-    /// Active vote account close
-    #[error("Cannot close vote account unless it stopped voting at least one full epoch ago")]
-    ActiveVoteAccountClose,
+    /// Max accounts exceeded
+    #[error("Max accounts exceeded")]
+    MaxAccountsExceeded,
     // Note: For any new error added here an equivalent ProgramError and its
     // conversions must also be added
 }

sdk/src/feature_set.rs

@@ -464,6 +464,10 @@ pub mod prevent_crediting_accounts_that_end_rent_paying {
     solana_sdk::declare_id!("812kqX67odAp5NFwM8D2N24cku7WTm9CHUTFUXaDkWPn");
 }
 
+pub mod cap_bpf_program_instruction_accounts {
+    solana_sdk::declare_id!("9k5ijzTbYPtjzu8wj2ErH9v45xecHzQ1x4PMYMMxFgdM");
+}
+
 lazy_static! {
     /// Map of feature identifiers to user-visible description
     pub static ref FEATURE_NAMES: HashMap<Pubkey, &'static str> = [
@@ -574,6 +578,7 @@ lazy_static! {
         (enable_bpf_loader_extend_program_data_ix::id(), "enable bpf upgradeable loader ExtendProgramData instruction #25234"),
         (enable_early_verification_of_account_modifications::id(), "enable early verification of account modifications #25899"),
         (prevent_crediting_accounts_that_end_rent_paying::id(), "prevent crediting rent paying accounts #26606"),
+        (cap_bpf_program_instruction_accounts::id(), "enforce max number of accounts per bpf program instruction #26628"),
         /*************** ADD NEW FEATURES HERE ***************/
     ]
     .iter()

storage-proto/proto/transaction_by_addr.proto

@@ -121,7 +121,7 @@ enum InstructionErrorType {
     UNSUPPORTED_SYSVAR = 48;
     ILLEGAL_OWNER = 49;
     MAX_ACCOUNTS_DATA_SIZE_EXCEEDED = 50;
-    ACTIVE_VOTE_ACCOUNT_CLOSE = 51;
+    MAX_ACCOUNTS_EXCEEDED = 51;
 }
 
 message UnixTimestamp {

storage-proto/src/convert.rs

@@ -705,7 +705,7 @@ impl TryFrom<tx_by_addr::TransactionError> for TransactionError {
                     48 => InstructionError::UnsupportedSysvar,
                     49 => InstructionError::IllegalOwner,
                     50 => InstructionError::MaxAccountsDataSizeExceeded,
-                    51 => InstructionError::ActiveVoteAccountClose,
+                    51 => InstructionError::MaxAccountsExceeded,
                     _ => return Err("Invalid InstructionError"),
                 };
 
@@ -1022,8 +1022,8 @@ impl From<TransactionError> for tx_by_addr::TransactionError {
                             InstructionError::MaxAccountsDataSizeExceeded => {
                                 tx_by_addr::InstructionErrorType::MaxAccountsDataSizeExceeded
                             }
-                            InstructionError::ActiveVoteAccountClose => {
-                                tx_by_addr::InstructionErrorType::ActiveVoteAccountClose
+                            InstructionError::MaxAccountsExceeded => {
+                                tx_by_addr::InstructionErrorType::MaxAccountsExceeded
                             }
                         } as i32,
                         custom: match instruction_error {