Skip to content
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.

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

TransactionView - sanitization checks #2757

Merged
merged 12 commits into from
Aug 29, 2024
23 changes: 21 additions & 2 deletions transaction-view/benches/transaction_view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use {
signature::Keypair,
signer::Signer,
system_instruction,
transaction::VersionedTransaction,
transaction::{SanitizedVersionedTransaction, VersionedTransaction},
},
};

Expand All @@ -41,11 +41,30 @@ fn bench_transactions_parsing(
});
});

// Legacy Transaction Parsing and Sanitize checks
group.bench_function("SanitizedVersionedTransaction", |c| {
c.iter(|| {
for bytes in serialized_transactions.iter() {
let tx = bincode::deserialize::<VersionedTransaction>(black_box(bytes)).unwrap();
let _ = SanitizedVersionedTransaction::try_new(tx).unwrap();
}
});
});

// New Transaction Parsing
group.bench_function("TransactionView", |c| {
c.iter(|| {
for bytes in serialized_transactions.iter() {
let _ = TransactionView::try_new(black_box(bytes.as_ref())).unwrap();
let _ = TransactionView::try_new_unsanitized(black_box(bytes.as_ref())).unwrap();
}
});
});

// New Transaction Parsing and Sanitize checks
group.bench_function("TransactionView (Sanitized)", |c| {
c.iter(|| {
for bytes in serialized_transactions.iter() {
let _ = TransactionView::try_new_sanitized(black_box(bytes.as_ref())).unwrap();
}
});
});
Expand Down
33 changes: 29 additions & 4 deletions transaction-view/src/address_table_lookup_meta.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use {
advance_offset_for_array, advance_offset_for_type, check_remaining,
optimized_read_compressed_u16, read_byte, read_slice_data, read_type,
},
result::{Result, TransactionParsingError},
result::{Result, TransactionViewError},
},
solana_sdk::{hash::Hash, packet::PACKET_DATA_SIZE, pubkey::Pubkey, signature::Signature},
solana_svm_transaction::message_address_table_lookup::SVMMessageAddressTableLookup,
Expand Down Expand Up @@ -51,6 +51,10 @@ pub(crate) struct AddressTableLookupMeta {
pub(crate) num_address_table_lookups: u8,
/// The offset to the first address table lookup in the transaction.
pub(crate) offset: u16,
/// The total number of writable lookup accounts in the transaction.
pub(crate) total_writable_lookup_accounts: u16,
/// The total number of readonly lookup accounts in the transaction.
pub(crate) total_readonly_lookup_accounts: u16,
}

impl AddressTableLookupMeta {
Expand All @@ -66,7 +70,7 @@ impl AddressTableLookupMeta {
const _: () = assert!(MAX_ATLS_PER_PACKET & 0b1000_0000 == 0);
let num_address_table_lookups = read_byte(bytes, offset)?;
if num_address_table_lookups > MAX_ATLS_PER_PACKET {
return Err(TransactionParsingError);
return Err(TransactionViewError::ParseError);
}

// Check that the remaining bytes are enough to hold the ATLs.
Expand All @@ -80,6 +84,13 @@ impl AddressTableLookupMeta {
// length is less than u16::MAX, so we can safely cast to u16.
let address_table_lookups_offset = *offset as u16;

// Check that there is no chance of overflow when calculating the total
// number of writable and readonly lookup accounts using a u32.
const _: () =
assert!(u16::MAX as usize * MAX_ATLS_PER_PACKET as usize <= u32::MAX as usize);
let mut total_writable_lookup_accounts: u32 = 0;
let mut total_readonly_lookup_accounts: u32 = 0;

// The ATLs do not have a fixed size. So we must iterate over
// each ATL to find the total size of the ATLs in the packet,
// and check for any malformed ATLs or buffer overflows.
Expand All @@ -94,16 +105,24 @@ impl AddressTableLookupMeta {

// Read the number of write indexes, and then update the offset.
let num_write_accounts = optimized_read_compressed_u16(bytes, offset)?;
total_writable_lookup_accounts =
total_writable_lookup_accounts.wrapping_add(u32::from(num_write_accounts));
advance_offset_for_array::<u8>(bytes, offset, num_write_accounts)?;

// Read the number of read indexes, and then update the offset.
let num_read_accounts = optimized_read_compressed_u16(bytes, offset)?;
advance_offset_for_array::<u8>(bytes, offset, num_read_accounts)?
total_readonly_lookup_accounts =
total_readonly_lookup_accounts.wrapping_add(u32::from(num_read_accounts));
advance_offset_for_array::<u8>(bytes, offset, num_read_accounts)?;
}

Ok(Self {
num_address_table_lookups,
offset: address_table_lookups_offset,
total_writable_lookup_accounts: u16::try_from(total_writable_lookup_accounts)
.map_err(|_| TransactionViewError::SanitizeError)?,
total_readonly_lookup_accounts: u16::try_from(total_readonly_lookup_accounts)
.map_err(|_| TransactionViewError::SanitizeError)?,
})
}
}
Expand Down Expand Up @@ -195,6 +214,8 @@ mod tests {
assert_eq!(meta.num_address_table_lookups, 0);
assert_eq!(meta.offset, 1);
assert_eq!(offset, bytes.len());
assert_eq!(meta.total_writable_lookup_accounts, 0);
assert_eq!(meta.total_readonly_lookup_accounts, 0);
}

#[test]
Expand All @@ -221,6 +242,8 @@ mod tests {
assert_eq!(meta.num_address_table_lookups, 1);
assert_eq!(meta.offset, 1);
assert_eq!(offset, bytes.len());
assert_eq!(meta.total_writable_lookup_accounts, 3);
assert_eq!(meta.total_readonly_lookup_accounts, 3);
}

#[test]
Expand All @@ -234,7 +257,7 @@ mod tests {
MessageAddressTableLookup {
account_key: Pubkey::new_unique(),
writable_indexes: vec![1, 2, 3],
readonly_indexes: vec![4, 5, 6],
readonly_indexes: vec![4, 5],
},
]))
.unwrap();
Expand All @@ -243,6 +266,8 @@ mod tests {
assert_eq!(meta.num_address_table_lookups, 2);
assert_eq!(meta.offset, 1);
assert_eq!(offset, bytes.len());
assert_eq!(meta.total_writable_lookup_accounts, 6);
assert_eq!(meta.total_readonly_lookup_accounts, 5);
}

#[test]
Expand Down
19 changes: 11 additions & 8 deletions transaction-view/src/bytes.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::result::{Result, TransactionParsingError};
use crate::result::{Result, TransactionViewError};

/// Check that the buffer has at least `len` bytes remaining starting at
/// `offset`. Returns Err if the buffer is too short.
Expand All @@ -12,7 +12,7 @@ use crate::result::{Result, TransactionParsingError};
#[inline(always)]
pub fn check_remaining(bytes: &[u8], offset: usize, num_bytes: usize) -> Result<()> {
if num_bytes > bytes.len().wrapping_sub(offset) {
Err(TransactionParsingError)
Err(TransactionViewError::ParseError)
} else {
Ok(())
}
Expand All @@ -24,7 +24,10 @@ pub fn check_remaining(bytes: &[u8], offset: usize, num_bytes: usize) -> Result<
pub fn read_byte(bytes: &[u8], offset: &mut usize) -> Result<u8> {
// Implicitly checks that the offset is within bounds, no need
// to call `check_remaining` explicitly here.
let value = bytes.get(*offset).copied().ok_or(TransactionParsingError);
let value = bytes
.get(*offset)
.copied()
.ok_or(TransactionViewError::ParseError);
*offset = offset.wrapping_add(1);
value
}
Expand All @@ -49,10 +52,10 @@ pub fn read_compressed_u16(bytes: &[u8], offset: &mut usize) -> Result<u16> {
// to call check_remaining explicitly here.
let byte = *bytes
.get(offset.wrapping_add(i))
.ok_or(TransactionParsingError)?;
.ok_or(TransactionViewError::ParseError)?;
// non-minimal encoding or overflow
if (i > 0 && byte == 0) || (i == 2 && byte > 3) {
return Err(TransactionParsingError);
return Err(TransactionViewError::ParseError);
}
result |= ((byte & 0x7F) as u16) << shift;
shift = shift.wrapping_add(7);
Expand Down Expand Up @@ -86,7 +89,7 @@ pub fn optimized_read_compressed_u16(bytes: &[u8], offset: &mut usize) -> Result
let mut result = 0u16;

// First byte
let byte1 = *bytes.get(*offset).ok_or(TransactionParsingError)?;
let byte1 = *bytes.get(*offset).ok_or(TransactionViewError::ParseError)?;
result |= (byte1 & 0x7F) as u16;
if byte1 & 0x80 == 0 {
*offset = offset.wrapping_add(1);
Expand All @@ -96,9 +99,9 @@ pub fn optimized_read_compressed_u16(bytes: &[u8], offset: &mut usize) -> Result
// Second byte
let byte2 = *bytes
.get(offset.wrapping_add(1))
.ok_or(TransactionParsingError)?;
.ok_or(TransactionViewError::ParseError)?;
if byte2 == 0 || byte2 & 0x80 != 0 {
return Err(TransactionParsingError); // non-minimal encoding or overflow
return Err(TransactionViewError::ParseError); // non-minimal encoding or overflow
}
result |= ((byte2 & 0x7F) as u16) << 7;
*offset = offset.wrapping_add(2);
Expand Down
1 change: 1 addition & 0 deletions transaction-view/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ mod address_table_lookup_meta;
mod instructions_meta;
mod message_header_meta;
pub mod result;
mod sanitize;
mod signature_meta;
pub mod static_account_keys_meta;
pub mod transaction_data;
Expand Down
4 changes: 2 additions & 2 deletions transaction-view/src/message_header_meta.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use {
crate::{
bytes::read_byte,
result::{Result, TransactionParsingError},
result::{Result, TransactionViewError},
},
solana_sdk::message::MESSAGE_VERSION_PREFIX,
};
Expand Down Expand Up @@ -49,7 +49,7 @@ impl MessageHeaderMeta {
let version = message_prefix & !MESSAGE_VERSION_PREFIX;
match version {
0 => (TransactionVersion::V0, read_byte(bytes, offset)?),
_ => return Err(TransactionParsingError),
_ => return Err(TransactionViewError::ParseError),
}
} else {
// Legacy transaction. The `message_prefix` that was just read is
Expand Down
9 changes: 7 additions & 2 deletions transaction-view/src/result.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
#[derive(Debug, PartialEq, Eq)]
pub struct TransactionParsingError;
pub type Result<T> = core::result::Result<T, TransactionParsingError>; // no distinction between errors for now
#[repr(u8)] // repr(u8) is used to ensure that the enum is represented as a single byte in memory.
pub enum TransactionViewError {
ParseError,
SanitizeError,
}

pub type Result<T> = core::result::Result<T, TransactionViewError>;
Loading