From 865e1b9629c2d65c2c96714332b19e04c6f6f8ad Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Wed, 24 Apr 2024 18:55:45 +0300 Subject: [PATCH] Deny clippy::arithmetic_side_effects (#725) * Deny clippy::arithmetic_side_effects * Add lints to fuel-storage * Add changelog * Use saturating_add for rest of the offsets * Replace .sum() with saturating_add sum * Fix colon separator support for UtxoId::from_str, and improve error messages * Fix hex dependency on no-std targets * Update changelog * Add changelog mention about UtxoId::from_str colon separator * Address some PR feedback * Remove GasUnit * Combine some constant * Do serialized size checks in CommonMetadata::compute * Properly saturate on canonical::Serialize::size* overflow cases * Address PR feedback --- .npm/packages/fuel-tx/index.test.cjs | 2 +- CHANGELOG.md | 9 +- fuel-asm/src/lib.rs | 9 +- fuel-crypto/src/lib.rs | 9 +- fuel-derive/src/lib.rs | 7 + fuel-derive/src/serialize.rs | 18 +- fuel-merkle/test-helpers/src/binary/verify.rs | 2 +- .../test-helpers/src/suites/binary_proofs.rs | 2 +- fuel-storage/src/lib.rs | 8 +- fuel-tx/src/consts.rs | 4 + fuel-tx/src/contract.rs | 12 +- fuel-tx/src/lib.rs | 10 +- fuel-tx/src/tests/mod.rs | 2 + fuel-tx/src/transaction.rs | 1 - .../src/transaction/consensus_parameters.rs | 19 +- .../transaction/consensus_parameters/gas.rs | 135 +--------- fuel-tx/src/transaction/fee.rs | 2 +- fuel-tx/src/transaction/metadata.rs | 75 +++--- fuel-tx/src/transaction/policies.rs | 1 + .../types/chargeable_transaction.rs | 49 ++-- fuel-tx/src/transaction/types/create.rs | 23 +- fuel-tx/src/transaction/types/input.rs | 22 +- fuel-tx/src/transaction/types/mint.rs | 12 +- fuel-tx/src/transaction/types/script.rs | 11 +- fuel-tx/src/transaction/types/upgrade.rs | 15 +- fuel-tx/src/transaction/types/upload.rs | 17 +- fuel-tx/src/transaction/types/utxo_id.rs | 18 +- fuel-tx/src/transaction/validity/error.rs | 12 + fuel-tx/src/tx_pointer.rs | 2 +- fuel-types/Cargo.toml | 4 +- fuel-types/src/array_types.rs | 57 ++-- fuel-types/src/bytes.rs | 6 +- fuel-types/src/canonical.rs | 41 ++- fuel-types/src/fmt.rs | 4 +- fuel-types/src/lib.rs | 17 +- fuel-types/src/numeric_types.rs | 23 +- fuel-types/src/tests/types.rs | 2 + fuel-vm/src/call.rs | 19 +- fuel-vm/src/checked_transaction.rs | 2 +- fuel-vm/src/checked_transaction/balances.rs | 15 +- fuel-vm/src/constraints/reg_key.rs | 59 ++--- fuel-vm/src/constraints/reg_key/tests.rs | 2 + fuel-vm/src/interpreter.rs | 5 +- fuel-vm/src/interpreter/alu/muldiv.rs | 19 +- fuel-vm/src/interpreter/alu/tests.rs | 2 + fuel-vm/src/interpreter/alu/wideint.rs | 77 +++--- fuel-vm/src/interpreter/balances.rs | 37 ++- fuel-vm/src/interpreter/blockchain.rs | 13 +- .../src/interpreter/blockchain/other_tests.rs | 2 + .../src/interpreter/blockchain/smo_tests.rs | 2 + fuel-vm/src/interpreter/contract.rs | 4 +- fuel-vm/src/interpreter/contract/tests.rs | 3 +- fuel-vm/src/interpreter/diff.rs | 2 +- .../src/interpreter/executors/instruction.rs | 17 +- fuel-vm/src/interpreter/executors/main.rs | 6 +- fuel-vm/src/interpreter/flow.rs | 4 +- fuel-vm/src/interpreter/flow/tests.rs | 2 + fuel-vm/src/interpreter/initialization.rs | 26 +- fuel-vm/src/interpreter/internal.rs | 35 +-- .../src/interpreter/internal/message_tests.rs | 2 + fuel-vm/src/interpreter/memory.rs | 56 ++-- .../interpreter/memory/allocation_tests.rs | 22 +- fuel-vm/src/interpreter/memory/stack_tests.rs | 2 + fuel-vm/src/interpreter/metadata.rs | 247 +++++++++--------- fuel-vm/src/lib.rs | 9 +- fuel-vm/src/profiler.rs | 5 +- fuel-vm/src/state/debug.rs | 8 +- fuel-vm/src/storage/memory.rs | 3 + fuel-vm/src/tests/cgas.rs | 2 +- fuel-vm/src/tests/gas_factor.rs | 4 +- fuel-vm/src/tests/mod.rs | 3 +- fuel-vm/src/util.rs | 9 +- 72 files changed, 712 insertions(+), 675 deletions(-) diff --git a/.npm/packages/fuel-tx/index.test.cjs b/.npm/packages/fuel-tx/index.test.cjs index ae6d8d5b77..21f0ea63f2 100644 --- a/.npm/packages/fuel-tx/index.test.cjs +++ b/.npm/packages/fuel-tx/index.test.cjs @@ -19,7 +19,7 @@ describe('fuel-tx [cjs]', () => { }) it('should serialize and deserialize UtxoId correctly', () => { - let utxo_id = new tx.UtxoId("0x0c0000000000000000000000000000000000000000000000000000000000000b00001a"); + let utxo_id = new tx.UtxoId("0x0c0000000000000000000000000000000000000000000000000000000000000b001a"); let bytes = utxo_id.to_bytes(); let utxo_id2 = tx.UtxoId.from_bytes(bytes); expect(utxo_id.toString()).to.equal(utxo_id2.toString()) diff --git a/CHANGELOG.md b/CHANGELOG.md index 81a42ec3bb..0dca5404cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] -### Breaking +### Changed + +- [#725](https://github.com/FuelLabs/fuel-vm/pull/725): Adds more clippy lints to catch possible integer overflow and casting bugs on compile time. + +### Added + +#### Breaking +- [#725](https://github.com/FuelLabs/fuel-vm/pull/725): `UtxoId::from_str` now rejects inputs with multiple `0x` prefixes. Many `::from_str` implementations also reject extra data in the end of the input, instead of silently ignoring it. `UtxoId::from_str` allows a single `:` between the fields. Unused `GasUnit` struct removed. - [#726](https://github.com/FuelLabs/fuel-vm/pull/726): Removed code related to Binary Merkle Sum Trees (BMSTs). The BMST is deprecated and not used in production environments. ## [Version 0.49.0] diff --git a/fuel-asm/src/lib.rs b/fuel-asm/src/lib.rs index 7f1b075b1d..1c26587960 100644 --- a/fuel-asm/src/lib.rs +++ b/fuel-asm/src/lib.rs @@ -3,8 +3,13 @@ #![cfg_attr(docsrs, feature(doc_auto_cfg))] #![cfg_attr(not(feature = "std"), no_std)] #![cfg_attr(feature = "std", doc = include_str!("../README.md"))] -#![deny(clippy::cast_possible_truncation)] -#![deny(clippy::string_slice)] +#![deny( + clippy::arithmetic_side_effects, + clippy::cast_sign_loss, + clippy::cast_possible_truncation, + clippy::cast_possible_wrap, + clippy::string_slice +)] #![deny(missing_docs)] #![deny(unsafe_code)] #![deny(unused_crate_dependencies)] diff --git a/fuel-crypto/src/lib.rs b/fuel-crypto/src/lib.rs index b8d9c69f35..8cb33e497b 100644 --- a/fuel-crypto/src/lib.rs +++ b/fuel-crypto/src/lib.rs @@ -4,11 +4,16 @@ #![cfg_attr(not(feature = "std"), no_std)] // Wrong clippy convention; check // https://rust-lang.github.io/api-guidelines/naming.html -#![deny(clippy::string_slice)] #![warn(missing_docs)] #![deny(unsafe_code)] #![deny(unused_crate_dependencies)] -#![deny(clippy::cast_possible_truncation)] +#![deny( + clippy::arithmetic_side_effects, + clippy::cast_sign_loss, + clippy::cast_possible_truncation, + clippy::cast_possible_wrap, + clippy::string_slice +)] // Satisfy unused_crate_dependencies lint for self-dependency enabling test features #[cfg(test)] diff --git a/fuel-derive/src/lib.rs b/fuel-derive/src/lib.rs index d130ba6b46..758a7d4a07 100644 --- a/fuel-derive/src/lib.rs +++ b/fuel-derive/src/lib.rs @@ -1,6 +1,13 @@ //! Derive macros for canonical type serialization and deserialization. #![deny(unused_must_use, missing_docs)] +#![deny( + clippy::arithmetic_side_effects, + clippy::cast_sign_loss, + clippy::cast_possible_truncation, + clippy::cast_possible_wrap, + clippy::string_slice +)] extern crate proc_macro; mod attribute; diff --git a/fuel-derive/src/serialize.rs b/fuel-derive/src/serialize.rs index d468d46dfd..42b2eefdc1 100644 --- a/fuel-derive/src/serialize.rs +++ b/fuel-derive/src/serialize.rs @@ -29,24 +29,24 @@ fn serialize_struct(s: &synstructure::Structure) -> TokenStream2 { let size_static_code = variant.each(|binding| { quote! { - size = ::fuel_types::canonical::add_sizes(size, #binding.size_static()); + size = size.saturating_add(#binding.size_static()); } }); let initial_size = if attrs.prefix.is_some() { - quote! { let mut size = 8; } + quote! { let mut size = 8usize; } } else { - quote! { let mut size = 0; } + quote! { let mut size = 0usize; } }; let size_static_code = quote! { #initial_size match self { #size_static_code}; size }; let size_dynamic_code = variant.each(|binding| { quote! { - size = ::fuel_types::canonical::add_sizes(size, #binding.size_dynamic()); + size = size.saturating_add(#binding.size_dynamic()); } }); let size_dynamic_code = - quote! { let mut size = 0; match self { #size_dynamic_code}; size }; + quote! { let mut size = 0usize; match self { #size_dynamic_code}; size }; let prefix = if let Some(prefix_type) = attrs.prefix.as_ref() { quote! { @@ -143,14 +143,14 @@ fn serialize_enum(s: &synstructure::Structure) -> TokenStream2 { .map(|variant| { variant.each(|binding| { quote! { - size = ::fuel_types::canonical::add_sizes(size, #binding.size_static()); + size = size.saturating_add(#binding.size_static()); } }) }) .collect(); let match_size_static = quote! {{ // `repr(128)` is unstable, so because of that we can use 8 bytes. - let mut size = 8; + let mut size = 8usize; match self { #match_size_static } size } }; @@ -160,13 +160,13 @@ fn serialize_enum(s: &synstructure::Structure) -> TokenStream2 { .map(|variant| { variant.each(|binding| { quote! { - size = ::fuel_types::canonical::add_sizes(size, #binding.size_dynamic()); + size = size.saturating_add(#binding.size_dynamic()); } }) }) .collect(); let match_size_dynamic = - quote! {{ let mut size = 0; match self { #match_size_dynamic } size }}; + quote! {{ let mut size = 0usize; match self { #match_size_dynamic } size }}; let impl_code = s.gen_impl(quote! { gen impl ::fuel_types::canonical::Serialize for @Self { diff --git a/fuel-merkle/test-helpers/src/binary/verify.rs b/fuel-merkle/test-helpers/src/binary/verify.rs index 73a37e8c0e..708e4e59f8 100644 --- a/fuel-merkle/test-helpers/src/binary/verify.rs +++ b/fuel-merkle/test-helpers/src/binary/verify.rs @@ -7,7 +7,7 @@ use crate::binary::{ pub fn verify>( root: &Data, data: &T, - proof_set: &Vec, + proof_set: &[Data], proof_index: u64, num_leaves: u64, ) -> bool { diff --git a/fuel-merkle/test-helpers/src/suites/binary_proofs.rs b/fuel-merkle/test-helpers/src/suites/binary_proofs.rs index 8bbbe47112..1bbadddef3 100644 --- a/fuel-merkle/test-helpers/src/suites/binary_proofs.rs +++ b/fuel-merkle/test-helpers/src/suites/binary_proofs.rs @@ -27,7 +27,7 @@ fn generate_test( name: String, function_name: String, description: String, - sample_data: &Vec, + sample_data: &[Bytes32], proof_index: u64, ) -> ProofTest { let (root, proof_set) = { diff --git a/fuel-storage/src/lib.rs b/fuel-storage/src/lib.rs index 7463f01271..719c2bc6d7 100644 --- a/fuel-storage/src/lib.rs +++ b/fuel-storage/src/lib.rs @@ -1,5 +1,11 @@ #![no_std] -#![deny(clippy::cast_possible_truncation)] +#![deny( + clippy::arithmetic_side_effects, + clippy::cast_sign_loss, + clippy::cast_possible_truncation, + clippy::cast_possible_wrap, + clippy::string_slice +)] #![deny(unsafe_code)] #![deny(unused_crate_dependencies)] diff --git a/fuel-tx/src/consts.rs b/fuel-tx/src/consts.rs index 26aa5e86c7..5c09977310 100644 --- a/fuel-tx/src/consts.rs +++ b/fuel-tx/src/consts.rs @@ -1,10 +1,14 @@ use crate::TxPointer; use fuel_types::{ bytes::WORD_SIZE, + AssetId, Bytes32, Salt, }; +/// Size of balance entry, i.e. asset id and associated balance. +pub const BALANCE_ENTRY_SIZE: usize = AssetId::LEN + WORD_SIZE; + pub const TRANSACTION_SCRIPT_FIXED_SIZE: usize = WORD_SIZE // Identifier + WORD_SIZE // Gas price + WORD_SIZE // Gas limit diff --git a/fuel-tx/src/contract.rs b/fuel-tx/src/contract.rs index 3e3c71c5fe..fb400f3205 100644 --- a/fuel-tx/src/contract.rs +++ b/fuel-tx/src/contract.rs @@ -34,11 +34,6 @@ const LEAF_SIZE: usize = 16 * 1024; const PADDING_BYTE: u8 = 0u8; const MULTIPLE: usize = 8; -/// See https://stackoverflow.com/a/9194117 -fn next_multiple(x: usize) -> usize { - x + (N - x % N) % N -} - #[derive(Default, Derivative, Clone, PartialEq, Eq, Hash)] #[derivative(Debug)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] @@ -87,7 +82,7 @@ impl Contract { if len == LEAF_SIZE || len % MULTIPLE == 0 { tree.push(leaf); } else { - let padding_size = next_multiple::(len); + let padding_size = len.next_multiple_of(MULTIPLE); let mut padded_leaf = [PADDING_BYTE; LEAF_SIZE]; padded_leaf[0..len].clone_from_slice(leaf); tree.push(padded_leaf[..padding_size].as_ref()); @@ -178,6 +173,7 @@ impl TryFrom<&Transaction> for Contract { } } +#[allow(clippy::arithmetic_side_effects, clippy::cast_possible_truncation)] #[cfg(test)] mod tests { use super::*; @@ -302,7 +298,7 @@ mod tests { // length rounded to the nearest multiple of 8, and each byte is the // PADDING_BYTE by default. The leaf is generated by copying the // remaining data bytes into the start of this array. - let sz = next_multiple::<8>(partial_leaf_size); + let sz = partial_leaf_size.next_multiple_of(8); if sz > 0 { let mut padded_leaf = vec![PADDING_BYTE; sz]; padded_leaf[0..code_len].clone_from_slice(&code); @@ -346,7 +342,7 @@ mod tests { // length rounded to the nearest multiple of 8, and each byte is the // PADDING_BYTE by default. The leaf is generated by copying the // remaining data bytes into the start of this array. - let sz = next_multiple::<8>(partial_leaf_size); + let sz = partial_leaf_size.next_multiple_of(8); if sz > 0 { let mut padded_leaf = vec![PADDING_BYTE; sz]; padded_leaf[0..partial_leaf_size].clone_from_slice(leaves[3]); diff --git a/fuel-tx/src/lib.rs b/fuel-tx/src/lib.rs index 927709a1a9..e16196f196 100644 --- a/fuel-tx/src/lib.rs +++ b/fuel-tx/src/lib.rs @@ -4,8 +4,13 @@ // Wrong clippy convention; check // https://rust-lang.github.io/api-guidelines/naming.html #![allow(clippy::wrong_self_convention)] -#![deny(clippy::cast_possible_truncation)] -#![deny(clippy::string_slice)] +#![deny( + clippy::arithmetic_side_effects, + clippy::cast_sign_loss, + clippy::cast_possible_truncation, + clippy::cast_possible_wrap, + clippy::string_slice +)] #![deny(unused_crate_dependencies)] #![deny(unsafe_code)] @@ -91,7 +96,6 @@ pub use transaction::{ FormatValidityChecks, GasCosts, GasCostsValues, - GasUnit, Mint, PredicateParameters, Script, diff --git a/fuel-tx/src/tests/mod.rs b/fuel-tx/src/tests/mod.rs index c788236abf..5929a30612 100644 --- a/fuel-tx/src/tests/mod.rs +++ b/fuel-tx/src/tests/mod.rs @@ -1,3 +1,5 @@ +#![allow(clippy::arithmetic_side_effects, clippy::cast_possible_truncation)] + mod offset; mod valid_cases; diff --git a/fuel-tx/src/transaction.rs b/fuel-tx/src/transaction.rs index 925e047361..f7597eb56f 100644 --- a/fuel-tx/src/transaction.rs +++ b/fuel-tx/src/transaction.rs @@ -64,7 +64,6 @@ pub use consensus_parameters::{ FeeParameters, GasCosts, GasCostsValues, - GasUnit, PredicateParameters, ScriptParameters, TxParameters, diff --git a/fuel-tx/src/transaction/consensus_parameters.rs b/fuel-tx/src/transaction/consensus_parameters.rs index 40f8367212..6edc8bfb6f 100644 --- a/fuel-tx/src/transaction/consensus_parameters.rs +++ b/fuel-tx/src/transaction/consensus_parameters.rs @@ -12,9 +12,10 @@ pub use gas::{ DependentCost, GasCosts, GasCostsValues, - GasUnit, }; +use crate::consts::BALANCE_ENTRY_SIZE; + #[cfg(feature = "test-helpers")] const MAX_GAS: u64 = 100_000_000; #[cfg(feature = "test-helpers")] @@ -512,11 +513,19 @@ impl TxParameters { /// Transaction memory offset in VM runtime pub const fn tx_offset(&self) -> usize { - Bytes32::LEN // Tx ID - + AssetId::LEN // Base asset ID - // Asset ID/Balance coin input pairs - + self.max_inputs() as usize * (AssetId::LEN + WORD_SIZE) + let Some(balances_size) = + (self.max_inputs() as usize).checked_mul(BALANCE_ENTRY_SIZE) + else { + panic!( + "Consensus parameters shouldn't allow max_inputs to cause overflow here" + ); + }; + + balances_size.saturating_add( + Bytes32::LEN // Tx ID + WORD_SIZE // Tx size + + AssetId::LEN, // Base asset ID + ) } /// Replace the max inputs with the given argument diff --git a/fuel-tx/src/transaction/consensus_parameters/gas.rs b/fuel-tx/src/transaction/consensus_parameters/gas.rs index 8981c61a59..73e5d04f25 100644 --- a/fuel-tx/src/transaction/consensus_parameters/gas.rs +++ b/fuel-tx/src/transaction/consensus_parameters/gas.rs @@ -16,134 +16,6 @@ use fuel_types::Word; #[allow(dead_code)] mod default_gas_costs; -/// Gas unit cost that embeds a unit price and operations count. -/// -/// The operations count will be the argument of every variant except -/// `Accumulated`, that will hold the total acumulated gas. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] -pub enum GasUnit { - /// Atomic operation. - Atom(Word), - /// Arithmetic operation. - Arithmetic(Word), - /// Expensive arithmetic operation. - ArithmeticExpensive(Word), - /// Write to a register. - RegisterWrite(Word), - /// Branching cost. - Branching(Word), - /// Hash crypto operation. - Hash(Word), - /// Memory ownership test cost. - MemoryOwnership(Word), - /// Cost of memory read, per byte. - MemoryRead(Word), - /// Cost of memory write, per byte. - MemoryWrite(Word), - /// Crypto public key recover. - Recover(Word), - /// Cost to read bytes from a storage tree - StorageReadTree(Word), - /// Cost to write bytes to a storage tree - StorageWriteTree(Word), - /// Cost to write a word to the storage - StorageWriteWord(Word), - /// Accumulated cost of several operations. - Accumulated(Word), -} - -impl GasUnit { - /// Return the `cost := price ยท N`. - pub const fn cost(&self) -> Word { - use GasUnit::*; - - match self { - Atom(1) => self.unit_price(), - Arithmetic(1) => self.unit_price(), - ArithmeticExpensive(1) => self.unit_price(), - RegisterWrite(1) => self.unit_price(), - Branching(1) => self.unit_price(), - Hash(1) => self.unit_price(), - MemoryOwnership(1) => self.unit_price(), - MemoryRead(1) => self.unit_price(), - MemoryWrite(1) => self.unit_price(), - Recover(1) => self.unit_price(), - StorageReadTree(1) => self.unit_price(), - StorageWriteTree(1) => self.unit_price(), - StorageWriteWord(1) => self.unit_price(), - - Atom(n) => *n * Atom(1).cost(), - Arithmetic(n) => *n * Arithmetic(1).cost(), - ArithmeticExpensive(n) => *n * ArithmeticExpensive(1).cost(), - RegisterWrite(n) => *n * RegisterWrite(1).cost(), - Branching(n) => *n * Branching(1).cost(), - Hash(n) => *n * Hash(1).cost(), - MemoryOwnership(n) => *n * MemoryOwnership(1).cost(), - MemoryRead(n) => *n * MemoryRead(1).cost(), - MemoryWrite(n) => *n * MemoryWrite(1).cost(), - Recover(n) => *n * Recover(1).cost(), - StorageReadTree(n) => *n * StorageReadTree(1).cost(), - StorageWriteTree(n) => *n * StorageWriteTree(1).cost(), - StorageWriteWord(n) => *n * StorageWriteWord(1).cost(), - Accumulated(c) => *c, - } - } - - /// Return the price per unit. - pub const fn unit_price(&self) -> Word { - use GasUnit::*; - - // the values are defined empirically from tests performed in fuel-core-benches. - // - // the worst case scenario of execution is a memory write for chunks larger than - // the OS page size, that is commonly set to `4096` bytes. - // - // the storage, as expected from a production-ready implementation, didn't present - // alarming computing power demand from increased operations because - // tree-seek should be, in worst case scenario, logarithmic. - match self { - // base price for pc inc - Atom(_) => 10, - // arithmetic operations - Arithmetic(_) => 15, - // expensive arith operations - ArithmeticExpensive(_) => 100, - // write a register with reserved branching check - RegisterWrite(_) => 20, - // branching different than reserved reg - Branching(_) => 20, - // native hash operation - Hash(_) => 300, - // memory ownership branching check - MemoryOwnership(_) => 20, - // memory read per page. should increase exponentially to the number of used - // pages - MemoryRead(_) => 15, - // memory write per page. should increase exponentially to the number of used - // pages - MemoryWrite(_) => 20, - // native ecrecover operation - Recover(_) => 950, - // storage read. the storage backend should offer logarithmic worst case - // scenarios - StorageReadTree(_) => 75, - // storage write. the storage backend should offer logarithmic worst case - // scenarios - StorageWriteTree(_) => 150, - // storage write word. the storage backend should offer logarithmic worst case - // scenarios - StorageWriteWord(_) => 130, - // accumulated cost for different operations - Accumulated(c) => *c, - } - } - - /// Combine two gas computations, accumulating their cost. - pub const fn join(self, other: Self) -> Self { - Self::Accumulated(self.cost() + other.cost()) - } -} - /// Gas costings for every op. /// The inner values are wrapped in an [`Arc`] /// so this is cheap to clone. @@ -1006,6 +878,7 @@ pub enum DependentCost { /// higher the `units_per_gas`, the less additional cost you will incur /// for a given number of units, because you need more units to increase /// the total cost. + /// This must be nonzero. units_per_gas: Word, }, @@ -1331,7 +1204,7 @@ impl DependentCost { pub fn resolve(&self, units: Word) -> Word { let base = self.base(); let dependent_value = self.resolve_without_base(units); - base + dependent_value + base.saturating_add(dependent_value) } pub fn resolve_without_base(&self, units: Word) -> Word { @@ -1342,7 +1215,9 @@ impl DependentCost { // where: // x is the number of units // 1/m is the gas_per_unit - units.saturating_div(*units_per_gas) + units + .checked_div(*units_per_gas) + .expect("units_per_gas cannot be zero") } DependentCost::HeavyOperation { gas_per_unit, .. } => { // Apply the linear transformation: diff --git a/fuel-tx/src/transaction/fee.rs b/fuel-tx/src/transaction/fee.rs index ac0524e827..b8d41d3228 100644 --- a/fuel-tx/src/transaction/fee.rs +++ b/fuel-tx/src/transaction/fee.rs @@ -126,10 +126,10 @@ where let vm_initialization_gas = gas_costs.vm_initialization().resolve(bytes_size as Word); - let bytes_gas = bytes_size as u64 * fee.gas_per_byte(); // It's okay to saturate because we have the `max_gas_per_tx` rule for transaction // validity. In the production, the value always will be lower than // `u64::MAX`. + let bytes_gas = fee.gas_per_byte().saturating_mul(bytes_size as u64); tx.gas_used_by_inputs(gas_costs) .saturating_add(tx.gas_used_by_metadata(gas_costs)) .saturating_add(bytes_gas) diff --git a/fuel-tx/src/transaction/metadata.rs b/fuel-tx/src/transaction/metadata.rs index 44d90a975a..c73acfb221 100644 --- a/fuel-tx/src/transaction/metadata.rs +++ b/fuel-tx/src/transaction/metadata.rs @@ -59,7 +59,8 @@ pub struct CommonMetadata { impl CommonMetadata { /// Computes the `Metadata` for the `tx` transaction. - pub fn compute(tx: &Tx, chain_id: &ChainId) -> Self + /// Returns `None` if the transaction is invalid. + pub fn compute(tx: &Tx, chain_id: &ChainId) -> Result where Tx: UniqueIdentifier, Tx: field::Inputs, @@ -78,54 +79,44 @@ impl CommonMetadata { .collect_vec(); let mut offset = tx.inputs_offset(); - let inputs_offset = offset; - let inputs_offset_at = tx - .inputs() - .iter() - .map(|input| { - let i = offset; - offset += input.size(); - i - }) - .collect_vec(); - - let outputs_offset = offset; - #[cfg(feature = "internals")] - assert_eq!(outputs_offset, tx.outputs_offset()); - - let outputs_offset_at = tx - .outputs() - .iter() - .map(|output| { - let i = offset; - offset += output.size(); - i - }) - .collect_vec(); + let mut inputs_offset_at = Vec::new(); + for (index, input) in tx.inputs().iter().enumerate() { + let i = offset; + offset = offset + .checked_add(input.size()) + .ok_or(ValidityError::SerializedInputTooLarge { index })?; + inputs_offset_at.push(i); + } - let witnesses_offset = offset; - #[cfg(feature = "internals")] - assert_eq!(witnesses_offset, tx.witnesses_offset()); + let mut offset = tx.outputs_offset(); + let mut outputs_offset_at = Vec::new(); + for (index, output) in tx.outputs().iter().enumerate() { + let i = offset; + offset = offset + .checked_add(output.size()) + .ok_or(ValidityError::SerializedOutputTooLarge { index })?; + outputs_offset_at.push(i); + } - let witnesses_offset_at = tx - .witnesses() - .iter() - .map(|witness| { - let i = offset; - offset += witness.size(); - i - }) - .collect_vec(); + let mut offset = tx.witnesses_offset(); + let mut witnesses_offset_at = Vec::new(); + for (index, witnesses) in tx.witnesses().iter().enumerate() { + let i = offset; + offset = offset + .checked_add(witnesses.size()) + .ok_or(ValidityError::SerializedWitnessTooLarge { index })?; + witnesses_offset_at.push(i); + } - Self { + Ok(Self { id, - inputs_offset, + inputs_offset: tx.inputs_offset(), inputs_offset_at, inputs_predicate_offset_at, - outputs_offset, + outputs_offset: tx.outputs_offset(), outputs_offset_at, - witnesses_offset, + witnesses_offset: tx.witnesses_offset(), witnesses_offset_at, - } + }) } } diff --git a/fuel-tx/src/transaction/policies.rs b/fuel-tx/src/transaction/policies.rs index a77d0b85ae..53459a1b75 100644 --- a/fuel-tx/src/transaction/policies.rs +++ b/fuel-tx/src/transaction/policies.rs @@ -210,6 +210,7 @@ impl Serialize for Policies { self.bits.bits().size_static() } + #[allow(clippy::arithmetic_side_effects)] // Bit count is not large enough to overflow. fn size_dynamic(&self) -> usize { self.bits.bits().count_ones() as usize * Word::MIN.size() } diff --git a/fuel-tx/src/transaction/types/chargeable_transaction.rs b/fuel-tx/src/transaction/types/chargeable_transaction.rs index 1bdf666542..9b1d8aa7ef 100644 --- a/fuel-tx/src/transaction/types/chargeable_transaction.rs +++ b/fuel-tx/src/transaction/types/chargeable_transaction.rs @@ -214,7 +214,8 @@ mod field { return *inputs_offset; } - self.policies_offset() + self.policies.size_dynamic() + self.policies_offset() + .saturating_add(self.policies.size_dynamic()) } #[inline(always)] @@ -232,13 +233,14 @@ mod field { if idx < self.inputs.len() { Some( - self.inputs_offset() - + self - .inputs() + self.inputs_offset().saturating_add( + self.inputs() .iter() .take(idx) .map(|i| i.size()) - .sum::(), + .reduce(usize::saturating_add) + .unwrap_or_default(), + ), ) } else { None @@ -263,7 +265,8 @@ mod field { input .predicate_offset() .and_then(|predicate| { - self.inputs_offset_at(idx).map(|inputs| inputs + predicate) + self.inputs_offset_at(idx) + .map(|inputs| inputs.saturating_add(predicate)) }) .zip( input @@ -298,7 +301,13 @@ mod field { return *outputs_offset; } - self.inputs_offset() + self.inputs().iter().map(|i| i.size()).sum::() + self.inputs_offset().saturating_add( + self.inputs() + .iter() + .map(|i| i.size()) + .reduce(usize::saturating_add) + .unwrap_or_default(), + ) } #[inline(always)] @@ -316,13 +325,14 @@ mod field { if idx < self.outputs.len() { Some( - self.outputs_offset() - + self - .outputs() + self.outputs_offset().saturating_add( + self.outputs() .iter() .take(idx) .map(|i| i.size()) - .sum::(), + .reduce(usize::saturating_add) + .unwrap_or_default(), + ), ) } else { None @@ -357,7 +367,13 @@ mod field { return *witnesses_offset; } - self.outputs_offset() + self.outputs().iter().map(|i| i.size()).sum::() + self.outputs_offset().saturating_add( + self.outputs() + .iter() + .map(|i| i.size()) + .reduce(usize::saturating_add) + .unwrap_or_default(), + ) } #[inline(always)] @@ -376,13 +392,14 @@ mod field { if idx < self.witnesses.len() { Some( - self.witnesses_offset() - + self - .witnesses() + self.witnesses_offset().saturating_add( + self.witnesses() .iter() .take(idx) .map(|i| i.size()) - .sum::(), + .reduce(usize::saturating_add) + .unwrap_or_default(), + ), ) } else { None diff --git a/fuel-tx/src/transaction/types/create.rs b/fuel-tx/src/transaction/types/create.rs index 3ead4d1014..e44cb506ab 100644 --- a/fuel-tx/src/transaction/types/create.rs +++ b/fuel-tx/src/transaction/types/create.rs @@ -258,7 +258,7 @@ impl crate::Cacheable for Create { fn precompute(&mut self, chain_id: &ChainId) -> Result<(), ValidityError> { self.metadata = None; self.metadata = Some(ChargeableMetadata { - common: CommonMetadata::compute(self, chain_id), + common: CommonMetadata::compute(self, chain_id)?, body: CreateMetadata::compute(self)?, }); Ok(()) @@ -302,7 +302,7 @@ mod field { #[inline(always)] fn salt_offset_static() -> usize { - Self::bytecode_witness_index_offset_static() + WORD_SIZE + Self::bytecode_witness_index_offset_static().saturating_add(WORD_SIZE) } } @@ -321,17 +321,22 @@ mod field { #[inline(always)] fn storage_slots_offset_static() -> usize { - Self::salt_offset_static() + Salt::LEN + Self::salt_offset_static().saturating_add( + Salt::LEN + WORD_SIZE // Storage slots size + WORD_SIZE // Policies size + WORD_SIZE // Inputs size + WORD_SIZE // Outputs size - + WORD_SIZE // Witnesses size + + WORD_SIZE, // Witnesses size + ) } fn storage_slots_offset_at(&self, idx: usize) -> Option { if idx < self.body.storage_slots.len() { - Some(Self::storage_slots_offset_static() + idx * StorageSlot::SLOT_SIZE) + Some( + Self::storage_slots_offset_static() + .checked_add(idx.checked_mul(StorageSlot::SLOT_SIZE)?)?, + ) } else { None } @@ -348,8 +353,12 @@ mod field { } fn body_offset_end(&self) -> usize { - Self::storage_slots_offset_static() - + self.body.storage_slots.len() * StorageSlot::SLOT_SIZE + Self::storage_slots_offset_static().saturating_add( + self.body + .storage_slots + .len() + .saturating_mul(StorageSlot::SLOT_SIZE), + ) } } } diff --git a/fuel-tx/src/transaction/types/input.rs b/fuel-tx/src/transaction/types/input.rs index 4e4033e74f..ce45015ddb 100644 --- a/fuel-tx/src/transaction/types/input.rs +++ b/fuel-tx/src/transaction/types/input.rs @@ -775,18 +775,16 @@ impl Input { impl Serialize for Input { fn size_static(&self) -> usize { - canonical::add_sizes( - 8, // Discriminant - match self { - Input::CoinSigned(coin) => coin.size_static(), - Input::CoinPredicate(coin) => coin.size_static(), - Input::Contract(contract) => contract.size_static(), - Input::MessageCoinSigned(message) => message.size_static(), - Input::MessageCoinPredicate(message) => message.size_static(), - Input::MessageDataSigned(message) => message.size_static(), - Input::MessageDataPredicate(message) => message.size_static(), - }, - ) + (match self { + Input::CoinSigned(coin) => coin.size_static(), + Input::CoinPredicate(coin) => coin.size_static(), + Input::Contract(contract) => contract.size_static(), + Input::MessageCoinSigned(message) => message.size_static(), + Input::MessageCoinPredicate(message) => message.size_static(), + Input::MessageDataSigned(message) => message.size_static(), + Input::MessageDataPredicate(message) => message.size_static(), + }) + .saturating_add(8) // Discriminant } fn size_dynamic(&self) -> usize { diff --git a/fuel-tx/src/transaction/types/mint.rs b/fuel-tx/src/transaction/types/mint.rs index 9ac204e769..c8b79fe589 100644 --- a/fuel-tx/src/transaction/types/mint.rs +++ b/fuel-tx/src/transaction/types/mint.rs @@ -170,7 +170,7 @@ mod field { #[inline(always)] fn input_contract_offset(&self) -> usize { - Self::tx_pointer_static() + TxPointer::LEN + Self::tx_pointer_static().saturating_add(TxPointer::LEN) } } @@ -187,7 +187,8 @@ mod field { #[inline(always)] fn output_contract_offset(&self) -> usize { - self.input_contract_offset() + self.input_contract.size() + self.input_contract_offset() + .saturating_add(self.input_contract.size()) } } @@ -204,7 +205,8 @@ mod field { #[inline(always)] fn mint_amount_offset(&self) -> usize { - self.output_contract_offset() + self.output_contract.size() + self.output_contract_offset() + .saturating_add(self.output_contract.size()) } } @@ -221,7 +223,7 @@ mod field { #[inline(always)] fn mint_asset_id_offset(&self) -> usize { - self.mint_amount_offset() + WORD_SIZE + self.mint_amount_offset().saturating_add(WORD_SIZE) } } @@ -238,7 +240,7 @@ mod field { #[inline(always)] fn gas_price_offset(&self) -> usize { - self.mint_asset_id_offset() + AssetId::LEN + self.mint_asset_id_offset().saturating_add(AssetId::LEN) } } } diff --git a/fuel-tx/src/transaction/types/script.rs b/fuel-tx/src/transaction/types/script.rs index 36fa9613f5..dcba11451b 100644 --- a/fuel-tx/src/transaction/types/script.rs +++ b/fuel-tx/src/transaction/types/script.rs @@ -145,7 +145,7 @@ impl crate::Cacheable for Script { fn precompute(&mut self, chain_id: &ChainId) -> Result<(), ValidityError> { self.metadata = None; self.metadata = Some(ChargeableMetadata { - common: CommonMetadata::compute(self, chain_id), + common: CommonMetadata::compute(self, chain_id)?, body: ScriptMetadata { script_data_offset: self.script_data_offset(), }, @@ -188,7 +188,7 @@ mod field { #[inline(always)] fn receipts_root_offset_static() -> usize { - Self::script_gas_limit_offset_static() + WORD_SIZE + Self::script_gas_limit_offset_static().saturating_add(WORD_SIZE) } } @@ -205,14 +205,15 @@ mod field { #[inline(always)] fn script_offset_static() -> usize { - Self::receipts_root_offset_static() - + Bytes32::LEN // Receipts root + Self::receipts_root_offset_static().saturating_add( + Bytes32::LEN // Receipts root + WORD_SIZE // Script size + WORD_SIZE // Script data size + WORD_SIZE // Policies size + WORD_SIZE // Inputs size + WORD_SIZE // Outputs size - + WORD_SIZE // Witnesses size + + WORD_SIZE, // Witnesses size + ) } } diff --git a/fuel-tx/src/transaction/types/upgrade.rs b/fuel-tx/src/transaction/types/upgrade.rs index 544a8ea976..2ed6a97c65 100644 --- a/fuel-tx/src/transaction/types/upgrade.rs +++ b/fuel-tx/src/transaction/types/upgrade.rs @@ -266,7 +266,7 @@ impl crate::Cacheable for Upgrade { fn precompute(&mut self, chain_id: &ChainId) -> Result<(), ValidityError> { self.metadata = None; self.metadata = Some(ChargeableMetadata { - common: CommonMetadata::compute(self, chain_id), + common: CommonMetadata::compute(self, chain_id)?, body: UpgradeMetadata::compute(self)?, }); Ok(()) @@ -307,11 +307,14 @@ mod field { } fn body_offset_end(&self) -> usize { - Self::upgrade_purpose_offset_static() + self.body.purpose.size() - + WORD_SIZE // Policies size - + WORD_SIZE // Inputs size - + WORD_SIZE // Outputs size - + WORD_SIZE // Witnesses size + Self::upgrade_purpose_offset_static() + .saturating_add(self.body.purpose.size()) + .saturating_add( + WORD_SIZE // Policies size + + WORD_SIZE // Inputs size + + WORD_SIZE // Outputs size + + WORD_SIZE, // Witnesses size + ) } } } diff --git a/fuel-tx/src/transaction/types/upload.rs b/fuel-tx/src/transaction/types/upload.rs index 91320a4147..9893b2ad62 100644 --- a/fuel-tx/src/transaction/types/upload.rs +++ b/fuel-tx/src/transaction/types/upload.rs @@ -276,7 +276,7 @@ impl crate::Cacheable for Upload { fn precompute(&mut self, chain_id: &ChainId) -> Result<(), ValidityError> { self.metadata = None; self.metadata = Some(ChargeableMetadata { - common: CommonMetadata::compute(self, chain_id), + common: CommonMetadata::compute(self, chain_id)?, body: UploadMetadata {}, }); Ok(()) @@ -324,7 +324,7 @@ mod field { #[inline(always)] fn bytecode_witness_index_offset_static() -> usize { - Self::bytecode_root_offset_static() + Bytes32::LEN + Self::bytecode_root_offset_static().saturating_add(Bytes32::LEN) } } @@ -341,7 +341,7 @@ mod field { #[inline(always)] fn subsection_index_offset_static() -> usize { - Self::bytecode_witness_index_offset_static() + WORD_SIZE + Self::bytecode_witness_index_offset_static().saturating_add(WORD_SIZE) } } @@ -358,7 +358,7 @@ mod field { #[inline(always)] fn subsections_number_offset_static() -> usize { - Self::subsection_index_offset_static() + WORD_SIZE + Self::subsection_index_offset_static().saturating_add(WORD_SIZE) } } @@ -375,12 +375,14 @@ mod field { #[inline(always)] fn proof_set_offset_static() -> usize { - Self::subsections_number_offset_static() + WORD_SIZE + Self::subsections_number_offset_static().saturating_add( + WORD_SIZE + WORD_SIZE // Proof set size + WORD_SIZE // Policies size + WORD_SIZE // Inputs size + WORD_SIZE // Outputs size - + WORD_SIZE // Witnesses size + + WORD_SIZE, // Witnesses size + ) } } @@ -394,7 +396,8 @@ mod field { } fn body_offset_end(&self) -> usize { - Self::proof_set_offset_static() + self.body.proof_set.len() * Bytes32::LEN + Self::proof_set_offset_static() + .saturating_add(self.body.proof_set.len().saturating_mul(Bytes32::LEN)) } } } diff --git a/fuel-tx/src/transaction/types/utxo_id.rs b/fuel-tx/src/transaction/types/utxo_id.rs index 97382b2319..014a9633bb 100644 --- a/fuel-tx/src/transaction/types/utxo_id.rs +++ b/fuel-tx/src/transaction/types/utxo_id.rs @@ -97,8 +97,8 @@ impl str::FromStr for UtxoId { /// the last two characters are the output index and the part /// optionally preceeding it is the transaction id. fn from_str(s: &str) -> Result { - const ERR: &str = "Invalid encoded byte"; - let s = s.trim_start_matches("0x"); + const ERR: &str = "Invalid encoded byte in UtxoId"; + let s = s.strip_prefix("0x").unwrap_or(s); Ok(if s.is_empty() { UtxoId::new(Bytes32::default(), 0) @@ -108,11 +108,13 @@ impl str::FromStr for UtxoId { u16::from_str_radix(s, 16).map_err(|_| ERR)?, ) } else { + #[allow(clippy::arithmetic_side_effects)] // Checked above let i = s.len() - 4; if !s.is_char_boundary(i) { return Err(ERR) } let (tx_id, output_index) = s.split_at(i); + let tx_id = tx_id.strip_suffix(':').unwrap_or(tx_id); UtxoId::new( Bytes32::from_str(tx_id)?, @@ -221,6 +223,18 @@ mod tests { Ok(()) } + #[test] + fn from_str_utxo_id_colon_separator() -> Result<(), &'static str> { + let utxo_id = UtxoId::from_str( + "0c0000000000000000000000000000000000000000000000000000000000000b:abcd", + )?; + + assert_eq!(utxo_id.output_index, 0xabcd); + assert_eq!(utxo_id.tx_id[31], 11); + assert_eq!(utxo_id.tx_id[0], 12); + Ok(()) + } + /// See https://github.com/FuelLabs/fuel-vm/issues/521 #[test] fn from_str_utxo_id_multibyte_bug() { diff --git a/fuel-tx/src/transaction/validity/error.rs b/fuel-tx/src/transaction/validity/error.rs index a13374f37a..f42e7fd7ef 100644 --- a/fuel-tx/src/transaction/validity/error.rs +++ b/fuel-tx/src/transaction/validity/error.rs @@ -162,4 +162,16 @@ pub enum ValidityError { BalanceOverflow, /// The given gas costs is are too large GasCostsCoinsOverflow, + /// Serialized input length is too large. + SerializedInputTooLarge { + index: usize, + }, + /// Serialized output length is too large. + SerializedOutputTooLarge { + index: usize, + }, + /// Serialized witness length is too large. + SerializedWitnessTooLarge { + index: usize, + }, } diff --git a/fuel-tx/src/tx_pointer.rs b/fuel-tx/src/tx_pointer.rs index 95872e5666..03bedcf46b 100644 --- a/fuel-tx/src/tx_pointer.rs +++ b/fuel-tx/src/tx_pointer.rs @@ -85,7 +85,7 @@ impl str::FromStr for TxPointer { /// - 8 characters for block height /// - 4 characters for tx index fn from_str(s: &str) -> Result { - const ERR: &str = "Invalid encoded byte"; + const ERR: &str = "Invalid encoded byte in TxPointer"; if s.len() != 12 || !s.is_char_boundary(8) { return Err(ERR) diff --git a/fuel-types/Cargo.toml b/fuel-types/Cargo.toml index 3acae8bdf8..c97e94e98c 100644 --- a/fuel-types/Cargo.toml +++ b/fuel-types/Cargo.toml @@ -12,7 +12,7 @@ description = "Atomic types of the FuelVM." [dependencies] fuel-derive = { workspace = true } -hex = { version = "0.4", default-features = false, optional = true } +hex = { version = "0.4", default-features = false } rand = { version = "0.8", default-features = false, optional = true } serde = { version = "1.0", default-features = false, features = ["derive", "alloc"], optional = true } wasm-bindgen = { version = "0.2.88", optional = true } @@ -31,7 +31,7 @@ typescript = ["wasm-bindgen"] alloc = ["hex/alloc"] random = ["rand"] serde = ["dep:serde", "alloc"] -std = ["alloc", "serde?/std", "hex?/std"] +std = ["alloc", "serde?/std", "hex/std"] unsafe = [] [[bench]] diff --git a/fuel-types/src/array_types.rs b/fuel-types/src/array_types.rs index 1c57f4a37d..f62f0c2251 100644 --- a/fuel-types/src/array_types.rs +++ b/fuel-types/src/array_types.rs @@ -27,8 +27,6 @@ use rand::{ #[cfg(all(feature = "alloc", feature = "typescript"))] use alloc::format; -use crate::hex_val; - macro_rules! key { ($i:ident, $s:expr) => { #[derive(Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] @@ -246,14 +244,15 @@ macro_rules! key_methods { write!(f, "0x")? } - match f.width() { - Some(w) if w > 0 => { - self.0.chunks(2 * Self::LEN / w).try_for_each(|c| { - write!(f, "{:02x}", c.iter().fold(0u8, |acc, x| acc ^ x)) - }) - } - - _ => self.0.iter().try_for_each(|b| write!(f, "{:02x}", &b)), + if let Some(w) = f + .width() + .and_then(|w| Self::LEN.saturating_mul(2).checked_div(w)) + { + self.0.chunks(w).try_for_each(|c| { + write!(f, "{:02x}", c.iter().fold(0u8, |acc, x| acc ^ x)) + }) + } else { + self.0.iter().try_for_each(|b| write!(f, "{:02x}", &b)) } } } @@ -264,14 +263,15 @@ macro_rules! key_methods { write!(f, "0x")? } - match f.width() { - Some(w) if w > 0 => { - self.0.chunks(2 * Self::LEN / w).try_for_each(|c| { - write!(f, "{:02X}", c.iter().fold(0u8, |acc, x| acc ^ x)) - }) - } - - _ => self.0.iter().try_for_each(|b| write!(f, "{:02X}", &b)), + if let Some(w) = f + .width() + .and_then(|w| Self::LEN.saturating_mul(2).checked_div(w)) + { + self.0.chunks(w).try_for_each(|c| { + write!(f, "{:02X}", c.iter().fold(0u8, |acc, x| acc ^ x)) + }) + } else { + self.0.iter().try_for_each(|b| write!(f, "{:02X}", &b)) } } } @@ -292,25 +292,10 @@ macro_rules! key_methods { type Err = &'static str; fn from_str(s: &str) -> Result { - const ERR: &str = "Invalid encoded byte"; - - let alternate = s.starts_with("0x"); - - let mut b = s.bytes(); + const ERR: &str = concat!("Invalid encoded byte in ", stringify!($i)); let mut ret = $i::zeroed(); - - if alternate { - b.next(); - b.next(); - } - - for r in ret.as_mut() { - let h = b.next().and_then(hex_val).ok_or(ERR)?; - let l = b.next().and_then(hex_val).ok_or(ERR)?; - - *r = h << 4 | l; - } - + let s = s.strip_prefix("0x").unwrap_or(s); + hex::decode_to_slice(&s, &mut ret.0).map_err(|_| ERR)?; Ok(ret) } } diff --git a/fuel-types/src/bytes.rs b/fuel-types/src/bytes.rs index 6300699ce2..a1ad4c58bb 100644 --- a/fuel-types/src/bytes.rs +++ b/fuel-types/src/bytes.rs @@ -11,11 +11,13 @@ pub const fn padded_len(bytes: &[u8]) -> Option { /// Return the word-padded length of an arbitrary length. /// Returns None if the length is too large to be represented as usize. +#[allow(clippy::arithmetic_side_effects)] // Safety: (a % b) < b pub const fn padded_len_usize(len: usize) -> Option { - if len % WORD_SIZE == 0 { + let modulo = len % WORD_SIZE; + if modulo == 0 { Some(len) } else { - let padding = WORD_SIZE - len % WORD_SIZE; + let padding = WORD_SIZE - modulo; len.checked_add(padding) } } diff --git a/fuel-types/src/canonical.rs b/fuel-types/src/canonical.rs index 60f56e2d7d..ad8871e650 100644 --- a/fuel-types/src/canonical.rs +++ b/fuel-types/src/canonical.rs @@ -72,12 +72,15 @@ pub trait Serialize { const UNALIGNED_BYTES: bool = false; /// Size of the static part of the serialized object, in bytes. + /// Saturates to usize::MAX on overflow. fn size_static(&self) -> usize; /// Size of the dynamic part, in bytes. + /// Saturates to usize::MAX on overflow. fn size_dynamic(&self) -> usize; /// Total size of the serialized object, in bytes. + /// Saturates to usize::MAX on overflow. fn size(&self) -> usize { self.size_static().saturating_add(self.size_dynamic()) } @@ -176,22 +179,23 @@ pub trait Deserialize: Sized { } } -/// Returns the sum of two sizes, or panics if the sum overflows. -pub const fn add_sizes(a: usize, b: usize) -> usize { - a.saturating_add(b) -} - /// The data of each field should be aligned to 64 bits. pub const ALIGN: usize = 8; /// The number of padding bytes required to align the given length correctly. +#[allow(clippy::arithmetic_side_effects)] // Safety: (a % b) < b const fn alignment_bytes(len: usize) -> usize { - (ALIGN - (len % ALIGN)) % ALIGN + let modulo = len % ALIGN; + if modulo == 0 { + 0 + } else { + ALIGN - modulo + } } -/// Size after alignment. +/// Size after alignment. Saturates on overflow. pub const fn aligned_size(len: usize) -> usize { - add_sizes(len, alignment_bytes(len)) + len.saturating_add(alignment_bytes(len)) } macro_rules! impl_for_primitives { @@ -283,7 +287,12 @@ impl Serialize for Vec { if T::UNALIGNED_BYTES { aligned_size(self.len()) } else { - aligned_size(self.iter().map(|e| e.size()).sum()) + aligned_size( + self.iter() + .map(|e| e.size()) + .reduce(usize::saturating_add) + .unwrap_or_default(), + ) } } @@ -358,7 +367,12 @@ impl Serialize for [T; N] { if T::UNALIGNED_BYTES { aligned_size(N) } else { - aligned_size(self.iter().map(|e| e.size_static()).sum()) + aligned_size( + self.iter() + .map(|e| e.size_static()) + .reduce(usize::saturating_add) + .unwrap_or_default(), + ) } } @@ -367,7 +381,12 @@ impl Serialize for [T; N] { if T::UNALIGNED_BYTES { 0 } else { - aligned_size(self.iter().map(|e| e.size_dynamic()).sum()) + aligned_size( + self.iter() + .map(|e| e.size_dynamic()) + .reduce(usize::saturating_add) + .unwrap_or_default(), + ) } } diff --git a/fuel-types/src/fmt.rs b/fuel-types/src/fmt.rs index 25f5aeac4d..e918777636 100644 --- a/fuel-types/src/fmt.rs +++ b/fuel-types/src/fmt.rs @@ -10,7 +10,7 @@ pub fn fmt_truncated_hex( f: &mut Formatter, ) -> fmt::Result { let formatted = if data.len() > N { - let mut s = hex::encode(&data[0..N - 3]); + let mut s = hex::encode(&data[0..N.saturating_sub(3)]); s.push_str("..."); s } else { @@ -27,7 +27,7 @@ pub fn fmt_option_truncated_hex( ) -> fmt::Result { if let Some(data) = data { let formatted = if data.len() > N { - let mut s = hex::encode(&data[0..N - 3]); + let mut s = hex::encode(&data[0..N.saturating_sub(3)]); s.push_str("..."); s } else { diff --git a/fuel-types/src/lib.rs b/fuel-types/src/lib.rs index bb48145a73..3f2fbee715 100644 --- a/fuel-types/src/lib.rs +++ b/fuel-types/src/lib.rs @@ -4,7 +4,13 @@ #![warn(unsafe_code)] #![warn(missing_docs)] #![deny(unused_crate_dependencies)] -#![deny(clippy::cast_possible_truncation)] +#![deny( + clippy::arithmetic_side_effects, + clippy::cast_sign_loss, + clippy::cast_possible_truncation, + clippy::cast_possible_wrap, + clippy::string_slice +)] // `fuel-derive` requires `fuel_types` import // TODO: Move canonical serialization to `fuel-canonical` crate #![allow(unused_crate_dependencies)] @@ -49,12 +55,3 @@ pub type Immediate18 = u32; /// 24-bits immediate value type pub type Immediate24 = u32; - -pub(crate) const fn hex_val(c: u8) -> Option { - match c { - b'A'..=b'F' => Some(c - b'A' + 10), - b'a'..=b'f' => Some(c - b'a' + 10), - b'0'..=b'9' => Some(c - b'0'), - _ => None, - } -} diff --git a/fuel-types/src/numeric_types.rs b/fuel-types/src/numeric_types.rs index 50c25fb36b..7511683115 100644 --- a/fuel-types/src/numeric_types.rs +++ b/fuel-types/src/numeric_types.rs @@ -25,8 +25,6 @@ use rand::{ #[cfg(all(feature = "alloc", feature = "typescript"))] use alloc::vec::Vec; -use crate::hex_val; - macro_rules! key { ($i:ident, $t:ty) => { #[derive(Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] @@ -215,25 +213,10 @@ macro_rules! key_methods { type Err = &'static str; fn from_str(s: &str) -> Result { - const ERR: &str = "Invalid encoded byte"; - - let alternate = s.starts_with("0x"); - - let mut b = s.bytes(); + const ERR: &str = concat!("Invalid encoded byte in ", stringify!($i)); let mut ret = <[u8; SIZE]>::default(); - - if alternate { - b.next(); - b.next(); - } - - for r in ret.as_mut() { - let h = b.next().and_then(hex_val).ok_or(ERR)?; - let l = b.next().and_then(hex_val).ok_or(ERR)?; - - *r = h << 4 | l; - } - + let s = s.strip_prefix("0x").unwrap_or(s); + hex::decode_to_slice(&s, &mut ret).map_err(|_| ERR)?; Ok(ret.into()) } } diff --git a/fuel-types/src/tests/types.rs b/fuel-types/src/tests/types.rs index 947e584b99..c450604bd5 100644 --- a/fuel-types/src/tests/types.rs +++ b/fuel-types/src/tests/types.rs @@ -1,3 +1,5 @@ +#![allow(clippy::arithmetic_side_effects)] + use fuel_types::*; use rand::{ rngs::StdRng, diff --git a/fuel-vm/src/call.rs b/fuel-vm/src/call.rs index d2a6366fcb..88d433ab16 100644 --- a/fuel-vm/src/call.rs +++ b/fuel-vm/src/call.rs @@ -5,6 +5,7 @@ use fuel_asm::{ RegId, }; use fuel_types::{ + bytes::padded_len_usize, canonical::{ Deserialize, Serialize, @@ -116,32 +117,32 @@ impl CallFrame { /// Start of the asset id offset from the beginning of the call frame. pub const fn asset_id_offset() -> usize { - Self::contract_id_offset() + ContractId::LEN + Self::contract_id_offset().saturating_add(ContractId::LEN) } /// Start of the registers offset from the beginning of the call frame. pub const fn registers_offset() -> usize { - Self::asset_id_offset() + AssetId::LEN + Self::asset_id_offset().saturating_add(AssetId::LEN) } /// Start of the code size offset from the beginning of the call frame. pub const fn code_size_offset() -> usize { - Self::registers_offset() + WORD_SIZE * (VM_REGISTER_COUNT) + Self::registers_offset().saturating_add(WORD_SIZE * VM_REGISTER_COUNT) } /// Start of the `a` argument offset from the beginning of the call frame. pub const fn a_offset() -> usize { - Self::code_size_offset() + WORD_SIZE + Self::code_size_offset().saturating_add(WORD_SIZE) } /// Start of the `b` argument offset from the beginning of the call frame. pub const fn b_offset() -> usize { - Self::a_offset() + WORD_SIZE + Self::a_offset().saturating_add(WORD_SIZE) } /// Size of the call frame in bytes. pub const fn serialized_size() -> usize { - Self::b_offset() + WORD_SIZE + Self::b_offset().saturating_add(WORD_SIZE) } /// Registers prior to the called execution. @@ -161,12 +162,14 @@ impl CallFrame { /// Padding to the next word boundary. pub fn code_size_padding(&self) -> usize { - (WORD_SIZE - self.code_size() % WORD_SIZE) % WORD_SIZE + self.total_code_size() + .checked_sub(self.code_size) + .expect("Padding is always less than size + padding") } /// Total code size including padding. pub fn total_code_size(&self) -> usize { - self.code_size() + self.code_size_padding() + padded_len_usize(self.code_size).expect("Code size cannot overflow with padding") } /// `a` argument. diff --git a/fuel-vm/src/checked_transaction.rs b/fuel-vm/src/checked_transaction.rs index 0a58cf333f..d253330aa6 100644 --- a/fuel-vm/src/checked_transaction.rs +++ b/fuel-vm/src/checked_transaction.rs @@ -803,9 +803,9 @@ impl From for CheckError { #[cfg(feature = "random")] #[allow(non_snake_case)] +#[allow(clippy::arithmetic_side_effects, clippy::cast_possible_truncation)] #[cfg(test)] mod tests { - #![allow(clippy::cast_possible_truncation)] use super::*; use alloc::vec; diff --git a/fuel-vm/src/checked_transaction/balances.rs b/fuel-vm/src/checked_transaction/balances.rs index b2dc5b50e8..c25b205daf 100644 --- a/fuel-vm/src/checked_transaction/balances.rs +++ b/fuel-vm/src/checked_transaction/balances.rs @@ -33,7 +33,7 @@ where T: Chargeable + field::Inputs + field::Outputs, { let (mut non_retryable_balances, retryable_balance) = - add_up_input_balances(tx, base_asset_id); + add_up_input_balances(tx, base_asset_id).ok_or(ValidityError::BalanceOverflow)?; let max_fee = tx .policies() @@ -49,10 +49,11 @@ where }) } +/// Returns None if any of the balances would overflow. fn add_up_input_balances( transaction: &T, base_asset_id: &AssetId, -) -> (BTreeMap, Word) { +) -> Option<(BTreeMap, Word)> { let mut non_retryable_balances = BTreeMap::::new(); // The sum of [`AssetId::Base`] from metadata messages. let mut retryable_balance: Word = 0; @@ -67,23 +68,25 @@ fn add_up_input_balances( | Input::CoinSigned(CoinSigned { asset_id, amount, .. }) => { - *non_retryable_balances.entry(*asset_id).or_default() += amount; + let balance = non_retryable_balances.entry(*asset_id).or_default(); + *balance = (*balance).checked_add(*amount)?; } // Sum message coin inputs Input::MessageCoinSigned(MessageCoinSigned { amount, .. }) | Input::MessageCoinPredicate(MessageCoinPredicate { amount, .. }) => { - *non_retryable_balances.entry(*base_asset_id).or_default() += amount; + let balance = non_retryable_balances.entry(*base_asset_id).or_default(); + *balance = (*balance).checked_add(*amount)?; } // Sum data messages Input::MessageDataSigned(MessageDataSigned { amount, .. }) | Input::MessageDataPredicate(MessageDataPredicate { amount, .. }) => { - retryable_balance += *amount; + retryable_balance = retryable_balance.checked_add(*amount)?; } Input::Contract(_) => {} } } - (non_retryable_balances, retryable_balance) + Some((non_retryable_balances, retryable_balance)) } fn deduct_max_fee_from_base_asset( diff --git a/fuel-vm/src/constraints/reg_key.rs b/fuel-vm/src/constraints/reg_key.rs index 870188ed02..ca80f5055e 100644 --- a/fuel-vm/src/constraints/reg_key.rs +++ b/fuel-vm/src/constraints/reg_key.rs @@ -49,6 +49,7 @@ impl WriteRegKey { /// to a program register index. /// /// This subtracts the number of system registers from the key. + #[allow(clippy::arithmetic_side_effects)] // Safety: checked in constructor fn translate(self) -> usize { self.0 - VM_REGISTER_SYSTEM_COUNT } @@ -278,40 +279,34 @@ impl<'r> ProgramRegisters<'r> { a: WriteRegKey, b: WriteRegKey, ) -> Option<(&mut Word, &mut Word)> { - match a.cmp(&b) { - core::cmp::Ordering::Less => { - // Translate the `a` absolute register index to a program register index. - let a = a.translate(); - // Split the array at the first register which is a. - let [i, rest @ ..] = &mut self.0[a..] else { - return None - }; - // Translate the `b` absolute register index to a program register index. - // Subtract 1 because the first register is `a`. - // Subtract `a` registers because we split the array at `a`. - let b = b.translate() - 1 - a; - // Get the `b` register. - let j = &mut rest[b]; - Some((i, j)) - } + if a == b { // Cannot mutably borrow the same register twice. - core::cmp::Ordering::Equal => None, - core::cmp::Ordering::Greater => { - // Translate the `b` absolute register index to a program register index. - let b = b.translate(); - // Split the array at the first register which is b. - let [i, rest @ ..] = &mut self.0[b..] else { - return None - }; - // Translate the `a` absolute register index to a program register index. - // Subtract 1 because the first register is `b`. - // Subtract `b` registers because we split the array at `b`. - let a = a.translate() - 1 - b; - // Get the `a` register. - let j = &mut rest[a]; - Some((j, i)) - } + return None } + + // Order registers + let swap = a > b; + let (a, b) = if swap { (b, a) } else { (a, b) }; + + // Translate the absolute register indices to a program register indeces. + let a = a.translate(); + + // Subtract a + 1 because because we split the array at `a`. + let b = b + .translate() + .checked_sub(a.saturating_add(1)) + .expect("Cannot underflow as the values are ordered"); + + // Split the array at the first register which is a. + let [i, rest @ ..] = &mut self.0[a..] else { + return None + }; + + // Translate the higher absolute register index to a program register index. + // Get the `b` register. + let j = &mut rest[b]; + + Some(if swap { (j, i) } else { (i, j) }) } } diff --git a/fuel-vm/src/constraints/reg_key/tests.rs b/fuel-vm/src/constraints/reg_key/tests.rs index a10ee47ad4..6f09f17726 100644 --- a/fuel-vm/src/constraints/reg_key/tests.rs +++ b/fuel-vm/src/constraints/reg_key/tests.rs @@ -1,3 +1,5 @@ +#![allow(clippy::arithmetic_side_effects, clippy::cast_possible_truncation)] + use alloc::vec::Vec; use super::*; diff --git a/fuel-vm/src/interpreter.rs b/fuel-vm/src/interpreter.rs index 5afc67462f..8833504360 100644 --- a/fuel-vm/src/interpreter.rs +++ b/fuel-vm/src/interpreter.rs @@ -338,7 +338,10 @@ fn current_location( pc: crate::constraints::reg_key::Reg<{ crate::constraints::reg_key::PC }>, is: crate::constraints::reg_key::Reg<{ crate::constraints::reg_key::IS }>, ) -> InstructionLocation { - InstructionLocation::new(current_contract, *pc - *is) + // Safety: pc should always be above is, but fallback to zero here for weird cases, + // as the profiling code should be robust against regards cases like this. + let offset = (*pc).saturating_sub(*is); + InstructionLocation::new(current_contract, offset) } impl AsRef for Interpreter { diff --git a/fuel-vm/src/interpreter/alu/muldiv.rs b/fuel-vm/src/interpreter/alu/muldiv.rs index 9e72b01c57..d097653234 100644 --- a/fuel-vm/src/interpreter/alu/muldiv.rs +++ b/fuel-vm/src/interpreter/alu/muldiv.rs @@ -59,13 +59,22 @@ where /// Divider 0 is treated as `1<<64`. #[allow(clippy::cast_possible_truncation)] pub(crate) fn muldiv(lhs: u64, rhs: u64, divider: u64) -> (u64, u64) { - let intermediate = lhs as u128 * rhs as u128; // Never overflows - if divider == 0 { - ((intermediate >> 64) as u64, 0) - } else { - let result = intermediate / (divider as u128); + // Widen all inputs so we never overflow + let lhs = lhs as u128; + let rhs = rhs as u128; + let divider = divider as u128; + + // Compute intermediate result + let intermediate = lhs + .checked_mul(rhs) + .expect("Cannot overflow as we have enough bits"); + + // Divide + if let Some(result) = intermediate.checked_div(divider) { // We want to truncate the `result` here and return a non-empty `overflow`. (result as u64, (result >> 64) as u64) + } else { + ((intermediate >> 64) as u64, 0) } } diff --git a/fuel-vm/src/interpreter/alu/tests.rs b/fuel-vm/src/interpreter/alu/tests.rs index 6ca6ea030e..de00ca3bda 100644 --- a/fuel-vm/src/interpreter/alu/tests.rs +++ b/fuel-vm/src/interpreter/alu/tests.rs @@ -1,3 +1,5 @@ +#![allow(clippy::arithmetic_side_effects, clippy::cast_possible_truncation)] + use alloc::vec::Vec; use super::*; diff --git a/fuel-vm/src/interpreter/alu/wideint.rs b/fuel-vm/src/interpreter/alu/wideint.rs index ecbcdb1b66..198fec2ba0 100644 --- a/fuel-vm/src/interpreter/alu/wideint.rs +++ b/fuel-vm/src/interpreter/alu/wideint.rs @@ -222,23 +222,27 @@ macro_rules! wideint_ops { let rhs: $t = $t::from_be_bytes(self.memory.read_bytes(c)?); let modulus: $t = $t::from_be_bytes(self.memory.read_bytes(d)?); - let result: $t = if modulus == 0 { - if is_unsafe_math(flag.into()) { - *err = 1; - $t::default() // Zero - } else { - return Err(PanicReason::ArithmeticError.into()); + // Use wider types to avoid overflow + let lhs = [](lhs); + let rhs = [](rhs); + let modulus = [](modulus); + + let pre_mod = lhs.checked_add(rhs) + .expect("Cannot overflow as we're using wider types"); + let result: $t = match pre_mod.checked_rem(modulus) { + Some(result) => { + *err = 0; + // Truncate never loses data as modulus is still in domain of the original type + [](result) + }, + None => { + if is_unsafe_math(flag.into()) { + *err = 1; + $t::default() // Zero + } else { + return Err(PanicReason::ArithmeticError.into()); + } } - } else { - *err = 0; - - // Use wider types to avoid overflow - let lhs = [](lhs); - let rhs = [](rhs); - let modulus = [](modulus); - - // Truncate never loses data as modulus is still in domain of the original type - []((lhs + rhs) % modulus) }; *of = 0; @@ -262,24 +266,25 @@ macro_rules! wideint_ops { let rhs: $t = $t::from_be_bytes(self.memory.read_bytes(c)?); let modulus: $t = $t::from_be_bytes(self.memory.read_bytes(d)?); - let result: $t = if modulus == 0 { - if is_unsafe_math(flag.into()) { - *err = 1; - $t::default() // Zero - } else { - return Err(PanicReason::ArithmeticError.into()); - } - } else { - *err = 0; - - let lhs = [](lhs); - let rhs = [](rhs); - let modulus = [](modulus); + let lhs = [](lhs); + let rhs = [](rhs); + let modulus = [](modulus); - let result = lhs.full_mul(rhs) % modulus; + let result = match lhs.full_mul(rhs).checked_rem(modulus) { + None => { + if is_unsafe_math(flag.into()) { + *err = 1; + $t::default() // Zero + } else { + return Err(PanicReason::ArithmeticError.into()); + } + }, + Some(result) => { + *err = 0; + // This never loses data, since the modulus type has same width as the result + [](result) + } - // This never loses data, since the modulus type has same width as the result - [](result) }; *of = 0; @@ -309,11 +314,9 @@ macro_rules! wideint_ops { let rhs = [](rhs); let product = lhs.full_mul(rhs); - let result = if divider == 0 { - product >> (S * 8) - } else { - product / [](divider) - }; + #[allow(clippy::arithmetic_side_effects)] // Safety: the shift has less bits than the product + let product_div_max = product >> (S * 8); + let result = product.checked_div([](divider)).unwrap_or(product_div_max); let mut buffer = [0u8; 2 * S]; result.to_little_endian(&mut buffer); diff --git a/fuel-vm/src/interpreter/balances.rs b/fuel-vm/src/interpreter/balances.rs index 787107247e..85a444d2e8 100644 --- a/fuel-vm/src/interpreter/balances.rs +++ b/fuel-vm/src/interpreter/balances.rs @@ -12,7 +12,10 @@ use fuel_asm::{ RegId, Word, }; -use fuel_tx::ValidityError; +use fuel_tx::{ + consts::BALANCE_ENTRY_SIZE, + ValidityError, +}; use fuel_types::AssetId; use itertools::Itertools; @@ -90,7 +93,8 @@ impl RuntimeBalances { .sorted_by_key(|k| k.0) .enumerate() .try_fold(HashMap::new(), |mut state, (i, (asset, balance))| { - let offset = VM_MEMORY_BALANCES_OFFSET + i * (AssetId::LEN + WORD_SIZE); + let offset = VM_MEMORY_BALANCES_OFFSET + .saturating_add(i.saturating_mul(BALANCE_ENTRY_SIZE)); state .entry(asset) @@ -115,7 +119,7 @@ impl RuntimeBalances { let value = balance.value(); let offset = balance.offset(); - let offset = offset + AssetId::LEN; + let offset = offset.saturating_add(AssetId::LEN); memory.write_bytes_noownerchecks(offset, value.to_be_bytes())?; Ok(value) @@ -157,17 +161,21 @@ impl RuntimeBalances { .map_or((value == 0).then_some(0), |r| r.ok()) } - /// Write all assets into the VM memory. + /// Write all assets into the start of VM stack, i.e. at $ssp. + /// Panics if the assets cannot fit. pub fn to_vm(self, vm: &mut Interpreter) where Tx: ExecutableTransaction, { - let len = (vm.max_inputs() as usize * (AssetId::LEN + WORD_SIZE)) as Word; + let len = (vm.max_inputs() as usize).saturating_mul(BALANCE_ENTRY_SIZE) as Word; - vm.registers[RegId::SP] += len; - vm.reserve_stack(len).expect( - "consensus parameters won't allow stack overflow for VM initialization", + let new_ssp = vm.registers[RegId::SSP].checked_add(len).expect( + "Consensus parameters must not allow stack overflow during VM initialization", + ); + vm.memory.grow_stack(new_ssp).expect( + "Consensus parameters must not allow stack overflow during VM initialization", ); + vm.registers[RegId::SSP] = new_ssp; self.state.iter().for_each(|(asset, balance)| { let value = balance.value(); @@ -175,10 +183,13 @@ impl RuntimeBalances { vm.memory .write_bytes_noownerchecks(ofs, **asset) - .expect("Assets must fit in memory"); + .expect("Checked above"); vm.memory - .write_bytes_noownerchecks(ofs + AssetId::LEN, value.to_be_bytes()) - .expect("Assets must fit in memory"); + .write_bytes_noownerchecks( + ofs.saturating_add(AssetId::LEN), + value.to_be_bytes(), + ) + .expect("Checked above"); }); vm.balances = self; @@ -253,10 +264,10 @@ fn writes_to_memory_correctly() { assert_eq!(asset.as_ref(), &memory[ofs..ofs + AssetId::LEN]); assert_eq!( &value.to_be_bytes(), - &memory[ofs + AssetId::LEN..ofs + AssetId::LEN + WORD_SIZE] + &memory[ofs + AssetId::LEN..ofs + BALANCE_ENTRY_SIZE] ); - ofs + AssetId::LEN + WORD_SIZE + ofs + BALANCE_ENTRY_SIZE }); } diff --git a/fuel-vm/src/interpreter/blockchain.rs b/fuel-vm/src/interpreter/blockchain.rs index 6b0a0e02c3..b1309d5ab4 100644 --- a/fuel-vm/src/interpreter/blockchain.rs +++ b/fuel-vm/src/interpreter/blockchain.rs @@ -50,6 +50,7 @@ use alloc::vec::Vec; use fuel_asm::PanicReason; use fuel_storage::StorageSize; use fuel_tx::{ + consts::BALANCE_ENTRY_SIZE, ContractIdExt, DependentCost, Receipt, @@ -736,7 +737,7 @@ where self.cgas, self.ggas, profiler, - ((AssetId::LEN + WORD_SIZE) as u64) * self.new_storage_gas_per_byte, + (BALANCE_ENTRY_SIZE as u64).saturating_mul(self.new_storage_gas_per_byte), )?; } @@ -1062,7 +1063,9 @@ pub(crate) fn state_write_word( cgas, ggas, profiler, - (2 * Bytes32::LEN as u64) * new_storage_gas_per_byte, + (Bytes32::LEN as u64) + .saturating_mul(2) + .saturating_mul(new_storage_gas_per_byte), )?; } @@ -1265,8 +1268,10 @@ fn state_write_qword<'vm, S: InterpreterStorage>( cgas, ggas, profiler, - // Overflow safety: unset_count * 32 can be at most VM_MAX_RAM - (unset_count as u64) * (2 * Bytes32::LEN as u64) * new_storage_gas_per_byte, + (unset_count as u64) + .saturating_mul(2) + .saturating_mul(Bytes32::LEN as u64) + .saturating_mul(new_storage_gas_per_byte), )?; } diff --git a/fuel-vm/src/interpreter/blockchain/other_tests.rs b/fuel-vm/src/interpreter/blockchain/other_tests.rs index 8a0626ba3e..ce9c830b9b 100644 --- a/fuel-vm/src/interpreter/blockchain/other_tests.rs +++ b/fuel-vm/src/interpreter/blockchain/other_tests.rs @@ -1,3 +1,5 @@ +#![allow(clippy::arithmetic_side_effects, clippy::cast_possible_truncation)] + use alloc::vec; use crate::{ diff --git a/fuel-vm/src/interpreter/blockchain/smo_tests.rs b/fuel-vm/src/interpreter/blockchain/smo_tests.rs index 4ab1e80884..f293fd0631 100644 --- a/fuel-vm/src/interpreter/blockchain/smo_tests.rs +++ b/fuel-vm/src/interpreter/blockchain/smo_tests.rs @@ -1,3 +1,5 @@ +#![allow(clippy::arithmetic_side_effects, clippy::cast_possible_truncation)] + use core::convert::Infallible; use alloc::{ diff --git a/fuel-vm/src/interpreter/contract.rs b/fuel-vm/src/interpreter/contract.rs index 22f4986d4f..3c8edb4913 100644 --- a/fuel-vm/src/interpreter/contract.rs +++ b/fuel-vm/src/interpreter/contract.rs @@ -294,8 +294,8 @@ impl<'vm, S, Tx> TransferCtx<'vm, S, Tx> { self.cgas, self.ggas, profiler, - // Overflow safety: unset_count * 32 can be at most VM_MAX_RAM - ((Bytes32::LEN + WORD_SIZE) as u64) * self.new_storage_gas_per_byte, + ((Bytes32::LEN + WORD_SIZE) as u64) + .saturating_mul(self.new_storage_gas_per_byte), )?; } diff --git a/fuel-vm/src/interpreter/contract/tests.rs b/fuel-vm/src/interpreter/contract/tests.rs index 2013338307..2e4e66586b 100644 --- a/fuel-vm/src/interpreter/contract/tests.rs +++ b/fuel-vm/src/interpreter/contract/tests.rs @@ -1,4 +1,5 @@ -#![allow(clippy::cast_possible_truncation)] +#![allow(clippy::arithmetic_side_effects, clippy::cast_possible_truncation)] + use core::convert::Infallible; use alloc::vec; diff --git a/fuel-vm/src/interpreter/diff.rs b/fuel-vm/src/interpreter/diff.rs index 3bf2107f33..9da2474256 100644 --- a/fuel-vm/src/interpreter/diff.rs +++ b/fuel-vm/src/interpreter/diff.rs @@ -436,7 +436,7 @@ fn invert_vec(vector: &mut Vec, value: &VecState>) { }, Ordering::Equal | Ordering::Greater, ) => { - vector.resize(*index + 1, value.clone()); + vector.resize((*index).saturating_add(1), value.clone()); vector[*index] = value.clone(); } ( diff --git a/fuel-vm/src/interpreter/executors/instruction.rs b/fuel-vm/src/interpreter/executors/instruction.rs index 323dc99df4..6bef98ecd9 100644 --- a/fuel-vm/src/interpreter/executors/instruction.rs +++ b/fuel-vm/src/interpreter/executors/instruction.rs @@ -664,7 +664,7 @@ where Instruction::LW(lw) => { self.gas_charge(self.gas_costs().lw())?; let (a, b, imm) = lw.unpack(); - self.load_word(a.into(), r!(b), imm.into())?; + self.load_word(a.into(), r!(b), imm)?; } Instruction::MCL(mcl) => { @@ -711,7 +711,7 @@ where Instruction::SW(sw) => { self.gas_charge(self.gas_costs().sw())?; let (a, b, imm) = sw.unpack(); - self.store_word(r!(a), r!(b), imm.into())?; + self.store_word(r!(a), r!(b), imm)?; } Instruction::BAL(bal) => { @@ -930,7 +930,7 @@ fn checked_nth_root(target: u64, nth_root: u64) -> Option { #[cfg(not(feature = "std"))] let powf = libm::pow; - #[allow(clippy::cast_possible_truncation)] + #[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)] let guess = powf(target as f64, (nth_root as f64).recip()) as u64; debug_assert!(guess != 0, "This should never occur for {{target, n}} > 1"); @@ -946,16 +946,19 @@ fn checked_nth_root(target: u64, nth_root: u64) -> Option { // Note that if guess == 1, then g1 == 1 as well, meaning that we will not return // here. if is_nth_power_below_target(guess) { - return Some(guess - 1) + return Some(guess.saturating_sub(1)) } // Check if the initial guess was correct - if is_nth_power_below_target(guess + 1) { + let guess_plus_one = guess.checked_add(1).expect( + "Guess cannot be u64::MAX, as we have taken a root > 2 of a value to get it", + ); + if is_nth_power_below_target(guess_plus_one) { return Some(guess) } - // Check if the guess was correct - Some(guess + 1) + // If not, then the value above must be the correct one. + Some(guess_plus_one) } #[cfg(test)] diff --git a/fuel-vm/src/interpreter/executors/main.rs b/fuel-vm/src/interpreter/executors/main.rs index 432f8e1e8f..e29594d339 100644 --- a/fuel-vm/src/interpreter/executors/main.rs +++ b/fuel-vm/src/interpreter/executors/main.rs @@ -12,6 +12,7 @@ use crate::{ IntoChecked, ParallelExecutor, }, + consts::VM_MAX_RAM, context::Context, error::{ Bug, @@ -769,10 +770,13 @@ where let gas_limit; let is_empty_script; if let Some(script) = self.transaction().as_script() { - let offset = (self.tx_offset() + script.script_offset()) as Word; + let offset = + self.tx_offset().saturating_add(script.script_offset()) as Word; gas_limit = *script.script_gas_limit(); is_empty_script = script.script().is_empty(); + debug_assert!(offset < VM_MAX_RAM); + self.registers[RegId::PC] = offset; self.registers[RegId::IS] = offset; } else { diff --git a/fuel-vm/src/interpreter/flow.rs b/fuel-vm/src/interpreter/flow.rs index 488ef9e90b..29e87c0821 100644 --- a/fuel-vm/src/interpreter/flow.rs +++ b/fuel-vm/src/interpreter/flow.rs @@ -535,8 +535,8 @@ where self.registers.system_registers.cgas.as_mut(), self.registers.system_registers.ggas.as_mut(), profiler, - // Overflow safety: unset_count * 32 can be at most VM_MAX_RAM - ((Bytes32::LEN + WORD_SIZE) as u64) * self.new_storage_gas_per_byte, + ((Bytes32::LEN + WORD_SIZE) as u64) + .saturating_mul(self.new_storage_gas_per_byte), )?; } diff --git a/fuel-vm/src/interpreter/flow/tests.rs b/fuel-vm/src/interpreter/flow/tests.rs index 430c920bd2..e09ecc1dfa 100644 --- a/fuel-vm/src/interpreter/flow/tests.rs +++ b/fuel-vm/src/interpreter/flow/tests.rs @@ -1,3 +1,5 @@ +#![allow(clippy::arithmetic_side_effects, clippy::cast_possible_truncation)] + use core::convert::Infallible; use alloc::{ diff --git a/fuel-vm/src/interpreter/initialization.rs b/fuel-vm/src/interpreter/initialization.rs index 51b3e447a9..a1abba4f9e 100644 --- a/fuel-vm/src/interpreter/initialization.rs +++ b/fuel-vm/src/interpreter/initialization.rs @@ -51,21 +51,37 @@ where // Set heap area self.registers[RegId::HP] = VM_MAX_RAM; - self.push_stack(self.transaction().id(&self.chain_id()).as_ref())?; + // Initialize stack + macro_rules! push_stack { + ($v:expr) => {{ + let data = $v; + let old_ssp = self.registers[RegId::SSP]; + let new_ssp = old_ssp + .checked_add(data.len() as Word) + .expect("VM initialization data must fit into the stack"); + self.memory.grow_stack(new_ssp)?; + self.registers[RegId::SSP] = new_ssp; + self.memory + .write_noownerchecks(old_ssp, data.len()) + .expect("VM initialization data must fit into the stack") + .copy_from_slice(data); + }}; + } + + push_stack!(&*self.transaction().id(&self.chain_id())); let base_asset_id = self.interpreter_params.base_asset_id; - self.push_stack(&*base_asset_id)?; + push_stack!(&*base_asset_id); runtime_balances.to_vm(self); let tx_size = self.transaction().size() as Word; self.set_gas(gas_limit); - self.push_stack(&tx_size.to_be_bytes())?; + push_stack!(&tx_size.to_be_bytes()); let tx_bytes = self.tx.to_bytes(); - - self.push_stack(tx_bytes.as_slice())?; + push_stack!(tx_bytes.as_slice()); self.registers[RegId::SP] = self.registers[RegId::SSP]; diff --git a/fuel-vm/src/interpreter/internal.rs b/fuel-vm/src/interpreter/internal.rs index eb62960742..6d1ea07b75 100644 --- a/fuel-vm/src/interpreter/internal.rs +++ b/fuel-vm/src/interpreter/internal.rs @@ -14,7 +14,6 @@ use fuel_asm::{ Flags, Instruction, PanicReason, - RegId, }; use fuel_tx::{ field::Outputs, @@ -29,10 +28,7 @@ use fuel_types::{ Word, }; -use core::{ - mem, - ops::Range, -}; +use core::ops::Range; #[cfg(test)] mod message_tests; @@ -67,7 +63,8 @@ fn absolute_output_offset( tx_offset: usize, idx: usize, ) -> Option { - tx.outputs_offset_at(idx).map(|offset| tx_offset + offset) + tx.outputs_offset_at(idx) + .map(|offset| tx_offset.saturating_add(offset)) } pub(crate) fn absolute_output_mem_range( @@ -100,28 +97,6 @@ pub(crate) fn update_memory_output( } impl Interpreter { - pub(crate) fn reserve_stack(&mut self, len: Word) -> Result { - let (new_sp, overflow) = self.registers[RegId::SSP].overflowing_add(len); - - if overflow || !self.is_external_context() && new_sp > self.registers[RegId::SP] { - Err(PanicReason::MemoryOverflow) - } else { - self.memory.grow_stack(new_sp)?; - Ok(mem::replace(&mut self.registers[RegId::SSP], new_sp)) - } - } - - pub(crate) fn push_stack(&mut self, data: &[u8]) -> SimpleResult<()> { - let old_ssp = usize::try_from(self.reserve_stack(data.len() as Word)?) - .expect("SSP couldn't be more than `usize`"); - - self.memory - .write_noownerchecks(old_ssp, data.len())? - .copy_from_slice(data); - - Ok(()) - } - pub(crate) fn set_flag(&mut self, a: Word) -> SimpleResult<()> { let (SystemRegisters { flag, pc, .. }, _) = split_registers(&mut self.registers); set_flag(flag, pc, a) @@ -131,10 +106,6 @@ impl Interpreter { &self.context } - pub(crate) const fn is_external_context(&self) -> bool { - self.context().is_external() - } - pub(crate) const fn is_predicate(&self) -> bool { matches!( self.context, diff --git a/fuel-vm/src/interpreter/internal/message_tests.rs b/fuel-vm/src/interpreter/internal/message_tests.rs index 681133c3a8..f8c62e0e2c 100644 --- a/fuel-vm/src/interpreter/internal/message_tests.rs +++ b/fuel-vm/src/interpreter/internal/message_tests.rs @@ -1,3 +1,5 @@ +#![allow(clippy::arithmetic_side_effects, clippy::cast_possible_truncation)] + use alloc::{ vec, vec::Vec, diff --git a/fuel-vm/src/interpreter/memory.rs b/fuel-vm/src/interpreter/memory.rs index aae3bc6f5b..2e0a427805 100644 --- a/fuel-vm/src/interpreter/memory.rs +++ b/fuel-vm/src/interpreter/memory.rs @@ -13,6 +13,7 @@ use crate::{ use derivative::Derivative; use fuel_asm::{ + Imm12, Imm24, PanicReason, RegId, @@ -63,7 +64,10 @@ fn reverse_resize_at_least(vec: &mut Vec, new_len: usize) { let cap = new_len.next_power_of_two().clamp(256, MEM_SIZE); let mut new_vec = Vec::new(); new_vec.reserve_exact(cap); - new_vec.extend(core::iter::repeat(0).take(cap - vec.len())); + let prefix_zeroes = cap + .checked_sub(vec.len()) + .expect("Attempting to resize impossibly large heap memory"); + new_vec.extend(core::iter::repeat(0).take(prefix_zeroes)); new_vec.extend(vec.iter().copied()); *vec = new_vec; } @@ -140,8 +144,11 @@ impl Memory { return Err(PanicReason::MemoryGrowthOverlap) } + #[allow(clippy::arithmetic_side_effects)] // Safety: ensured above with min + let new_len = MEM_SIZE - new_hp; + // Expand the heap allocation - reverse_resize_at_least(&mut self.heap, MEM_SIZE - new_hp); + reverse_resize_at_least(&mut self.heap, new_len); self.hp = new_hp; // If heap enters region where stack has been, truncate the stack @@ -179,6 +186,7 @@ impl Memory { } /// Returns a reference to memory for reading, if possible. + #[allow(clippy::arithmetic_side_effects)] // Safety: subtractions are checked pub fn read( &self, addr: A, @@ -209,6 +217,7 @@ impl Memory { /// Gets write access to memory, if possible. /// Doesn't perform any ownership checks. + #[allow(clippy::arithmetic_side_effects)] // Safety: subtractions are checked pub fn write_noownerchecks( &mut self, addr: A, @@ -421,7 +430,7 @@ impl MemoryRange { /// Splits range at given relative offset. Panics if offset > range length. pub fn split_at_offset(self, at: usize) -> (Self, Self) { - let mid = self.0.start + at; + let mid = self.0.start.saturating_add(at); assert!(mid <= self.0.end); (Self(self.0.start..mid), Self(mid..self.0.end)) } @@ -507,7 +516,7 @@ impl Interpreter { &mut self, ra: RegisterId, b: Word, - c: Word, + c: Imm12, ) -> SimpleResult<()> { let (SystemRegisters { pc, .. }, mut w) = split_registers(&mut self.registers); let result = &mut w[WriteRegKey::try_from(ra)?]; @@ -519,7 +528,7 @@ impl Interpreter { store_byte(&mut self.memory, owner, self.registers.pc_mut(), a, b, c) } - pub(crate) fn store_word(&mut self, a: Word, b: Word, c: Word) -> SimpleResult<()> { + pub(crate) fn store_word(&mut self, a: Word, b: Word, c: Imm12) -> SimpleResult<()> { let owner = self.ownership_registers(); store_word(&mut self.memory, owner, self.registers.pc_mut(), a, b, c) } @@ -615,14 +624,18 @@ pub(crate) fn push_selected_registers( // First update the new stack pointer, as that's the only error condition let count: u64 = bitmask.count_ones().into(); - let write_size = count * (core::mem::size_of::() as u64); + let write_size = count + .checked_mul(WORD_SIZE as u64) + .expect("Bitmask size times 8 can never oveflow"); let write_at = *sp; - try_update_stack_pointer(sp, ssp, hp, write_at + write_size, memory)?; + // If this would overflow, the stack pointer update below will fail + let new_sp = write_at.saturating_add(write_size); + try_update_stack_pointer(sp, ssp, hp, new_sp, memory)?; // Write the registers to the stack let mut it = memory .write_noownerchecks(write_at, write_size)? - .chunks_exact_mut(8); + .chunks_exact_mut(WORD_SIZE); for (i, reg) in program_regs.segment(segment).iter().enumerate() { if (bitmask & (1 << i)) != 0 { let item = it @@ -650,17 +663,19 @@ pub(crate) fn pop_selected_registers( // First update the stack pointer, as that's the only error condition let count: u64 = bitmask.count_ones().into(); - let size_in_stack = count * 8; + let size_in_stack = count + .checked_mul(WORD_SIZE as u64) + .expect("Bitmask size times 8 can never oveflow"); let new_sp = sp .checked_sub(size_in_stack) .ok_or(PanicReason::MemoryOverflow)?; try_update_stack_pointer(sp, ssp, hp, new_sp, memory)?; // Restore registers from the stack - let mut it = memory.read(new_sp, size_in_stack)?.chunks_exact(8); + let mut it = memory.read(new_sp, size_in_stack)?.chunks_exact(WORD_SIZE); for (i, reg) in program_regs.segment_mut(segment).iter_mut().enumerate() { if (bitmask & (1 << i)) != 0 { - let mut buf = [0u8; 8]; + let mut buf = [0u8; WORD_SIZE]; buf.copy_from_slice(it.next().expect("Count mismatch")); *reg = Word::from_be_bytes(buf); } @@ -686,11 +701,12 @@ pub(crate) fn load_word( pc: RegMut, result: &mut Word, b: Word, - c: Word, + c: Imm12, ) -> SimpleResult<()> { - // C is expressed in words; mul by 8. This cannot overflow since it's a 12 bit - // immediate value. - let addr = b.checked_add(c * 8).ok_or(PanicReason::MemoryOverflow)?; + let offset = u64::from(c) + .checked_mul(WORD_SIZE as u64) + .expect("u12 * 8 cannot overflow a Word"); + let addr = b.checked_add(offset).ok_or(PanicReason::MemoryOverflow)?; *result = Word::from_be_bytes(memory.read_bytes(addr)?); Ok(inc_pc(pc)?) } @@ -714,11 +730,13 @@ pub(crate) fn store_word( pc: RegMut, a: Word, b: Word, - c: Word, + c: Imm12, ) -> SimpleResult<()> { - // C is expressed in words; mul by 8. This cannot overflow since it's a 12 bit - // immediate value. - let addr = a.saturating_add(c * 8); + #[allow(clippy::arithmetic_side_effects)] + let offset = u64::from(c) + .checked_mul(WORD_SIZE as u64) + .expect("12-bits number multiplied by 8 cannot overflow a Word"); + let addr = a.saturating_add(offset); memory.write_bytes(owner, addr, b.to_be_bytes())?; Ok(inc_pc(pc)?) } diff --git a/fuel-vm/src/interpreter/memory/allocation_tests.rs b/fuel-vm/src/interpreter/memory/allocation_tests.rs index 05f344b127..9603c70735 100644 --- a/fuel-vm/src/interpreter/memory/allocation_tests.rs +++ b/fuel-vm/src/interpreter/memory/allocation_tests.rs @@ -1,4 +1,5 @@ -#![allow(clippy::cast_possible_truncation)] +#![allow(clippy::arithmetic_side_effects, clippy::cast_possible_truncation)] + use alloc::vec; use super::*; @@ -185,15 +186,15 @@ fn test_load_byte(b: Word, c: Word) -> SimpleResult<()> { Ok(()) } -#[test_case(20, 20 => Ok(()); "Can load a word")] -#[test_case(VM_MAX_RAM, 1 => Err(PanicOrBug::Panic(PanicReason::MemoryOverflow)); "b + 8 * c gteq VM_MAX_RAM")] -fn test_load_word(b: Word, c: Word) -> SimpleResult<()> { +#[test_case(20, Imm12::from(20) => Ok(()); "Can load a word")] +#[test_case(VM_MAX_RAM, Imm12::from(1) => Err(PanicOrBug::Panic(PanicReason::MemoryOverflow)); "b + 8 * c gteq VM_MAX_RAM")] +fn test_load_word(b: Word, c: Imm12) -> SimpleResult<()> { // create a mutable memory with size `MEM_SIZE` let mut memory: Memory = vec![1u8; MEM_SIZE].try_into().unwrap(); // calculate start location where 8 bytes of value will be stored based on `b` and `c` // values. - let start = (b as usize + (c as usize * 8)).min(MEM_SIZE - 8); + let start = (b as usize + (u64::from(c) * 8) as usize).min(MEM_SIZE - 8); // write 2u8 to a slice of memory (starting at the 'start' location with a length of // 8) @@ -279,10 +280,9 @@ fn test_store_byte_more( Ok(()) } -#[test_case(true, 20, 30, 40 => Ok(()); "Can store a word")] -#[test_case(true, 20, 30, VM_MAX_RAM => Err(PanicOrBug::Panic(PanicReason::MemoryOverflow)); "Fails due to memory overflow")] -#[test_case(false, 20, 30, 40 => Err(PanicOrBug::Panic(PanicReason::MemoryOwnership)); "Fails due to not having ownership of the range")] -fn test_store_word(has_ownership: bool, a: Word, b: Word, c: Word) -> SimpleResult<()> { +#[test_case(true, 20, 30, Imm12::from(40) => Ok(()); "Can store a word")] +#[test_case(false, 20, 30, Imm12::from(40) => Err(PanicOrBug::Panic(PanicReason::MemoryOwnership)); "Fails due to not having ownership of the range")] +fn test_store_word(has_ownership: bool, a: Word, b: Word, c: Imm12) -> SimpleResult<()> { let mut memory: Memory = vec![1u8; MEM_SIZE].try_into().unwrap(); let mut pc = 4; let mut owner = OwnershipRegisters { @@ -296,13 +296,13 @@ fn test_store_word(has_ownership: bool, a: Word, b: Word, c: Word) -> SimpleResu }; if has_ownership { owner.ssp = b; - owner.sp = b + (8 * c); + owner.sp = b + 8 * u64::from(c); } store_word(&mut memory, owner, RegMut::new(&mut pc), a, b, c)?; assert_eq!(pc, 8); - let start = (a + c * 8) as usize; + let start = (a + u64::from(c) * 8) as usize; assert_eq!(memory[start..start + 8], b.to_be_bytes()[..]); Ok(()) diff --git a/fuel-vm/src/interpreter/memory/stack_tests.rs b/fuel-vm/src/interpreter/memory/stack_tests.rs index 3744f9f729..7d137b1973 100644 --- a/fuel-vm/src/interpreter/memory/stack_tests.rs +++ b/fuel-vm/src/interpreter/memory/stack_tests.rs @@ -1,3 +1,5 @@ +#![allow(clippy::arithmetic_side_effects, clippy::cast_possible_truncation)] + use alloc::vec; use fuel_asm::{ diff --git a/fuel-vm/src/interpreter/metadata.rs b/fuel-vm/src/interpreter/metadata.rs index 39caa25766..3c0a41b356 100644 --- a/fuel-vm/src/interpreter/metadata.rs +++ b/fuel-vm/src/interpreter/metadata.rs @@ -76,11 +76,11 @@ where imm: Immediate12, ) -> SimpleResult<()> { let tx_offset = self.tx_offset(); + // Tx size is stored just below the tx bytes + let tx_size_ptr = tx_offset.checked_sub(8).expect("Tx offset is not valid"); let tx_size = Word::from_be_bytes( self.memory - .read_bytes( - tx_offset - 8, // Tx size is stored just below the tx bytes - ) + .read_bytes(tx_size_ptr) .expect("Tx length not in memory"), ); let (SystemRegisters { pc, .. }, mut w) = split_registers(&mut self.registers); @@ -154,6 +154,10 @@ impl GTFInput<'_, Tx> { let tx = self.tx; let ofs = self.tx_offset; + // We use saturating_add with tx offset below. + // In case any addition overflows, this function returns value + // for the field that's above VM_MAX_RAM. + let a = match args { GTFArgs::Type => Tx::transaction_type(), @@ -188,17 +192,19 @@ impl GTFInput<'_, Tx> { GTFArgs::ScriptWitnessesCount | GTFArgs::CreateWitnessesCount => { tx.witnesses().len() as Word } - GTFArgs::ScriptInputAtIndex | GTFArgs::CreateInputAtIndex => { - (ofs + tx.inputs_offset_at(b).ok_or(PanicReason::InputNotFound)?) as Word - } + GTFArgs::ScriptInputAtIndex | GTFArgs::CreateInputAtIndex => ofs + .saturating_add(tx.inputs_offset_at(b).ok_or(PanicReason::InputNotFound)?) + as Word, GTFArgs::ScriptOutputAtIndex | GTFArgs::CreateOutputAtIndex => { - (ofs + tx.outputs_offset_at(b).ok_or(PanicReason::OutputNotFound)?) - as Word + ofs.saturating_add( + tx.outputs_offset_at(b).ok_or(PanicReason::OutputNotFound)?, + ) as Word } GTFArgs::ScriptWitnessAtIndex | GTFArgs::CreateWitnessAtIndex => { - (ofs + tx - .witnesses_offset_at(b) - .ok_or(PanicReason::WitnessNotFound)?) as Word + ofs.saturating_add( + tx.witnesses_offset_at(b) + .ok_or(PanicReason::WitnessNotFound)?, + ) as Word } GTFArgs::TxLength => self.tx_size, @@ -209,16 +215,15 @@ impl GTFInput<'_, Tx> { .map(InputRepr::from) .ok_or(PanicReason::InputNotFound)? as Word } - GTFArgs::InputCoinTxId => { - (ofs + tx - .inputs() + GTFArgs::InputCoinTxId => ofs.saturating_add( + tx.inputs() .get(b) .filter(|i| i.is_coin()) .map(Input::repr) .and_then(|r| r.utxo_id_offset()) - .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o + ofs)) - .ok_or(PanicReason::InputNotFound)?) as Word - } + .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o.saturating_add(ofs))) + .ok_or(PanicReason::InputNotFound)?, + ) as Word, GTFArgs::InputCoinOutputIndex => { tx.inputs() .get(b) @@ -227,42 +232,39 @@ impl GTFInput<'_, Tx> { .map(UtxoId::output_index) .ok_or(PanicReason::InputNotFound)? as Word } - GTFArgs::InputCoinOwner => { - (ofs + tx - .inputs() + GTFArgs::InputCoinOwner => ofs.saturating_add( + tx.inputs() .get(b) .filter(|i| i.is_coin()) .map(Input::repr) .and_then(|r| r.owner_offset()) - .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o + ofs)) - .ok_or(PanicReason::InputNotFound)?) as Word - } + .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o.saturating_add(ofs))) + .ok_or(PanicReason::InputNotFound)?, + ) as Word, GTFArgs::InputCoinAmount => tx .inputs() .get(b) .filter(|i| i.is_coin()) .and_then(Input::amount) .ok_or(PanicReason::InputNotFound)?, - GTFArgs::InputCoinAssetId => { - (ofs + tx - .inputs() + GTFArgs::InputCoinAssetId => ofs.saturating_add( + tx.inputs() .get(b) .filter(|i| i.is_coin()) .map(Input::repr) .and_then(|r| r.asset_id_offset()) - .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o + ofs)) - .ok_or(PanicReason::InputNotFound)?) as Word - } - GTFArgs::InputCoinTxPointer => { - (ofs + tx - .inputs() + .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o.saturating_add(ofs))) + .ok_or(PanicReason::InputNotFound)?, + ) as Word, + GTFArgs::InputCoinTxPointer => ofs.saturating_add( + tx.inputs() .get(b) .filter(|i| i.is_coin()) .map(Input::repr) .and_then(|r| r.tx_pointer_offset()) - .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o + ofs)) - .ok_or(PanicReason::InputNotFound)?) as Word - } + .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o.saturating_add(ofs))) + .ok_or(PanicReason::InputNotFound)?, + ) as Word, GTFArgs::InputCoinWitnessIndex => { tx.inputs() .get(b) @@ -291,85 +293,78 @@ impl GTFInput<'_, Tx> { .and_then(Input::predicate_gas_used) .ok_or(PanicReason::InputNotFound)? as Word } - GTFArgs::InputCoinPredicate => { - (ofs + tx - .inputs() + GTFArgs::InputCoinPredicate => ofs.saturating_add( + tx.inputs() .get(b) .filter(|i| i.is_coin()) .and_then(Input::predicate_offset) - .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o + ofs)) - .ok_or(PanicReason::InputNotFound)?) as Word - } - GTFArgs::InputCoinPredicateData => { - (ofs + tx - .inputs() + .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o.saturating_add(ofs))) + .ok_or(PanicReason::InputNotFound)?, + ) as Word, + GTFArgs::InputCoinPredicateData => ofs.saturating_add( + tx.inputs() .get(b) .filter(|i| i.is_coin()) .and_then(Input::predicate_data_offset) - .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o + ofs)) - .ok_or(PanicReason::InputNotFound)?) as Word - } - GTFArgs::InputContractTxId => { - (ofs + tx - .inputs() + .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o.saturating_add(ofs))) + .ok_or(PanicReason::InputNotFound)?, + ) as Word, + GTFArgs::InputContractTxId => ofs.saturating_add( + tx.inputs() .get(b) .filter(|i| i.is_contract()) .map(Input::repr) .and_then(|r| r.utxo_id_offset()) - .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o + ofs)) - .ok_or(PanicReason::InputNotFound)?) as Word - } + .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o.saturating_add(ofs))) + .ok_or(PanicReason::InputNotFound)?, + ) as Word, GTFArgs::InputContractOutputIndex => { tx.find_output_contract(b) .map(|(idx, _o)| idx) .ok_or(PanicReason::InputNotFound)? as Word } - GTFArgs::InputContractId => { - (ofs + tx - .inputs() + GTFArgs::InputContractId => ofs.saturating_add( + tx.inputs() .get(b) .filter(|i| i.is_contract()) .map(Input::repr) .and_then(|r| r.contract_id_offset()) - .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o + ofs)) - .ok_or(PanicReason::InputNotFound)?) as Word - } - GTFArgs::InputMessageSender => { - (ofs + tx - .inputs() + .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o.saturating_add(ofs))) + .ok_or(PanicReason::InputNotFound)?, + ) as Word, + GTFArgs::InputMessageSender => ofs.saturating_add( + tx.inputs() .get(b) .filter(|i| i.is_message()) .map(Input::repr) .and_then(|r| r.message_sender_offset()) - .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o + ofs)) - .ok_or(PanicReason::InputNotFound)?) as Word - } - GTFArgs::InputMessageRecipient => { - (ofs + tx - .inputs() + .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o.saturating_add(ofs))) + .ok_or(PanicReason::InputNotFound)?, + ) as Word, + GTFArgs::InputMessageRecipient => ofs.saturating_add( + tx.inputs() .get(b) .filter(|i| i.is_message()) .map(Input::repr) .and_then(|r| r.message_recipient_offset()) - .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o + ofs)) - .ok_or(PanicReason::InputNotFound)?) as Word - } + .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o.saturating_add(ofs))) + .ok_or(PanicReason::InputNotFound)?, + ) as Word, GTFArgs::InputMessageAmount => tx .inputs() .get(b) .filter(|i| i.is_message()) .and_then(Input::amount) .ok_or(PanicReason::InputNotFound)?, - GTFArgs::InputMessageNonce => { - (ofs + tx - .inputs() + GTFArgs::InputMessageNonce => ofs.saturating_add( + tx.inputs() .get(b) .filter(|i| i.is_message()) .map(Input::repr) .and_then(|r| r.message_nonce_offset()) - .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o + ofs)) - .ok_or(PanicReason::InputNotFound)?) as Word - } + .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o.saturating_add(ofs))) + .ok_or(PanicReason::InputNotFound)?, + ) as Word, GTFArgs::InputMessageWitnessIndex => { tx.inputs() .get(b) @@ -405,34 +400,31 @@ impl GTFInput<'_, Tx> { .and_then(Input::predicate_gas_used) .ok_or(PanicReason::InputNotFound)? as Word } - GTFArgs::InputMessageData => { - (ofs + tx - .inputs() + GTFArgs::InputMessageData => ofs.saturating_add( + tx.inputs() .get(b) .filter(|i| i.is_message()) .map(Input::repr) .and_then(|r| r.data_offset()) - .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o + ofs)) - .ok_or(PanicReason::InputNotFound)?) as Word - } - GTFArgs::InputMessagePredicate => { - (ofs + tx - .inputs() + .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o.saturating_add(ofs))) + .ok_or(PanicReason::InputNotFound)?, + ) as Word, + GTFArgs::InputMessagePredicate => ofs.saturating_add( + tx.inputs() .get(b) .filter(|i| i.is_message()) .and_then(Input::predicate_offset) - .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o + ofs)) - .ok_or(PanicReason::InputNotFound)?) as Word - } - GTFArgs::InputMessagePredicateData => { - (ofs + tx - .inputs() + .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o.saturating_add(ofs))) + .ok_or(PanicReason::InputNotFound)?, + ) as Word, + GTFArgs::InputMessagePredicateData => ofs.saturating_add( + tx.inputs() .get(b) .filter(|i| i.is_message()) .and_then(Input::predicate_data_offset) - .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o + ofs)) - .ok_or(PanicReason::InputNotFound)?) as Word - } + .and_then(|ofs| tx.inputs_offset_at(b).map(|o| o.saturating_add(ofs))) + .ok_or(PanicReason::InputNotFound)?, + ) as Word, // Output GTFArgs::OutputType => { @@ -441,32 +433,34 @@ impl GTFInput<'_, Tx> { .map(OutputRepr::from) .ok_or(PanicReason::OutputNotFound)? as Word } - GTFArgs::OutputCoinTo => { - (ofs + tx - .outputs() + GTFArgs::OutputCoinTo => ofs.saturating_add( + tx.outputs() .get(b) .filter(|o| o.is_coin() || o.is_change()) .map(Output::repr) .and_then(|r| r.to_offset()) - .and_then(|ofs| tx.outputs_offset_at(b).map(|o| o + ofs)) - .ok_or(PanicReason::OutputNotFound)?) as Word - } + .and_then(|ofs| { + tx.outputs_offset_at(b).map(|o| o.saturating_add(ofs)) + }) + .ok_or(PanicReason::OutputNotFound)?, + ) as Word, GTFArgs::OutputCoinAmount => tx .outputs() .get(b) .filter(|o| o.is_coin()) .and_then(Output::amount) .ok_or(PanicReason::OutputNotFound)?, - GTFArgs::OutputCoinAssetId => { - (ofs + tx - .outputs() + GTFArgs::OutputCoinAssetId => ofs.saturating_add( + tx.outputs() .get(b) .filter(|o| o.is_coin() || o.is_change()) .map(Output::repr) .and_then(|r| r.asset_id_offset()) - .and_then(|ofs| tx.outputs_offset_at(b).map(|o| o + ofs)) - .ok_or(PanicReason::OutputNotFound)?) as Word - } + .and_then(|ofs| { + tx.outputs_offset_at(b).map(|o| o.saturating_add(ofs)) + }) + .ok_or(PanicReason::OutputNotFound)?, + ) as Word, GTFArgs::OutputContractInputIndex => { tx.outputs() .get(b) @@ -474,26 +468,28 @@ impl GTFInput<'_, Tx> { .and_then(Output::input_index) .ok_or(PanicReason::InputNotFound)? as Word } - GTFArgs::OutputContractCreatedContractId => { - (ofs + tx - .outputs() + GTFArgs::OutputContractCreatedContractId => ofs.saturating_add( + tx.outputs() .get(b) .filter(|o| o.is_contract_created()) .map(Output::repr) .and_then(|r| r.contract_id_offset()) - .and_then(|ofs| tx.outputs_offset_at(b).map(|o| o + ofs)) - .ok_or(PanicReason::OutputNotFound)?) as Word - } - GTFArgs::OutputContractCreatedStateRoot => { - (ofs + tx - .outputs() + .and_then(|ofs| { + tx.outputs_offset_at(b).map(|o| o.saturating_add(ofs)) + }) + .ok_or(PanicReason::OutputNotFound)?, + ) as Word, + GTFArgs::OutputContractCreatedStateRoot => ofs.saturating_add( + tx.outputs() .get(b) .filter(|o| o.is_contract_created()) .map(Output::repr) .and_then(|r| r.contract_created_state_root_offset()) - .and_then(|ofs| tx.outputs_offset_at(b).map(|o| o + ofs)) - .ok_or(PanicReason::OutputNotFound)?) as Word - } + .and_then(|ofs| { + tx.outputs_offset_at(b).map(|o| o.saturating_add(ofs)) + }) + .ok_or(PanicReason::OutputNotFound)?, + ) as Word, // Witness GTFArgs::WitnessDataLength => { @@ -504,7 +500,7 @@ impl GTFInput<'_, Tx> { } GTFArgs::WitnessData => { tx.witnesses_offset_at(b) - .map(|w| ofs + w + WORD_SIZE) + .map(|w| ofs.saturating_add(w).saturating_add(WORD_SIZE)) .ok_or(PanicReason::WitnessNotFound)? as Word } @@ -522,10 +518,10 @@ impl GTFInput<'_, Tx> { script.script_data().len() as Word } (Some(script), None, GTFArgs::Script) => { - (ofs + script.script_offset()) as Word + ofs.saturating_add(script.script_offset()) as Word } (Some(script), None, GTFArgs::ScriptData) => { - (ofs + script.script_data_offset()) as Word + ofs.saturating_add(script.script_data_offset()) as Word } // Create @@ -536,13 +532,14 @@ impl GTFInput<'_, Tx> { create.storage_slots().len() as Word } (None, Some(create), GTFArgs::CreateSalt) => { - (ofs + create.salt_offset()) as Word + ofs.saturating_add(create.salt_offset()) as Word } (None, Some(create), GTFArgs::CreateStorageSlotAtIndex) => { // TODO: Maybe we need to return panic error // `StorageSlotsNotFound`? - (ofs + create.storage_slots_offset_at(b).unwrap_or_default()) - as Word + (ofs.saturating_add( + create.storage_slots_offset_at(b).unwrap_or_default(), + )) as Word } _ => return Err(PanicReason::InvalidMetadataIdentifier.into()), } diff --git a/fuel-vm/src/lib.rs b/fuel-vm/src/lib.rs index 6e0e78d87d..b72b02620d 100644 --- a/fuel-vm/src/lib.rs +++ b/fuel-vm/src/lib.rs @@ -5,8 +5,13 @@ #![deny(unsafe_code)] #![deny(unused_must_use)] #![deny(unused_crate_dependencies)] -#![deny(clippy::cast_possible_truncation)] -#![deny(clippy::string_slice)] +#![deny( + clippy::arithmetic_side_effects, + clippy::cast_sign_loss, + clippy::cast_possible_truncation, + clippy::cast_possible_wrap, + clippy::string_slice +)] #[doc(hidden)] // Needed by some of the exported macros pub extern crate alloc; diff --git a/fuel-vm/src/profiler.rs b/fuel-vm/src/profiler.rs index 720d3c4b60..2e09d90e12 100644 --- a/fuel-vm/src/profiler.rs +++ b/fuel-vm/src/profiler.rs @@ -322,7 +322,10 @@ impl<'a> GasProfilingData { /// Increase gas used at location pub fn add(&mut self, location: InstructionLocation, amount: u64) { - *self.gas_use.entry(location).or_insert(0) += amount; + let gas_use = self.gas_use.entry(location).or_insert(0); + // Saturating is ok for profiling. + // This should never matter, as gas is deducted on each iteration. + *gas_use = gas_use.saturating_add(amount); } /// Iterate through locations and gas values diff --git a/fuel-vm/src/state/debug.rs b/fuel-vm/src/state/debug.rs index 605311a505..d6823e662e 100644 --- a/fuel-vm/src/state/debug.rs +++ b/fuel-vm/src/state/debug.rs @@ -4,6 +4,8 @@ use fuel_types::{ Word, }; +use crate::consts::VM_MAX_RAM; + #[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Hash)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] /// Breakpoint description that binds a tuple `(contract, $pc)` to a debugger @@ -27,9 +29,11 @@ impl Breakpoint { /// The `$pc` is provided in op count and internally is multiplied by the op /// size. Also, the op count is always relative to `$is` so it should /// consider only the bytecode of the contract. + /// + /// Panics if the `pc` cannot ever fit into the VM memory. pub const fn new(contract: ContractId, pc: Word) -> Self { - let pc = pc * (Instruction::SIZE as Word); - + let pc = pc.saturating_mul(Instruction::SIZE as Word); + assert!(pc <= VM_MAX_RAM, "Breakpoint cannot fit into vm memory"); Self::raw(contract, pc) } diff --git a/fuel-vm/src/storage/memory.rs b/fuel-vm/src/storage/memory.rs index df8760c22f..93becb7d7c 100644 --- a/fuel-vm/src/storage/memory.rs +++ b/fuel-vm/src/storage/memory.rs @@ -476,6 +476,7 @@ impl InterpreterStorage for MemoryStorage { Ok(self.state_transition_version) } + #[allow(clippy::arithmetic_side_effects)] // Safety: not enough bits to overflow fn timestamp(&self, height: BlockHeight) -> Result { const GENESIS: Tai64 = Tai64::UNIX_EPOCH; const INTERVAL: Word = 10; @@ -573,6 +574,8 @@ impl InterpreterStorage for MemoryStorage { .zip(values) .try_for_each(|(key, value)| { let key: ContractsStateKey = (contract, &Bytes32::from(key)).into(); + // Safety: we never have over usize::MAX items in one call + #[allow(clippy::arithmetic_side_effects)] if !storage.contains_key(&key)? { unset_count += 1; } diff --git a/fuel-vm/src/tests/cgas.rs b/fuel-vm/src/tests/cgas.rs index 04acf35b91..870713af66 100644 --- a/fuel-vm/src/tests/cgas.rs +++ b/fuel-vm/src/tests/cgas.rs @@ -164,7 +164,7 @@ fn cgas_uses_min_available_gas() { // load contract id op::addi(reg_contract_id, reg_call_b, 32 as Immediate12), // set call depth - op::movi(reg_max_call_depth, call_depth as Immediate18), + op::movi(reg_max_call_depth, call_depth), // set inner call cgas limit op::movi(reg_forward_gas, gas_forward_amount), op::call(reg_contract_id, reg_call_a, reg_call_b, RegId::CGAS), diff --git a/fuel-vm/src/tests/gas_factor.rs b/fuel-vm/src/tests/gas_factor.rs index a7440cd5a6..0d2faa3aeb 100644 --- a/fuel-vm/src/tests/gas_factor.rs +++ b/fuel-vm/src/tests/gas_factor.rs @@ -21,13 +21,13 @@ fn gas_factor_rounds_correctly() { let gas_limit = 1_000_000; // arbitrary non-negligible primes - let factor = 5479_f64; + let factor = 5479; let gas_price = 6197; let large_max_fee_limit = input; let gas_costs = GasCosts::default(); - let fee_params = FeeParameters::default().with_gas_price_factor(factor as Word); + let fee_params = FeeParameters::default().with_gas_price_factor(factor); // Random script to consume some gas let script = iter::repeat(op::add(0x10, 0x00, 0x01)) diff --git a/fuel-vm/src/tests/mod.rs b/fuel-vm/src/tests/mod.rs index f0afe50002..a94cfcf385 100644 --- a/fuel-vm/src/tests/mod.rs +++ b/fuel-vm/src/tests/mod.rs @@ -1,4 +1,5 @@ -#![allow(clippy::cast_possible_truncation)] +#![allow(clippy::arithmetic_side_effects, clippy::cast_possible_truncation)] + use futures as _; use tokio as _; use tokio_rayon as _; diff --git a/fuel-vm/src/util.rs b/fuel-vm/src/util.rs index c836152c1f..7ebb0eb5b6 100644 --- a/fuel-vm/src/util.rs +++ b/fuel-vm/src/util.rs @@ -771,7 +771,14 @@ pub mod gas_profiling { pub fn total_gas(&self) -> Word { self.data() - .map(|d| d.gas().iter().map(|(_, gas)| gas).sum()) + .map(|d| { + d.gas() + .iter() + .map(|(_, gas)| gas) + .copied() + .reduce(Word::saturating_add) + .unwrap_or_default() + }) .unwrap_or_default() } }