From 834fae684bd22ebe0ea986dd3d467a9c0d0f5bed Mon Sep 17 00:00:00 2001 From: Trent Nelson Date: Fri, 19 Mar 2021 15:29:50 -0600 Subject: [PATCH] perf: use saturating/checked integer arithmetic --- perf/src/cuda_runtime.rs | 12 ++--- perf/src/lib.rs | 1 - perf/src/recycler.rs | 20 ++++++--- perf/src/sigverify.rs | 94 +++++++++++++++++++++++++++------------- 4 files changed, 86 insertions(+), 41 deletions(-) diff --git a/perf/src/cuda_runtime.rs b/perf/src/cuda_runtime.rs index e850100ff..4d61c09db 100644 --- a/perf/src/cuda_runtime.rs +++ b/perf/src/cuda_runtime.rs @@ -26,7 +26,7 @@ pub fn pin(_mem: &mut Vec) { let err = (api.cuda_host_register)( _mem.as_mut_ptr() as *mut c_void, - _mem.capacity() * size_of::(), + _mem.capacity().saturating_mul(size_of::()), 0, ); if err != CUDA_SUCCESS { @@ -34,7 +34,7 @@ pub fn pin(_mem: &mut Vec) { "cudaHostRegister error: {} ptr: {:?} bytes: {}", err, _mem.as_ptr(), - _mem.capacity() * size_of::() + _mem.capacity().saturating_mul(size_of::()), ); } } @@ -279,7 +279,7 @@ impl PinnedVec { } pub fn push(&mut self, x: T) { - let (old_ptr, old_capacity) = self.prepare_realloc(self.x.len() + 1); + let (old_ptr, old_capacity) = self.prepare_realloc(self.x.len().saturating_add(1)); self.x.push(x); self.check_ptr(old_ptr, old_capacity, "push"); } @@ -295,13 +295,15 @@ impl PinnedVec { } pub fn append(&mut self, other: &mut Vec) { - let (old_ptr, old_capacity) = self.prepare_realloc(self.x.len() + other.len()); + let (old_ptr, old_capacity) = + self.prepare_realloc(self.x.len().saturating_add(other.len())); self.x.append(other); self.check_ptr(old_ptr, old_capacity, "resize"); } pub fn append_pinned(&mut self, other: &mut Self) { - let (old_ptr, old_capacity) = self.prepare_realloc(self.x.len() + other.len()); + let (old_ptr, old_capacity) = + self.prepare_realloc(self.x.len().saturating_add(other.len())); self.x.append(&mut other.x); self.check_ptr(old_ptr, old_capacity, "resize"); } diff --git a/perf/src/lib.rs b/perf/src/lib.rs index 9559e052e..82feefaa7 100644 --- a/perf/src/lib.rs +++ b/perf/src/lib.rs @@ -1,4 +1,3 @@ -#![allow(clippy::integer_arithmetic)] pub mod cuda_runtime; pub mod packet; pub mod perf_libs; diff --git a/perf/src/recycler.rs b/perf/src/recycler.rs index 728fb2916..e1dfaa937 100644 --- a/perf/src/recycler.rs +++ b/perf/src/recycler.rs @@ -1,6 +1,7 @@ use rand::{thread_rng, Rng}; use solana_measure::measure::Measure; use std::{ + convert::TryFrom, sync::{ atomic::{AtomicBool, AtomicUsize, Ordering}, Arc, Mutex, Weak, @@ -112,7 +113,14 @@ impl ObjectPool { } fn get_shrink_target(shrink_pct: u32, current_size: u32) -> u32 { - ((shrink_pct * current_size) + 99) / 100 + let shrink_pct = u64::from(shrink_pct); + let current_size = u64::from(current_size); + let shrink_target = shrink_pct + .saturating_mul(current_size) + .saturating_add(99) + .checked_div(100) + .unwrap_or(0); + u32::try_from(shrink_target).unwrap_or(u32::MAX) } fn shrink_if_necessary( @@ -138,7 +146,7 @@ impl ObjectPool { if self.len() > self.minimum_object_count as usize && self.len() > shrink_threshold_count as usize { - self.above_shrink_pct_count += 1; + self.above_shrink_pct_count = self.above_shrink_pct_count.saturating_add(1); } else { self.above_shrink_pct_count = 0; } @@ -147,7 +155,7 @@ impl ObjectPool { let mut recycler_shrink_elapsed = Measure::start("recycler_shrink"); // Do the shrink let target_size = std::cmp::max(self.minimum_object_count, shrink_threshold_count); - let ideal_num_to_remove = self.total_allocated_count - target_size; + let ideal_num_to_remove = self.total_allocated_count.saturating_sub(target_size); let mut shrink_removed_objects = Vec::with_capacity(ideal_num_to_remove as usize); for _ in 0..ideal_num_to_remove { if let Some(mut expired_object) = self.objects.pop() { @@ -160,7 +168,7 @@ impl ObjectPool { // before the object is allocated (see `should_allocate_new` logic below). // This race allows a difference of up to the number of threads allocating // with this recycler. - self.total_allocated_count -= 1; + self.total_allocated_count = self.total_allocated_count.saturating_sub(1); } else { break; } @@ -191,13 +199,13 @@ impl ObjectPool { AllocationDecision::Reuse(reused_object) } else if let Some(limit) = self.limit { if self.total_allocated_count < limit { - self.total_allocated_count += 1; + self.total_allocated_count = self.total_allocated_count.saturating_add(1); AllocationDecision::Allocate(self.total_allocated_count, self.len()) } else { AllocationDecision::AllocationLimitReached } } else { - self.total_allocated_count += 1; + self.total_allocated_count = self.total_allocated_count.saturating_add(1); AllocationDecision::Allocate(self.total_allocated_count, self.len()) } } diff --git a/perf/src/sigverify.rs b/perf/src/sigverify.rs index 1a2e1682f..846504198 100644 --- a/perf/src/sigverify.rs +++ b/perf/src/sigverify.rs @@ -18,6 +18,7 @@ use solana_sdk::short_vec::decode_len; use solana_sdk::signature::Signature; #[cfg(test)] use solana_sdk::transaction::Transaction; +use std::convert::TryFrom; use std::mem::size_of; // Representing key tKeYE4wtowRb8yRroZShTipE18YVnqwXjsSAoNsFU6g @@ -109,8 +110,8 @@ fn verify_packet(packet: &mut Packet) { let msg_end = packet.meta.size; for _ in 0..packet_offsets.sig_len { - let pubkey_end = pubkey_start as usize + size_of::(); - let sig_end = sig_start as usize + size_of::(); + let pubkey_end = pubkey_start.saturating_add(size_of::()); + let sig_end = sig_start.saturating_add(size_of::()); if pubkey_end >= packet.meta.size || sig_end >= packet.meta.size { packet.meta.discard = true; @@ -134,8 +135,8 @@ fn verify_packet(packet: &mut Packet) { packet.meta.is_tracer_tx = true; } - pubkey_start += size_of::(); - sig_start += size_of::(); + pubkey_start = pubkey_end; + sig_start = sig_end; } } @@ -150,7 +151,11 @@ fn do_get_packet_offsets( ) -> Result { let message_header_size = serialized_size(&MessageHeader::default()).unwrap() as usize; // should have at least 1 signature, sig lengths and the message header - if (1 + size_of::() + message_header_size) > packet.meta.size { + let min_packet_size = 1usize + .checked_add(size_of::()) + .and_then(|v| v.checked_add(message_header_size)) + .ok_or(PacketError::InvalidLen)?; + if min_packet_size > packet.meta.size { return Err(PacketError::InvalidLen); } @@ -160,23 +165,35 @@ fn do_get_packet_offsets( // Using msg_start_offset which is based on sig_len_untrusted introduces uncertainty. // Ultimately, the actual sigverify will determine the uncertainty. - let msg_start_offset = sig_size + sig_len_untrusted * size_of::(); + let msg_start_offset = sig_len_untrusted + .checked_mul(size_of::()) + .and_then(|v| v.checked_add(sig_size)) + .ok_or(PacketError::InvalidLen)?; // Packet should have data at least for signatures, MessageHeader, 1 byte for Message.account_keys.len - if (msg_start_offset + message_header_size + 1) > packet.meta.size { + let min_message_end_offset = msg_start_offset + .checked_add(message_header_size) + .and_then(|v| v.checked_add(1)) + .ok_or(PacketError::InvalidSignatureLen)?; + if min_message_end_offset > packet.meta.size { return Err(PacketError::InvalidSignatureLen); } // read MessageHeader.num_required_signatures (serialized with u8) let sig_len_maybe_trusted = packet.data[msg_start_offset] as usize; - let message_account_keys_len_offset = msg_start_offset + message_header_size; + let message_account_keys_len_offset = msg_start_offset + .checked_add(message_header_size) + .ok_or(PacketError::InvalidLen)?; // This reads and compares the MessageHeader num_required_signatures and // num_readonly_signed_accounts bytes. If num_required_signatures is not larger than // num_readonly_signed_accounts, the first account is not debitable, and cannot be charged // required transaction fees. - if packet.data[msg_start_offset] <= packet.data[msg_start_offset + 1] { + let readonly_signer_offset = msg_start_offset + .checked_add(1) + .ok_or(PacketError::InvalidLen)?; + if packet.data[msg_start_offset] <= packet.data[readonly_signer_offset] { return Err(PacketError::PayerNotWritable); } @@ -184,25 +201,41 @@ fn do_get_packet_offsets( let (pubkey_len, pubkey_len_size) = decode_len(&packet.data[message_account_keys_len_offset..]) .map_err(|_| PacketError::InvalidShortVec)?; - if (message_account_keys_len_offset + pubkey_len * size_of::() + pubkey_len_size) - > packet.meta.size - { + let min_account_keys_end_offset = pubkey_len + .checked_mul(size_of::()) + .and_then(|v| v.checked_add(message_account_keys_len_offset)) + .and_then(|v| v.checked_add(pubkey_len_size)) + .ok_or(PacketError::InvalidPubkeyLen)?; + if min_account_keys_end_offset > packet.meta.size { return Err(PacketError::InvalidPubkeyLen); } - let sig_start = current_offset as usize + sig_size; - let msg_start = current_offset as usize + msg_start_offset; - let pubkey_start = msg_start + message_header_size + pubkey_len_size; + let sig_start = usize::try_from(current_offset) + .ok() + .and_then(|v| v.checked_add(sig_size)) + .ok_or(PacketError::InvalidLen)?; + let msg_start = usize::try_from(current_offset) + .ok() + .and_then(|v| v.checked_add(msg_start_offset)) + .ok_or(PacketError::InvalidLen)?; + let pubkey_start = msg_start + .checked_add(message_header_size) + .and_then(|v| v.checked_add(pubkey_len_size)) + .ok_or(PacketError::InvalidLen)?; if sig_len_maybe_trusted != sig_len_untrusted { return Err(PacketError::MismatchSignatureLen); } + fn to_u32(value: usize) -> Result { + u32::try_from(value).map_err(|_| PacketError::InvalidLen) + } + Ok(PacketOffsets::new( - sig_len_untrusted as u32, - sig_start as u32, - msg_start as u32, - pubkey_start as u32, + to_u32(sig_len_untrusted)?, + to_u32(sig_start)?, + to_u32(msg_start)?, + to_u32(pubkey_start)?, )) } @@ -226,12 +259,12 @@ pub fn generate_offsets(batches: &[Packets], recycler: &Recycler) -> T msg_start_offsets.set_pinnable(); let mut msg_sizes: PinnedVec<_> = recycler.allocate().unwrap(); msg_sizes.set_pinnable(); - let mut current_packet = 0; + let mut current_packet: u32 = 0; let mut v_sig_lens = Vec::new(); batches.iter().for_each(|p| { let mut sig_lens = Vec::new(); p.packets.iter().for_each(|packet| { - let current_offset = current_packet as u32 * size_of::() as u32; + let current_offset = current_packet.saturating_mul(size_of::() as u32); let packet_offsets = get_packet_offsets(packet, current_offset); @@ -243,17 +276,19 @@ pub fn generate_offsets(batches: &[Packets], recycler: &Recycler) -> T let mut sig_offset = packet_offsets.sig_start; for _ in 0..packet_offsets.sig_len { signature_offsets.push(sig_offset); - sig_offset += size_of::() as u32; + sig_offset = sig_offset.saturating_add(size_of::() as u32); pubkey_offsets.push(pubkey_offset); - pubkey_offset += size_of::() as u32; + pubkey_offset = pubkey_offset.saturating_add(size_of::() as u32); msg_start_offsets.push(packet_offsets.msg_start); - msg_sizes - .push(current_offset + (packet.meta.size as u32) - packet_offsets.msg_start); + let msg_size = current_offset + .saturating_add(packet.meta.size as u32) + .saturating_sub(packet_offsets.msg_start); + msg_sizes.push(msg_size); } - current_packet += 1; + current_packet = current_packet.saturating_add(1); }); v_sig_lens.push(sig_lens); }); @@ -302,7 +337,7 @@ pub fn copy_return_values(sig_lens: &[Vec], out: &PinnedVec, rvs: &mut if 0 == out[num] { vout = 0; } - num += 1; + num = num.saturating_add(1); } *v = vout; } @@ -382,7 +417,7 @@ pub fn ed25519_verify( let mut elems = Vec::new(); let mut rvs = Vec::new(); - let mut num_packets = 0; + let mut num_packets: usize = 0; for p in batches.iter() { elems.push(perf_libs::Elems { elems: p.packets.as_ptr(), @@ -391,7 +426,7 @@ pub fn ed25519_verify( let mut v = Vec::new(); v.resize(p.packets.len(), 0); rvs.push(v); - num_packets += p.packets.len(); + num_packets = num_packets.saturating_add(p.packets.len()); } out.resize(signature_offsets.len(), 0); trace!("Starting verify num packets: {}", num_packets); @@ -435,6 +470,7 @@ pub fn make_packet_from_transaction(tx: Transaction) -> Packet { } #[cfg(test)] +#[allow(clippy::integer_arithmetic)] mod tests { use super::*; use crate::packet::{Packet, Packets};