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
FuelLabs · Apr 24, 2024 · 865e1b9 · 865e1b9
1 parent 15018bd
commit 865e1b9
Show file tree

Hide file tree

Showing 72 changed files with 712 additions and 675 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -7,7 +7,7 @@ use crate::binary::{
 pub fn verify<T: AsRef<[u8]>>(
     root: &Data,
     data: &T,
-    proof_set: &Vec<Data>,
+    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
@@ -27,7 +27,7 @@ fn generate_test(
     name: String,
     function_name: String,
     description: String,
-    sample_data: &Vec<Bytes32>,
+    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
@@ -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
@@ -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
@@ -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<const N: usize>(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::<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
@@ -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
@@ -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
@@ -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
@@ -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