diff --git a/lang/src/accounts/account_loader.rs b/lang/src/accounts/account_loader.rs index 20ada8f406..aa215331c3 100644 --- a/lang/src/accounts/account_loader.rs +++ b/lang/src/accounts/account_loader.rs @@ -165,8 +165,13 @@ impl<'info, T: ZeroCopy + Owner> AccountLoader<'info, T> { return Err(ErrorCode::AccountDiscriminatorMismatch.into()); } + let end = required_zero_copy_len::(disc.len())?; + if data.len() < end { + return Err(ErrorCode::AccountDidNotDeserialize.into()); + } + Ok(Ref::map(data, |data| { - bytemuck::from_bytes(&data[disc.len()..mem::size_of::() + disc.len()]) + bytemuck::from_bytes(&data[disc.len()..end]) })) } @@ -189,10 +194,13 @@ impl<'info, T: ZeroCopy + Owner> AccountLoader<'info, T> { return Err(ErrorCode::AccountDiscriminatorMismatch.into()); } + let end = required_zero_copy_len::(disc.len())?; + if data.len() < end { + return Err(ErrorCode::AccountDidNotDeserialize.into()); + } + Ok(RefMut::map(data, |data| { - bytemuck::from_bytes_mut( - &mut data.deref_mut()[disc.len()..mem::size_of::() + disc.len()], - ) + bytemuck::from_bytes_mut(&mut data.deref_mut()[disc.len()..end]) })) } @@ -209,20 +217,35 @@ impl<'info, T: ZeroCopy + Owner> AccountLoader<'info, T> { // The discriminator should be zero, since we're initializing. let disc = T::DISCRIMINATOR; + if data.len() < disc.len() { + return Err(ErrorCode::AccountDiscriminatorNotFound.into()); + } let given_disc = &data[..disc.len()]; let has_disc = given_disc.iter().any(|b| *b != 0); if has_disc { return Err(ErrorCode::AccountDiscriminatorAlreadySet.into()); } + let end = required_zero_copy_len::(disc.len())?; + if data.len() < end { + return Err(ErrorCode::AccountDidNotDeserialize.into()); + } + Ok(RefMut::map(data, |data| { - bytemuck::from_bytes_mut( - &mut data.deref_mut()[disc.len()..mem::size_of::() + disc.len()], - ) + bytemuck::from_bytes_mut(&mut data.deref_mut()[disc.len()..end]) })) } } +/// Returns `disc_len + size_of::()`, mapping arithmetic overflow to a +/// structured `AccountDidNotDeserialize` error rather than panicking. +#[inline] +fn required_zero_copy_len(disc_len: usize) -> Result { + disc_len + .checked_add(mem::size_of::()) + .ok_or_else(|| ErrorCode::AccountDidNotDeserialize.into()) +} + impl<'info, B, T: ZeroCopy + Owner> Accounts<'info, B> for AccountLoader<'info, T> { #[inline(never)] fn try_accounts( diff --git a/lang/syn/src/codegen/accounts/constraints.rs b/lang/syn/src/codegen/accounts/constraints.rs index 4eb69fd734..1173414b2e 100644 --- a/lang/syn/src/codegen/accounts/constraints.rs +++ b/lang/syn/src/codegen/accounts/constraints.rs @@ -276,6 +276,13 @@ pub fn generate_constraint_zeroed( quote! { let #field: #ty_decl = { let mut __data: &[u8] = &#field.try_borrow_data()?; + if __data.len() < #discriminator.len() { + return Err( + anchor_lang::error::Error::from( + anchor_lang::error::ErrorCode::AccountDiscriminatorNotFound + ).with_account_name(#name_str) + ); + } let __disc = &__data[..#discriminator.len()]; let __has_disc = __disc.iter().any(|b| *b != 0); if __has_disc { diff --git a/lang/tests/account_loader_truncation.rs b/lang/tests/account_loader_truncation.rs new file mode 100644 index 0000000000..4c607ce9e0 --- /dev/null +++ b/lang/tests/account_loader_truncation.rs @@ -0,0 +1,145 @@ +//! Regression tests for `AccountLoader::{load, load_mut, load_init}` panicking +//! on accounts that pass the discriminator length check but whose data is +//! truncated before the end of the zero-copy body. +//! +//! Before the fix, the three accessors sliced +//! data[disc.len()..disc.len() + size_of::()] +//! without verifying the upper bound, which caused an index-out-of-bounds +//! panic (surfaced to clients as `Program failed to complete`) instead of a +//! structured `AnchorError`. `load_init` additionally panicked on the +//! discriminator slice itself when the account was shorter than the +//! discriminator. +//! +//! Tracking: solana-foundation/anchor#4509 + +use { + anchor_lang::{error::ErrorCode, prelude::*}, + std::mem, +}; + +declare_id!("Fg6PaFpoGXkYsidMpWTK6W2BeZ7FEfcYkg476zPFsLnS"); + +#[account(zero_copy)] +#[derive(Default, Debug)] +pub struct Foo { + pub a: u64, + pub b: u64, + pub c: u64, +} + +/// Build an `AccountInfo` whose data is exactly `len` bytes long. If +/// `with_disc` is true the discriminator prefix is written; otherwise the +/// buffer is zeroed. +fn make_owned_account_bytes(len: usize, with_disc: bool) -> Vec { + let mut data = vec![0u8; len]; + if with_disc { + let disc = Foo::DISCRIMINATOR; + let n = std::cmp::min(disc.len(), len); + data[..n].copy_from_slice(&disc[..n]); + } + data +} + +#[test] +fn load_returns_error_when_data_shorter_than_zero_copy_body() { + // Discriminator-sized but truncated body — used to panic. + let mut data = make_owned_account_bytes(Foo::DISCRIMINATOR.len(), true); + let mut lamports: u64 = 1; + let owner: Pubkey = crate::ID; + let key: Pubkey = Pubkey::new_unique(); + + let acc_info: AccountInfo<'_> = + AccountInfo::new(&key, false, false, &mut lamports, &mut data, &owner, false); + + let loader: AccountLoader<'_, Foo> = AccountLoader::try_from(&acc_info).unwrap(); + let err = loader.load().unwrap_err(); + assert_eq!( + err, + anchor_lang::error::Error::from(ErrorCode::AccountDidNotDeserialize) + ); +} + +#[test] +fn load_mut_returns_error_when_data_shorter_than_zero_copy_body() { + // One byte short of the full zero-copy body — used to panic. + let needed = Foo::DISCRIMINATOR.len() + mem::size_of::(); + let mut data = make_owned_account_bytes(needed - 1, true); + let mut lamports: u64 = 1; + let owner: Pubkey = crate::ID; + let key: Pubkey = Pubkey::new_unique(); + + let acc_info: AccountInfo<'_> = + AccountInfo::new(&key, false, true, &mut lamports, &mut data, &owner, false); + + let loader: AccountLoader<'_, Foo> = AccountLoader::try_from(&acc_info).unwrap(); + let err = loader.load_mut().unwrap_err(); + assert_eq!( + err, + anchor_lang::error::Error::from(ErrorCode::AccountDidNotDeserialize) + ); +} + +#[test] +fn load_init_returns_error_when_data_shorter_than_zero_copy_body() { + // Discriminator-region is zero (as init expects) but body is missing. + let mut data = make_owned_account_bytes(Foo::DISCRIMINATOR.len(), false); + let mut lamports: u64 = 1; + let owner: Pubkey = crate::ID; + let key: Pubkey = Pubkey::new_unique(); + + let acc_info: AccountInfo<'_> = + AccountInfo::new(&key, false, true, &mut lamports, &mut data, &owner, false); + + let loader: AccountLoader<'_, Foo> = + AccountLoader::try_from_unchecked(&crate::ID, &acc_info).unwrap(); + let err = loader.load_init().unwrap_err(); + assert_eq!( + err, + anchor_lang::error::Error::from(ErrorCode::AccountDidNotDeserialize) + ); +} + +#[test] +fn load_init_returns_error_when_data_shorter_than_discriminator() { + // Below `disc.len()` — `load_init` previously sliced without bounds-checking + // the discriminator prefix and panicked. + let mut data = vec![0u8; Foo::DISCRIMINATOR.len() - 1]; + let mut lamports: u64 = 1; + let owner: Pubkey = crate::ID; + let key: Pubkey = Pubkey::new_unique(); + + let acc_info: AccountInfo<'_> = + AccountInfo::new(&key, false, true, &mut lamports, &mut data, &owner, false); + + let loader: AccountLoader<'_, Foo> = + AccountLoader::try_from_unchecked(&crate::ID, &acc_info).unwrap(); + let err = loader.load_init().unwrap_err(); + assert_eq!( + err, + anchor_lang::error::Error::from(ErrorCode::AccountDiscriminatorNotFound) + ); +} + +#[test] +fn load_succeeds_on_exactly_sized_account() { + // Sanity: the happy path is unaffected by the new bound check. + let needed = Foo::DISCRIMINATOR.len() + mem::size_of::(); + let mut data = make_owned_account_bytes(needed, true); + let mut lamports: u64 = 1; + let owner: Pubkey = crate::ID; + let key: Pubkey = Pubkey::new_unique(); + + let acc_info: AccountInfo<'_> = + AccountInfo::new(&key, false, true, &mut lamports, &mut data, &owner, false); + + let loader: AccountLoader<'_, Foo> = AccountLoader::try_from(&acc_info).unwrap(); + { + let foo = loader.load().unwrap(); + assert_eq!(foo.a, 0); + } + { + let mut foo = loader.load_mut().unwrap(); + foo.a = 7; + } + assert_eq!(loader.load().unwrap().a, 7); +}