From 72796d5b1bd09f015d2499452564aef26ab38697 Mon Sep 17 00:00:00 2001 From: Victor Lopes Date: Thu, 19 Aug 2021 01:00:06 +0200 Subject: [PATCH] Add `from_slice_unchecked` to tx types (#27) Very often the VM needs to convert between arbitrary memory slices into the tx types. The memory bounds of the VM memory are logically verified and this is a safe operation. This pattern of expecting the converted type from a slice via `TryFrom` is recurrent and should be upgraded into a native function of the type itself. --- fuel-tx/src/bytes.rs | 51 +++++++++------ fuel-tx/src/lib.rs | 4 +- fuel-tx/src/transaction.rs | 4 +- fuel-tx/src/transaction/txio.rs | 41 ++++++------ fuel-tx/src/transaction/types.rs | 86 +++++++++++++++++++++++++ fuel-tx/src/transaction/types/input.rs | 29 +++++---- fuel-tx/src/transaction/types/output.rs | 20 +++--- 7 files changed, 174 insertions(+), 61 deletions(-) diff --git a/fuel-tx/src/bytes.rs b/fuel-tx/src/bytes.rs index 32f2d2c4b7..841b451834 100644 --- a/fuel-tx/src/bytes.rs +++ b/fuel-tx/src/bytes.rs @@ -113,12 +113,14 @@ pub fn store_raw_bytes<'a>( } pub fn restore_bytes(mut buf: &[u8]) -> io::Result<(usize, Vec, &[u8])> { + // Safety: chunks_exact will guarantee the size of the slice is correct let len = buf .chunks_exact(WORD_SIZE) .next() - .map(|chunk| <[u8; WORD_SIZE]>::try_from(chunk).unwrap_or_else(|_| unreachable!())) + .map(|b| unsafe { from_slice_unchecked(b) }) .map(|len| Word::from_be_bytes(len) as usize) .ok_or_else(eof)?; + buf = &buf[WORD_SIZE..]; let pad = len % WORD_SIZE; @@ -167,46 +169,46 @@ where &mut buf[WORD_SIZE..] } -pub fn restore_number_unchecked(buf: &[u8]) -> (T, &[u8]) +pub unsafe fn restore_number_unchecked(buf: &[u8]) -> (T, &[u8]) where T: From, { - let number = <[u8; WORD_SIZE]>::try_from(&buf[..WORD_SIZE]).unwrap_or_else(|_| unreachable!()); + let number = from_slice_unchecked(buf); let number = Word::from_be_bytes(number).into(); (number, &buf[WORD_SIZE..]) } -pub fn restore_word_unchecked(buf: &[u8]) -> (Word, &[u8]) { - let number = <[u8; WORD_SIZE]>::try_from(&buf[..WORD_SIZE]).unwrap_or_else(|_| unreachable!()); +pub unsafe fn restore_word_unchecked(buf: &[u8]) -> (Word, &[u8]) { + let number = from_slice_unchecked(buf); let number = Word::from_be_bytes(number); (number, &buf[WORD_SIZE..]) } -pub fn restore_u8_unchecked(buf: &[u8]) -> (u8, &[u8]) { - let number = <[u8; WORD_SIZE]>::try_from(&buf[..WORD_SIZE]).unwrap_or_else(|_| unreachable!()); +pub unsafe fn restore_u8_unchecked(buf: &[u8]) -> (u8, &[u8]) { + let number = from_slice_unchecked(buf); let number = Word::from_be_bytes(number) as u8; (number, &buf[WORD_SIZE..]) } -pub fn restore_u16_unchecked(buf: &[u8]) -> (u16, &[u8]) { - let number = <[u8; WORD_SIZE]>::try_from(&buf[..WORD_SIZE]).unwrap_or_else(|_| unreachable!()); +pub unsafe fn restore_u16_unchecked(buf: &[u8]) -> (u16, &[u8]) { + let number = from_slice_unchecked(buf); let number = Word::from_be_bytes(number) as u16; (number, &buf[WORD_SIZE..]) } -pub fn restore_u32_unchecked(buf: &[u8]) -> (u32, &[u8]) { - let number = <[u8; WORD_SIZE]>::try_from(&buf[..WORD_SIZE]).unwrap_or_else(|_| unreachable!()); +pub unsafe fn restore_u32_unchecked(buf: &[u8]) -> (u32, &[u8]) { + let number = from_slice_unchecked(buf); let number = Word::from_be_bytes(number) as u32; (number, &buf[WORD_SIZE..]) } -pub fn restore_usize_unchecked(buf: &[u8]) -> (usize, &[u8]) { - let number = <[u8; WORD_SIZE]>::try_from(&buf[..WORD_SIZE]).unwrap_or_else(|_| unreachable!()); +pub unsafe fn restore_usize_unchecked(buf: &[u8]) -> (usize, &[u8]) { + let number = from_slice_unchecked(buf); let number = Word::from_be_bytes(number) as usize; (number, &buf[WORD_SIZE..]) @@ -216,10 +218,11 @@ pub fn restore_number(buf: &[u8]) -> io::Result<(T, &[u8])> where T: From, { + // Safe checked memory bounds let number = buf .chunks_exact(WORD_SIZE) .next() - .map(|chunk| <[u8; WORD_SIZE]>::try_from(chunk).unwrap_or_else(|_| unreachable!())) + .map(|b| unsafe { from_slice_unchecked(b) }) .map(|chunk| Word::from_be_bytes(chunk).into()) .ok_or_else(eof)?; @@ -247,10 +250,8 @@ pub fn store_array_unchecked<'a, const N: usize>( &mut buf[N..] } -pub fn restore_array_unchecked(buf: &[u8]) -> ([u8; N], &[u8]) { - <[u8; N]>::try_from(&buf[..N]) - .map(|array| (array, &buf[N..])) - .unwrap_or_else(|_| unreachable!()) +pub unsafe fn restore_array_unchecked(buf: &[u8]) -> ([u8; N], &[u8]) { + (from_slice_unchecked(buf), &buf[N..]) } pub fn restore_array(buf: &[u8]) -> io::Result<([u8; N], &[u8])> { @@ -258,3 +259,17 @@ pub fn restore_array(buf: &[u8]) -> io::Result<([u8; N], &[u8])> .map_err(|_| eof()) .map(|array| (array, &buf[N..])) } + +/// Add a conversion from arbitrary slices into arrays +/// +/// # Warning +/// +/// This function will not panic if the length of the slice is smaller than `N`. Instead, it will +/// cause undefined behavior and read random disowned bytes. +pub unsafe fn from_slice_unchecked(buf: &[u8]) -> [u8; N] { + let ptr = buf.as_ptr() as *const [u8; N]; + + // Static assertions are not applicable to runtime length check (e.g. slices). + // This is safe if the size of `bytes` is consistent to `N` + *ptr +} diff --git a/fuel-tx/src/lib.rs b/fuel-tx/src/lib.rs index 9a48a9e42b..12d23ab989 100644 --- a/fuel-tx/src/lib.rs +++ b/fuel-tx/src/lib.rs @@ -13,6 +13,6 @@ pub mod consts; pub mod crypto; pub use transaction::{ - Address, Bytes32, Color, ContractId, Input, Metadata, Output, Salt, Transaction, - ValidationError, Witness, + Address, Bytes32, Bytes4, Bytes8, Color, ContractId, Input, Metadata, Output, Salt, + Transaction, ValidationError, Witness, }; diff --git a/fuel-tx/src/transaction.rs b/fuel-tx/src/transaction.rs index e8f9e842af..89f3ad3298 100644 --- a/fuel-tx/src/transaction.rs +++ b/fuel-tx/src/transaction.rs @@ -13,7 +13,9 @@ mod types; mod validation; pub use metadata::Metadata; -pub use types::{Address, Bytes32, Color, ContractId, Input, Output, Salt, Witness}; +pub use types::{ + Address, Bytes32, Bytes4, Bytes8, Color, ContractId, Input, Output, Salt, Witness, +}; pub use validation::ValidationError; const WORD_SIZE: usize = mem::size_of::(); diff --git a/fuel-tx/src/transaction/txio.rs b/fuel-tx/src/transaction/txio.rs index d534d9d5be..2409b5f18e 100644 --- a/fuel-tx/src/transaction/txio.rs +++ b/fuel-tx/src/transaction/txio.rs @@ -146,7 +146,8 @@ impl io::Write for Transaction { return Err(bytes::eof()); } - let (identifier, buf): (Word, _) = bytes::restore_number_unchecked(buf); + // Safety: buffer size is checked + let (identifier, buf): (Word, _) = unsafe { bytes::restore_number_unchecked(buf) }; let identifier = TransactionRepr::try_from(identifier)?; match identifier { @@ -156,14 +157,15 @@ impl io::Write for Transaction { return Err(bytes::eof()); } - let (gas_price, buf) = bytes::restore_number_unchecked(buf); - let (gas_limit, buf) = bytes::restore_number_unchecked(buf); - let (maturity, buf) = bytes::restore_number_unchecked(buf); - let (script_len, buf) = bytes::restore_usize_unchecked(buf); - let (script_data_len, buf) = bytes::restore_usize_unchecked(buf); - let (inputs_len, buf) = bytes::restore_usize_unchecked(buf); - let (outputs_len, buf) = bytes::restore_usize_unchecked(buf); - let (witnesses_len, buf) = bytes::restore_usize_unchecked(buf); + // Safety: buffer size is checked + let (gas_price, buf) = unsafe { bytes::restore_number_unchecked(buf) }; + let (gas_limit, buf) = unsafe { bytes::restore_number_unchecked(buf) }; + let (maturity, buf) = unsafe { bytes::restore_number_unchecked(buf) }; + let (script_len, buf) = unsafe { bytes::restore_usize_unchecked(buf) }; + let (script_data_len, buf) = unsafe { bytes::restore_usize_unchecked(buf) }; + let (inputs_len, buf) = unsafe { bytes::restore_usize_unchecked(buf) }; + let (outputs_len, buf) = unsafe { bytes::restore_usize_unchecked(buf) }; + let (witnesses_len, buf) = unsafe { bytes::restore_usize_unchecked(buf) }; let (size, script, buf) = bytes::restore_raw_bytes(buf, script_len)?; n += size; @@ -213,16 +215,17 @@ impl io::Write for Transaction { return Err(bytes::eof()); } - let (gas_price, buf) = bytes::restore_number_unchecked(buf); - let (gas_limit, buf) = bytes::restore_number_unchecked(buf); - let (maturity, buf) = bytes::restore_number_unchecked(buf); - let (_bytecode_length, buf) = bytes::restore_u16_unchecked(buf); - let (bytecode_witness_index, buf) = bytes::restore_u8_unchecked(buf); - let (static_contracts_len, buf) = bytes::restore_usize_unchecked(buf); - let (inputs_len, buf) = bytes::restore_usize_unchecked(buf); - let (outputs_len, buf) = bytes::restore_usize_unchecked(buf); - let (witnesses_len, buf) = bytes::restore_usize_unchecked(buf); - let (salt, mut buf) = bytes::restore_array_unchecked(buf); + // Safety: buffer size is checked + let (gas_price, buf) = unsafe { bytes::restore_number_unchecked(buf) }; + let (gas_limit, buf) = unsafe { bytes::restore_number_unchecked(buf) }; + let (maturity, buf) = unsafe { bytes::restore_number_unchecked(buf) }; + let (_bytecode_length, buf) = unsafe { bytes::restore_u16_unchecked(buf) }; + let (bytecode_witness_index, buf) = unsafe { bytes::restore_u8_unchecked(buf) }; + let (static_contracts_len, buf) = unsafe { bytes::restore_usize_unchecked(buf) }; + let (inputs_len, buf) = unsafe { bytes::restore_usize_unchecked(buf) }; + let (outputs_len, buf) = unsafe { bytes::restore_usize_unchecked(buf) }; + let (witnesses_len, buf) = unsafe { bytes::restore_usize_unchecked(buf) }; + let (salt, mut buf) = unsafe { bytes::restore_array_unchecked(buf) }; let salt = salt.into(); diff --git a/fuel-tx/src/transaction/types.rs b/fuel-tx/src/transaction/types.rs index c8e583138c..fa7529a20f 100644 --- a/fuel-tx/src/transaction/types.rs +++ b/fuel-tx/src/transaction/types.rs @@ -1,3 +1,4 @@ +use crate::bytes; use rand::distributions::{Distribution, Standard}; use rand::Rng; use std::array::TryFromSliceError; @@ -22,6 +23,27 @@ macro_rules! key { pub const fn size_of() -> usize { $s } + + /// Add a conversion from arbitrary slices into owned + /// + /// # Warning + /// + /// This function will not panic if the length of the slice is smaller than + /// `Self::size_of`. Instead, it will cause undefined behavior and read random disowned + /// bytes + pub unsafe fn from_slice_unchecked(bytes: &[u8]) -> Self { + $i(bytes::from_slice_unchecked(bytes)) + } + + /// Copy-free reference cast + pub unsafe fn as_ref_unchecked(bytes: &[u8]) -> &Self { + // The interpreter will frequently make references to keys and values using + // logically checked slices. + // + // This function will save unnecessary copy to owned slices for the interpreter + // access + &*(bytes.as_ptr() as *const Self) + } } impl rand::Fill for $i { @@ -93,9 +115,73 @@ pub use witness::Witness; key!(Address, 32); key!(Color, 32); key!(ContractId, 32); +key!(Bytes4, 4); +key!(Bytes8, 8); key!(Bytes32, 32); key!(Salt, 32); impl ContractId { pub const SEED: [u8; 4] = 0x4655454C_u32.to_be_bytes(); } + +#[cfg(test)] +mod tests { + use crate::*; + use rand::rngs::StdRng; + use rand::{Rng, RngCore, SeedableRng}; + use std::convert::TryFrom; + + macro_rules! check_consistency { + ($i:ident,$r:expr,$b:expr) => { + unsafe { + let n = $i::size_of(); + let s = $r.gen_range(0..$b.len() - n); + let e = $r.gen_range(s + n..$b.len()); + let r = $r.gen_range(1..n - 1); + let i = &$b[s..s + n]; + + let a = $i::from_slice_unchecked(i); + let b = $i::from_slice_unchecked(&$b[s..e]); + let c = $i::try_from(i).expect("Memory conversion"); + + // `d` will create random smaller slices and expect the value to be parsed correctly + // + // However, this is not the expected usage of the function + let d = $i::from_slice_unchecked(&i[..i.len() - r]); + + let e = $i::as_ref_unchecked(i); + + // Assert `from_slice_unchecked` will not create two references to the same owned + // memory + assert_ne!(a.as_ptr(), b.as_ptr()); + + // Assert `as_ref_unchecked` is copy-free + assert_ne!(e.as_ptr(), a.as_ptr()); + assert_eq!(e.as_ptr(), i.as_ptr()); + + assert_eq!(a, b); + assert_eq!(a, c); + assert_eq!(a, d); + assert_eq!(&a, e); + } + }; + } + + #[test] + fn from_slice_unchecked_safety() { + let rng = &mut StdRng::seed_from_u64(8586); + + let mut bytes = [0u8; 257]; + rng.fill_bytes(&mut bytes); + + for _ in 0..100 { + check_consistency!(Address, rng, bytes); + check_consistency!(Color, rng, bytes); + check_consistency!(ContractId, rng, bytes); + check_consistency!(Bytes4, rng, bytes); + check_consistency!(Bytes8, rng, bytes); + check_consistency!(Bytes32, rng, bytes); + check_consistency!(Salt, rng, bytes); + } + } +} diff --git a/fuel-tx/src/transaction/types/input.rs b/fuel-tx/src/transaction/types/input.rs index 8063bb7849..c4c5137238 100644 --- a/fuel-tx/src/transaction/types/input.rs +++ b/fuel-tx/src/transaction/types/input.rs @@ -232,7 +232,8 @@ impl io::Write for Input { return Err(bytes::eof()); } - let (identifier, buf): (Word, _) = bytes::restore_number_unchecked(buf); + // Safety: buf len is checked + let (identifier, buf): (Word, _) = unsafe { bytes::restore_number_unchecked(buf) }; let identifier = InputRepr::try_from(identifier)?; match identifier { @@ -241,15 +242,16 @@ impl io::Write for Input { InputRepr::Coin => { let mut n = INPUT_COIN_FIXED_SIZE; - let (utxo_id, buf) = bytes::restore_array_unchecked(buf); - let (owner, buf) = bytes::restore_array_unchecked(buf); - let (amount, buf) = bytes::restore_number_unchecked(buf); - let (color, buf) = bytes::restore_array_unchecked(buf); - let (witness_index, buf) = bytes::restore_u8_unchecked(buf); - let (maturity, buf) = bytes::restore_number_unchecked(buf); + // Safety: buf len is checked + let (utxo_id, buf) = unsafe { bytes::restore_array_unchecked(buf) }; + let (owner, buf) = unsafe { bytes::restore_array_unchecked(buf) }; + let (amount, buf) = unsafe { bytes::restore_number_unchecked(buf) }; + let (color, buf) = unsafe { bytes::restore_array_unchecked(buf) }; + let (witness_index, buf) = unsafe { bytes::restore_u8_unchecked(buf) }; + let (maturity, buf) = unsafe { bytes::restore_number_unchecked(buf) }; - let (predicate_len, buf) = bytes::restore_usize_unchecked(buf); - let (predicate_data_len, buf) = bytes::restore_usize_unchecked(buf); + let (predicate_len, buf) = unsafe { bytes::restore_usize_unchecked(buf) }; + let (predicate_data_len, buf) = unsafe { bytes::restore_usize_unchecked(buf) }; let (size, predicate, buf) = bytes::restore_raw_bytes(buf, predicate_len)?; n += size; @@ -278,10 +280,11 @@ impl io::Write for Input { InputRepr::Contract if buf.len() < INPUT_CONTRACT_SIZE - WORD_SIZE => Err(bytes::eof()), InputRepr::Contract => { - let (utxo_id, buf) = bytes::restore_array_unchecked(buf); - let (balance_root, buf) = bytes::restore_array_unchecked(buf); - let (state_root, buf) = bytes::restore_array_unchecked(buf); - let (contract_id, _) = bytes::restore_array_unchecked(buf); + // Safety: checked buffer len + let (utxo_id, buf) = unsafe { bytes::restore_array_unchecked(buf) }; + let (balance_root, buf) = unsafe { bytes::restore_array_unchecked(buf) }; + let (state_root, buf) = unsafe { bytes::restore_array_unchecked(buf) }; + let (contract_id, _) = unsafe { bytes::restore_array_unchecked(buf) }; let utxo_id = utxo_id.into(); let balance_root = balance_root.into(); diff --git a/fuel-tx/src/transaction/types/output.rs b/fuel-tx/src/transaction/types/output.rs index e31b57b766..42a0c87fba 100644 --- a/fuel-tx/src/transaction/types/output.rs +++ b/fuel-tx/src/transaction/types/output.rs @@ -209,7 +209,8 @@ impl io::Write for Output { return Err(bytes::eof()); } - let (identifier, buf): (Word, _) = bytes::restore_number_unchecked(buf); + // Bounds safely checked + let (identifier, buf): (Word, _) = unsafe { bytes::restore_number_unchecked(buf) }; let identifier = OutputRepr::try_from(identifier)?; match identifier { @@ -234,9 +235,10 @@ impl io::Write for Output { | OutputRepr::Withdrawal | OutputRepr::Change | OutputRepr::Variable => { - let (to, buf) = bytes::restore_array_unchecked(buf); - let (amount, buf) = bytes::restore_number_unchecked(buf); - let (color, _) = bytes::restore_array_unchecked(buf); + // Safety: buf len is checked + let (to, buf) = unsafe { bytes::restore_array_unchecked(buf) }; + let (amount, buf) = unsafe { bytes::restore_number_unchecked(buf) }; + let (color, _) = unsafe { bytes::restore_array_unchecked(buf) }; let to = to.into(); let color = color.into(); @@ -254,9 +256,10 @@ impl io::Write for Output { } OutputRepr::Contract => { - let (input_index, buf) = bytes::restore_u8_unchecked(buf); - let (balance_root, buf) = bytes::restore_array_unchecked(buf); - let (state_root, _) = bytes::restore_array_unchecked(buf); + // Safety: buf len is checked + let (input_index, buf) = unsafe { bytes::restore_u8_unchecked(buf) }; + let (balance_root, buf) = unsafe { bytes::restore_array_unchecked(buf) }; + let (state_root, _) = unsafe { bytes::restore_array_unchecked(buf) }; let balance_root = balance_root.into(); let state_root = state_root.into(); @@ -271,7 +274,8 @@ impl io::Write for Output { } OutputRepr::ContractCreated => { - let (contract_id, _) = bytes::restore_array_unchecked(buf); + // Safety: buf len is checked + let (contract_id, _) = unsafe { bytes::restore_array_unchecked(buf) }; let contract_id = contract_id.into(); *self = Self::ContractCreated { contract_id };