Fix - Upcoming `arithmetic_side_effects` lints (#33000)

* dereplicode address alignment check

* Uses `checked_div` and `checked_rem` in built-in loaders.

* Uses `checked_div` and `checked_rem`.

* sdk: replace sub() with saturating_sub()

* eliminate `String` "arithmetic"

* allow arithmetic side-effects in tests and benches and on types we don't control

---------

Co-authored-by: Trent Nelson <trent@solana.com>

Cargo.lock

@@ -5428,6 +5428,7 @@ dependencies = [
  "bzip2",
  "cargo_metadata",
  "clap 3.2.23",
+ "itertools",
  "log",
  "predicates",
  "regex",
@@ -5454,6 +5455,7 @@ version = "1.17.0"
 dependencies = [
  "cargo_metadata",
  "clap 3.2.23",
+ "itertools",
  "log",
  "solana-logger",
 ]

bloom/src/bloom.rs

@@ -101,7 +101,9 @@ impl<T: BloomHashIndex> Bloom<T> {
         }
     }
     fn pos(&self, key: &T, k: u64) -> u64 {
-        key.hash_at_index(k).wrapping_rem(self.bits.len())
+        key.hash_at_index(k)
+            .checked_rem(self.bits.len())
+            .unwrap_or(0)
     }
     pub fn clear(&mut self) {
         self.bits = BitVec::new_fill(false, self.bits.len());
@@ -164,7 +166,10 @@ impl<T: BloomHashIndex> From<Bloom<T>> for AtomicBloom<T> {
 
 impl<T: BloomHashIndex> AtomicBloom<T> {
     fn pos(&self, key: &T, hash_index: u64) -> (usize, u64) {
-        let pos = key.hash_at_index(hash_index).wrapping_rem(self.num_bits);
+        let pos = key
+            .hash_at_index(hash_index)
+            .checked_rem(self.num_bits)
+            .unwrap_or(0);
         // Divide by 64 to figure out which of the
         // AtomicU64 bit chunks we need to modify.
         let index = pos.wrapping_shr(6);

perf/src/perf_libs.rs

@@ -140,9 +140,11 @@ fn find_cuda_home(perf_libs_path: &Path) -> Option<PathBuf> {
     None
 }
 
-pub fn append_to_ld_library_path(path: String) {
-    let ld_library_path =
-        path + ":" + &env::var("LD_LIBRARY_PATH").unwrap_or_else(|_| "".to_string());
+pub fn append_to_ld_library_path(mut ld_library_path: String) {
+    if let Ok(env_value) = env::var("LD_LIBRARY_PATH") {
+        ld_library_path.push(':');
+        ld_library_path.push_str(&env_value);
+    }
     info!("setting ld_library_path to: {:?}", ld_library_path);
     env::set_var("LD_LIBRARY_PATH", ld_library_path);
 }

perf/src/sigverify.rs

@@ -614,13 +614,15 @@ pub fn ed25519_verify(
     // power-of-two number around that accounting for the fact that the CPU
     // may be busy doing other things while being a real validator
     // TODO: dynamically adjust this crossover
-    if valid_packet_count < 64
-        || 100usize
-            .wrapping_mul(valid_packet_count)
-            .wrapping_div(total_packet_count)
-            < 90
-    {
-        return ed25519_verify_cpu(batches, reject_non_vote, valid_packet_count);
+    let maybe_valid_percentage = 100usize
+        .wrapping_mul(valid_packet_count)
+        .checked_div(total_packet_count);
+    let Some(valid_percentage) = maybe_valid_percentage else {
+        return;
+    };
+    if valid_percentage < 90 || valid_packet_count < 64 {
+        ed25519_verify_cpu(batches, reject_non_vote, valid_packet_count);
+        return;
     }
 
     let (signature_offsets, pubkey_offsets, msg_start_offsets, msg_sizes, sig_lens) =

program-runtime/src/prioritization_fee.rs

@@ -20,26 +20,23 @@ impl PrioritizationFeeDetails {
         match fee_type {
             // TODO: remove support of 'Deprecated' after feature remove_deprecated_request_unit_ix::id() is activated
             PrioritizationFeeType::Deprecated(fee) => {
-                let priority = if compute_unit_limit == 0 {
-                    0
-                } else {
-                    let micro_lamport_fee: MicroLamports =
-                        (fee as u128).saturating_mul(MICRO_LAMPORTS_PER_LAMPORT as u128);
-                    let priority = micro_lamport_fee.saturating_div(compute_unit_limit as u128);
-                    u64::try_from(priority).unwrap_or(u64::MAX)
-                };
+                let micro_lamport_fee: MicroLamports =
+                    (fee as u128).saturating_mul(MICRO_LAMPORTS_PER_LAMPORT as u128);
+                let priority = micro_lamport_fee
+                    .checked_div(compute_unit_limit as u128)
+                    .map(|priority| u64::try_from(priority).unwrap_or(u64::MAX))
+                    .unwrap_or(0);
 
                 Self { fee, priority }
             }
             PrioritizationFeeType::ComputeUnitPrice(cu_price) => {
-                let fee = {
-                    let micro_lamport_fee: MicroLamports =
-                        (cu_price as u128).saturating_mul(compute_unit_limit as u128);
-                    let fee = micro_lamport_fee
-                        .saturating_add(MICRO_LAMPORTS_PER_LAMPORT.saturating_sub(1) as u128)
-                        .saturating_div(MICRO_LAMPORTS_PER_LAMPORT as u128);
-                    u64::try_from(fee).unwrap_or(u64::MAX)
-                };
+                let micro_lamport_fee: MicroLamports =
+                    (cu_price as u128).saturating_mul(compute_unit_limit as u128);
+                let fee = micro_lamport_fee
+                    .saturating_add(MICRO_LAMPORTS_PER_LAMPORT.saturating_sub(1) as u128)
+                    .checked_div(MICRO_LAMPORTS_PER_LAMPORT as u128)
+                    .and_then(|fee| u64::try_from(fee).ok())
+                    .unwrap_or(u64::MAX);
 
                 Self {
                     fee,

programs/bpf_loader/src/lib.rs

@@ -183,7 +183,8 @@ pub fn calculate_heap_cost(heap_size: u64, heap_cost: u64, enable_rounding_fix:
             .saturating_add(PAGE_SIZE_KB.saturating_mul(KIBIBYTE).saturating_sub(1));
     }
     rounded_heap_size
-        .saturating_div(PAGE_SIZE_KB.saturating_mul(KIBIBYTE))
+        .checked_div(PAGE_SIZE_KB.saturating_mul(KIBIBYTE))
+        .expect("PAGE_SIZE_KB * KIBIBYTE > 0")
         .saturating_sub(1)
         .saturating_mul(heap_cost)
 }

programs/bpf_loader/src/syscalls/cpi.rs

@@ -82,7 +82,8 @@ impl<'a> CallerAccount<'a> {
             consume_compute_meter(
                 invoke_context,
                 (data.len() as u64)
-                    .saturating_div(invoke_context.get_compute_budget().cpi_bytes_per_unit),
+                    .checked_div(invoke_context.get_compute_budget().cpi_bytes_per_unit)
+                    .unwrap_or(u64::MAX),
             )?;
 
             let translated = translate(
@@ -181,7 +182,8 @@ impl<'a> CallerAccount<'a> {
             invoke_context,
             account_info
                 .data_len
-                .saturating_div(invoke_context.get_compute_budget().cpi_bytes_per_unit),
+                .checked_div(invoke_context.get_compute_budget().cpi_bytes_per_unit)
+                .unwrap_or(u64::MAX),
         )?;
 
         let bpf_account_data_direct_mapping = invoke_context
@@ -333,7 +335,8 @@ impl SyscallInvokeSigned for SyscallInvokeSignedRust {
             consume_compute_meter(
                 invoke_context,
                 (ix_data_len)
-                    .saturating_div(invoke_context.get_compute_budget().cpi_bytes_per_unit),
+                    .checked_div(invoke_context.get_compute_budget().cpi_bytes_per_unit)
+                    .unwrap_or(u64::MAX),
             )?;
         }
 
@@ -551,7 +554,8 @@ impl SyscallInvokeSigned for SyscallInvokeSignedC {
             consume_compute_meter(
                 invoke_context,
                 (ix_data_len)
-                    .saturating_div(invoke_context.get_compute_budget().cpi_bytes_per_unit),
+                    .checked_div(invoke_context.get_compute_budget().cpi_bytes_per_unit)
+                    .unwrap_or(u64::MAX),
             )?;
         }
 
@@ -756,7 +760,8 @@ where
             consume_compute_meter(
                 invoke_context,
                 (callee_account.get_data().len() as u64)
-                    .saturating_div(invoke_context.get_compute_budget().cpi_bytes_per_unit),
+                    .checked_div(invoke_context.get_compute_budget().cpi_bytes_per_unit)
+                    .unwrap_or(u64::MAX),
             )?;
 
             accounts.push((instruction_account.index_in_caller, None));

programs/bpf_loader/src/syscalls/mem_ops.rs

@@ -7,9 +7,10 @@ use {
 
 fn mem_op_consume(invoke_context: &mut InvokeContext, n: u64) -> Result<(), Error> {
     let compute_budget = invoke_context.get_compute_budget();
-    let cost = compute_budget
-        .mem_op_base_cost
-        .max(n.saturating_div(compute_budget.cpi_bytes_per_unit));
+    let cost = compute_budget.mem_op_base_cost.max(
+        n.checked_div(compute_budget.cpi_bytes_per_unit)
+            .unwrap_or(u64::MAX),
+    );
     consume_compute_meter(invoke_context, cost)
 }
 

programs/bpf_loader/src/syscalls/mod.rs

@@ -331,6 +331,13 @@ pub fn create_program_runtime_environment_v1<'a>(
     Ok(result)
 }
 
+fn address_is_aligned<T>(address: u64) -> bool {
+    (address as *mut T as usize)
+        .checked_rem(align_of::<T>())
+        .map(|rem| rem == 0)
+        .expect("T to be non-zero aligned")
+}
+
 fn translate(
     memory_mapping: &MemoryMapping,
     access_type: AccessType,
@@ -348,7 +355,7 @@ fn translate_type_inner<'a, T>(
 ) -> Result<&'a mut T, Error> {
     let host_addr = translate(memory_mapping, access_type, vm_addr, size_of::<T>() as u64)?;
 
-    if check_aligned && (host_addr as *mut T as usize).wrapping_rem(align_of::<T>()) != 0 {
+    if check_aligned && !address_is_aligned::<T>(host_addr) {
         return Err(SyscallError::UnalignedPointer.into());
     }
     Ok(unsafe { &mut *(host_addr as *mut T) })
@@ -388,7 +395,7 @@ fn translate_slice_inner<'a, T>(
 
     let host_addr = translate(memory_mapping, access_type, vm_addr, total_size)?;
 
-    if check_aligned && (host_addr as *mut T as usize).wrapping_rem(align_of::<T>()) != 0 {
+    if check_aligned && !address_is_aligned::<T>(host_addr) {
         return Err(SyscallError::UnalignedPointer.into());
     }
     Ok(unsafe { from_raw_parts_mut(host_addr as *mut T, len as usize) })
@@ -761,9 +768,11 @@ declare_syscall!(
                     invoke_context.get_check_size(),
                 )?;
                 let cost = compute_budget.mem_op_base_cost.max(
-                    compute_budget
-                        .sha256_byte_cost
-                        .saturating_mul((val.len() as u64).saturating_div(2)),
+                    compute_budget.sha256_byte_cost.saturating_mul(
+                        (val.len() as u64)
+                            .checked_div(2)
+                            .expect("div by non-zero literal"),
+                    ),
                 );
                 consume_compute_meter(invoke_context, cost)?;
                 hasher.hash(bytes);
@@ -824,9 +833,11 @@ declare_syscall!(
                     invoke_context.get_check_size(),
                 )?;
                 let cost = compute_budget.mem_op_base_cost.max(
-                    compute_budget
-                        .sha256_byte_cost
-                        .saturating_mul((val.len() as u64).saturating_div(2)),
+                    compute_budget.sha256_byte_cost.saturating_mul(
+                        (val.len() as u64)
+                            .checked_div(2)
+                            .expect("div by non-zero literal"),
+                    ),
                 );
                 consume_compute_meter(invoke_context, cost)?;
                 hasher.hash(bytes);
@@ -1321,9 +1332,11 @@ declare_syscall!(
                     invoke_context.get_check_size(),
                 )?;
                 let cost = compute_budget.mem_op_base_cost.max(
-                    compute_budget
-                        .sha256_byte_cost
-                        .saturating_mul((val.len() as u64).saturating_div(2)),
+                    compute_budget.sha256_byte_cost.saturating_mul(
+                        (val.len() as u64)
+                            .checked_div(2)
+                            .expect("div by non-zero literal"),
+                    ),
                 );
                 consume_compute_meter(invoke_context, cost)?;
                 hasher.hash(bytes);
@@ -1349,7 +1362,8 @@ declare_syscall!(
         let budget = invoke_context.get_compute_budget();
 
         let cost = len
-            .saturating_div(budget.cpi_bytes_per_unit)
+            .checked_div(budget.cpi_bytes_per_unit)
+            .unwrap_or(u64::MAX)
             .saturating_add(budget.syscall_base_cost);
         consume_compute_meter(invoke_context, cost)?;
 
@@ -1403,7 +1417,8 @@ declare_syscall!(
         if length != 0 {
             let cost = length
                 .saturating_add(size_of::<Pubkey>() as u64)
-                .saturating_div(budget.cpi_bytes_per_unit);
+                .checked_div(budget.cpi_bytes_per_unit)
+                .unwrap_or(u64::MAX);
             consume_compute_meter(invoke_context, cost)?;
 
             let return_data_result = translate_slice_mut::<u8>(
@@ -1636,7 +1651,9 @@ declare_syscall!(
                 ALT_BN128_MULTIPLICATION_OUTPUT_LEN,
             ),
             ALT_BN128_PAIRING => {
-                let ele_len = input_size.saturating_div(ALT_BN128_PAIRING_ELEMENT_LEN as u64);
+                let ele_len = input_size
+                    .checked_div(ALT_BN128_PAIRING_ELEMENT_LEN as u64)
+                    .expect("div by non-zero constant");
                 let cost = budget
                     .alt_bn128_pairing_one_pair_cost_first
                     .saturating_add(
@@ -1732,7 +1749,8 @@ declare_syscall!(
             budget.syscall_base_cost.saturating_add(
                 input_len
                     .saturating_mul(input_len)
-                    .saturating_div(budget.big_modular_exponentiation_cost),
+                    .checked_div(budget.big_modular_exponentiation_cost)
+                    .unwrap_or(u64::MAX),
             ),
         )?;
 
@@ -2405,10 +2423,7 @@ mod tests {
             );
             let address = result.unwrap();
             assert_ne!(address, 0);
-            assert_eq!(
-                (address as *const u8 as usize).wrapping_rem(align_of::<T>()),
-                0
-            );
+            assert!(address_is_aligned::<T>(address));
         }
         aligned::<u8>();
         aligned::<u16>();
@@ -4032,4 +4047,11 @@ mod tests {
         let size = val.len().wrapping_mul(mem::size_of::<T>());
         unsafe { slice::from_raw_parts_mut(val.as_mut_ptr().cast(), size) }
     }
+
+    #[test]
+    fn test_address_is_aligned() {
+        for address in 0..std::mem::size_of::<u64>() {
+            assert_eq!(address_is_aligned::<u64>(address as u64), address == 0);
+        }
+    }
 }

programs/loader-v4/src/lib.rs

@@ -107,7 +107,8 @@ fn calculate_heap_cost(heap_size: u64, heap_cost: u64) -> u64 {
     const PAGE_SIZE_KB: u64 = 32;
     heap_size
         .saturating_add(PAGE_SIZE_KB.saturating_mul(KIBIBYTE).saturating_sub(1))
-        .saturating_div(PAGE_SIZE_KB.saturating_mul(KIBIBYTE))
+        .checked_div(PAGE_SIZE_KB.saturating_mul(KIBIBYTE))
+        .expect("PAGE_SIZE_KB * KIBIBYTE > 0")
         .saturating_sub(1)
         .saturating_mul(heap_cost)
 }

programs/sbf/build.rs

@@ -42,10 +42,11 @@ fn main() {
         return;
     }
 
+    let build_profile = env::var("PROFILE").expect("`PROFILE` envvar to be set");
+    let install_dir = format!("target/{build_profile}/sbf");
     let sbf_c = env::var("CARGO_FEATURE_SBF_C").is_ok();
     if sbf_c {
-        let install_dir = "OUT_DIR=../target/".to_string() + &env::var("PROFILE").unwrap() + "/sbf";
-
+        let install_dir = format!("OUT_DIR=../{install_dir}");
         println!("cargo:warning=(not a warning) Building C-based on-chain programs");
         assert!(Command::new("make")
             .current_dir("c")
@@ -60,8 +61,6 @@ fn main() {
 
     let sbf_rust = env::var("CARGO_FEATURE_SBF_RUST").is_ok();
     if sbf_rust {
-        let install_dir = "target/".to_string() + &env::var("PROFILE").unwrap() + "/sbf";
-
         let rust_programs = [
             "128bit",
             "alloc",

programs/sbf/rust/secp256k1_recover/src/lib.rs

@@ -1,3 +1,4 @@
+#![allow(clippy::arithmetic_side_effects)]
 //! Secp256k1Recover Syscall test
 
 extern crate solana_program;

programs/zk-token-proof/benches/verify_proofs.rs

@@ -1,3 +1,4 @@
+#![allow(clippy::arithmetic_side_effects)]
 use {
     criterion::{criterion_group, criterion_main, Criterion},
     curve25519_dalek::scalar::Scalar,

sdk/cargo-build-sbf/Cargo.toml

@@ -13,6 +13,7 @@ edition = { workspace = true }
 bzip2 = { workspace = true }
 cargo_metadata = { workspace = true }
 clap = { version = "3.1.5", features = ["cargo", "env"] }
+itertools = { workspace = true }
 log = { workspace = true, features = ["std"] }
 regex = { workspace = true }
 reqwest = { workspace = true, features = ["blocking", "rustls-tls"] }

sdk/cargo-build-sbf/src/main.rs

@@ -1,6 +1,7 @@
 use {
     bzip2::bufread::BzDecoder,
     clap::{crate_description, crate_name, crate_version, Arg},
+    itertools::Itertools,
     log::*,
     regex::Regex,
     solana_download_utils::download_file,
@@ -73,15 +74,15 @@ where
     I: IntoIterator<Item = S>,
     S: AsRef<OsStr>,
 {
-    let args = args.into_iter().collect::<Vec<_>>();
-    let mut msg = format!("spawn: {}", program.display());
-    for arg in args.iter() {
-        msg = msg + &format!(" {}", arg.as_ref().to_str().unwrap_or("?")).to_string();
-    }
-    info!("{}", msg);
+    let args = Vec::from_iter(args);
+    let msg = args
+        .iter()
+        .map(|arg| arg.as_ref().to_str().unwrap_or("?"))
+        .join(" ");
+    info!("spawn: {}", msg);
 
     let child = Command::new(program)
-        .args(&args)
+        .args(args)
         .stdout(Stdio::piped())
         .spawn()
         .unwrap_or_else(|err| {
@@ -105,10 +106,7 @@ where
             writeln!(out, "{key}=\"{value}\" \\").unwrap();
         }
         write!(out, "{}", program.display()).unwrap();
-        for arg in args.iter() {
-            write!(out, " {}", arg.as_ref().to_str().unwrap_or("?")).unwrap();
-        }
-        writeln!(out).unwrap();
+        writeln!(out, "{}", msg).unwrap();
         out.flush().unwrap();
         error!(
             "To rerun the failed command for debugging use {}",

sdk/cargo-test-sbf/Cargo.toml

@@ -12,6 +12,7 @@ edition = { workspace = true }
 [dependencies]
 cargo_metadata = { workspace = true }
 clap = { version = "3.1.5", features = ["cargo"] }
+itertools = { workspace = true }
 log = { workspace = true, features = ["std"] }
 solana-logger = { workspace = true }
 

sdk/cargo-test-sbf/src/main.rs

@@ -1,5 +1,6 @@
 use {
     clap::{crate_description, crate_name, crate_version, Arg},
+    itertools::Itertools,
     log::*,
     std::{
         env,
@@ -58,15 +59,15 @@ where
     I: IntoIterator<Item = S>,
     S: AsRef<OsStr>,
 {
-    let args = args.into_iter().collect::<Vec<_>>();
-    let mut msg = format!("spawn: {}", program.display());
-    for arg in args.iter() {
-        msg = msg + &format!(" {}", arg.as_ref().to_str().unwrap_or("?")).to_string();
-    }
-    info!("{}", msg);
+    let args = Vec::from_iter(args);
+    let msg = args
+        .iter()
+        .map(|arg| arg.as_ref().to_str().unwrap_or("?"))
+        .join(" ");
+    info!("spawn: {}", msg);
 
     let mut child = Command::new(program)
-        .args(&args)
+        .args(args)
         .spawn()
         .unwrap_or_else(|err| {
             error!("Failed to execute {}: {}", program.display(), err);
@@ -89,10 +90,7 @@ where
             writeln!(out, "{key}=\"{value}\" \\").unwrap();
         }
         write!(out, "{}", program.display()).unwrap();
-        for arg in args.iter() {
-            write!(out, " {}", arg.as_ref().to_str().unwrap_or("?")).unwrap();
-        }
-        writeln!(out).unwrap();
+        writeln!(out, "{}", msg).unwrap();
         out.flush().unwrap();
         error!(
             "To rerun the failed command for debugging use {}",

sdk/program/src/alt_bn128.rs

@@ -176,6 +176,7 @@ mod target_arch {
         )
         .try_into()?;
 
+        #[allow(clippy::arithmetic_side_effects)]
         let result_point = p + q;
 
         let mut result_point_data = [0u8; ALT_BN128_ADDITION_OUTPUT_LEN];

sdk/program/src/program_stubs.rs

@@ -223,15 +223,14 @@ pub(crate) fn sol_get_epoch_rewards_sysvar(var_addr: *mut u8) -> u64 {
 #[doc(hidden)]
 pub fn is_nonoverlapping<N>(src: N, src_len: N, dst: N, dst_len: N) -> bool
 where
-    N: Ord + std::ops::Sub<Output = N>,
-    <N as std::ops::Sub>::Output: Ord,
+    N: Ord + num_traits::SaturatingSub,
 {
     // If the absolute distance between the ptrs is at least as big as the size of the other,
     // they do not overlap.
     if src > dst {
-        src - dst >= dst_len
+        src.saturating_sub(&dst) >= dst_len
     } else {
-        dst - src >= src_len
+        dst.saturating_sub(&src) >= src_len
     }
 }