Skip to content

SIMD-406: Restrict to 255 the number of accounts in instructions for transactions v0 and legacy #520

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
LucasSte wants to merge 1 commit into from
Closed

SIMD-406: Restrict to 255 the number of accounts in instructions for transactions v0 and legacy #520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions message/src/compiled_instruction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@ use serde_derive::{Deserialize, Serialize};
use solana_frozen_abi_macro::AbiExample;
#[cfg(feature = "wincode")]
use wincode::{containers, len::ShortU16Len, SchemaRead, SchemaWrite};
use {solana_address::Address, solana_sanitize::Sanitize};
use {
solana_address::Address,
solana_sanitize::{Sanitize, SanitizeError},
};

/// A compact encoding of an instruction.
///
Expand Down Expand Up @@ -34,7 +37,14 @@ pub struct CompiledInstruction {
pub data: Vec<u8>,
}

impl Sanitize for CompiledInstruction {}
impl Sanitize for CompiledInstruction {
fn sanitize(&self, limit_ix_accounts_simd_406: bool) -> Result<(), SanitizeError> {
if limit_ix_accounts_simd_406 && self.accounts.len() > 255 {
return Err(SanitizeError::ValueOutOfBounds);
}
Ok(())
}
}

impl CompiledInstruction {
#[cfg(feature = "bincode")]
Expand Down
31 changes: 27 additions & 4 deletions message/src/legacy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ pub struct Message {
}

impl Sanitize for Message {
fn sanitize(&self) -> std::result::Result<(), SanitizeError> {
fn sanitize(&self, limit_ix_accounts_simd_406: bool) -> std::result::Result<(), SanitizeError> {
// signing area and read-only non-signing area should not overlap
if self.header.num_required_signatures as usize
+ self.header.num_readonly_unsigned_accounts as usize
Expand Down Expand Up @@ -131,9 +131,9 @@ impl Sanitize for Message {
}
}
}
self.account_keys.sanitize()?;
self.recent_blockhash.sanitize()?;
self.instructions.sanitize()?;
self.account_keys.sanitize(limit_ix_accounts_simd_406)?;
self.recent_blockhash.sanitize(limit_ix_accounts_simd_406)?;
self.instructions.sanitize(limit_ix_accounts_simd_406)?;
Ok(())
}
}
Expand Down Expand Up @@ -893,4 +893,27 @@ mod tests {
assert!(!message5.is_writable_index(0));
assert!(!message5.is_writable_index(1));
}

#[test]
fn test_more_than_255_accounts_in_ix() {
let mut accounts: Vec<u8> = (0..255).collect();
accounts.push(5);
accounts.push(4);
let message = crate::v0::Message {
account_keys: vec![Address::new_unique(); 256],
instructions: vec![CompiledInstruction::new_from_raw_parts(
4,
Vec::new(),
accounts,
)],
header: MessageHeader {
num_required_signatures: 2,
num_readonly_signed_accounts: 1,
num_readonly_unsigned_accounts: 0,
},
..crate::v0::Message::default()
};
let result = message.sanitize(true);
assert_eq!(result.err().unwrap(), SanitizeError::ValueOutOfBounds);
}
}
9 changes: 8 additions & 1 deletion message/src/sanitized.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,9 @@ impl SanitizedMessage {
pub fn try_from_legacy_message(
message: legacy::Message,
reserved_account_keys: &HashSet<Address>,
limit_ix_accounts_simd_406: bool,
) -> Result<Self, SanitizeMessageError> {
message.sanitize()?;
message.sanitize(limit_ix_accounts_simd_406)?;
Ok(Self::Legacy(LegacyMessage::new(
message,
reserved_account_keys,
Expand Down Expand Up @@ -456,6 +457,7 @@ mod tests {
SanitizedMessage::try_from_legacy_message(
legacy_message_with_no_signers,
&HashSet::default(),
true,
)
.err(),
Some(SanitizeMessageError::IndexOutOfBounds),
Expand All @@ -482,6 +484,7 @@ mod tests {
..legacy::Message::default()
},
&HashSet::default(),
true,
)
.unwrap();

Expand Down Expand Up @@ -529,6 +532,7 @@ mod tests {
instructions,
),
&HashSet::default(),
true,
)
.unwrap();

Expand Down Expand Up @@ -571,6 +575,7 @@ mod tests {
..legacy::Message::default()
},
&HashSet::default(),
true,
)
.unwrap();
match legacy_message {
Expand Down Expand Up @@ -655,6 +660,7 @@ mod tests {
],
),
&HashSet::new(),
true,
)
.unwrap();

Expand Down Expand Up @@ -688,6 +694,7 @@ mod tests {
..legacy::Message::default()
},
&HashSet::default(),
true,
)
.unwrap();
assert_eq!(legacy_message.static_account_keys(), &keys);
Expand Down
6 changes: 3 additions & 3 deletions message/src/versions/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,10 @@ pub enum VersionedMessage {
}

impl VersionedMessage {
pub fn sanitize(&self) -> Result<(), SanitizeError> {
pub fn sanitize(&self, limit_ix_accounts_simd_406: bool) -> Result<(), SanitizeError> {
match self {
Self::Legacy(message) => message.sanitize(),
Self::V0(message) => message.sanitize(),
Self::Legacy(message) => message.sanitize(limit_ix_accounts_simd_406),
Self::V0(message) => message.sanitize(limit_ix_accounts_simd_406),
}
}

Expand Down
9 changes: 6 additions & 3 deletions message/src/versions/sanitized.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,16 @@ pub struct SanitizedVersionedMessage {
impl TryFrom<VersionedMessage> for SanitizedVersionedMessage {
type Error = SanitizeError;
fn try_from(message: VersionedMessage) -> Result<Self, Self::Error> {
Self::try_new(message)
Self::try_new(message, true)
}
}

impl SanitizedVersionedMessage {
pub fn try_new(message: VersionedMessage) -> Result<Self, SanitizeError> {
message.sanitize()?;
pub fn try_new(
message: VersionedMessage,
limit_ix_accounts_simd_406: bool,
) -> Result<Self, SanitizeError> {
message.sanitize(limit_ix_accounts_simd_406)?;
Ok(Self { message })
}

Expand Down
56 changes: 41 additions & 15 deletions message/src/versions/v0/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ pub struct Message {

impl Message {
/// Sanitize message fields and compiled instruction indexes
pub fn sanitize(&self) -> Result<(), SanitizeError> {
pub fn sanitize(&self, limit_ix_accounts_simd_406: bool) -> Result<(), SanitizeError> {
let num_static_account_keys = self.account_keys.len();
if usize::from(self.header.num_required_signatures)
.saturating_add(usize::from(self.header.num_readonly_unsigned_accounts))
Expand Down Expand Up @@ -187,6 +187,9 @@ impl Message {
return Err(SanitizeError::IndexOutOfBounds);
}
}
if limit_ix_accounts_simd_406 && ci.accounts.len() > 255 {
return Err(SanitizeError::ValueOutOfBounds);
}
}

Ok(())
Expand Down Expand Up @@ -407,7 +410,7 @@ mod tests {
account_keys: vec![Address::new_unique()],
..Message::default()
}
.sanitize()
.sanitize(true)
.is_ok());
}

Expand All @@ -426,7 +429,7 @@ mod tests {
}],
..Message::default()
}
.sanitize()
.sanitize(true)
.is_ok());
}

Expand All @@ -445,7 +448,7 @@ mod tests {
}],
..Message::default()
}
.sanitize()
.sanitize(true)
.is_ok());
}

Expand All @@ -470,7 +473,7 @@ mod tests {
..Message::default()
};

assert!(message.sanitize().is_err());
assert!(message.sanitize(true).is_err());
}

#[test]
Expand All @@ -493,7 +496,7 @@ mod tests {
}],
..Message::default()
}
.sanitize()
.sanitize(true)
.is_ok());
}

Expand All @@ -504,7 +507,7 @@ mod tests {
account_keys: vec![Address::new_unique()],
..Message::default()
}
.sanitize()
.sanitize(true)
.is_err());
}

Expand All @@ -519,7 +522,7 @@ mod tests {
account_keys: vec![Address::new_unique()],
..Message::default()
}
.sanitize()
.sanitize(true)
.is_err());
}

Expand All @@ -538,7 +541,7 @@ mod tests {
}],
..Message::default()
}
.sanitize()
.sanitize(true)
.is_err());
}

Expand All @@ -552,7 +555,7 @@ mod tests {
account_keys: (0..=u8::MAX).map(|_| Address::new_unique()).collect(),
..Message::default()
}
.sanitize()
.sanitize(true)
.is_ok());
}

Expand All @@ -566,7 +569,7 @@ mod tests {
account_keys: (0..=256).map(|_| Address::new_unique()).collect(),
..Message::default()
}
.sanitize()
.sanitize(true)
.is_err());
}

Expand All @@ -585,7 +588,7 @@ mod tests {
}],
..Message::default()
}
.sanitize()
.sanitize(true)
.is_ok());
}

Expand All @@ -604,7 +607,7 @@ mod tests {
}],
..Message::default()
}
.sanitize()
.sanitize(true)
.is_err());
}

Expand All @@ -629,7 +632,7 @@ mod tests {
..Message::default()
};

assert!(message.sanitize().is_err());
assert!(message.sanitize(true).is_err());
}

#[test]
Expand All @@ -652,7 +655,7 @@ mod tests {
}],
..Message::default()
}
.sanitize()
.sanitize(true)
.is_err());
}

Expand Down Expand Up @@ -788,4 +791,27 @@ mod tests {
assert!(!message.is_account_maybe_reserved(3, None));
assert!(!message.is_account_maybe_reserved(4, None));
}

#[test]
fn test_more_than_255_accounts_in_ix() {
let mut accounts: Vec<u8> = (0..255).collect();
accounts.push(5);
accounts.push(4);
let message = Message {
account_keys: vec![Address::new_unique(); 256],
instructions: vec![CompiledInstruction::new_from_raw_parts(
4,
Vec::new(),
accounts,
)],
header: MessageHeader {
num_required_signatures: 2,
num_readonly_signed_accounts: 1,
num_readonly_unsigned_accounts: 0,
},
..Message::default()
};
let result = message.sanitize(true);
assert_eq!(result.err().unwrap(), SanitizeError::ValueOutOfBounds);
}
}
6 changes: 3 additions & 3 deletions sanitize/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,15 @@ impl fmt::Display for SanitizeError {
/// - All index values are in range.
/// - All values are within their static max/min bounds.
pub trait Sanitize {
fn sanitize(&self) -> Result<(), SanitizeError> {
fn sanitize(&self, _limit_ix_accounts_simd_406: bool) -> Result<(), SanitizeError> {
Copy link
Copy Markdown
Contributor

@jstarry jstarry Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of things in the agave repo that implement this trait so I don't think it's a good idea to change this trait method. Check out anza-xyz/agave#8182 for another approach that doesn't involve changing the Sanitize trait. After these features are activated we can update the Sanitize implementation in the SDK without a boolean for the feature.

Copy link
Copy Markdown
Contributor Author

@LucasSte LucasSte Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the hint. I'll follow an approach similar to the PR you pointed.

Ok(())
}
}

impl<T: Sanitize> Sanitize for [T] {
fn sanitize(&self) -> Result<(), SanitizeError> {
fn sanitize(&self, limit_ix_accounts_simd_406: bool) -> Result<(), SanitizeError> {
for x in self.iter() {
x.sanitize()?;
x.sanitize(limit_ix_accounts_simd_406)?;
}
Ok(())
}
Expand Down
3 changes: 3 additions & 0 deletions sdk/benches/serialize_instructions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ fn bench_construct_instructions_data(b: &mut Bencher) {
let message = SanitizedMessage::try_from_legacy_message(
Message::new(&instructions, Some(&Pubkey::new_unique())),
&HashSet::default(),
true,
)
.unwrap();
b.iter(|| {
Expand All @@ -56,6 +57,7 @@ fn bench_manual_instruction_deserialize(b: &mut Bencher) {
let message = SanitizedMessage::try_from_legacy_message(
Message::new(&instructions, Some(&Pubkey::new_unique())),
&HashSet::default(),
true,
)
.unwrap();
let serialized = construct_instructions_data(&message.decompile_instructions());
Expand All @@ -73,6 +75,7 @@ fn bench_manual_instruction_deserialize_single(b: &mut Bencher) {
let message = SanitizedMessage::try_from_legacy_message(
Message::new(&instructions, Some(&Pubkey::new_unique())),
&HashSet::default(),
true,
)
.unwrap();
let serialized = construct_instructions_data(&message.decompile_instructions());
Expand Down
Loading