diff --git a/bench/programs/multisig/anchor-v2/src/lib.rs b/bench/programs/multisig/anchor-v2/src/lib.rs index 5e67533c05..791eee6ef2 100644 --- a/bench/programs/multisig/anchor-v2/src/lib.rs +++ b/bench/programs/multisig/anchor-v2/src/lib.rs @@ -16,7 +16,7 @@ pub mod multisig_v2 { #[discrim = 0] pub fn create(ctx: &mut Context, threshold: u8) -> Result<()> { - let remaining = ctx.remaining_accounts(); + let remaining = ctx.remaining_accounts()?; ctx.accounts.create_multisig(threshold, &remaining)?; ctx.accounts.config.bump = ctx.bumps.config; Ok(()) @@ -34,7 +34,7 @@ pub mod multisig_v2 { #[discrim = 3] pub fn execute_transfer(ctx: &mut Context, amount: u64) -> Result<()> { - let remaining = ctx.remaining_accounts(); + let remaining = ctx.remaining_accounts()?; ctx.accounts.verify_and_transfer(amount, ctx.bumps.vault, &remaining) } } diff --git a/lang-v2/Cargo.toml b/lang-v2/Cargo.toml index 6000bd2d7d..0a7050315b 100644 --- a/lang-v2/Cargo.toml +++ b/lang-v2/Cargo.toml @@ -135,3 +135,7 @@ required-features = ["testing"] [[test]] name = "miri_wrapper_accounts" required-features = ["testing"] + +[[test]] +name = "remaining_accounts_mut_check" +required-features = ["testing"] diff --git a/lang-v2/derive/src/lib.rs b/lang-v2/derive/src/lib.rs index f2657884ba..986ff2d12b 100644 --- a/lang-v2/derive/src/lib.rs +++ b/lang-v2/derive/src/lib.rs @@ -468,6 +468,62 @@ fn impl_accounts(input: &DeriveInput) -> TokenStream2 { } } }; + let dynamic_mut_mask_terms: Vec = fields + .iter() + .filter_map(|f| { + if f.contributes_active_mut_bit { + Some(quote::quote! { true }) + } else { + parse::extract_nested_inner_type(&f.ty).map(|inner_ty| { + quote::quote! { + <#inner_ty as anchor_lang_v2::TryAccounts>::HAS_DYNAMIC_MUT_MASK + } + }) + } + }) + .collect(); + let has_dynamic_mut_mask_expr = if dynamic_mut_mask_terms.is_empty() { + quote::quote! { false } + } else { + quote::quote! { + false #(|| #dynamic_mut_mask_terms)* + } + }; + let active_mut_mask_steps: Vec = fields + .iter() + .filter_map(|f| { + let field_name = &f.name; + let offset = &f.offset_expr; + if f.contributes_active_mut_bit { + Some(quote! { + if self.#field_name.is_some() { + __mask = anchor_lang_v2::mut_mask_set_bit(__mask, #offset); + } + }) + } else if let Some(inner_ty) = parse::extract_nested_inner_type(&f.ty) { + Some(quote! { + if <#inner_ty as anchor_lang_v2::TryAccounts>::HAS_DYNAMIC_MUT_MASK { + __mask = anchor_lang_v2::mut_mask_or_shifted( + __mask, + <#inner_ty as anchor_lang_v2::TryAccounts>::active_mut_mask(&self.#field_name.0), + #offset, + ); + } + }) + } else { + None + } + }) + .collect(); + let active_mut_mask_body = if active_mut_mask_steps.is_empty() { + quote::quote! { Self::MUT_MASK } + } else { + quote::quote! { + let mut __mask = Self::MUT_MASK; + #(#active_mut_mask_steps)* + __mask + } + }; // IDL collection — the accounts-JSON emission is a runtime function // (not a `&'static str` const) so it can read @@ -1158,6 +1214,12 @@ fn impl_accounts(input: &DeriveInput) -> TokenStream2 { impl anchor_lang_v2::TryAccounts for #name { const HEADER_SIZE: usize = #header_size_expr; const MUT_MASK: [u64; 4] = #mut_mask_expr; + const HAS_DYNAMIC_MUT_MASK: bool = #has_dynamic_mut_mask_expr; + + #[inline(always)] + fn active_mut_mask(&self) -> [u64; 4] { + #active_mut_mask_body + } #ix_args_assoc diff --git a/lang-v2/derive/src/parse.rs b/lang-v2/derive/src/parse.rs index 34ce5bf25d..93b0d91312 100644 --- a/lang-v2/derive/src/parse.rs +++ b/lang-v2/derive/src/parse.rs @@ -652,6 +652,9 @@ pub struct AccountField { /// client sends `program_id` as the address) should still silence the /// dup check; the derive keeps the gated per-field `get()` for those. pub contributes_mut_bit: bool, + /// `true` iff this optional field contributes to the runtime active + /// mutable mask when it loads as `Some`. + pub contributes_active_mut_bit: bool, /// The local payer field named by this field's `init`/`init_if_needed` /// constraint, if present. pub init_payer: Option, @@ -1256,6 +1259,7 @@ pub fn parse_field( // into the parent's; they don't set a bit at the nested field's // own offset. contributes_mut_bit: false, + contributes_active_mut_bit: false, init_payer: None, idl_writable: false, idl_init_signer: false, @@ -1985,6 +1989,7 @@ pub fn parse_field( }; let contributes_mut_bit = attrs.is_mut && !attrs.is_dup && !is_optional; + let contributes_active_mut_bit = attrs.is_mut && !attrs.is_dup && is_optional; let init_payer = (attrs.is_init || attrs.is_init_if_needed) .then(|| attrs.payer.as_ref().map(ToString::to_string)) .flatten(); @@ -2001,6 +2006,7 @@ pub fn parse_field( is_optional, offset_expr, contributes_mut_bit, + contributes_active_mut_bit, init_payer, idl_writable, idl_init_signer, diff --git a/lang-v2/src/context.rs b/lang-v2/src/context.rs index 903278f2f8..441a61003b 100644 --- a/lang-v2/src/context.rs +++ b/lang-v2/src/context.rs @@ -1,6 +1,7 @@ use { crate::cursor::AccountCursor, pinocchio::{account::AccountView, address::Address}, + solana_program_error::ProgramError, }; /// Instruction-scoped context passed to every handler. Holds the @@ -19,21 +20,56 @@ pub struct Context<'a, T: Bumps> { /// pass them in as arguments. pub bumps: T::Bumps, - /// Cursor into the serialized input buffer, positioned at the + /// Holds either a cursor into the serialized input buffer, pointing to the /// *start* of the remaining-accounts region (after `try_accounts` /// has consumed exactly `T::HEADER_SIZE` declared accounts). Used /// by [`Self::remaining_accounts`] for on-demand walking. - cursor: &'a mut AccountCursor, + /// After `remaining_accounts` is called this holds a cache of the result. + remaining_accounts: RemainingAccounts<'a>, - /// Number of accounts after the declared region. Decremented to - /// zero once the remaining region is walked. - remaining_num: u8, + /// Mutable-account mask covering active mutable accounts in the declared + /// region. Starts from `T::MUT_MASK` and includes optional mutable fields + /// only when the loaded account is `Some`. Used by + /// [`Self::remaining_accounts`] to re-check each trailing account against + /// declared mut slots — + /// without this, a trailing account whose dup index points at a + /// mut declared account would silently alias it (bug 3: the + /// `HEADER_SIZE`-only check in `run_handler` can't see dups that + /// only surface during the trailing walk). + mut_mask: MutMask, +} + +pub enum MutMask { + Static(&'static [u64; 4]), + Dynamic([u64; 4]), +} - /// Cache of the parsed remaining accounts. Populated lazily on the - /// first call to [`Self::remaining_accounts`]; subsequent calls - /// return a clone of the cached vec so the caller receives an - /// owned value (avoiding borrow conflicts with `self.accounts`). - remaining_cache: Option>, +impl MutMask { + #[inline(always)] + fn as_ref(&self) -> &[u64; 4] { + match self { + Self::Static(mask) => mask, + Self::Dynamic(mask) => mask, + } + } +} + +enum RemainingAccounts<'a> { + Unparsed { + /// Points to the `remaining-accounts` region + cursor: &'a mut AccountCursor, + /// Number of accounts remaining after the initially declared region + remaining: u8, + }, + /// Cached result of walking `remaining-accounts`; will return an error if + /// duplicate mutable constraints are violated. + Cached(Result, ProgramError>), +} + +impl<'a> RemainingAccounts<'a> { + fn is_unparsed(&self) -> bool { + matches!(self, RemainingAccounts::Unparsed { .. }) + } } impl<'a, T: Bumps> Context<'a, T> { @@ -44,33 +80,75 @@ impl<'a, T: Bumps> Context<'a, T> { bumps: T::Bumps, cursor: &'a mut AccountCursor, remaining_num: u8, + mut_mask: MutMask, ) -> Self { Self { program_id, accounts, bumps, - cursor, - remaining_num, - remaining_cache: None, + remaining_accounts: RemainingAccounts::Unparsed { + cursor, + remaining: remaining_num, + }, + mut_mask, } } /// Returns trailing accounts beyond the declared `T` fields as an - /// owned `Vec`. First call walks the cursor and caches; - /// subsequent calls clone the cache. Owned vec avoids borrow conflicts - /// with `self.accounts` / `self.bumps`. - pub fn remaining_accounts(&mut self) -> alloc::vec::Vec { - if self.remaining_cache.is_none() { - let mut v = alloc::vec::Vec::with_capacity(self.remaining_num as usize); - for _ in 0..self.remaining_num { - // SAFETY: cursor is positioned at the start of the - // remaining region and `remaining_num` is the exact - // number of accounts to walk. - v.push(unsafe { self.cursor.next() }); + /// owned `Vec`. First call walks the cursor and caches + /// the resulting `Result`; subsequent calls replay the cache (clone + /// of the vec on success, clone of the error on failure). Caching + /// the error is important: the walk advances an unsafe cursor, and + /// a handler that calls this again after an error must not trigger + /// another `cursor.next()` loop. + /// + /// After each cursor advance, re-tests the cursor's duplicate bitvec + /// against the active mutable mask. If a trailing account's dup index + /// resolves to a declared mut slot, returns + /// `ConstraintDuplicateMutableAccount`. The `HEADER_SIZE`-only check + /// in `run_handler` only sees duplicates that existed at the end of + /// the declared walk; trailing-region dups can only be caught here. + /// + /// The mask is sized per declared field, so bits set for trailing indices + /// (past `HEADER_SIZE`) are naturally zero — the intersect only fires when + /// a trailing slot's bit overlaps with an active declared mut slot's bit, + /// which by construction means the runtime resolved the trailing slot as a + /// dup of that declared mut account. + pub fn remaining_accounts(&mut self) -> Result, ProgramError> { + if self.remaining_accounts.is_unparsed() { + self.remaining_accounts = RemainingAccounts::Cached(self.walk_remaining()); + } + + match &self.remaining_accounts { + RemainingAccounts::Cached(Ok(accs)) => Ok(accs.clone()), + RemainingAccounts::Cached(Err(err)) => Err(err.clone()), + RemainingAccounts::Unparsed { .. } => unreachable!(), + } + } + + fn walk_remaining(&mut self) -> Result, ProgramError> { + let RemainingAccounts::Unparsed { + ref mut cursor, + remaining, + } = self.remaining_accounts + else { + unreachable!() + }; + let mut v = alloc::vec::Vec::with_capacity(remaining as usize); + let mut_mask = self.mut_mask.as_ref(); + for _ in 0..remaining { + // SAFETY: cursor is positioned at the start of the remaining + // region and `remaining_num` is the exact number of accounts + // to walk. Walking stops on the first mut-alias error so we + // never advance past the remaining region. + v.push(unsafe { cursor.next() }); + if let Some(dups) = cursor.duplicates() { + if dups.intersects(mut_mask) { + return Err(crate::ErrorCode::ConstraintDuplicateMutableAccount.into()); + } } - self.remaining_cache = Some(v); } - self.remaining_cache.as_ref().unwrap().clone() + Ok(v) } } diff --git a/lang-v2/src/cursor.rs b/lang-v2/src/cursor.rs index 846d4fe8aa..cee1a70181 100644 --- a/lang-v2/src/cursor.rs +++ b/lang-v2/src/cursor.rs @@ -82,6 +82,18 @@ impl AccountCursor { self.consumed } + /// Current duplicate-tracking bitvec. `None` if the cursor has not + /// yet yielded a duplicate account (lazy allocation — see + /// [`Self::next`]). Used by + /// [`Context::remaining_accounts`](crate::context::Context::remaining_accounts) + /// to re-check `MUT_MASK` after each trailing account is walked, + /// catching aliases of declared mut accounts that only surface past + /// `HEADER_SIZE`. + #[inline(always)] + pub fn duplicates(&self) -> Option<&AccountBitvec> { + self.duplicate.as_ref() + } + /// Walk N accounts in a tight loop, storing views in the lookup array. /// Returns a slice of the walked views and the duplicate tracking bitvec. /// This avoids interleaving cursor math with validation logic, letting diff --git a/lang-v2/src/dispatch.rs b/lang-v2/src/dispatch.rs index 599a181b28..bfe7b155b1 100644 --- a/lang-v2/src/dispatch.rs +++ b/lang-v2/src/dispatch.rs @@ -1,6 +1,6 @@ use { crate::{ - context::{Bumps, Context}, + context::{Bumps, Context, MutMask}, cursor::{AccountBitvec, AccountCursor}, loader::AccountLoader, }, @@ -31,6 +31,16 @@ pub trait TryAccounts: Bumps + Sized { /// does today. const MUT_MASK: [u64; 4]; + /// True when [`Self::active_mut_mask`] can differ from [`Self::MUT_MASK`]. + /// This lets accounts without optional mutable fields keep the old static + /// mask path. + const HAS_DYNAMIC_MUT_MASK: bool; + + /// Runtime mutable-account mask for checks that happen after declared + /// accounts are loaded. This includes optional mutable accounts only when + /// they resolved to `Some`. + fn active_mut_mask(&self) -> [u64; 4]; + /// Parsed instruction args carried alongside validated accounts. /// Accounts structs without `#[instruction(...)]` use `()`. type IxArgs<'ix>; @@ -86,7 +96,19 @@ pub fn run_handler<'a, T: TryAccounts, R>( }; const _: () = assert!(pinocchio::MAX_TX_ACCOUNTS <= u8::MAX as usize); let remaining_num = (num_accounts - T::HEADER_SIZE) as u8; - let mut ctx = Context::new(program_id, ctx_accounts, bumps, cursor, remaining_num); + let mut_mask = if remaining_num != 0 && T::HAS_DYNAMIC_MUT_MASK { + MutMask::Dynamic(ctx_accounts.active_mut_mask()) + } else { + MutMask::Static(&T::MUT_MASK) + }; + let mut ctx = Context::new( + program_id, + ctx_accounts, + bumps, + cursor, + remaining_num, + mut_mask, + ); let result = handler(&mut ctx, ix_args)?; ctx.accounts.exit_accounts()?; Ok(result) diff --git a/lang-v2/src/lib.rs b/lang-v2/src/lib.rs index 92fc80a3c7..c9ea2ddda8 100644 --- a/lang-v2/src/lib.rs +++ b/lang-v2/src/lib.rs @@ -169,7 +169,7 @@ pub use { program, Accounts, InitSpace, }, bytemuck, - context::{Bumps, Context}, + context::{Bumps, Context, MutMask}, context_cpi::{invoke_signed_fixed, CpiContext}, cpi::{ create_account, create_account_signed, create_program_address, diff --git a/lang-v2/tests/cursor_and_remaining_accounts.rs b/lang-v2/tests/cursor_and_remaining_accounts.rs index 7cefb25a38..9330a42e50 100644 --- a/lang-v2/tests/cursor_and_remaining_accounts.rs +++ b/lang-v2/tests/cursor_and_remaining_accounts.rs @@ -23,7 +23,7 @@ use { anchor_lang_v2::{ cursor::{mut_mask_or_shifted, mut_mask_set_bit, AccountBitvec, AccountCursor}, testing::{AccountRecord, SbfInputBuffer}, - AccountViewCompat, Bumps, Context, + AccountViewCompat, Bumps, Context, MutMask, }, core::mem::MaybeUninit, pinocchio::account::AccountView, @@ -38,6 +38,11 @@ impl Bumps for DummyHeader { type Bumps = (); } +/// No declared mut fields — these tests don't exercise the +/// trailing-dup-vs-declared-mut check (that's covered by +/// `remaining_accounts_mut_check.rs`). +const EMPTY_MUT_MASK: &[u64; 4] = &[0, 0, 0, 0]; + fn unique_addr(i: u8) -> [u8; 32] { let mut a = [0u8; 32]; a[0] = i + 1; // avoid [0;32] which collides with the System program id. @@ -202,9 +207,10 @@ fn remaining_accounts_walks_trailing_region() { (), &mut cursor, /*remaining_num*/ 3, + MutMask::Static(EMPTY_MUT_MASK), ); - let remaining = ctx.remaining_accounts(); + let remaining = ctx.remaining_accounts().expect("walk"); assert_eq!(remaining.len(), 3); assert_eq!(remaining[0].address().to_bytes(), unique_addr(2)); assert_eq!(remaining[1].address().to_bytes(), unique_addr(3)); @@ -232,9 +238,10 @@ fn remaining_account_views_expose_compat_helpers() { (), &mut cursor, /*remaining_num*/ 2, + MutMask::Static(EMPTY_MUT_MASK), ); - let mut remaining = ctx.remaining_accounts(); + let mut remaining = ctx.remaining_accounts().expect("walk"); assert_eq!(remaining[0].key().to_bytes(), unique_addr(1)); assert!(!remaining[0].data_is_empty()); assert_eq!(remaining[0].try_data_len().unwrap(), 16); @@ -251,12 +258,18 @@ fn remaining_accounts_returns_empty_when_nothing_trails() { let _ = unsafe { cursor.walk_n(2) }; let program_id = Address::new_from_array([0x42; 32]); - let mut ctx: Context<'_, DummyHeader> = - Context::new(&program_id, DummyHeader, (), &mut cursor, 0); + let mut ctx: Context<'_, DummyHeader> = Context::new( + &program_id, + DummyHeader, + (), + &mut cursor, + 0, + MutMask::Static(EMPTY_MUT_MASK), + ); - assert!(ctx.remaining_accounts().is_empty()); + assert!(ctx.remaining_accounts().expect("walk").is_empty()); // Second call on empty — still empty, no cache bookkeeping bug. - assert!(ctx.remaining_accounts().is_empty()); + assert!(ctx.remaining_accounts().expect("walk").is_empty()); } #[test] @@ -278,10 +291,11 @@ fn remaining_accounts_caches_and_does_not_re_walk_cursor() { (), &mut cursor, /*remaining_num*/ 2, + MutMask::Static(EMPTY_MUT_MASK), ); - let first = ctx.remaining_accounts(); - let second = ctx.remaining_accounts(); + let first = ctx.remaining_accounts().expect("first walk"); + let second = ctx.remaining_accounts().expect("second walk"); // Structural equality via address, since AccountView is Copy and the // cache returns a clone each call. diff --git a/lang-v2/tests/remaining_accounts_mut_check.rs b/lang-v2/tests/remaining_accounts_mut_check.rs new file mode 100644 index 0000000000..5900af13e9 --- /dev/null +++ b/lang-v2/tests/remaining_accounts_mut_check.rs @@ -0,0 +1,264 @@ +//! Bug-3 fix: `Context::remaining_accounts()` re-checks `MUT_MASK`. +//! +//! ## The bug +//! +//! `run_handler` (`lang-v2/src/dispatch.rs`) performs exactly one +//! `duplicates.intersects(&T::MUT_MASK)` test against the bitvec the +//! `HEADER_SIZE` walk produced. `Context::remaining_accounts()` then +//! calls `cursor.next()` for each trailing account. `cursor.next()` +//! does detect dups and flips bits in the cursor's bitvec — but no +//! re-test of `MUT_MASK` happens. +//! +//! Attack: supply `HEADER_SIZE + 1` accounts where the trailing slot +//! aliases a declared mut slot. The handler's +//! `ctx.remaining_accounts()[0]` now points at the same +//! `RuntimeAccount` as `ctx.accounts.` without tripping +//! `ConstraintDuplicateMutableAccount`. +//! +//! ## The fix +//! +//! `Context::remaining_accounts()` re-intersects the cursor's live +//! bitvec against `MUT_MASK` after every `cursor.next()`. Because +//! `MUT_MASK`'s bits only cover declared field indices (0..HEADER_SIZE +//! for top-level structs), a trailing account's own index bit never +//! collides with the mask — the test only fires when the cursor +//! resolved a dup pointing at a declared mut slot. +//! +//! These tests drive `Context` directly (a `Bumps` impl for a +//! stand-in accounts struct plus a fake `mut_mask`) rather than going +//! through `#[derive(Accounts)]` — that keeps the test focused on +//! the `Context::remaining_accounts` control flow and the cursor's +//! bitvec exposure, which are the bits that actually changed. + +extern crate alloc; + +use { + anchor_lang_v2::{ + testing::{AccountRecord, SbfInputBuffer}, + AccountCursor, AccountView, Bumps, Context, MutMask, + }, + core::mem::MaybeUninit, + solana_address::Address, + solana_program_error::ProgramError, +}; + +/// Trivial accounts struct with no real declared fields — the test +/// constructs `Context` by hand, so the generic bound is all we need. +struct NoAccounts; +impl Bumps for NoAccounts { + type Bumps = (); +} + +const PROGRAM_ID: [u8; 32] = [0x42; 32]; +const MAX_ACCOUNTS: usize = 8; + +/// `ProgramError::Custom(2005)` — `ErrorCode::ConstraintDuplicateMutableAccount` +/// (`lang-v2/src/lib.rs`). +const DUP_MUT_ERROR: u32 = 2005; + +/// Build a fresh non-dup record with a distinct address. +fn fresh(tag: u8) -> AccountRecord { + AccountRecord::NonDup { + address: [tag; 32], + owner: PROGRAM_ID, + lamports: 100, + is_signer: false, + is_writable: true, + executable: false, + data_len: 0, + } +} + +/// Run one `Context::remaining_accounts()` invocation over a cursor +/// built from `records`, treating the first `header_size` accounts as +/// the declared region and the remainder as trailing. `mut_mask` +/// bitmap covers the declared region (as `T::MUT_MASK` would). +fn run_with( + records: &[AccountRecord], + header_size: usize, + mut_mask: &'static [u64; 4], +) -> Result, ProgramError> { + let mut buf = SbfInputBuffer::build(records); + let mut lookup: [MaybeUninit; MAX_ACCOUNTS] = + [const { MaybeUninit::uninit() }; MAX_ACCOUNTS]; + + let program_id = Address::new_from_array(PROGRAM_ID); + // SAFETY: buf outlives cursor; lookup is aligned + sized for MAX_ACCOUNTS. + let mut cursor = + unsafe { AccountCursor::new(buf.as_mut_ptr(), lookup.as_mut_ptr() as *mut AccountView) }; + + // Walk the declared region exactly as `run_handler` would. Check + // `MUT_MASK` once for parity with the dispatcher. + let (_views, dups) = unsafe { cursor.walk_n(header_size) }; + if let Some(d) = dups { + if d.intersects(mut_mask) { + return Err(solana_program_error::ProgramError::Custom(DUP_MUT_ERROR)); + } + } + + let remaining_num = (records.len() - header_size) as u8; + let mut ctx: Context = Context::new( + &program_id, + NoAccounts, + (), + &mut cursor, + remaining_num, + MutMask::Static(mut_mask), + ); + ctx.remaining_accounts() +} + +// --------------------------------------------------------------------------- +// Bug reproducer + negative controls +// --------------------------------------------------------------------------- + +/// MUT_MASK marking slot 0 as mut. Mirrors what `#[derive(Accounts)]` +/// emits for `{ #[account(mut)] a: UncheckedAccount }`. +const MUT_MASK_SLOT0: &[u64; 4] = &[0b1, 0, 0, 0]; + +/// MUT_MASK marking slot 1 as mut (slot 0 is non-mut). +const MUT_MASK_SLOT1: &[u64; 4] = &[0b10, 0, 0, 0]; + +/// Empty MUT_MASK — no declared mut fields. +const MUT_MASK_NONE: &[u64; 4] = &[0, 0, 0, 0]; + +#[test] +fn trailing_account_aliasing_declared_mut_is_rejected() { + // Layout: [ mut declared, trailing-dup-of-slot-0 ] + // Declared region has 1 slot, marked mut. Trailing slot's dup + // index resolves to slot 0 → alias of a mut declared account. + let records = [fresh(1), AccountRecord::Dup { index: 0 }]; + let result = run_with(&records, /*header_size*/ 1, MUT_MASK_SLOT0); + match result { + Err(ProgramError::Custom(code)) if code == DUP_MUT_ERROR => {} + other => panic!( + "expected Err(ConstraintDuplicateMutableAccount), got {:?}", + other.map(|v| v.len()) + ), + } +} + +#[test] +fn trailing_account_aliasing_non_mut_declared_is_allowed() { + // Declared region has 2 slots, only slot 1 is mut. Trailing slot + // dups slot 0 (non-mut) → must be allowed. + let records = [fresh(1), fresh(2), AccountRecord::Dup { index: 0 }]; + let result = run_with(&records, /*header_size*/ 2, MUT_MASK_SLOT1); + match result { + Ok(v) => assert_eq!(v.len(), 1), + Err(e) => panic!("expected Ok, got Err({:?})", e), + } +} + +#[test] +fn two_trailing_dups_of_each_other_are_allowed() { + // Declared region has 1 slot, non-mut. Two trailing slots: the + // second dups the first trailing slot. Neither aliases a + // declared mut, so the mut-mask check must not fire. + let records = [fresh(1), fresh(2), AccountRecord::Dup { index: 1 }]; + let result = run_with(&records, /*header_size*/ 1, MUT_MASK_NONE); + match result { + Ok(v) => assert_eq!(v.len(), 2), + Err(e) => panic!("expected Ok, got Err({:?})", e), + } +} + +#[test] +fn fresh_trailing_accounts_are_allowed() { + // Regression: all-fresh trailing accounts must pass even when + // slot 0 is declared mut. + let records = [fresh(1), fresh(2), fresh(3)]; + let result = run_with(&records, /*header_size*/ 1, MUT_MASK_SLOT0); + match result { + Ok(v) => assert_eq!(v.len(), 2), + Err(e) => panic!("expected Ok, got Err({:?})", e), + } +} + +#[test] +fn trailing_account_aliasing_nested_mut_is_rejected() { + // Simulate `Parent { a: UncheckedAccount, inner: Nested }` + // where `Inner { #[account(mut)] b }`. Parent's `MUT_MASK` has + // bit 1 set (slot 0 = parent's plain `a`; slot 1 = Inner's mut + // `b` after the nested-shift). A trailing account aliasing slot 1 + // must still be caught — this is the "Nested children merge into + // the top-level mask" case. + let mut mask = [0u64; 4]; + mask[0] = 0b10; + // Leaked to get 'static; test-only. + let mask_ref: &'static [u64; 4] = alloc::boxed::Box::leak(alloc::boxed::Box::new(mask)); + + let records = [fresh(1), fresh(2), AccountRecord::Dup { index: 1 }]; + let result = run_with(&records, /*header_size*/ 2, mask_ref); + match result { + Err(ProgramError::Custom(code)) if code == DUP_MUT_ERROR => {} + other => panic!( + "expected Err(ConstraintDuplicateMutableAccount), got {:?}", + other.map(|v| v.len()) + ), + } +} + +#[test] +fn remaining_accounts_error_is_cached_on_failure() { + // Before the fix, Err left `remaining_cache` unset, so a repeat + // call would re-enter the walk loop and call `cursor.next()` again + // — past the remaining region on an unsafe cursor. After the fix, + // the error is cached and replayed; the cursor advances exactly + // once per instruction regardless of how many times the handler + // calls `remaining_accounts()`. + let records = [fresh(1), AccountRecord::Dup { index: 0 }]; + let mut buf = SbfInputBuffer::build(&records); + let mut lookup: [MaybeUninit; MAX_ACCOUNTS] = + [const { MaybeUninit::uninit() }; MAX_ACCOUNTS]; + let program_id = Address::new_from_array(PROGRAM_ID); + let mut cursor = + unsafe { AccountCursor::new(buf.as_mut_ptr(), lookup.as_mut_ptr() as *mut AccountView) }; + let (_views, _dups) = unsafe { cursor.walk_n(1) }; + let mut ctx: Context = Context::new( + &program_id, + NoAccounts, + (), + &mut cursor, + 1, + MutMask::Static(MUT_MASK_SLOT0), + ); + + for _ in 0..5 { + match ctx.remaining_accounts() { + Err(ProgramError::Custom(code)) => assert_eq!(code, DUP_MUT_ERROR), + other => panic!( + "expected cached Err(ConstraintDuplicateMutableAccount), got {:?}", + other.map(|v| v.len()) + ), + } + } +} + +#[test] +fn remaining_accounts_result_is_cached_on_success() { + // Regression on the cache path: two successful calls must return + // equal-length vecs without re-walking the cursor (second walk + // would read past the end). + let records = [fresh(1), fresh(2), fresh(3)]; + let mut buf = SbfInputBuffer::build(&records); + let mut lookup: [MaybeUninit; MAX_ACCOUNTS] = + [const { MaybeUninit::uninit() }; MAX_ACCOUNTS]; + let program_id = Address::new_from_array(PROGRAM_ID); + let mut cursor = + unsafe { AccountCursor::new(buf.as_mut_ptr(), lookup.as_mut_ptr() as *mut AccountView) }; + let (_views, _dups) = unsafe { cursor.walk_n(1) }; + let mut ctx: Context = Context::new( + &program_id, + NoAccounts, + (), + &mut cursor, + 2, + MutMask::Static(MUT_MASK_SLOT0), + ); + + let first = ctx.remaining_accounts().expect("first call"); + let second = ctx.remaining_accounts().expect("second call"); + assert_eq!(first.len(), 2); + assert_eq!(second.len(), 2); +} diff --git a/tests-v2/programs/dispatch-remaining/src/lib.rs b/tests-v2/programs/dispatch-remaining/src/lib.rs index 2447749377..d6ee5f88eb 100644 --- a/tests-v2/programs/dispatch-remaining/src/lib.rs +++ b/tests-v2/programs/dispatch-remaining/src/lib.rs @@ -26,7 +26,7 @@ pub mod dispatch_remaining { #[discrim = 2] pub fn read_remaining_once(ctx: &mut Context, expected_count: u8) -> Result<()> { - let remaining = ctx.remaining_accounts(); + let remaining = ctx.remaining_accounts()?; if remaining.len() != expected_count as usize { return Err(ProgramError::InvalidArgument.into()); } @@ -39,8 +39,8 @@ pub mod dispatch_remaining { #[discrim = 3] pub fn read_remaining_twice(ctx: &mut Context) -> Result<()> { - let first = ctx.remaining_accounts(); - let second = ctx.remaining_accounts(); + let first = ctx.remaining_accounts()?; + let second = ctx.remaining_accounts()?; if first.len() != 2 || second.len() != 2 { return Err(ProgramError::InvalidArgument.into()); } @@ -56,7 +56,7 @@ pub mod dispatch_remaining { value: u64, ) -> Result<()> { ctx.accounts.counter.value = value; - let remaining = ctx.remaining_accounts(); + let remaining = ctx.remaining_accounts()?; if remaining.len() != 1 { return Err(ProgramError::InvalidArgument.into()); } @@ -70,6 +70,21 @@ pub mod dispatch_remaining { } Ok(()) } + + #[discrim = 6] + pub fn optional_mut_then_read_remaining( + ctx: &mut Context, + expected_count: u8, + ) -> Result<()> { + if let Some(counter) = ctx.accounts.counter.as_mut() { + counter.value = counter.value.saturating_add(1); + } + let remaining = ctx.remaining_accounts()?; + if remaining.len() != expected_count as usize { + return Err(ProgramError::InvalidArgument.into()); + } + Ok(()) + } } #[derive(Accounts)] @@ -103,3 +118,9 @@ pub struct MutateThenReadRemaining { #[account(mut)] pub counter: Account, } + +#[derive(Accounts)] +pub struct OptionalMutThenReadRemaining { + #[account(mut)] + pub counter: Option>, +} diff --git a/tests-v2/tests/dispatch_remaining.rs b/tests-v2/tests/dispatch_remaining.rs index ac113e156c..9921d00eb0 100644 --- a/tests-v2/tests/dispatch_remaining.rs +++ b/tests-v2/tests/dispatch_remaining.rs @@ -22,6 +22,8 @@ fn counter_pda() -> Pubkey { Pubkey::find_program_address(&[b"counter"], &program_id()).0 } +const DUPLICATE_MUT_ERROR: u32 = 2005; + fn setup() -> (LiteSVM, Keypair) { let test_dir = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")); let deploy_dir = test_dir.join("target/deploy"); @@ -72,6 +74,19 @@ fn counter_value(svm: &LiteSVM, counter: Pubkey) -> u64 { u64::from_le_bytes(account.data[8..16].try_into().unwrap()) } +#[track_caller] +fn assert_custom_error(result: &TransactionResult, expected: u32) { + let failure = match result { + Ok(_) => panic!("expected transaction to fail with Custom({expected}), got success"), + Err(f) => f, + }; + let rendered = format!("{:?}", failure.err); + assert!( + rendered.contains(&format!("Custom({expected})")), + "expected Custom({expected}), got: {rendered}", + ); +} + #[test] fn too_few_declared_accounts_fails_before_handler() { let (mut svm, payer) = setup(); @@ -90,6 +105,126 @@ fn too_few_declared_accounts_fails_before_handler() { ); } +#[test] +fn remaining_duplicate_of_declared_mut_is_rejected() { + let (mut svm, payer) = setup(); + let counter = initialize(&mut svm, &payer); + let data = dispatch_remaining::instruction::MutateThenReadRemaining { value: 123 }.data(); + + let result = call_raw( + &mut svm, + &payer, + data, + vec![ + AccountMeta::new(counter, false), + AccountMeta::new(counter, false), + ], + ); + assert_custom_error(&result, DUPLICATE_MUT_ERROR); +} + +#[test] +fn remaining_duplicate_of_declared_readonly_is_allowed() { + let (mut svm, payer) = setup(); + let counter = initialize(&mut svm, &payer); + let data = dispatch_remaining::instruction::ReadRemainingOnce { expected_count: 1 }.data(); + + send_instruction( + &mut svm, + program_id(), + data, + vec![ + AccountMeta::new_readonly(counter, false), + AccountMeta::new_readonly(counter, false), + ], + &payer, + &[], + ) + .expect("readonly declared account may be duplicated through remaining accounts"); +} + +#[test] +fn remaining_duplicate_of_trailing_account_is_allowed() { + let (mut svm, payer) = setup(); + let counter = initialize(&mut svm, &payer); + let marker = Pubkey::new_unique(); + + send_instruction( + &mut svm, + program_id(), + vec![3], + vec![ + AccountMeta::new_readonly(counter, false), + AccountMeta::new_readonly(marker, false), + AccountMeta::new_readonly(marker, false), + ], + &payer, + &[], + ) + .expect("remaining accounts may duplicate each other when no declared mut slot aliases"); +} + +#[test] +fn remaining_duplicate_of_optional_some_mut_is_rejected() { + let (mut svm, payer) = setup(); + let counter = initialize(&mut svm, &payer); + let data = + dispatch_remaining::instruction::OptionalMutThenReadRemaining { expected_count: 1 }.data(); + + let result = call_raw( + &mut svm, + &payer, + data, + vec![ + AccountMeta::new(counter, false), + AccountMeta::new(counter, false), + ], + ); + assert_custom_error(&result, DUPLICATE_MUT_ERROR); +} + +#[test] +fn remaining_duplicate_of_optional_none_sentinel_is_allowed() { + let (mut svm, payer) = setup(); + let data = + dispatch_remaining::instruction::OptionalMutThenReadRemaining { expected_count: 1 }.data(); + + send_instruction( + &mut svm, + program_id(), + data, + vec![ + AccountMeta::new(program_id(), false), + AccountMeta::new(program_id(), false), + ], + &payer, + &[], + ) + .expect("None sentinel should not count as an active optional mut account"); +} + +#[test] +fn optional_some_mut_with_distinct_remaining_is_allowed() { + let (mut svm, payer) = setup(); + let counter = initialize(&mut svm, &payer); + let marker = Pubkey::new_unique(); + let data = + dispatch_remaining::instruction::OptionalMutThenReadRemaining { expected_count: 1 }.data(); + + send_instruction( + &mut svm, + program_id(), + data, + vec![ + AccountMeta::new(counter, false), + AccountMeta::new_readonly(marker, false), + ], + &payer, + &[], + ) + .expect("optional Some mut account should allow a distinct remaining account"); +} + #[test] fn extra_accounts_are_available_as_trailing_remaining_region() { let (mut svm, payer) = setup();